feat: Add workaround for CSRF SameSite=None cookies #1810
Conversation
|
|
jcxplorer
changed the title
|
The nancy test runs fine locally, so probably just needs to be re-triggered in the CI. Seems like the webservice it uses was down at the time. |
|
Yup, triggering CI |
There was a problem hiding this comment.
This PR is legit, good job :)
Could you add a paragraph here to document this feature? https://github.com/ory/hydra/blob/master/docs/docs/advanced.md#cookies
Thank you!
consent/helper_test.go
Outdated
| { | ||
| name: cookieAuthenticationCSRFName, | ||
| csrfValue: "CSRF-VALUE", | ||
| sameSite: http.SameSiteDefaultMode, |
There was a problem hiding this comment.
Shouldn't this be http.SameSiteNoneMode?
There was a problem hiding this comment.
Yup, will update.
consent/helper_test.go
Outdated
| { | ||
| name: cookieAuthenticationCSRFName, | ||
| csrfValue: "CSRF-VALUE", | ||
| sameSite: http.SameSiteDefaultMode, |
There was a problem hiding this comment.
Shouldn't this be http.SameSiteNoneMode?
|
Great, I'll make the few changes and rebase against master. @aeneasr do you want me to amend the current commit or have a separate commit? |
|
I'm squash-merging PRs always anyways so two commits is fine! |
jcxplorer
force-pushed
the
same-site-none-cookie-workaround
branch
from
424d8bd
to
1d4effa
Compare
|
All done! 🙌 |
|
Thank you! |
Enables legacy compatibility on iOS version < 13 and macOS version < 10.15 #1810 incorrectly implements https://web.dev/samesite-cookie-recipes/#handling-incompatible-clients Notice Set-cookie: 3pcookie-legacy=value; Secure the cookie does not have the SameSite attribute present. The http.SameSiteDefaultMode used in hydra implementation results in attribute without the value, see https://github.com/golang/go/blob/release-branch.go1.14/src/net/http/cookie.go#L221 That triggers the problems with the older iOS and macOS versions, as Apple did not follow the https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00#section-4.1 see https://trac.webkit.org/browser/webkit/trunk/Source/WebInspectorUI/UserInterface/Models/Cookie.js?rev=239226#L118 Closes: #1907
Related issue
Closes #1753 as discussed with @aeneasr.
Proposed changes
Implements the workaround from https://web.dev/samesite-cookie-recipes/ for the CSRF cookies only when using SameSite=None. This is configurable and disabled by default.
Also adds some unit tests for the existing CSRF cookie helpers, along with unit tests for this change.
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further comments
Regenerating the documentation changed quite a few bits in the configuration section that were unrelated to my change. Please double check those changes 😬