Repository: tornadoweb/tornado
Pull request #2524 (2018-10-31 10:14:33)

Define XSRF cookie expires_day via settings
Hi,

We are adopting Tornado at work, and I started reading its internal API (which is super neat! kudos!, and pending issues to get familiar with the code.

Found #462, and thought it looked simple, so decided to try to understand where the change would occur and suggest a fix in a pull request.

First time contributing to Tornado, so feel free to point me if I forgot anything. Not sure if that's something easy to be tested? If so, if someone could point me to a similar test, I'd be happy to include one here. It worked testing locally and confirming in Firefox web developer Storage tab for the cookie info.

Thank you very much for Tornado!
Bruno

bdarnell: Thanks for the contribution!

There's already a `xsrf_cookie_kwargs` setting, so I think it would be better to make `expires_days` work in that dict instead of introducing a separate setting for it. I think all that would be required would be to replace the `expires_days` local variable with an entry in the `kwargs` dict. There's also already a test for `xsrf_cookie_kwargs` which would be easy to extend to test this.
kinow: Oh! Makes sense! Much easier to implement I think. And thanks for pointing the tests. Will try to update the pull request over the weekend. Thanks
kinow: @bdarnell first tentative update. Do we have to keep the previous behaviour, of setting the expiration date to whatever the user defined iff the `self.current_user` is defined? I thought it would perhaps make more sense to let it be defined to any value defined by the caller? Or maybe not?

Also fixed the formatting issue pointed by `black` in one of the Travis-CI builds.
ploxiln: I think it'll be necessary to have the old default still work. It could be as simple as:

```python
if self.current_user and 'expires_days' not in cookie_kwargs:
    cookie_kwargs['expires_days'] = 30
```
kinow: Updated pull request (rebase/squash/wait for travis...). Now the old behaviour should be unaffected. Thanks for the suggestion @ploxiln! :tada: :+1: 
bdarnell: The change looks good, but the test isn't really doing much. There will be an `expires=` field even if the web.py change were missing since there's a 30-day default. I think the test needs to parse the expiration date and ensure that it's 2 days in the future instead of 30 days (approximately - we don't want the test to break around DST switches).

If you're up for doing a little more plumbing and a little less date parsing, you could add a setting to pass in a value for "now" so the test wouldn't depend on the current time. 

Patch:
diff --git a/tornado/web.py b/tornado/web.py
index 263f429b..1f4a1698 100644
--- a/tornado/web.py
+++ b/tornado/web.py
@@ -1418,13 +1418,9 @@ class RequestHandler(object):
             else:
                 raise ValueError("unknown xsrf cookie version %d", output_version)
             if version is None:
-                expires_days = 30 if self.current_user else None
-                self.set_cookie(
-                    "_xsrf",
-                    self._xsrf_token,
-                    expires_days=expires_days,
-                    **cookie_kwargs
-                )
+                if self.current_user and "expires_days" not in cookie_kwargs:
+                    cookie_kwargs["expires_days"] = 30
+                self.set_cookie("_xsrf", self._xsrf_token, **cookie_kwargs)
         return self._xsrf_token
 
     def _get_raw_xsrf_token(self) -> Tuple[Optional[int], bytes, float]:


Test patch:
diff --git a/tornado/test/web_test.py b/tornado/test/web_test.py
index be38a998..a6f5d1bd 100644
--- a/tornado/test/web_test.py
+++ b/tornado/test/web_test.py
@@ -2902,11 +2902,23 @@ class XSRFCookieKwargsTest(SimpleHandlerTestCase):
             self.write(self.xsrf_token)
 
     def get_app_kwargs(self):
-        return dict(xsrf_cookies=True, xsrf_cookie_kwargs=dict(httponly=True))
+        return dict(
+            xsrf_cookies=True, xsrf_cookie_kwargs=dict(httponly=True, expires_days=2)
+        )
 
     def test_xsrf_httponly(self):
         response = self.fetch("/")
         self.assertIn("httponly;", response.headers["Set-Cookie"].lower())
+        self.assertIn("expires=", response.headers["Set-Cookie"].lower())
+        header = response.headers.get("Set-Cookie")
+        match = re.match(".*; expires=(?P<expires>.+);.*", header)
+        assert match is not None
+
+        expires = datetime.datetime.utcnow() + datetime.timedelta(days=2)
+        parsed = email.utils.parsedate(match.groupdict()["expires"])
+        assert parsed is not None
+        header_expires = datetime.datetime(*parsed[:6])
+        self.assertTrue(abs((expires - header_expires).total_seconds()) < 10)
 
 
 class FinishExceptionTest(SimpleHandlerTestCase):