NEW Allow setting "samesite" attribute for individual cookies. #11632

GuySartorelli · 2025-03-11T00:19:55Z

Also update secure-ness of authentication cookies to match session cookies, and finish securing session cookies.

Issue

Add parameters to set samesite for any arbitrary cookie #10342

GuySartorelli · 2025-03-11T00:23:39Z

src/Security/MemberAuthenticator/CookieAuthenticationHandler.php

+    private bool $tokenCookieSecure = true;
+
    /**
-     * @var boolean
+     * The SameSite value to use for authentication cookies.
+     * If set to an empty string, the default value from Cookie.default_samesite will be used.
     */
-    private $tokenCookieSecure = false;
+    private string $tokenCookieSameSite = Cookie::SAMESITE_STRICT;


In https://docs.silverstripe.org/en/6/developer_guides/security/secure_coding/#secure-sessions-and-cookies when talking about updating the session cookie security it says

The same treatment should be applied to the cookie responsible for remembering logins across sessions

For this reason I've chosen to do exactly that - I've updated its default secure and samesite values to match the session cookie values.

GuySartorelli · 2025-03-11T00:24:05Z

src/Security/MemberAuthenticator/CookieAuthenticationHandler.php

-    public function getTokenCookieSecure()
+    public function getTokenCookieSecure(): bool
    {
+        if ($this->getTokenCookieSameSite() === Cookie::SAMESITE_NONE) {
+            return true;
+        }
        return $this->tokenCookieSecure;
    }


Matches how "secure" is set for session cookies

GuySartorelli · 2025-03-11T00:24:28Z

src/Control/Cookie.php

-    public const SAMESITE_LAX = 'Lax';
-
    public const SAMESITE_STRICT = 'Strict';

+    public const SAMESITE_LAX = 'Lax';
+
    public const SAMESITE_NONE = 'None';


Updated the order so it's most to least secure

GuySartorelli · 2025-03-11T00:24:41Z

src/Control/Cookie.php

-    /**
-     * @config
-     *
-     * @var bool
-     */
-    private static $report_errors = true;
+    private static bool $report_errors = true;


A fair amount of unrelated strict typing being added in this PR

GuySartorelli · 2025-03-11T00:26:16Z

src/Control/Cookie.php

+        string $name,
+        string|false $value,
+        int $expiry = 90,
+        ?string $path = null,
+        ?string $domain = null,
+        bool $secure = false,
+        bool $httpOnly = true,
+        string $sameSite = ''


$value can only be a string or false if you trace through the calls in the actual implementation.

$expiry can only be int.

Chose to keep the nullability of $path and $domain to reduce the change of unexpected side effects and regressions given we're so close to the beta.

These types are shared with CookieJar and Cookie_Backend as well.

GuySartorelli · 2025-03-11T00:34:43Z

src/Control/Cookie.php

+    public static function force_expiry(
+        string $name,
+        ?string $path = null,
+        ?string $domain = null,
+        bool $secure = false,
+        bool $httpOnly = true,
+        string $sameSite = ''
+    ): void {
+        Cookie::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly, $sameSite);
    }


According to https://www.php.net/manual/en/function.setcookie.php#refsect1-function.setcookie-notes

Cookies must be deleted with the same parameters as they were set with.

So it seems like we need to allow setting samesite here.

GuySartorelli · 2025-03-11T00:35:13Z

src/Control/CookieJar.php

-     * When creating the backend we want to store the existing cookies in our
-     * "existing" array. This allows us to distinguish between cookies we received
-     * or we set ourselves (and didn't get from the browser)
-     *
-     * @param array $cookies The existing cookies to load into the cookie jar.
-     * Omit this to default to $_COOKIE
+     * @inheritDoc


Removed a lot of duplicate PHPDoc from this class

GuySartorelli · 2025-03-11T00:35:44Z

src/Control/Session.php

-        $sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX;
+        $sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_STRICT;


This and any other changes from lax to strict in this class are necessary as part of #11597

Should move this logic to a new method Session::getCookieSamesite()

It's only used in one place so that seems excessive - but done anyway. Private method so we can just axe it later if we want.

GuySartorelli · 2025-03-11T00:36:18Z

src/Control/Session.php

-                $path = $this->config()->get('cookie_path') ?: Director::baseURL();
-                $domain = $this->config()->get('cookie_domain');
-                $secure = Director::is_https($request) && $this->config()->get('cookie_secure');
-                Cookie::force_expiry(session_name(), $path, $domain, $secure, true);
+                $cookieParams = $this->buildCookieParams($request);
+                Cookie::force_expiry(
+                    session_name(),
+                    $cookieParams['path'],
+                    $cookieParams['domain'],
+                    $cookieParams['secure'],
+                    true,
+                    $cookieParams['samesite']
+                );


No sense rebuilding the params - updated this to use buildCookieParams()

GuySartorelli · 2025-03-11T00:37:10Z

src/Security/Member.php

@@ -220,6 +220,7 @@ class Member extends DataObject
     * @config
     * @var String If this is set, then a session cookie with the given name will be set on log-in,
     * and cleared on logout.
+     * This cookie shares the same SameSite and Secure parameters as the main session cookie.


See SessionAuthenticationHandler

GuySartorelli · 2025-03-11T00:42:08Z

Based on the existing tests it doesn't look like there's a sensible way to test this new parameter is actually coming through when cookies get set.

emteknetnz · 2025-03-12T02:12:47Z

src/Control/CookieJar.php

-     * @param string $path The path to save the cookie on (falls back to site base)
-     * @param string $domain The domain to make the cookie available on
-     * @param boolean $secure Can the cookie only be sent over SSL?
-     * @param boolean $httpOnly Prevent the cookie being accessible by JS


Shouldn't be deleting docblock params when they're explaining something - e.g. $expiry and $path are non obvious.

This applies other instances of deleting docblock params in this PR

Note the @inheritdoc immediately below the text you selected. These are duplicate PHPDocs as mentioned in #11632 (comment)

emteknetnz · 2025-03-12T02:16:37Z

src/Control/CookieJar.php

+        string $sameSite = ''
+    ): void {
+        if ($sameSite === '') {
+            $sameSite = Cookie::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX;


This should logic should be on a new method Cookie::getDefaultSameSite()

emteknetnz · 2025-03-12T02:22:17Z

src/Control/CookieJar.php

+        string $sameSite = ''
+    ): bool {
+        if ($sameSite === '') {
+            $sameSite = Cookie::config()->get('default_samesite') ?? Cookie::SAMESITE_LAX;


This should logic should be on a new method Cookie::getDefaultSameSite()

emteknetnz · 2025-03-12T02:26:07Z

src/Control/Session.php

-        $sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX;
+        $sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_STRICT;


Should move this logic to a new method Session::getCookieSamesite()

emteknetnz · 2025-03-12T02:36:37Z

src/Security/MemberAuthenticator/CookieAuthenticationHandler.php

    {
-        return $this->deviceCookieName;
+        return $this->deviceCookieName ?? '';


Suggested change

return $this->deviceCookieName ?? '';

return $this->deviceCookieName;

Won't PHP complain about trying to access an un-initilised value?

Seems like it would be better to just give the property a default value of blank string

No, it only does that for properties which are explicitly typed. But I guess I can make it explicitly string.

emteknetnz · 2025-03-12T02:36:58Z

src/Security/MemberAuthenticator/CookieAuthenticationHandler.php

    {
-        return $this->tokenCookieName;
+        return $this->tokenCookieName ?? '';


Suggested change

return $this->tokenCookieName ?? '';

return $this->tokenCookieName;

Same as getDeviceCookieName() above

emteknetnz · 2025-03-12T02:58:14Z

src/Control/Cookie_Backend.php

+     * @param ?string $path The path to save the cookie on (falls back to site base)
+     * @param ?string $domain The domain to make the cookie available on


Suggested change

* @param ?string $path The path to save the cookie on (falls back to site base)

* @param ?string $domain The domain to make the cookie available on

* @param null|string $path The path to save the cookie on (falls back to site base)

* @param null|string $domain The domain to make the cookie available on

Don't think I've seen ? in a docblock param before, should be consistent

GuySartorelli · 2025-03-12T04:05:19Z

CI failures are pre-existing and will be resolved in #11635

Also update secure-ness of authentication cookies to match session cookies, and finish securing session cookies.

GuySartorelli mentioned this pull request Mar 11, 2025

Add parameters to set samesite for any arbitrary cookie #10342

Open

3 tasks

GuySartorelli commented Mar 11, 2025

View reviewed changes

GuySartorelli force-pushed the pulls/6.0/cookie-samesite branch 2 times, most recently from bc01ee8 to 9ad275d Compare March 11, 2025 01:02

emteknetnz requested changes Mar 12, 2025

View reviewed changes

GuySartorelli force-pushed the pulls/6.0/cookie-samesite branch from 9ad275d to f200906 Compare March 12, 2025 03:57

NEW Allow setting "samesite" attribute for individual cookies.

33a6a13

Also update secure-ness of authentication cookies to match session cookies, and finish securing session cookies.

GuySartorelli force-pushed the pulls/6.0/cookie-samesite branch from f200906 to 33a6a13 Compare March 12, 2025 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NEW Allow setting "samesite" attribute for individual cookies. #11632

NEW Allow setting "samesite" attribute for individual cookies. #11632

GuySartorelli commented Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

emteknetnz Mar 12, 2025

GuySartorelli Mar 12, 2025

GuySartorelli Mar 11, 2025

GuySartorelli Mar 11, 2025

GuySartorelli commented Mar 11, 2025

emteknetnz Mar 12, 2025

GuySartorelli Mar 12, 2025

emteknetnz Mar 12, 2025

GuySartorelli Mar 12, 2025

emteknetnz Mar 12, 2025

GuySartorelli Mar 12, 2025

emteknetnz Mar 12, 2025

emteknetnz Mar 12, 2025

GuySartorelli Mar 12, 2025

GuySartorelli Mar 12, 2025

emteknetnz Mar 12, 2025

emteknetnz Mar 12, 2025

GuySartorelli Mar 12, 2025

GuySartorelli commented Mar 12, 2025

		$sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_LAX;
		$sameSite = static::config()->get('cookie_samesite') ?? Cookie::SAMESITE_STRICT;

	return $this->deviceCookieName ?? '';
	return $this->deviceCookieName;

	return $this->tokenCookieName ?? '';
	return $this->tokenCookieName;

		* @param ?string $path The path to save the cookie on (falls back to site base)
		* @param ?string $domain The domain to make the cookie available on

NEW Allow setting "samesite" attribute for individual cookies. #11632

Are you sure you want to change the base?

NEW Allow setting "samesite" attribute for individual cookies. #11632

Conversation

GuySartorelli commented Mar 11, 2025

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Mar 12, 2025