Skip to content
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 #30862 -- Allowed setting SameSite cookies flags to 'None'. #11894

Merged
merged 3 commits into from Dec 12, 2019

Conversation

@danidee10
Copy link
Contributor

danidee10 commented Oct 9, 2019

Other changes:

  • Improve documentation for 'secure' flag
  • Fix method signatures for 'samesite' in .set_cookie and
    .set_signed_cookie

Ticket: https://code.djangoproject.com/ticket/30862#ticket

@danidee10 danidee10 changed the title Fixes #30862 Explicitly set SameSite cookies to 'None' Fixed #30862 Explicitly set SameSite cookies to 'None' Oct 9, 2019
@danidee10 danidee10 force-pushed the danidee10:master branch from 71c8e41 to 3206566 Oct 9, 2019
@carltongibson carltongibson changed the title Fixed #30862 Explicitly set SameSite cookies to 'None' Fixed #30862 -- Explicitly set SameSite cookies when 'None'. Oct 9, 2019
Copy link
Member

carltongibson left a comment

We'll need a new test case (to check the cookie was set with the right value).

And (not looking) I'd guess the existing tests were broken by your changes, so'll need updating.

@danidee10

This comment has been minimized.

Copy link
Contributor Author

danidee10 commented Oct 12, 2019

I've added a test and fixed the broken tests

@ethan92429

This comment has been minimized.

Copy link

ethan92429 commented Nov 16, 2019

How can I help this get added? It would be great to have 😄

@JakeUrban

This comment has been minimized.

Copy link

JakeUrban commented Dec 10, 2019

Is there anything left that needs to be done here? When can we expect this to be released? Without this, cookies for iframes are not sent in subsequent requests.

@felixxm felixxm force-pushed the danidee10:master branch from 5e408b8 to 3a8f549 Dec 11, 2019
@felixxm felixxm changed the title Fixed #30862 -- Explicitly set SameSite cookies when 'None'. Fixed #30862 -- Allowed setting SameSite cookies flags to None. Dec 11, 2019
@felixxm felixxm self-assigned this Dec 11, 2019
@felixxm felixxm force-pushed the danidee10:master branch from 3a8f549 to 24ed7af Dec 11, 2019
@felixxm

This comment has been minimized.

Copy link
Member

felixxm commented Dec 11, 2019

@danidee10 Thanks 👍 I reorganized commits, added docs and release notes 👍

samesite='none' is based on WIP RFC.

@felixxm felixxm force-pushed the danidee10:master branch from 24ed7af to 460b574 Dec 11, 2019
@felixxm felixxm changed the title Fixed #30862 -- Allowed setting SameSite cookies flags to None. Fixed #30862 -- Allowed setting SameSite cookies flags to 'none'. Dec 11, 2019
@felixxm felixxm changed the title Fixed #30862 -- Allowed setting SameSite cookies flags to 'none'. Fixed #30862 -- Allowed setting SameSite cookies flags to 'None'. Dec 11, 2019
Copy link
Member

carltongibson left a comment

The set_cookie() samesite paragraph needs adjusting to say what samesite='None' is all about.

(Currently the release notes have it but not the main reference.)

  • It should mention (explicitly) that it's (str) 'None' not None...

Q: Is the case important? (Lax/lax, None/none, etc.)

@felixxm felixxm force-pushed the danidee10:master branch from 460b574 to b33bfc3 Dec 12, 2019
@felixxm

This comment has been minimized.

Copy link
Member

felixxm commented Dec 12, 2019

@carltongibson Thanks for checking 🕵 I updated docs.

Q: Is the case important? (Lax/lax, None/none, etc.)

No.

