-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
oauth: ability to specify SameSite cookie attribute value #35455
oauth: ability to specify SameSite cookie attribute value #35455
Conversation
Signed-off-by: Derek Argueta <[email protected]>
Signed-off-by: Derek Argueta <[email protected]>
Signed-off-by: Derek Argueta <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
@@ -24,6 +24,19 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
// [#extension: envoy.filters.http.oauth2] | |||
// | |||
|
|||
message CookieSettings { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a comment for this message type would be helpful
@@ -24,6 +24,19 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
// [#extension: envoy.filters.http.oauth2] | |||
// | |||
|
|||
message CookieSettings { | |||
enum SameSiteValues { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SameSite
would be better as a name.
|
||
// The value used for the SameSite cookie attribute. Defaults to DISABLED which does not add the | ||
// SameSite attribute. | ||
SameSiteValues same_site_attribute_value = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/same_site_attribute_value/same_site_attribute
/wait |
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
@derekargueta thanks for initiating this work -- are you still working on this? we have an interest to be able to set the cookie attribute. |
We are also looking forward to the fix here. Any plan to restart to effort? |
Hi @mchen391 Lyft will take this work and continue the testing. I will be working on this. |
@Yueren-Wang what's your expected timeline to work-on/fix this? |
I am a new contributor, So I am targeting to get this commit checked in and shipped by end of this year, taking into account that the holiday season may cause some delays. |
@Yueren-Wang any update or progress on this? |
Commit Message: oauth: ability to specify SameSite cookie attribute value
Additional Description: The
SameSite
attribute has three different values to allow control over whether the cookies get shared same-site/cross-site. It's optional so there's also aDisabled
option which excludes the SameSite attribute. This is the default setting so existing deployments are not modified in any way, but now operators can enable SameSite.Risk Level: Low
Testing: unit
Docs Changes: proto is documented
Release Notes: changelog entry added