Merge pull request isso-comments#700 from ix5/secure-cookie

Improve cookie SameSite/secure handling

Author: blatinier

CHANGES.rst

@@ -8,6 +8,10 @@ Changelog for Isso
 
 - Don't ignore missing configuration files.
   (Jelmer Vernooij)
+- New "samesite" option in [server] section to override SameSite header for
+  cookies. (#700, ix5)
+- Fallback for SameSite header depending on whether host is served over https
+  or http (#700, ix5)
 
 0.12.4 (2021-02-03)
 -------------------

docs/docs/configuration/server.rst

@@ -202,6 +202,17 @@ trusted-proxies
     `X-Forwarded-For` HTTP header, which is important for the mechanism
     forbiding several comment votes coming from the same subnet.
 
+samesite
+    override ``Set-Cookie`` header ``SameSite`` value.
+    Needed for setups where isso is not hosted on the same domain, e.g. called
+    from example.org and hosted under comments.example.org.
+    By default, isso will set ``SameSite=None`` when served over https and
+    ``SameSite=Lax`` when served over http
+    (see `MDM: SameSite <https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite>`_
+    and `#682 <https://github.com/posativ/isso/issues/682>`_ for details).
+
+    Accepted values: ``None``, ``Lax``, ``Strict``
+
 .. _configure-smtp:
 
 SMTP

isso/tests/fixtures.py

@@ -16,6 +16,19 @@ def __call__(self, environ, start_response):
         return self.app(environ, start_response)
 
 
+class FakeHost(object):
+
+    def __init__(self, app, host, scheme):
+        self.app = app
+        self.host = host
+        self.scheme = scheme
+
+    def __call__(self, environ, start_response):
+        environ['HTTP_HOST'] = self.host
+        environ['wsgi.url_scheme'] = self.scheme
+        return self.app(environ, start_response)
+
+
 class JSONClient(Client):
 
     def open(self, *args, **kwargs):

isso/tests/test_comments.py

@@ -17,7 +17,7 @@
 from isso.utils import http
 from isso.views import comments
 
-from fixtures import curl, loads, FakeIP, JSONClient
+from fixtures import curl, loads, FakeIP, FakeHost, JSONClient
 http.curl = curl
 
 
@@ -495,6 +495,70 @@ def testLatestNotEnabled(self):
         self.assertEqual(response.status_code, 404)
 
 
+class TestHostDependent(unittest.TestCase):
+
+    def setUp(self):
+        fd, self.path = tempfile.mkstemp()
+        conf = config.load(os.path.join(dist.location, dist.project_name, "defaults.ini"))
+        conf.set("general", "dbpath", self.path)
+        self.conf = conf
+
+        class App(Isso, core.Mixin):
+            pass
+
+        self.app = App(conf)
+
+        self.client = JSONClient(self.app, Response)
+        self.post = self.client.post
+
+    def tearDown(self):
+        os.unlink(self.path)
+
+    def testSecureCookieNoConf(self):
+        self.app.wsgi_app = FakeHost(self.app.wsgi_app, "isso-dev.local", "https")
+        rv = self.post('/new?uri=%2Fpath%2F',
+                       data=json.dumps({'text': 'Lorem ipsum ...'}))
+
+        self.assertIn("Secure", rv.headers["Set-Cookie"])
+        self.assertIn("Secure", rv.headers["X-Set-Cookie"])
+        self.assertIn("SameSite=None", rv.headers["Set-Cookie"])
+
+    def testInSecureCookieNoConf(self):
+        self.app.wsgi_app = FakeHost(self.app.wsgi_app, "isso-dev.local", "http")
+        rv = self.post('/new?uri=%2Fpath%2F',
+                       data=json.dumps({'text': 'Lorem ipsum ...'}))
+
+        self.assertNotIn("Secure", rv.headers["Set-Cookie"])
+        self.assertNotIn("Secure", rv.headers["X-Set-Cookie"])
+        self.assertIn("SameSite=Lax", rv.headers["Set-Cookie"])
+
+    def testSameSiteConfNone(self):
+        # By default, isso should set SameSite=Lax when served over http
+        self.app.wsgi_app = FakeHost(self.app.wsgi_app, "isso-dev.local", "http")
+        # Conf overrides SameSite setting
+        self.conf.set("server", "samesite", "None")
+
+        rv = self.post('/new?uri=%2Fpath%2F',
+                       data=json.dumps({'text': 'Lorem ipsum ...'}))
+
+        self.assertNotIn("Secure", rv.headers["Set-Cookie"])
+        self.assertNotIn("Secure", rv.headers["X-Set-Cookie"])
+        self.assertIn("SameSite=None", rv.headers["Set-Cookie"])
+
+    def testSameSiteConfLax(self):
+        # By default, isso should set SameSite=None when served over https
+        self.app.wsgi_app = FakeHost(self.app.wsgi_app, "isso-dev.local", "https")
+        # Conf overrides SameSite setting
+        self.conf.set("server", "samesite", "Lax")
+
+        rv = self.post('/new?uri=%2Fpath%2F',
+                       data=json.dumps({'text': 'Lorem ipsum ...'}))
+
+        self.assertIn("Secure", rv.headers["Set-Cookie"])
+        self.assertIn("Secure", rv.headers["X-Set-Cookie"])
+        self.assertIn("SameSite=Lax", rv.headers["Set-Cookie"])
+
+
 class TestModeratedComments(unittest.TestCase):
 
     def setUp(self):

isso/views/comments.py

@@ -309,10 +309,9 @@ def new(self, environ, request, uri):
         # notify extension, that the new comment has been successfully saved
         self.signal("comments.new:after-save", thread, rv)
 
-        cookie = functools.partial(dump_cookie,
-                                   value=self.isso.sign(
-                                       [rv["id"], sha1(rv["text"])]),
-                                   max_age=self.conf.getint('max-age'))
+        cookie = self.create_cookie(
+            value=self.isso.sign([rv["id"], sha1(rv["text"])]),
+            max_age=self.conf.getint('max-age'))
 
         rv["text"] = self.isso.render(rv["text"])
         rv["hash"] = self.hash(rv['email'] or rv['remote_addr'])
@@ -348,6 +347,24 @@ def _remote_addr(self, request):
                                 if addr not in self.trusted_proxies), remote_addr)
         return utils.anonymize(str(remote_addr))
 
