-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support samesite xsrf cookie #2573
Comments
Samesite cookies are new to me, but here are my initial thoughts: If you send a cookie with If you can eliminate the legacy fallback, things get a lot simpler: you can avoid all the If you're not willing to give up on users of older browsers that don't support samesite, is it worthwhile to support two modes so you can use samesite where available? I'm not sure that it is - if you're not willing to rely solely on samesite I'd probably just stick with old-style XSRF protection. (You can set samesite in |
This will cause issues after Chrome 80 upgrade in cases where tornado server is running inside a cross origin iframe.
|
If your application requires the use of the SameSite attribute, I think you'll need to either upgrade to python 3.8 or apply the Morsel monkey-patch yourself. I don't think it would be appropriate for Tornado to do this kind of monkey-patch. If SameSite cookies are becoming more important, consider lobbying python-dev to backport this change to older versions of python, or see if someone will create and maintain a copy of It's also possible for tornado to move away from |
Thanks Ben. We ended up monkey-patching our student virtual environments at Udacity. Upgrading to 3.8 was not an immediate option, because some of our courses are taught in Python 3.6 which requires a larger scale of content updates. |
Or maybe Tornado should just implement its own Morsel class since this is simple and defined on a RFC. This class could also be extensible to support future names instead of how Python handles it. There is nothing special, to be honest, in this class that would require it to enforce a small collection of names |
I think there are good reasons to enforce that names are on a small list - it helps catch mistakes earlier, and it can handle special formatting requirements (notice that there is special per-field handling of integer arguments to the The problem is that the web platform evolves at its own pace and it doesn't make sense for these limits to be baked into slow-moving language releases. There must either be a way to say "trust me, i want to use this flag that's not on the list" or the list must be updated more frequently even if this looks like "adding features to old releases". Moving away from the standard library's cookie module makes sense, but somebody has to do the work to make that happen. And given the non-trivial security risks of anything touching cookie parsing, I wouldn't count on that being ready for production usage by the time the chrome policy change goes into effect, so monkey-patching Morsel is likely to be the most expedient approach. |
Thanks @bdarnell, this is actually a good argument. Well, I had monkey patched the std lib anyway but would be good if this can go through some roadmap to be part of tornado at some point. |
@bdarnell I suppose we can do same as Bottle guys did |
#2911 is related to this. While one can set the |
Since this issue was opened, Chrome and Edge have made I've also talked to a couple of security researchers about SameSite and modern XSRF protection, and come to the following conclusions:
So now we can either deprecate |
This feature is more invasive than using the samesite cookie attribute but does not provide additional protection, so it is no longer something that we should recommend. Now that this feature is deprecated, the open issues related to it will not be fixed (however, I intend to keep the current code around indefinitely; there are no plans to remove it). Closes tornadoweb#865 Closes tornadoweb#2573 Closes tornadoweb#3026
I've created PR #3226 to deprecate |
A somewhat more recent approach to CSRF is setting
samesite=strict
on a cookie. SameSite is handy because it allows the browser to enforce the same-site-ness of a request, without needing application code to send the token separately. SameSite can be used almost everywhere, but not quite. It would be handy to support this strategy of csrf checking (cookie is present and samesite=strict) as equivalent to the current check of token delivery by both cookie and argument.If I wanted to adopt this strategy now, do you suppose it would be preferable to:
?
Unfortunately, only Python 3.8 has support for setting samesite cookies in the stdlib. This is easily patched (
Morsel._reserved['samesite'] = 'SameSite'
), but needing a patch is never great.The text was updated successfully, but these errors were encountered: