Fixed #27863 -- Added support for the SameSite cookie flag. #8380
Conversation
charettes
left a comment
There was a problem hiding this comment.
This would require tests for CSRF_COOKIE_SAMESITE as well and mention in the release notes. Looks like this will require adjustment to the stdlib http.cookie module as noted by Alex on the ticket, see python/cpython#214. We'd also have to make sure this doesn't break on browses that don't support the SameSite directive.
django/conf/global_settings.py
Outdated
| LANGUAGE_COOKIE_AGE = None | ||
| LANGUAGE_COOKIE_DOMAIN = None | ||
| LANGUAGE_COOKIE_PATH = '/' | ||
| LANGUAGE_COOKIE_SAMESITE = 'lax' |
There was a problem hiding this comment.
I'm not convinced this is worth adding for the language cookie as it's not carrying any critical data (notice how LANGUAGE_COOKIE_SECURE and LANGUAGE_COOKIE_HTTPONLY don't exist either.
There was a problem hiding this comment.
I wouldn't bother, there's nothing bad you can do with cross-origin language headers.
| domain=settings.SESSION_COOKIE_DOMAIN, | ||
| secure=settings.SESSION_COOKIE_SECURE or None, | ||
| httponly=settings.SESSION_COOKIE_HTTPONLY or None, | ||
| samesite=settings.SESSION_COOKIE_SAMESITE or None |
| path=settings.SESSION_COOKIE_PATH, | ||
| secure=settings.SESSION_COOKIE_SECURE or None, | ||
| httponly=settings.SESSION_COOKIE_HTTPONLY or None, | ||
| samesite=settings.SESSION_COOKIE_SAMESITE or None |
django/http/response.py
Outdated
| if httponly: | ||
| self.cookies[key]['httponly'] = True | ||
| if samesite: | ||
| self.cookies[key]['samesite'] = samesite.capitalize() |
There was a problem hiding this comment.
what's the reason for capitalizing it?
There was a problem hiding this comment.
Preserve the capitalization from the original standard.
There was a problem hiding this comment.
Ahh right! I misread this as upper() somehow.
There was a problem hiding this comment.
Why not just expect the setting to be the correct capitalization, 'Lax' or 'Strict'? Doing capitalize() feels like unnecessary post-processing to me. Is it a matter of settings aesthetics?
django/middleware/csrf.py
Outdated
| path=settings.CSRF_COOKIE_PATH, | ||
| secure=settings.CSRF_COOKIE_SECURE, | ||
| httponly=settings.CSRF_COOKIE_HTTPONLY, | ||
| samesite=settings.CSRF_COOKIE_SAMESITE |
django/views/i18n.py
Outdated
| max_age=settings.LANGUAGE_COOKIE_AGE, | ||
| path=settings.LANGUAGE_COOKIE_PATH, | ||
| domain=settings.LANGUAGE_COOKIE_DOMAIN, | ||
| samesite=settings.LANGUAGE_COOKIE_SAMESITE |
There was a problem hiding this comment.
You can remove references to LANGUAGE_COOKIE_SAMESITE.
docs/ref/settings.txt
Outdated
| from being sent in cross-site requests. | ||
|
|
||
| See :setting:`SESSION_COOKIE_SAMESITE` for details on ``SameSite``. | ||
|
|
There was a problem hiding this comment.
You can remove reference to LANGUAGE_COOKIE_SAMESITE.
tests/sessions_tests/tests.py
Outdated
| # Handle the response through the middleware | ||
| response = middleware.process_response(request, response) | ||
| self.assertTrue( | ||
| response.cookies[settings.SESSION_COOKIE_NAME]['samesite']) |
There was a problem hiding this comment.
Test the explicit value. This should fit on a single line.
|
Thanks for working on this! |
| domain=settings.SESSION_COOKIE_DOMAIN, | ||
| secure=settings.SESSION_COOKIE_SECURE or None, | ||
| httponly=settings.SESSION_COOKIE_HTTPONLY or None, | ||
| samesite=settings.SESSION_COOKIE_SAMESITE or None, |
There was a problem hiding this comment.
According to the code comment, SESSION_COOKIE_SAMESITE can be 'lax', 'strict' or None. When SESSION_COOKIE_SAMESITE is set to None, does this evaluate to None or None? If so, I don't think we need the or None, right? (Same in django/contrib/sessions/middleware.py)
|
This probably deserves a mention in the release notes, I don't remember how we maintain those these days. |
|
Release note added to 1.11.1. @alex @charettes |
|
@kravietz the code looks good but until SameSite support lands in the stdlib I'm afraid we'll have to adapt our vendored version of cookies. |
|
@charettes Any suggestions what would be the best approach? The factory Morsel won't serialize anything that is not in the _reserved dict. I have started patching it a bit like it was done with the Django's SimpleCookie but with Morsel it would be much more ugly, essentially rewriting half of the BaseCookie class... |
|
@kravietz I see, I really can't think of any other solution unfortunately. |
|
@charettes Actually, the patching didn't seem to be that hard. It seems to be working now. Also added a test specifically for the patched SimpleCookie and Morsel version. |
|
@kravietz Are you still working on this PR? Mozilla Observatory made changes that now check for |
|
@kravietz looking for help with this patch series? this helps reduce Spectre's attack surface. I'd love to help get this merged upstream. |
|
@script3r This hasn't been updated for a while, if you're interested in working on it I think you're good to take this and run with it. |
|
Is there anyway to set the flag currently in django 1.11? Some kind of workaround? |
|
Rebased and submitted as #9860 |
Resolving https://code.djangoproject.com/ticket/27863