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

KaseyJenkins commented 1 year ago

⚠️ This issue respects the following points: ⚠️

[X] This is a bug, not a question or a configuration/webserver/proxy issue.
[X] This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
[X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
[X] I agree to follow Nextcloud's Code of Conduct.

Bug description

In base.php:performSameSiteCookieProtection()

The way the code seems to work at the moment:

First request:

The $_COOKIE superglobal is checked for any cookies set. base.php:560
No cookies are set (this is the first request after all) - hence nc_sameSiteCookielax and nc_sameSiteCookiestrict are set (along with oc_sessionPassphrase and oc<10-character id>) base.php:584

Second and subsequent requests:

The $_COOKIE superglobal has some cookies set, hence we check whether $request->passesStrictCookieCheck() base.php:571; Request.php:532
We check whether cookieCheckRequired() - it is required, since a cookie with the session_name is present (oc<10-character id>) Request.php:489
We detect the presence of nc_sameSiteCookiestrict and nc_sameSiteCookiestrict set during the first request.
We successfully pass $request->passesStrictCookieCheck() base.php:571.

The above works when the assumption that the first request should come in without any cookies set is correct. When some cookie is already present during the first request (e.g. an Apache module may choose to set it for various reasons) we seem to encounter a small issue:

First request:

The $_COOKIE superglobal is checked for cookies set. base.php:560
There are some cookies set, hence we immediately start checking whether $request->passesStrictCookieCheck() base.php:571; Request.php:532.
We check whether cookieCheckRequired() - it is not required, since there's no cookie with the session_name as of yet (oc<10-character id>) Request.php:489.
We successfully pass $request->passesStrictCookieCheck() base.php:571, however, nc_sameSiteCookielax and nc_sameSiteCookiestrict haven't been set, only oc_sessionPassphrase and oc<10-character id>.

Second request:

The $_COOKIE superglobal is checked for cookies set. base.php:560
There are some cookies set (oc_sessionPassphrase and oc<10-character id> along with the external cookie(s) we didn't set that came in the first request), hence we check whether $request->passesStrictCookieCheck() base.php:571; Request.php:532
We check whether cookieCheckRequired() - it is indeed required, since a cookie with the session_name is present (oc<10-character id>) Request.php:489.
We do not detect the presence of nc_sameSiteCookiestrict and nc_sameSiteCookiestrict (they were not set during the first request).
The $request->passesStrictCookieCheck() fails base.php:571.
There's a warning 'Request does not pass strict cookie check' and, if the debug mode is off, we bail out with 412 (Http::STATUS_PRECONDITION_FAILED) and a 'Strict Cookie has not been found in request' json error.
We set nc_sameSiteCookielax and nc_sameSiteCookiestrict cookies here.

Third and consequent requests: Since all the necessary cookies are present (nc_sameSiteCookielax, nc_sameSiteCookiestrict, oc_sessionPassphrase, and oc<10-character id>) everything finally works as expected (akin to the second request in the first scenario).

Steps to reproduce

This could be seen running the following:

Simulating the first request without any cookies set:

curl -v localhost:8080/nextcloud/status.php 2>&1 | grep -i cookie
< Set-Cookie: ocldskj6c8iq=43ed5cb9f26fb8dcda77d955c0c5a498; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: oc_sessionPassphrase=BNeOCZoPV6nkHH5%2F3Mto34Opy8LXA0W%2BvgxRyYZSm1nJLrDDURBTasqnMDjqz4kFb1qRCHmivBWFgYUTTBDJeJb4wF36WMkh0C7ILfxSEyOArEz1OD32%2Bv2eTTRorg7j; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: ocldskj6c8iq=103ef2b40f82f46e50ca45afe29fa2e0; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: ocldskj6c8iq=103ef2b40f82f46e50ca45afe29fa2e0; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: nc_sameSiteCookielax=true; path=/nextcloud; httponly;expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=lax
< Set-Cookie: nc_sameSiteCookiestrict=true; path=/nextcloud; httponly;expires=Fri, 31-Dec-2100 23:59:59 GMT; SameSite=strict
< Set-Cookie: ocldskj6c8iq=103ef2b40f82f46e50ca45afe29fa2e0; path=/nextcloud; HttpOnly; SameSite=Lax

Simulating the first request with an 'external' cookie already present:

curl -v --cookie "UnexpectedCookie=yesImhere" localhost:8080/nextcloud/status.php 2>&1 | grep -i cookie
> Cookie: UnexpectedCookie=yesImhere
< Set-Cookie: ocldskj6c8iq=f0556410d8f47998ccb93ef82634ef94; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: oc_sessionPassphrase=9Vpc22L%2F0Ub3og1lolhwh3DH%2Fl0YIkOSz5A2T4wWIWO9semi8wfbuXAwCCHJmO3MR2v5MnOsHKXZLd9Xvh03wP3J0xJPBkQ9MPwsxlqrbig32nbhVlGW0Uw53rt0ap%2FY; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: ocldskj6c8iq=919c28d19797a231080cd510df513a4d; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: ocldskj6c8iq=919c28d19797a231080cd510df513a4d; path=/nextcloud; HttpOnly; SameSite=Lax
< Set-Cookie: ocldskj6c8iq=919c28d19797a231080cd510df513a4d; path=/nextcloud; HttpOnly; SameSite=Lax

You could also simulate first and second requests with the following php scripts respectively: The first one contains an unexpected cookie during the first request.

<?php

$apachePort = 8080;
if (isset($_GET['port']))
{
    $apachePort = $_GET['port'];
}

$url = 'http://127.0.0.1:' . $apachePort . '/nextcloud/status.php';

$opts = array(
  'http'=>array(
     'header'=> array(
       "Cookie: unexpectedCookie=yesImhere"
      )
   )
);

$context = stream_context_create($opts);

$fp = fopen($url, 'r', false, $context);

$content = stream_get_contents($fp);
// Response headers:
echo '<pre>' . print_r(stream_get_meta_data($fp), true) . '</pre>';

fpassthru($fp);
fclose($fp);
?>

The second has all the cookies set but nc_sameSiteCookielax, nc_sameSiteCookiestrict, which results in 412.

<?php

$apachePort = 8080;
if (isset($_GET['port']))
{
    $apachePort = $_GET['port'];
}

$url = 'http://127.0.0.1:' . $apachePort . '/nextcloud/status.php';

$opts = array(
  'http'=>array(
     'header'=> array(
      "Cookie: ocldskj6c8iq=0c036734f2a405628a56447b78b7e6c2; oc_sessionPassphrase=zUjFaxwbxcUwIpQxUWYRV5IeVHHVGcUG3JnlSh1QKd7gOAR%2Ba4XbzPVia0TqXzynJa6kCL4gz4JiI%2FutgiyCWJT1HzUQ5WbIaY63XEonH9Fr7bA%2FK7FhRHCM0q%2B%2FI8%2BM"
      )
   )
);

$context = stream_context_create($opts);

$fp = fopen($url, 'r', false, $context);

$content = stream_get_contents($fp);
// Response headers:
echo '<pre>' . print_r(stream_get_meta_data($fp), true) . '</pre>';

fpassthru($fp);
fclose($fp);
?>

solracsf commented 1 year ago

Does

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

fixes the probleme here?

https://github.com/nextcloud/server/blob/565dc36226d08d071c30d8ad4fd54126dfa4be79/lib/base.php#L560

KaseyJenkins commented 1 year ago

Hi @solracsf! Yes, that seems to have done the trick :)

If that's a preferred solution should I create a PR or will somebody else be assigned this task?

Thanks!

solracsf commented 1 year ago

@nickvergessen do you have an opinion here?

pointhi commented 11 months ago

@solracsf @nickvergessen is there any update on this issue?

nickvergessen commented 11 months ago

This is not my area of expertise

nextcloud / server