tornadoweb / tornado

Tornado is a Python web framework and asynchronous networking library, originally developed at FriendFeed.
http://www.tornadoweb.org/
Apache License 2.0
21.68k stars 5.5k forks source link

Allow resetting the XSRF cookie/token #3026

Open bhch opened 3 years ago

bhch commented 3 years ago

OWASP recommends to generate a new CSRF token after authentication:

Remember that pre-sessions cannot be transitioned to real sessions once the user is authenticated - the session should be destroyed and a new one should be made to avoid session fixation attacks.

Calling self.clear_cookie('_xsrf') followed by self.xsrf_token doesn't reset the cookie, for obvious reasons.

So, when the user logs in, currently, I'm doing this:

del self.cookies['_xsrf'] # delete cookie

if hasattr(self, '_xsrf_token'):
    delattr(self, '_xsrf_token') # remove cached token

if hasattr(self, '_raw_xsrf_token'):
    delattr(self, '_raw_xsrf_token') # remove cached token

self.xsrf_token # generate new token

But I fear this may break in future as this isn't official API. It would be better if the RequestHandler provided an official way to do it; something like self.reset_xsrf_token() maybe.

bdarnell commented 3 years ago

Calling self.clear_cookie('_xsrf') followed by self.xsrf_token doesn't reset the cookie, for obvious reasons.

It does work if you call self.clear_cookie("_xsrf") in your login handler which redirects to a logged-in page which will create a new cookie, so it may just be an issue for newfangled apps that never actually load a new page.

But in any case, that's still an undocumented implementation detail so we'd need to at least document it if that is intended to be the solution. I think a reset method probably makes sense, although I'd need to review exactly how all of this stuff works.

bhch commented 3 years ago

Ah, yes you're right. I was trying to generate a new token for the current response but that just reused the cached value. I saw Django's source code and they also reset the cookie within the same handler.

so it may just be an issue for newfangled apps that never actually load a new page.

Also true. Sending a second request post login just to get the new cookie seems rather redundant.

bdarnell commented 1 year ago

With the introduction of SameSite cookies, Tornado's xsrf_cookies system is obsolete. I intend to deprecate it which means that issues like this one will go unfixed (I do not intend to delete the feature entirely, though). Feedback is welcome in #3226.