Repository: twisted/twisted

Issue:
twisted.web.http.Request.addCookie does not support SameSite=None
|[<img alt="richvdh's avatar" src="https://avatars.githubusercontent.com/u/1389908?s=50" width="50" height="50">](https://github.com/richvdh)| @richvdh reported|
|-|-|
|Trac ID|trac#10088|
|Type|defect|
|Created|2021-02-10 13:12:26Z|

It looks like the support for `SameSite` on `Set-Cookie` headers, which was added by https://github.com/twisted/twisted/pull/997, was based on a draft of the RFC, https://tools.ietf.org/html/draft-west-first-party-cookies-07, and only accepts the values `lax` or `strict`.

This seems to have been superceded by https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-07, which adds a `None` option, and, as described on https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite, most browsers now default to `Lax` rather than `None`, so it's necessary to explicitly set it to `None` if you want that behaviour.

In short, it would be good to allow `"none"` here.

<details><summary>Searchable metadata</summary>

```
trac-id__10088 10088
type__defect defect
reporter__richvdh richvdh
priority__normal normal
milestone__None None
branch__ 
branch_author__ 
status__new new
resolution__None None
component__web web
keywords__None None
time__1612962746474365 1612962746474365
changetime__1620472010362606 1620472010362606
version__None None
owner__None None

```
</details>


Discussion:

Patch:
diff --git a/src/twisted/web/http.py b/src/twisted/web/http.py
index 3575a934fb..5c4850fa96 100644
--- a/src/twisted/web/http.py
+++ b/src/twisted/web/http.py
@@ -1378,10 +1378,9 @@ class Request:
             other than HTTP (and HTTPS) requests
         @type httpOnly: L{bool}
 
-        @param sameSite: One of L{None} (default), C{'lax'} or C{'strict'}.
-            Direct browsers not to send this cookie on cross-origin requests.
-            Please see:
-            U{https://tools.ietf.org/html/draft-west-first-party-cookies-07}
+        @param sameSite: One of L{None} (default), C{'lax'}, C{'none'} or C{'strict'}.
+        Direct browsers not to send this cookie on cross-origin requests.
+        See: U{https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value}
         @type sameSite: L{None}, L{bytes} or L{str}
 
         @raise ValueError: If the value for C{sameSite} is not supported.
@@ -1432,7 +1431,15 @@ class Request:
             cookie = cookie + b"; HttpOnly"
         if sameSite:
             sameSite = _ensureBytes(sameSite).lower()
-            if sameSite not in [b"lax", b"strict"]:
+            # See more info about sameSite usage here
+            # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#samesitesamesite-value
+            if not secure and sameSite == b"none":
+                raise ValueError(
+                    "Invalid value for sameSite: "
+                    + repr(sameSite)
+                    + '. Missing the "secure" attribute'
+                )
+            if sameSite not in [b"lax", b"strict", b"none"]:
                 raise ValueError("Invalid value for sameSite: " + repr(sameSite))
             cookie += b"; SameSite=" + sameSite
         self.cookies.append(cookie)
diff --git a/src/twisted/web/newsfragments/10088.bugfix b/src/twisted/web/newsfragments/10088.bugfix
new file mode 100644
index 0000000000..152bb485bc
--- /dev/null
+++ b/src/twisted/web/newsfragments/10088.bugfix
@@ -0,0 +1,1 @@
+twisted.web.http.Request.addCookie now supports the `none` value for cookie's `samesite` attribute.


Test patch:
diff --git a/src/twisted/web/test/test_http.py b/src/twisted/web/test/test_http.py
index a27c9ebfb3..3b4460c124 100644
--- a/src/twisted/web/test/test_http.py
+++ b/src/twisted/web/test/test_http.py
@@ -3428,6 +3428,7 @@ class RequestTests(unittest.TestCase, ResponseTestMixin):
     def test_addCookieSameSite(self):
         """
         L{http.Request.setCookie} supports a C{sameSite} argument.
+        It will set the cookie with samesite=lax and samesite=strict values.
         """
         self._checkCookie(b"foo=bar; SameSite=lax", b"foo", b"bar", sameSite="lax")
         self._checkCookie(b"foo=bar; SameSite=lax", b"foo", b"bar", sameSite="Lax")
@@ -3435,9 +3436,66 @@ class RequestTests(unittest.TestCase, ResponseTestMixin):
             b"foo=bar; SameSite=strict", b"foo", b"bar", sameSite="strict"
         )
 
-        self.assertRaises(
+    def test_addCookieSameSiteNoneSecure(self):
+        """
+        L{http.Request.setCookie} supports the C{sameSite} argument together with secure.
+        It will set the cookie with samesite=none and secure attributes to be sure that
+        all values of samesite are supported. secure attribute is a necessary condition
+        if the samesite is none
+        """
+        self._checkCookie(
+            b"foo=bar; Secure; SameSite=none",
+            b"foo",
+            b"bar",
+            sameSite="None",
+            secure=True,
+        )
+        self._checkCookie(
+            b"foo=bar; Secure; SameSite=none",
+            b"foo",
+            b"bar",
+            sameSite="none",
+            secure=True,
+        )
+        self._checkCookie(
+            b"foo=bar; Secure; SameSite=none",
+            b"foo",
+            b"bar",
+            sameSite=b"none",
+            secure=True,
+        )
+
+    def test_addCookieWrongValues(self):
+        """
+        Raises an exception when setting the cookie with not supported samesite value and without a necessary
+        secure attribute for the samesite=none cookie.
+        """
+        error = self.assertRaises(
             ValueError, self._checkCookie, b"", b"foo", b"bar", sameSite="anything-else"
         )
+        self.assertEqual("Invalid value for sameSite: b'anything-else'", error.args[0])
+
+        error = self.assertRaises(
+            ValueError,
+            self._checkCookie,
+            b"",
+            b"foo",
+            b"bar",
+            sameSite="none",
+            secure=False,
+        )
+        self.assertEqual(
+            "Invalid value for sameSite: b'none'. Missing the \"secure\" attribute",
+            error.args[0],
+        )
+
+        error = self.assertRaises(
+            ValueError, self._checkCookie, b"", b"foo", b"bar", sameSite="none"
+        )
+        self.assertEqual(
+            "Invalid value for sameSite: b'none'. Missing the \"secure\" attribute",
+            error.args[0],
+        )
 
     def test_firstWrite(self):
         """