Skip to content
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

Allow non Same-Site Cookies set on first request #44574

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pointhi
Copy link

@pointhi pointhi commented Mar 29, 2024

Summary

When any cookie is already present during the first request (e.g. an Apache module may choose to set it for various reasons) a 412 Precondition Failed error is returned on the first request. The second request works as intended as the Same-Site Cookies are now set correctly.

This breaks for example CalDAV/CardDAV syncs with Davx5 as the request is not retried after the first failure.

The proposed fix is to check for the explicit existence of nc_sameSiteCookielax or nc_sameSiteCookiestrict instead of just checking if any cookie exists. I used the proposed fix from the issue, but I think we can remove count($_COOKIE) > 0 as it looks redundant now.

Checklist

@solracsf solracsf changed the title Fix #41210 to allow non Same-Site Cookies set on first request Allow non Same-Site Cookies set on first request Mar 30, 2024
@solracsf solracsf added this to the Nextcloud 29 milestone Mar 30, 2024
@solracsf solracsf added the 3. to review Waiting for reviews label Mar 30, 2024
@pointhi pointhi force-pushed the fix/41210-allow-cookie-set-on-first-request branch from 37f6382 to 1c28f60 Compare April 1, 2024 12:32
This was referenced Apr 4, 2024
@blizzz blizzz modified the milestones: Nextcloud 29, Nextcloud 30 Apr 8, 2024
Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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!

@thebaron06
Copy link

I can confirm that this solution is working and would appreciate this being merged.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the idea of the fix and why it is not working correctly at the moment, but I fail to see why this is necessary in the first place. What use-case does this fix? When do you already have a cookie present?

@@ -563,7 +563,7 @@ private static function performSameSiteCookieProtection(\OCP\IConfig $config): v
return;
}

if (count($_COOKIE) > 0) {
if (count($_COOKIE) > 0 && (isset($_COOKIE['nc_sameSiteCookielax']) || isset($_COOKIE['nc_sameSiteCookiestrict']))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (count($_COOKIE) > 0 && (isset($_COOKIE['nc_sameSiteCookielax']) || isset($_COOKIE['nc_sameSiteCookiestrict']))) {
if (isset($_COOKIE['nc_sameSiteCookielax']) || isset($_COOKIE['nc_sameSiteCookiestrict'])) {

And adjust the elseif below to be a simple else.

@blizzz blizzz mentioned this pull request Aug 1, 2024
This was referenced Aug 5, 2024
@AndyScherzinger AndyScherzinger force-pushed the fix/41210-allow-cookie-set-on-first-request branch from 1c28f60 to 6dbd887 Compare August 7, 2024 13:44
@skjnldsv skjnldsv mentioned this pull request Aug 13, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
@thebaron06
Copy link

For me the use-case is to allow the DAVx5 App to sync my contacts and calendars to and AIO instance of nextcloud.
If I understood it correctly, DAVx5 has already a same site cookie set on it's first request and sticks strict to a spec that says: 'don't try again on anything else than a 200 http code' or so. But there exists davx5 issues where this behavior is discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Same Site Cookies Not Set if First Request has Cookies (412 Precondition Failed)
7 participants