-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Samesite auth cookie #5255
Samesite auth cookie #5255
Conversation
Allows setting the SameSite attribute on the auth cookie, and aligning with what browsers are moving towards to, default it to `Lax` Refactored the code to not rely on string manipulation for modifying the cookie attributes but rather the standard library module. This causes some changes in the case of the attributes and their order but it's worth the extra work. https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#SameSite_cookies
Order attributes before comparing, add tests for SameSite
# Set secure based on config value. Default is False | ||
secure = config.get(u'who.secure', False) | ||
secure = _bool(config.get(u'who.secure', False)) | ||
# Set samesite based on config value. Default is lax |
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.
This should be documented as a potentially breaking change (admittedly it shouldn't break sensible setups).
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.
Changelogs are updated during the release process, so it would be documented there.
@@ -69,9 +92,15 @@ def make_plugin(secret=None, | |||
if timeout is not None and reissue_time is None: | |||
reissue_time = int(math.ceil(int(timeout) * 0.1)) | |||
# Set httponly based on config value. Default is True | |||
httponly = config.get(u'who.httponly', True) | |||
httponly = _bool(config.get(u'who.httponly', True)) |
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.
This should probably be deferred until after we check the attributes that might cause an early termination.
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.
Boolean conversions were done in CkanAuthTktCookiePlugin call and those variables weren't used in anywhere else, so it makes sense to convert them right away.
Note about the change and how to disable it should be added to the release notes. |
@Zharktas did you cherry-pick this into the dev-v2.* branches already? If not add the "Backport pending" label otherwise we assume the backport has been done. |
@amercader ah right, missed the pending label, added now. |
Adapted version of #5255 PR for 2.8
Adapted version of #5255 PR for 2.8
Adapted version of #5255 PR for 2.8
Allows setting the SameSite attribute on the auth cookie, and aligning with what browsers are moving towards to, default it to
Lax
.Refactored the code to not rely on string manipulation for modifying the cookie attributes but rather the standard library module.
This causes some changes in the case of the attributes and their order which means updating the tests, but I think it's worth the extra work.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#SameSite_cookies