+    def create_cookie(self, **kwargs):
+        """
+        Setting cookies to SameSite=None requires "Secure" attribute.
+        For http-only, we need to override the dump_cookie() default SameSite=None
+        or the cookie will be rejected.
+        https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite#samesitenone_requires_secure
+        """
+        isso_host_script = self.isso.conf.get("server", "public-endpoint") or local.host
+        samesite = self.isso.conf.get("server", "samesite")
+        if isso_host_script.startswith("https://"):
+            secure = True
+            samesite = samesite or "None"
+        else:
+            secure = False
+            samesite = samesite or "Lax"
+        return functools.partial(dump_cookie, **kwargs,
+                                 secure=secure, samesite=samesite)
+
     """
     @api {get} /id/:id view
     @apiGroup Comment
@@ -458,10 +475,9 @@ def edit(self, environ, request, id):
 
         self.signal("comments.edit", rv)
 
-        cookie = functools.partial(dump_cookie,
-                                   value=self.isso.sign(
-                                       [rv["id"], sha1(rv["text"])]),
-                                   max_age=self.conf.getint('max-age'))
+        cookie = self.create_cookie(
+            value=self.isso.sign([rv["id"], sha1(rv["text"])]),
+            max_age=self.conf.getint('max-age'))
 
         rv["text"] = self.isso.render(rv["text"])
 
@@ -474,7 +490,7 @@ def edit(self, environ, request, id):
     @api {delete} '/id/:id' delete
     @apiGroup Comment
     @apiDescription
-        Delte an existing comment. Deleting a comment is only possible for a short period of time after it was created and only if the requestor has a valid cookie for it. See the [isso server documentation](https://posativ.org/isso/docs/configuration/server) for details.
+        Delete an existing comment. Deleting a comment is only possible for a short period of time after it was created and only if the requestor has a valid cookie for it. See the [isso server documentation](https://posativ.org/isso/docs/configuration/server) for details.
 
     @apiParam {number} id
         Id of the comment to delete.
@@ -518,7 +534,8 @@ def delete(self, environ, request, id, key=None):
         self.signal("comments.delete", id)
 
         resp = JSON(rv, 200)
-        cookie = functools.partial(dump_cookie, expires=0, max_age=0)
+        cookie = self.create_cookie(expires=0, max_age=0)
+
         resp.headers.add("Set-Cookie", cookie(str(id)))
         resp.headers.add("X-Set-Cookie", cookie("isso-%i" % id))
         return resp
@@ -1107,9 +1124,8 @@ def login(self, env, req):
                 '/admin',
                 get_current_url(env, strip_querystring=True)
             ))
-            cookie = functools.partial(dump_cookie,
-                                       value=self.isso.sign({"logged": True}),
-                                       expires=datetime.now() + timedelta(1))
+            cookie = self.create_cookie(value=self.isso.sign({"logged": True}),
+                                        expires=datetime.now() + timedelta(1))
             response.headers.add("Set-Cookie", cookie("admin-session"))
             response.headers.add("X-Set-Cookie", cookie("isso-admin-session"))
             return response

share/isso.conf

@@ -119,6 +119,16 @@ profile = off
 # forbiding several comment votes coming from the same subnet.
 trusted-proxies =
 
+# Override Set-Cookie header SameSite value.
+# Needed for setups where isso is not hosted on the same domain, e.g. called
+# from example.org and hosted under comments.example.org.
+# By default, isso will set SameSite=None when served over https and
+# SameSite=Lax when served over http.
+# See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite
+# and https://github.com/posativ/isso/issues/682
+# Accepted values: None, Lax, Strict
+samesite =
+
 
 [smtp]
 # Isso can notify you on new comments via SMTP. In the email notification, you