bpo-29613: Added support for SameSite cookies #214

akash0x53 · 2017-02-21T10:58:44Z

Implemented as per draft
https://tools.ietf.org/html/draft-west-first-party-cookies-07

the-knights-who-say-ni · 2017-02-21T10:58:45Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

alex · 2017-02-21T14:14:23Z

Looks good to me! @akash0x53 please go ahead and get the CLA filled out so this can be merged.

(Note for other core devs: I'm not really up to speed on merging procedures, so I'm hoping someone else will do that part)

Mariatta · 2017-02-21T14:59:10Z

Lib/http/cookies.py

@@ -281,6 +281,7 @@ class Morsel(dict):
        "secure"   : "Secure",
        "httponly" : "HttpOnly",
        "version"  : "Version",
+        "samesite" : "SameSite"


Please update the documentation with this entry at Doc/library/http.cookies.rst

Include a trailing comma so that if more items are added later, this line doesn't have to be modified again.

Mariatta

Please update the docs.

Mariatta · 2017-02-21T15:08:07Z

Should there be entries in misc/ACKS and/or misc/News for this?
Also, I think this can only go to master branch, correct? So that it doesn't need to be backported?

I can do the merge once CLA is signed.

alex · 2017-02-21T15:10:02Z

Per the usual feature/bug division, I think this would be considered a feature and only go on master. (This makes me a bit sad, but that's a debate for another time).

Thanks for catching the need for docs!

Mariatta · 2017-02-21T15:13:27Z

Sounds good. Thanks @alex 😄

akash0x53 · 2017-02-21T16:58:26Z

@Mariatta I pushed the changes, please review. Do I need change in Misc/News?
@alex I signed the CLA but seems it is still not reflected here

akash0x53 · 2017-02-21T17:01:21Z

@the-knights-who-say-ni I signed the CLA and appeared on b.p.o.

timgraham · 2017-02-21T17:01:27Z

I think a .. versionchanged:: 3.7 annotation is also required in the docs.

The first comment on the PR says, "If you just signed the CLA, please wait at least one US business day".

Mariatta · 2017-02-21T18:18:12Z

Thanks @timgraham !
Hmm... I'm wondering if it should be .. versionadded:: 3.7 instead of .. versionchanged:: 3.7 ? 😅 Can other core devs confirm which one?

@akash0x53:
Please update Misc/NEWS with the following entry.

- bpo-29613: http.cookies.Morsel now supports SameSite cookies.  Patch by <your name>.

^ ensure the above wraps at 80 chars, and add it under What's New in Python 3.7.0 alpha 1? , and Library subheading :)

Please add yourself to Misc/ACKS (names are sorted alphabetically by last name)

Thanks!

alex · 2017-02-21T19:20:45Z

Usually versionadded is when the entire API is new, and versionchanged is when the API changed.

Mariatta · 2017-02-21T20:14:32Z

Doc/library/http.cookies.rst

+   The attribute :attr:`samesite` specifies that browser is not allowed to send the
+   cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid
+   values for this attribute are "Strict" and "Lax".
+


Please add below this line:

.. versionchanged:: 3.7 Added support for :attr:`samesite` attribute.

Thanks @alex for the clarification about this :)

Mariatta · 2017-02-22T04:31:27Z

Doc/library/http.cookies.rst


   The attribute :attr:`httponly` specifies that the cookie is only transferred
   in HTTP requests, and is not accessible through JavaScript. This is intended
   to mitigate some forms of cross-site scripting.

+   .. versionchanged:: 3.7