@felixxm felixxm merged commit b33bfc3 into django:master Dec 12, 2019
25 checks passed
25 checks passed
docs Build #3324 ended
Details
flake8 Build #3320 ended
Details
isort Build #3329 succeeded in 20 sec
Details
pr-mariadb/database=mysql,label=mariadb,python=python3.8 Build #3280 ended
Details
pr-mariadb/database=mysql_gis,label=mariadb,python=python3.8 Build #3280 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.6 Build #3349 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.7 Build #3349 ended
Details
pull-requests-bionic/database=mysql,label=bionic-pr,python=python3.8 Build #3349 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.6 Build #3349 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.7 Build #3349 ended
Details
pull-requests-bionic/database=mysql_gis,label=bionic-pr,python=python3.8 Build #3349 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.6 Build #3349 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.7 Build #3349 ended
Details
pull-requests-bionic/database=postgis,label=bionic-pr,python=python3.8 Build #3349 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.6 Build #3349 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.7 Build #3349 ended
Details
pull-requests-bionic/database=postgres,label=bionic-pr,python=python3.8 Build #3349 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.6 Build #3349 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.7 Build #3349 ended
Details
pull-requests-bionic/database=spatialite,label=bionic-pr,python=python3.8 Build #3349 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.6 Build #3349 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.7 Build #3349 ended
Details
pull-requests-bionic/database=sqlite3,label=bionic-pr,python=python3.8 Build #3349 ended
Details
pull-requests-javascript Build #3327 ended
Details
pull-requests-windows/database=sqlite3,label=windows,python=Python38 Build #3287 ended
Details
@JakeUrban

This comment has been minimized.

Copy link

JakeUrban commented Dec 12, 2019

Awesome, thanks to the reviewers. @carltongibson Is there any way to know when or under what version this fix would be released?

@felixxm

This comment has been minimized.

Copy link
Member

felixxm commented Dec 13, 2019

Django 3.1

@arprajapat

This comment has been minimized.

Copy link

arprajapat commented Dec 24, 2019

Django 3.1

@felixxm When this will be released?

@felixxm

This comment has been minimized.

Copy link
Member

felixxm commented Dec 24, 2019

In August 2020, see Version3.1Roadmap.

@bezineb5

This comment has been minimized.

Copy link

bezineb5 commented Jan 2, 2020

Hi, it seems that Google's timeline is quite aggressive, with the deployment of Chrome 80 with this feature enabled planned for February 4th, 2020: https://www.chromium.org/updates/same-site

By the way, it seems that proper implementation of such cookies will end up being quite difficult, as there are many incompatible browsers: https://www.chromium.org/updates/same-site/incompatible-clients :-(

@jonespm

This comment has been minimized.

Copy link

jonespm commented Jan 9, 2020

I really think because of Google's timeline many of us are going to need this in the 2.2.x branch with a 2.2 release prior to February 4th. Without it being set it looks like these cookies are going to be set to Lax (via enabling the same-site-by-default-cookies flag) and this breaks some of our applications.

@jellllllle

This comment has been minimized.

Copy link

jellllllle commented Jan 15, 2020

What.... :( Django 3.1 in August? This is quite a serious issue. I guess many application will go down and I doubt if people realize this (Next month I expect a little bit more reactions on this when they do). This should be a "hotfix" for Django 2 and 3 (maybe even 1) or am I over reacting?

It will be kind of hard to update our Django 2 applications to Django 3 and August would be a few months to late according to Google. :(

Are there any workarounds to set the none value for the session cookie? Its the only thing I need.

SESSION_COOKIE_SAMESITE = 'none'

Prefer not to update whole applications because of a missing cookie flag.

Sry for this I love Django but I am kind of stressing here aaaaaaaaaaaaa. :)

@jonespm

This comment has been minimized.

Copy link

jonespm commented Jan 15, 2020

@jellllllle It will be available in Django 1.11 via the https://github.com/jotes/django-cookies-samesite package. Could probably monkey patch django/http/response set_cookie if it came down to that in 2.

@JakeUrban

This comment has been minimized.

Copy link

JakeUrban commented Jan 16, 2020

@jellllllle I got around this issue by creating a Middleware class, see it here.

@apollo13

This comment has been minimized.

Copy link
Member

apollo13 commented Jan 16, 2020

@felixxm, @carltongibson I know this is against our bugfix policy, but we have been adding support for new python versions after the fact even though Django didn't support them initially. Given the simplicity of the change do you see a problem with backporting this to the supported releases? Granted a custom middleware is simple enough in this case, but one way or another we have to fix stuff for browsers every now and then.

@apollo13

This comment has been minimized.

Copy link
Member

apollo13 commented Jan 16, 2020

@JakeUrban We are currently thinking about the fallout if we do not backport this. What is your usecase for SameSite=None? Also given https://www.chromium.org/updates/same-site/incompatible-clients it's probably not the best thing to do if you need to support some older browsers…

@jellllllle

This comment has been minimized.

Copy link

jellllllle commented Jan 20, 2020

@JakeUrban @apollo13 Thanks JakeUrban but as Apollo13 is saying should't we need somekind of useragent (request.META['HTTP_USER_AGENT']) check?

@JakeUrban

This comment has been minimized.

Copy link

JakeUrban commented Jan 20, 2020

@apollo13 I guess its true that some older browsers do not support SameSite=None but there doesn't seem to be an alternative solution for applications using iframes.

Our use case is this: If an application opens a third-party site within an iframe, that third-party site won't be able to persist session cookies unless they set SameSite=None. This is because the session cookie doesn't belong the URL in the address bar, i.e. its considered a third-party cookie.

Luckily, my organization is able to work with the applications opening these sites via iframes, and we've suggested they move to using popups instead. This removes the need for SameSite=None altogether since the cookies belong to the URL of the popup.

So @jellllllle, you could add a user agent check but it depends on your use case. If you have an alternative solution for the browsers that don't support SameSite=None then that makes sense. For us, our only other solution was to ask these applications to no longer use iframes.

@jonespm

This comment has been minimized.

Copy link

jonespm commented Jan 20, 2020

There's likely less secure solutions that would work around this like https://pypi.org/project/django-cookieless/ that could also be used as a fall back. But for us (locally) the usage of these unsupported devices on this list are so low (much lower than 1%) that it's easy enough for us to just say "use another browser". However the usage of Chrome is over 75%.

@bezineb5

This comment has been minimized.

Copy link

bezineb5 commented Jan 22, 2020

A workaround to set the samesite to none before Django 3.2 is simply to do it manually outside of the set_cookies method (works with Django 2.2 and probably older):

response.set_cookie("mykey", value="myvalue", secure=True)
response.cookies["mykey"]['samesite'] = "none"
@apollo13

This comment has been minimized.

Copy link
Member

apollo13 commented Jan 22, 2020

Okay, so the fallout is probably mostly authenticated iframes which is a valid usecase. @felixxm I am still +1 for a backport, would you be okay to adding this to our LTS as well as the next 3.0 bugfix release? If the backports are to much work I am pretty sure @jonespm or so will happily provide you the PRs to do so ;)

@Aamirsohel

This comment has been minimized.

Copy link

Aamirsohel commented Jan 23, 2020

@felixxm I was able to use this fix for chrome 80 but I am facing issue with safari 13.0.4, is there a bug in safari, where can I get help for this.my use case is same as @apollo13.

@felixxm

This comment has been minimized.

Copy link
Member

felixxm commented Jan 23, 2020

@apollo13 I'm not convince to backport 🤔, because this is against our policy and only small amount of users are affected. But above all, this can be fixed locally with a small middleware:

class SameSiteMiddleware(MiddlewareMixin):
    def process_response(self, request, response):
        if not response.cookies['example']['samesite']:
            response.cookies['example']['samesite'] = 'None'
    return response
@felixxm

This comment has been minimized.

Copy link
Member

felixxm commented Jan 23, 2020

@felixxm I was able to use this fix for chrome 80 but I am facing issue with safari 13.0.4, is there a bug in safari, where can I get help for this.my use case is same as @apollo13.

You can check related tickets ticket-30250 and ticket-29975.

@jonespm

This comment has been minimized.

Copy link

jonespm commented Jan 23, 2020

I kind of like @bezineb5 idea (which is similar to @felixxm) of just setting this explicitly. web.dev mentioned an interesting workaround for this of setting two cookies, one legacy and one new. I feel like that's easier than Chrome's User Agent detection idea and is going to have to be the short term fix until everything handles this as expected.

@bolinocroustibat

This comment has been minimized.

Copy link

bolinocroustibat commented Jan 30, 2020

Even though we can fix it with a middleware, I also thing this should be backported, since it is now a bug when we consider the new tech environment (Chrome), not a feature or an enhancement anymore. Also, it is a quite a serious issue affecting many projects.

@rowan-m rowan-m mentioned this pull request Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.