fix(core): Don't set samesite cookies on status.php #54528
Conversation
sshambar
requested review from
ArtificialOwl,
come-nc and
salmart-dev
and removed request for
a team
come-nc
left a comment
come-nc
left a comment
There was a problem hiding this comment.
I do not understand enough about this, but a small question that bugged me when reading surrounding code.
lib/base.php
Outdated
| exit(); | ||
| } | ||
| } | ||
| } elseif (!isset($_COOKIE['nc_sameSiteCookielax']) || !isset($_COOKIE['nc_sameSiteCookiestrict'])) { |
There was a problem hiding this comment.
Am I crazy or is that condition useless?
If count($_COOKIE) is <= 0, those can never be set?
So, an else would be enough.
There was a problem hiding this comment.
Yeah, I considered removing that, but I'm guessing someone put it there to make it clear which cookies are being set?
There was a problem hiding this comment.
| } elseif (!isset($_COOKIE['nc_sameSiteCookielax']) || !isset($_COOKIE['nc_sameSiteCookiestrict'])) { | |
| } else { | |
| /* set nc_sameSiteCookielax and nc_sameSiteCookiestrict */ |
Something like that would be clearer then.
There was a problem hiding this comment.
@come-nc Looks like a good plan, I've updated the commit
Fixes an issue where samesite cookies are sent on status.php before session is started; cookies with webroots other than '/' therefore have the wrong name and lead to samesite cookie validation failures. - resolves nextcloud#54227 Signed-off-by: Scott Shambarger <[email protected]>
|
@come-nc I can confirm that fixes the issue as well. I did consider that approach, but I was concerned about adding extra work to the nextcloud "ping" target - it's used by liveness/readiness probes in the helm chart (and probably elsewhere) so probably should do as little as possible :) |
|
@AndyScherzinger - no problem, they both fix the issue :) I might suggest updating the comment in the #54713 patch to state why the status.php is after the cookie_path setting, so the check's not moved later... |
Added. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
4 participants
Summary
Fixes an issue where samesite cookies are sent on status.php before session is started; cookies with webroots other than '/' therefore have the wrong name and lead to samesite cookie validation failures.