ENH Add samesite attribute to cookies.

Co-authored-by: pine3ree <[email protected]>

Author: GuySartorelli

docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md

@@ -762,11 +762,15 @@ disable this behaviour using `CanonicalURLMiddleware::singleton()->setForceBasic
 configuration in YAML.
 
 We also want to ensure cookies are not shared between secure and non-secure sessions, so we must tell Silverstripe CMS to 
-use a [secure session](https://docs.silverstripe.org/en/3/developer_guides/cookies_and_sessions/sessions/#secure-session-cookie). 
-To do this, you may set the `cookie_secure` parameter to `true` in your `config.yml` for `Session`
+use a [secure session](/developer_guides/cookies_and_sessions/sessions/#secure-session-cookie).
+To do this, you may set the `cookie_secure` parameter to `true` in your `config.yml` for `Session`.
+
+It is also a good idea to set the `samesite` attribute for the session cookie to `Strict` unless you have a specific use case for
+sharing the session cookie across domains.
 
 ```yml
 SilverStripe\Control\Session:
+  cookie_samesite: 'Strict'
   cookie_secure: true
 ```
 
@@ -784,6 +788,11 @@ SilverStripe\Core\Injector\Injector:
       TokenCookieSecure: true
 ```
 
+[info]
+There is not currently an easy way to pass a `samesite` attribute value for setting this cookie - but you can set the
+default value for the attribute for all cookies. See [the main cookies documentation](/developer_guides/cookies_and_sessions/cookies#samesite-attribute) for more information.
+[/info]
+
 For other cookies set by your application we should also ensure the users are provided with secure cookies by setting 
 the "Secure" and "HTTPOnly" flags. These flags prevent them from being stolen by an attacker through javascript. 
 

docs/en/02_Developer_Guides/18_Cookies_And_Sessions/01_Cookies.md

@@ -5,6 +5,10 @@ icon: cookie-bite
 ---
 
 # Cookies
+
+Note that cookies can have security implications - before setting your own cookies, make sure to read through the
+[secure coding](/developer_guides/security/secure_coding#secure-sessions-cookies-and-tls-https) documentation.
+
 ## Accessing and Manipulating Cookies
 
 Cookies are a mechanism for storing data in the remote browser and thus tracking or identifying return users. 
@@ -52,6 +56,20 @@ Cookie::force_expiry($name, $path = null, $domain = null);
 // Cookie::force_expiry('MyApplicationPreference')
 ```
 
+### Samesite attribute
+
+The `samesite` attribute is set on all cookies with a default value of `Lax`. You can change the default value by setting the `default_samesite` value on the
+[Cookie](api:SilverStripe\Control\Cookie) class:
+
+```yml
+SilverStripe\Control\Cookie:
+  default_samesite: 'Strict'
+```
+
+[info]
+Note that this _doesn't_ apply for the session cookie, which is handled separately. See [Sessions](/developer_guides/cookies_and_sessions/sessions#samesite-attribute).
+[/info]
+
 ## Cookie_Backend
 
 The [Cookie](api:SilverStripe\Control\Cookie) class manipulates and sets cookies using a [Cookie_Backend](api:SilverStripe\Control\Cookie_Backend). The backend is in charge of the logic

docs/en/02_Developer_Guides/18_Cookies_And_Sessions/02_Sessions.md

@@ -101,7 +101,19 @@ including form and page comment information. None of this is vital but `clear_al
 $session->clearAll();
 ```
 
-## Secure Session Cookie
+## Cookies
+
+### Samesite attribute
+
+The session cookie is handled slightly differently than most cookies on the site, which provides the opportunity to handle the samesite attribute separately from other cookies.
+You can change the `samesite` attribute for session cookies like so:
+
+```yml
+SilverStripe\Control\Session:
+  cookie_samesite: 'Strict'
+```
+
+### Secure Session Cookie
 
 In certain circumstances, you may want to use a different `session_name` cookie when using the `https` protocol for security purposes. To do this, you may set the `cookie_secure` parameter to `true` on your `config.yml`
 
@@ -113,6 +125,8 @@ SilverStripe\Control\Session:
 
 This uses the session_name `SECSESSID` for `https` connections instead of the default `PHPSESSID`. Doing so adds an extra layer of security to your session cookie since you no longer share `http` and `https` sessions.
 
+Note that if you set `cookie_samesite` to `None` (which is _strongly_ discouraged), the `cookie_secure` value will _always_ be `true`.
+
 ## Relaxing checks around user agent strings
 
 Out of the box, Silverstripe CMS will invalidate a user's session if the `User-Agent` header changes. This provides some supplemental protection against session high-jacking attacks.

docs/en/04_Changelogs/4.12.0.md

@@ -0,0 +1,55 @@
+---
+title: 4.12.0 (unreleased)
+---
+
+# 4.12.0 (unreleased)
+
+## Overview
+
+- [Regression test and Security audit](#audit)
+- [Features and enhancements](#features-and-enhancements)
+  - [Samesite attribute on cookies](#cookies-samesite)
+  - [Other features](#other-features)
+- [Bugfixes](#bugfixes)
+
+## Regression test and Security audit{#audit}
+
+This release has been comprehensively regression tested and passed to a third party for a security-focused audit.
+
+While it is still advised that you perform your own due diligence when upgrading your project, this work is performed to ensure a safe and secure upgrade with each recipe release.
+
+## Features and enhancements {#features-and-enhancements}
+
+### Samesite attribute on cookies {#cookies-samesite}
+
+The `samesite` attribute is now set on all cookies. To avoid backward compatability issues, the `Lax` value has been set by default, but we recommend reviewing the requirements of your project and setting an appropriate value.
+
+The default value can be set for all cookies (except for the session cookie) in yaml configuration like so:
+
+```yml
+SilverStripe\Control\Cookie:
+  default_samesite: 'Strict'
+```
+
+Check out the [cookies documentation](/developer_guides/cookies_and_sessions/cookies#samesite-attribute) for more information.
+
+The session cookie is handled separately. You can configure it like so:
+
+```yml
+SilverStripe\Control\Session:
+  cookie_samesite: 'Strict'
+```
+
+Note that if you set the `samesite` attribute to `None`, the `secure` is automatically set to `true` as required by the specification.
+
+For more information about the `samesite` attribute check out https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite
+
+### Other new features {#other-features}
+
+## Bugfixes {#bugfixes}
+
+This release includes a number of bug fixes to improve a broad range of areas. Check the change logs for full details of these fixes split by module. Thank you to the community members that helped contribute these fixes as part of the release!
+
+<!--- Changes below this line will be automatically regenerated -->
+
+<!--- Changes above this line will be automatically regenerated -->

src/Control/Cookie.php

@@ -2,6 +2,8 @@
 
 namespace SilverStripe\Control;
 
+use LogicException;
+use Psr\Log\LoggerInterface;
 use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Injector\Injector;
 
@@ -19,6 +21,12 @@ class Cookie
      */
     private static $report_errors = true;
 
+    /**
+     * Must be "Strict", "Lax", or "None"
+     * @config
+     */
+    private static string $default_samesite = 'Lax';
+
     /**
      * Fetch the current instance of the cookie backend.
      *
@@ -92,4 +100,37 @@ public static function force_expiry($name, $path = null, $domain = null, $secure
     {
         return self::get_inst()->forceExpiry($name, $path, $domain, $secure, $httpOnly);
     }
+
+    /**
+     * Validate if the samesite value for a cookie is valid for the current request.
+     *
+     * Logs a warning if the samesite value is "None" for an insecure request.
+     * @throws LogicException if the value is not "Strict", "Lax", or "None".
+     */
+    public static function validateSameSite(string $sameSite): void
+    {
+        $validValues = [
+            'Strict',
+            'Lax',
+            'None',
+        ];
+        if (!in_array($sameSite, $validValues)) {
+            throw new LogicException('samesite must be "Strict", "Lax", or "None"');
+        }
+        if ($sameSite === 'None' && !Director::is_https(self::getRequest())) {
+            Injector::inst()->get(LoggerInterface::class)->warning('Cookie samesite cannot be "None" for insecure requests.');
+        }
+    }
+
+    /**
+     * Get the current request, if any.
+     */
+    private static function getRequest(): ?HTTPRequest
+    {
+        $request = null;
+        if (Controller::has_curr()) {
+            $request = Controller::curr()->getRequest();
+        }
+        return ($request instanceof NullHTTPRequest) ? null : $request;
+    }
 }

src/Control/CookieJar.php

@@ -18,7 +18,6 @@
  */
 class CookieJar implements Cookie_Backend
 {
-
     /**
      * Hold the cookies that were existing at time of instantiation (ie: The ones
      * sent to PHP by the browser)
@@ -168,9 +167,18 @@ protected function outputCookie(
         $secure = false,
         $httpOnly = true
     ) {
+        $sameSite = $this->getSameSite($name);
+        Cookie::validateSameSite($sameSite);
         // if headers aren't sent, we can set the cookie
         if (!headers_sent($file, $line)) {
-            return setcookie($name ?? '', $value ?? '', $expiry ?? 0, $path ?? '', $domain ?? '', $secure ?? false, $httpOnly ?? false);
+            return setcookie($name ?? '', $value ?? '', [
+                'expires' => $expiry ?? 0,
+                'path' => $path ?? '',
+                'domain' => $domain ?? '',
+                'secure' => $this->cookieIsSecure($sameSite, $secure),
+                'httponly' => $httpOnly ?? false,
+                'samesite' => $sameSite,
+            ]);
         }
 
         if (Cookie::config()->uninherited('report_errors')) {
@@ -180,4 +188,25 @@ protected function outputCookie(
         }
         return false;
     }
+
+    /**
+     * Cookies must be secure if samesite is "None"
+     */
+    private function cookieIsSecure(string $sameSite, $secure): bool
+    {
+        return $sameSite === 'None' ? true : (bool) $secure;
+    }
+
+    /**
+     * Get the correct samesite value - Session cookies use a different configuration variable.
+     *
+     * @deprecated 5.0 The relevant methods will include a `$sameSite` parameter instead.
+     */
+    private function getSameSite(string $name): string
+    {
+        if ($name === session_name()) {
+            return Session::config()->get('cookie_samesite');
+        }
+        return Cookie::config()->get('default_samesite');
+    }
 }

src/Control/Session.php

@@ -135,6 +135,12 @@ class Session
      */
     private static $cookie_name_secure = 'SECSESSID';
 
+    /**
+     * Must be "Strict", "Lax", or "None".
+     * @config
+     */
+    private static string $cookie_samesite = 'Lax';
+
     /**
      * Name of session cache limiter to use.
      * Defaults to '' to disable cache limiter entirely.
@@ -283,31 +289,15 @@ public function start(HTTPRequest $request)
             throw new BadMethodCallException("Session has already started");
         }
 
-        $path = $this->config()->get('cookie_path');
-        if (!$path) {
-            $path = Director::baseURL();
-        }
-        $domain = $this->config()->get('cookie_domain');
-        $secure = Director::is_https($request) && $this->config()->get('cookie_secure');
         $session_path = $this->config()->get('session_store_path');
-        $timeout = $this->config()->get('timeout');
-
-        // Director::baseURL can return absolute domain names - this extracts the relevant parts
-        // for the session otherwise we can get broken session cookies
-        if (Director::is_absolute_url($path)) {
-            $urlParts = parse_url($path ?? '');
-            $path = $urlParts['path'];
-            if (!$domain) {
-                $domain = $urlParts['host'];
-            }
-        }
 
         // If the session cookie is already set, then the session can be read even if headers_sent() = true
         // This helps with edge-case such as debugging.
         $data = [];
         if (!session_id() && (!headers_sent() || $this->requestContainsSessionId($request))) {
             if (!headers_sent()) {
-                session_set_cookie_params($timeout ?: 0, $path, $domain ?: null, $secure, true);
+                $cookieParams = $this->buildCookieParams($request);
+                session_set_cookie_params($cookieParams);
 
                 $limiter = $this->config()->get('sessionCacheLimiter');
                 if (isset($limiter)) {
@@ -323,16 +313,24 @@ public function start(HTTPRequest $request)
                 // separate (less secure) session for non-HTTPS requests
                 // if headers_sent() is true then it's best to throw the resulting error rather than risk
                 // a security hole.
-                if ($secure) {
+                if ($cookieParams['secure']) {
                     session_name($this->config()->get('cookie_name_secure'));
                 }
 
                 session_start();
 
                 // Session start emits a cookie, but only if there's no existing session. If there is a session timeout
                 // tied to this request, make sure the session is held for the entire timeout by refreshing the cookie age.
-                if ($timeout && $this->requestContainsSessionId($request)) {
-                    Cookie::set(session_name(), session_id(), $timeout / 86400, $path, $domain ?: null, $secure, true);
+                if ($cookieParams['lifetime'] && $this->requestContainsSessionId($request)) {
+                    Cookie::set(
+                        session_name(),
+                        session_id(),
+                        $cookieParams['lifetime'] / 86400,
+                        $cookieParams['path'],
+                        $cookieParams['domain'] ?: null,
+                        $cookieParams['secure'],
+                        true
+                    );
                 }
             } else {
                 // If headers are sent then we can't have a session_cache_limiter otherwise we'll get a warning
@@ -354,6 +352,53 @@ public function start(HTTPRequest $request)
         $this->started = true;
     }
 
+    /**
+     * Build the parameters used for setting the session cookie.
+     */
+    private function buildCookieParams(HTTPRequest $request): array
+    {
+        $timeout = $this->config()->get('timeout');
+        $path = $this->config()->get('cookie_path');
+        $domain = $this->config()->get('cookie_domain');
+        if (!$path) {
+            $path = Director::baseURL();
+        }
+
+        // Director::baseURL can return absolute domain names - this extracts the relevant parts
+        // for the session otherwise we can get broken session cookies
+        if (Director::is_absolute_url($path)) {
+            $urlParts = parse_url($path ?? '');
+            $path = $urlParts['path'];
+            if (!$domain) {
+                $domain = $urlParts['host'];
+            }
+        }
+
+        $sameSite = static::config()->get('cookie_samesite');
+        Cookie::validateSameSite($sameSite);
+        $secure = $this->isCookieSecure($sameSite, Director::is_https($request));
+
+        return [
+            'lifetime' => $timeout ?: 0,
+            'path' => $path,
+            'domain' => $domain ?: null,
+            'secure' => $secure,
+            'httponly' => true,
+            'samesite' => $sameSite,
+        ];
+    }
+
+    /**
+     * Determines what the value for the `secure` cookie attribute should be.
+     */
+    private function isCookieSecure(string $sameSite, bool $isHttps): bool
+    {
+        if ($sameSite === 'None') {
+            return true;
+        }
+        return $isHttps && $this->config()->get('cookie_secure');
+    }
+
     /**
      * Destroy this session
      *