NEW Allow setting "samesite" attribute for individual cookies. by GuySartorelli · Pull Request #11632 · silverstripe/silverstripe-framework

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

+    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

     */
-    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


-    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

-     * @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

+        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

+    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

-     *
-     * @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

-        $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

-                $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

     * @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

-     * @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

+        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

+        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

-        $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

+    public function getDeviceCookieName(): string
    {
-        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

+    public function getTokenCookieName(): string
    {
-        return $this->tokenCookieName;
+        return $this->tokenCookieName ?? '';


Suggested change

return $this->tokenCookieName ?? '';

return $this->tokenCookieName;

Same as getDeviceCookieName() above

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

+     * @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

Closed

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

emteknetnz approved these changes Mar 13, 2025

View reviewed changes

emteknetnz merged commit a8e6cb4 into silverstripe:6.0 Mar 13, 2025
9 of 12 checks passed

emteknetnz deleted the pulls/6.0/cookie-samesite branch March 13, 2025 21:20

		$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

Conversation

GuySartorelli commented Mar 11, 2025

Issue

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuySartorelli commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuySartorelli commented Mar 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants