-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Only send samesite cookies #17075
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
Only send samesite cookies #17075
Conversation
ChristophWurst
left a comment
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.
Clicked around, shared some files. All fine 👍
b9323f2 to
b5e98dc
Compare
|
So... for some reason this does 💥 when mixed. So not this only happens on php7.3 and later. |
b5e98dc to
1585e2f
Compare
1585e2f to
ee37218
Compare
This makes the last remaining two cookies lax. The session cookie itself. And the session password as well (on php 7.3 that is). Samesite cookies are the best cookies! Signed-off-by: Roeland Jago Douma <[email protected]>
ee37218 to
2016e57
Compare
|
This change breaks SAML auth in many cases with PHP 7.3 and above. During a Nextcloud-SAML login, the user attempts to goto the nextcloud frontpage to login, nextcloud sets up the session cookies right away. Then redirects the user to login via SAML. After the SAML login, there is a POST request with the SAML authorization back to nextcloud: With these cookies set to LAX, the POST doesn't contain the needed session cookies, and the auth fails entirely. If we drop the PHP version to 7.2, the cookie doesn't get set to LAX, and the SAML response POST will contain the needed session cookie, and login via SAML is successful. This phenomenon is documented here, https://php.watch/articles/PHP-Samesite-cookies :
I can confirm this is the source of the issue two different ways: drop the server php version to 7.2, issue gone; makes sense since this change code only sets the flag if php is 7.3 or above. Also in chrome hitting nextcloud 19 with php 7.3 or 7.4, after the failed SAML login attempt from this issue, you can manually modify the session cookies in the F12 dev tools to remove the LAX flag from the two session cookies, then retrying login succeeds. A workaround is setting up the SAML IdP server on the same subdomain as the nextcloud instance: sso.mydomain.net In this scenario, the bug doesn't manifest, since they are both part of the same subdomain, LAX allows the cookie to be sent even in a POST. Unfortunately changing the hosted FQDN of the Identity provider to match your nextcloud instance may not be an option, or feasible. Perhaps an override for this cookie change can be put in place so SAML auth app users can continue to function as before? |
This makes the last remaining two cookies lax. The session cookie
itself. And the session password as well (on php 7.3 that is). Samesite
cookies are the best cookies!
Signed-off-by: Roeland Jago Douma [email protected]