@akash0x53 I'm really sorry that I wasn't being clear about this :(

The versionchanged info is to be added after the paragraph about samesite attribute.

Overall the change should look like this:

... to mitigate some forms of cross-site scripting. The attribute :attr:`samesite` specifies that browser is not allowed to send the cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid values for this attribute are ``'Strict'`` and ``'Lax'``. .. versionchanged:: 3.7 Added support for :attr:`samesite` attribute. The keys are case-insensitive and their default value is ``''``.

berkerpeksag

Unless I'm missing something, currently this will accept any value other than "Lax" and "Strict". I wonder if we should discard invalid values like "SameSite=invalid".

I think this PR needs some RFC reading and research on other programming languages.

berkerpeksag · 2017-02-22T05:27:25Z

Lib/test/test_http_cookies.py

@@ -121,6 +121,18 @@ def test_set_secure_httponly_attrs(self):
        self.assertEqual(C.output(),
            'Set-Cookie: Customer="WILE_E_COYOTE"; HttpOnly; Secure')

+    def test_samesite_strict_attrs(self):
+        C = cookies.SimpleCookie('Customer="WILE_E_COYOTE"')


We could use self.subTest() to make the tests shorter: https://docs.python.org/3/library/unittest.html#distinguishing-test-iterations-using-subtests

berkerpeksag · 2017-02-22T05:31:30Z

Doc/library/http.cookies.rst

+   cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid
+   values for this attribute are "Strict" and "Lax".
+
+   .. versionchanged:: 3.7


Please move this to below the other version directives:

[...] .. versionchanged:: 3.5 [...] .. versionchanged:: 3.7 [...] .. versionchanged:: 3.7 Added support for :attr:`samesite` attribute.

So that should be like this, (description of attribute also moved under 3.7)

+ .. versionchanged:: 3.7 + Added support for :attr:`samesite` attribute. + + The attribute :attr:`samesite` specifies that browser is not allowed to send the + cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid + values for this attribute are "Strict" and "Lax". +

Please see the diff below:

+ The attribute :attr:`samesite` specifies that browser is not allowed to send the + cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid + values for this attribute are "Strict" and "Lax". The keys are case-insensitive and their default value is ``''``. .. versionchanged:: 3.5 :meth:`~Morsel.__eq__` now takes :attr:`~Morsel.key` and :attr:`~Morsel.value` into account. .. versionchanged:: 3.7 Attributes :attr:`~Morsel.key`, :attr:`~Morsel.value` and :attr:`~Morsel.coded_value` are read-only. Use :meth:`~Morsel.set` for setting them. + .. versionchanged:: 3.7 + Added support for :attr:`samesite` attribute.

akash0x53 · 2017-02-22T13:23:03Z

Hi @Mariatta and @berkerpeksag , I made few changes as per suggestions.

timgraham · 2017-02-22T14:50:34Z

Doc/library/http.cookies.rst


   The attribute :attr:`httponly` specifies that the cookie is only transferred
   in HTTP requests, and is not accessible through JavaScript. This is intended
   to mitigate some forms of cross-site scripting.

+   The attribute :attr:`samesite` specifies that browser is not allowed to send the


I think "The samesite attribute" is better English (not sure if the pattern from the previous paragraph is commonly used).
"the browser" (add "the")

timgraham · 2017-02-22T14:50:59Z

Doc/library/http.cookies.rst


   The attribute :attr:`httponly` specifies that the cookie is only transferred
   in HTTP requests, and is not accessible through JavaScript. This is intended
   to mitigate some forms of cross-site scripting.

+   The attribute :attr:`samesite` specifies that browser is not allowed to send the
+   cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid


timgraham · 2017-02-22T14:51:20Z

Doc/library/http.cookies.rst

@@ -153,6 +159,9 @@ Morsel Objects
      :attr:`~Morsel.coded_value` are read-only.  Use :meth:`~Morsel.set` for
      setting them.

+   .. versionchanged:: 3.7
+      Added support for :attr:`samesite` attribute.


timgraham · 2017-02-22T14:52:46Z

Lib/test/test_http_cookies.py

+                C = cookies.SimpleCookie('Customer="WILE_E_COYOTE"')
+                C['Customer']['samesite'] = val
+                self.assertEqual(C.output(),
+                'Set-Cookie: Customer="WILE_E_COYOTE"; SameSite=%s' % val)


indentation missing

Negative. It just an illusion i guess. Perfectly showing in my vim.

What Tim meant was that the second argument needs to be indented, like:

self.assertEqual(C.output(), 'Set-Cookie: Customer="WILE_E_COYOTE"; SameSite=%s' % val)

or even better:

expected = f'Set-Cookie: Customer="WILE_E_COYOTE"; SameSite={val}' self.assertEqual(C.output(), expected)

timgraham · 2017-02-22T14:53:26Z

Doc/library/http.cookies.rst


   The attribute :attr:`httponly` specifies that the cookie is only transferred
   in HTTP requests, and is not accessible through JavaScript. This is intended
   to mitigate some forms of cross-site scripting.

+   The attribute :attr:`samesite` specifies that browser is not allowed to send the
+   cookie along with cross-site requests. This help to mitigate CSRF attacks. Valid
+   values for this attribute are "Strict" and "Lax".


What are the meaning of these values?
Are invalid values rejected? I don't see any code/tests for that.

Explaining the values would be out of scope of the Python documentation. I think invalid values should be accepted, after all its browser's job to discard invalid values. Suppose, in future they proposed or added another value for SameSite then we need to make space for that too. By the way i'm not sure about this, let the member decide.

Tim is correct that we need to add a test for invalid values. However, we need to decide on what we should do with invalid values first. I don't have time to do a research at the moment, but just a note that Firefox doesn't implement SameSite support yet: https://bugzilla.mozilla.org/show_bug.cgi?id=795346

You're right. However, chrome implemented SamSite. Right now only Chrome implemented this. https://bugs.chromium.org/p/chromium/issues/detail?id=459154
I checked the test cases they wrote for the same, i didn't find test cases for invalid values.
https://chromium.googlesource.com/chromium/src/+/f71d0bde417518f99f977a0ecbf480b375cf49ca/net/cookies/canonical_cookie_unittest.cc#86

I messed with this branch :( Should I open new PR?

You should be able to fix the branch by rebasing and force pushing.

timgraham · 2017-02-22T14:54:15Z

Lib/test/test_http_cookies.py

@@ -121,6 +121,15 @@ def test_set_secure_httponly_attrs(self):
        self.assertEqual(C.output(),
            'Set-Cookie: Customer="WILE_E_COYOTE"; HttpOnly; Secure')

+    def test_samesite_attrs(self):
+        samesite_values = ("Strict", "Lax")


Use single quotes for consistency?

berkerpeksag · 2017-02-27T13:15:40Z

@akash0x53 please fix your branch. There are a lot of unrelated commits.

Implemented as per draft https://tools.ietf.org/html/draft-west-first-party-cookies-07

And suggestions by members.

alex · 2017-04-01T01:25:58Z

It looks like there a few outstanding issues here still. Do you have the time to fix them up?

akash0x53 · 2017-04-01T03:57:59Z

Yes @alex , I fix it asap.

kravietz · 2017-05-03T14:26:19Z

@akash0x53 Are we still on track for 3.7 with this change? As Django PR#8380 depends on this one I can help if necessary!

akash0x53 · 2017-05-04T06:52:35Z

I definitely need help on this @kravietz . I checked your PR, it allow to set invalid values to the cookie. If that approach is taken here then this PR good to go. I'm confused whether we should allow invalid values or not 😕

kravietz · 2017-05-04T12:33:32Z

@akash0x53 Correct, I have updated the Django code to check for an invalid samesite values and ready to help with the Python patch. I guess it might be easier if we could chat in real time and plan next steps - IRC or something?

BTW for Django I've created a work-around the Python dependency by patching the Morsel and SimpleCookie objects.

Mariatta · 2017-11-24T05:21:01Z

Please rebase.

alex · 2018-04-07T18:13:11Z

I've rebased this over at #6413, should be ready to merge.

…ted in CPython. Ref python#214.

…and not a compatibility feature. Add DegenerateFiles for readers that don't implement the files API such that the rest of the API can rely on it being implemented. Fixes python#214.

the-knights-who-say-ni added the CLA not signed label Feb 21, 2017

alex approved these changes Feb 21, 2017

View reviewed changes

Mariatta reviewed Feb 21, 2017

View reviewed changes

Mariatta requested changes Feb 21, 2017

View reviewed changes

Mariatta removed the CLA not signed label Feb 21, 2017

the-knights-who-say-ni added the CLA signed label Feb 21, 2017

Mariatta reviewed Feb 21, 2017

View reviewed changes

Mariatta reviewed Feb 22, 2017

View reviewed changes

berkerpeksag requested changes Feb 22, 2017

View reviewed changes

timgraham reviewed Feb 22, 2017

View reviewed changes

akash0x53 added 7 commits February 28, 2017 22:10

bpo-29613: Added support for SameSite cookies

ef83909

Implemented as per draft https://tools.ietf.org/html/draft-west-first-party-cookies-07

Documented SameSite

c8bc135

And suggestions by members.

Missing space :(

c064da4

Updated News and contributors

364ea46

Added version changed details.

c3890a2

Fix in documentation

6ed3f3f

fix in documentation

aa54e50

Clubbed test cases for same attribute into single.

a6cbc20

akash0x53 force-pushed the fix-issue-29613 branch from 72156b7 to a6cbc20 Compare February 28, 2017 16:43

Mariatta approved these changes Jun 6, 2017

View reviewed changes

provinzkraut mentioned this pull request Jul 13, 2017

"SameSite" cookie feature (from issue #982) bottlepy/bottle#983

Merged

asvetlov mentioned this pull request Oct 3, 2017

Implement same-site cookies aio-libs/aiohttp#1049

Closed

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

brettcannon added the awaiting merge label Feb 21, 2018

alex mentioned this pull request Apr 7, 2018

bpo-29613: Added support for SameSite cookies #6413

Merged

alex closed this in #6413 Apr 7, 2018

btharper mentioned this pull request Oct 6, 2019

bpo-36356: Fix memory leak in _asynciomodule.c during "setup.py build_ext" #16598

Merged

jaraco added a commit to jaraco/cpython that referenced this pull request Feb 17, 2023

Avoid adapting the loader in 'no reader fallback' as it won't be adap…

00203eb

…ted in CPython. Ref python#214.

bpo-29613: Added support for SameSite cookies #214

bpo-29613: Added support for SameSite cookies #214

Conversation

akash0x53 commented Feb 21, 2017

the-knights-who-say-ni commented Feb 21, 2017

alex commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mariatta left a comment

Choose a reason for hiding this comment

Mariatta commented Feb 21, 2017

alex commented Feb 21, 2017

Mariatta commented Feb 21, 2017

akash0x53 commented Feb 21, 2017

akash0x53 commented Feb 21, 2017

timgraham commented Feb 21, 2017

Mariatta commented Feb 21, 2017

alex commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akash0x53 commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkerpeksag commented Feb 27, 2017

alex commented Apr 1, 2017

akash0x53 commented Apr 1, 2017

kravietz commented May 3, 2017

akash0x53 commented May 4, 2017 • edited Loading

kravietz commented May 4, 2017

Mariatta commented Nov 24, 2017

alex commented Apr 7, 2018

akash0x53 commented May 4, 2017 •

edited

Loading