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

Fixed #27863 -- Added support for the SameSite cookie flag. #8380

Closed
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@kravietz
Copy link

kravietz commented Apr 20, 2017

@charettes
Copy link
Member

charettes left a 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.

@@ -157,6 +157,7 @@ def gettext_noop(s):
LANGUAGE_COOKIE_AGE = None
LANGUAGE_COOKIE_DOMAIN = None
LANGUAGE_COOKIE_PATH = '/'
LANGUAGE_COOKIE_SAMESITE = 'lax'

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

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.

This comment has been minimized.

@alex

alex Apr 20, 2017

Member

I wouldn't bother, there's nothing bad you can do with cross-origin language headers.

@@ -86,6 +86,7 @@ def _update_cookie(self, encoded_data, response):
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

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

Add a trailing comma.

@@ -69,5 +69,6 @@ def process_response(self, request, response):
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

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

Add a trailing comma.

@@ -198,6 +198,8 @@ def set_cookie(self, key, value='', max_age=None, expires=None, path='/',
self.cookies[key]['secure'] = True
if httponly:
self.cookies[key]['httponly'] = True
if samesite:
self.cookies[key]['samesite'] = samesite.capitalize()

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

what's the reason for capitalizing it?

This comment has been minimized.

@kravietz

kravietz Apr 20, 2017

Preserve the capitalization from the original standard.

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

Ahh right! I misread this as upper() somehow.

This comment has been minimized.

@jdufresne

jdufresne Apr 21, 2017

Member

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?

This comment has been minimized.

@kravietz

kravietz Apr 21, 2017

Fair point, corrected this.

@@ -190,6 +190,7 @@ def _set_token(self, request, response):
path=settings.CSRF_COOKIE_PATH,
secure=settings.CSRF_COOKIE_SECURE,
httponly=settings.CSRF_COOKIE_HTTPONLY,
samesite=settings.CSRF_COOKIE_SAMESITE

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

Add a trailing comma.

@@ -55,6 +55,7 @@ def set_language(request):
max_age=settings.LANGUAGE_COOKIE_AGE,
path=settings.LANGUAGE_COOKIE_PATH,
domain=settings.LANGUAGE_COOKIE_DOMAIN,
samesite=settings.LANGUAGE_COOKIE_SAMESITE

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

You can remove references to LANGUAGE_COOKIE_SAMESITE.

from being sent in cross-site requests.

See :setting:`SESSION_COOKIE_SAMESITE` for details on ``SameSite``.

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

You can remove reference to LANGUAGE_COOKIE_SAMESITE.

# Handle the response through the middleware
response = middleware.process_response(request, response)
self.assertTrue(
response.cookies[settings.SESSION_COOKIE_NAME]['samesite'])

This comment has been minimized.

@charettes

charettes Apr 20, 2017

Member

Test the explicit value. This should fit on a single line.

@charettes charettes changed the title Add support for SameSite cookie flag Fixed #27863 -- Added support for the SameSite cookie flag. Apr 20, 2017

@alex

This comment has been minimized.

Copy link
Member

alex commented Apr 20, 2017

Thanks for working on this!

@@ -86,6 +86,7 @@ def _update_cookie(self, encoded_data, response):
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,

This comment has been minimized.

@jdufresne

jdufresne Apr 20, 2017

Member

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)

@alex

This comment has been minimized.

Copy link
Member

alex commented Apr 21, 2017

This probably deserves a mention in the release notes, I don't remember how we maintain those these days.

@kravietz

This comment has been minimized.

Copy link

kravietz commented Apr 25, 2017

Release note added to 1.11.1. @alex @charettes

@charettes

This comment has been minimized.

Copy link
Member

charettes commented Apr 25, 2017

@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.

@kravietz

This comment has been minimized.

Copy link

kravietz commented May 3, 2017

@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...

@charettes

This comment has been minimized.

Copy link
Member

charettes commented May 4, 2017

@kravietz I see, I really can't think of any other solution unfortunately.

@kravietz

This comment has been minimized.

Copy link

kravietz commented May 4, 2017

@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.

@OllieBuilds

This comment has been minimized.

Copy link

OllieBuilds commented Jan 22, 2018

@kravietz Are you still working on this PR? Mozilla Observatory made changes that now check for SameSite (mozilla/http-observatory#313), and I think this would meet these new requirements.

@script3r

This comment has been minimized.

Copy link

script3r commented Feb 21, 2018

@kravietz looking for help with this patch series? this helps reduce Spectre's attack surface. I'd love to help get this merged upstream.

@alex

This comment has been minimized.

Copy link
Member

alex commented Feb 21, 2018

@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.

@lane-c-wagner

This comment has been minimized.

Copy link

lane-c-wagner commented Mar 3, 2018

Is there anyway to set the flag currently in django 1.11? Some kind of workaround?

@alex

This comment has been minimized.

Copy link
Member

alex commented Apr 7, 2018

Rebased and submitted as #9860

@timgraham timgraham closed this Apr 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment