Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oauth2: fix Max-Age attribute of Set-Cookie response header #26715

Conversation

ggmoy
Copy link
Contributor

@ggmoy ggmoy commented Apr 12, 2023

Commit Message: oauth2: fix Max-Age attribute of Set-Cookie response header

Additional Description: The Max-Age attribute of Set-Cookie response header indicates the number of seconds until the cookie expires. Currently, we are assigning a value representing Seconds Since the Epoch to the Max-Age attribute. This is not correct and causes cookies to expire in ~53 years.

This PR updates the code to use the expires_in value received from the OAuth server response to set the Max-Age attribute.

Risk Level:
Testing: Adding unit tests for the fixed code and for the new runtime guard introduced by this PR
Docs Changes:
Release Notes:
Platform Specific Features:
Runtime guard: Adding envoy.reloadable_features.oauth_use_standard_max_age_value runtime guard
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
@repokitteh-read-only
Copy link

Hi @ggmoy, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #26715 was opened by ggmoy.

see: more, trace.

Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
@ggmoy ggmoy changed the title OAuth2: Fix Max-Age cookie attribute OAuth2: Fix Max-Age attribute of Set-Cookie HTTP response header Apr 13, 2023
@ggmoy ggmoy changed the title OAuth2: Fix Max-Age attribute of Set-Cookie HTTP response header OAuth2: Fix Max-Age attribute of Set-Cookie response header Apr 13, 2023
Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
@ggmoy ggmoy changed the title OAuth2: Fix Max-Age attribute of Set-Cookie response header oauth2: fix Max-Age attribute of Set-Cookie response header Apr 13, 2023
Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
@yanavlasov yanavlasov self-assigned this Apr 13, 2023
@yanavlasov
Copy link
Contributor

Looks like the original intention was to use the Expires cookie, instead of Max-Age. The fix looks good to me, however given that this is user visible change, it needs a runtime flag protection, in case someone was relying on old behavior.

Here is an example of using a runtime guard: #26326

/wait-any

Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #26715 was synchronize by ggmoy.

see: more, trace.

@phlax
Copy link
Member

phlax commented Apr 13, 2023

@ggmoy probably best to merge main (other than the runtime guard issue that CI is flagging)

@phlax
Copy link
Member

phlax commented Apr 13, 2023

wait! - i just saw this is against release/v1.25

is this an issue only on 1.25- - otherwise lets fix on main and then backport - also wondering about older versions

Signed-off-by: Gustavo Gabriel Moyano <gustavo.g.moyano@gmail.com>
@ggmoy
Copy link
Contributor Author

ggmoy commented Apr 13, 2023

wait! - i just saw this is against release/v1.25

is this an issue only on 1.25- - otherwise lets fix on main and then backport - also wondering about older versions

No, it is not only on release/v1.25. So, do I have to create a new PR againt main?

@phlax
Copy link
Member

phlax commented Apr 14, 2023

So, do I have to create a new PR againt main?

yeah, if you could, that would be great

@ggmoy ggmoy closed this Apr 14, 2023
@ggmoy ggmoy deleted the fix-oauth2-max-age-cookie-attribute branch April 14, 2023 13:14
@ggmoy
Copy link
Contributor Author

ggmoy commented Apr 15, 2023

So, do I have to create a new PR againt main?

yeah, if you could, that would be great

@phlax, I created a new one from main. Could you have a look, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants