GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29613: Added support for SameSite cookies by akash0x53 · Pull Request #214 · python/cpython

Conversation

@akash0x53

@akash0x53 akash0x53 commented Feb 21, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
alex, advance512, and sush-abc reacted with hooray emoji
All reactions
  • 🎉 3 reactions
@the-knights-who-say-ni

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

Copy link
Copy Markdown

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:

  1. If you don't have an account on b.p.o, please create one
  2. Make sure your GitHub username is listed in "Your Details" at b.p.o
  3. If you have not already done so, please sign the PSF contributor agreement
  4. 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)
  5. Reply here saying you have completed the above steps

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Feb 21, 2017
alex
alex approved these changes Feb 21, 2017
View reviewed changes
@alex

alex commented Feb 21, 2017

Copy link
Copy Markdown
Member

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)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta
Mariatta reviewed Feb 21, 2017
View reviewed changes
Comment thread Lib/http/cookies.py Outdated

@Mariatta Mariatta Feb 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@timgraham timgraham Feb 21, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta and sush-abc reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Mariatta
Mariatta requested changes Feb 21, 2017
View reviewed changes

@Mariatta Mariatta left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the docs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Mariatta

Mariatta commented Feb 21, 2017

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@alex

alex commented Feb 21, 2017

Copy link
Copy Markdown
Member

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta

Mariatta commented Feb 21, 2017

Copy link
Copy Markdown
Member

Sounds good. Thanks @alex 😄
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@akash0x53

akash0x53 commented Feb 21, 2017

Copy link
Copy Markdown
Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@akash0x53

akash0x53 commented Feb 21, 2017

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@timgraham

timgraham commented Feb 21, 2017

Copy link
Copy Markdown
Contributor

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".
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Mariatta Mariatta removed the CLA not signed label Feb 21, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 21, 2017
@Mariatta

Mariatta commented Feb 21, 2017

Copy link
Copy Markdown
Member

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@alex

alex commented Feb 21, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta
Mariatta reviewed Feb 21, 2017
View reviewed changes
Comment thread Doc/library/http.cookies.rst Outdated

@Mariatta Mariatta Feb 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add below this line:

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

Thanks @alex for the clarification about this :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Mariatta
Mariatta reviewed Feb 22, 2017
View reviewed changes
Comment thread Doc/library/http.cookies.rst Outdated

@Mariatta Mariatta Feb 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
berkerpeksag
berkerpeksag requested changes Feb 22, 2017
View reviewed changes

@berkerpeksag berkerpeksag left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
berkerpeksag
berkerpeksag requested changes Feb 22, 2017
View reviewed changes
Comment thread Lib/test/test_http_cookies.py Outdated

@berkerpeksag berkerpeksag Feb 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/http.cookies.rst Outdated

@berkerpeksag berkerpeksag Feb 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this to below the other version directives:

[...]

.. versionchanged:: 3.5
   [...]

.. versionchanged:: 3.7
   [...]

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akash0x53 akash0x53 Feb 22, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".
+

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Feb 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
akash0x53 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@akash0x53

akash0x53 commented Feb 22, 2017

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
timgraham
timgraham reviewed Feb 22, 2017
View reviewed changes
Comment thread Doc/library/http.cookies.rst Outdated

@timgraham timgraham Feb 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Doc/library/http.cookies.rst Outdated

@timgraham timgraham Feb 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Doc/library/http.cookies.rst Outdated

@timgraham timgraham Feb 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_http_cookies.py Outdated

@timgraham timgraham Feb 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation missing

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akash0x53 akash0x53 Feb 25, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Feb 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/library/http.cookies.rst Outdated

@timgraham timgraham Feb 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akash0x53 akash0x53 Feb 25, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@berkerpeksag berkerpeksag Feb 27, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@akash0x53 akash0x53 Feb 27, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@timgraham timgraham Feb 27, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_http_cookies.py Outdated

@timgraham timgraham Feb 22, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use single quotes for consistency?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
akash0x53 and Mariatta reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@berkerpeksag

berkerpeksag commented Feb 27, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
akash0x53 added 7 commits February 28, 2017 22:10
@akash0x53
bpo-29613: Added support for SameSite cookies
ef83909 
Implemented as per draft
https://tools.ietf.org/html/draft-west-first-party-cookies-07
@akash0x53
Documented SameSite
c8bc135 
And suggestions by members.
@akash0x53
Missing space :(
c064da4
@akash0x53
Updated News and contributors
364ea46
@akash0x53
Added version changed details.
c3890a2
@akash0x53
Fix in documentation
6ed3f3f
@akash0x53
fix in documentation
aa54e50
@akash0x53
Clubbed test cases for same attribute into single.
a6cbc20
@akash0x53
akash0x53 force-pushed the fix-issue-29613 branch from 72156b7 to a6cbc20 Compare February 28, 2017 16:43
@alex

alex commented Apr 1, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@akash0x53

akash0x53 commented Apr 1, 2017

Copy link
Copy Markdown
Author

Yes @alex , I fix it asap.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kravietz

kravietz commented May 3, 2017

Copy link
Copy Markdown

@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@akash0x53

akash0x53 commented May 4, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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 😕
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@kravietz

kravietz commented May 4, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Mariatta
Mariatta approved these changes Jun 6, 2017
View reviewed changes
@provinzkraut provinzkraut mentioned this pull request Jul 13, 2017
"SameSite" cookie feature (from issue #982) bottlepy/bottle#983
Merged
@asvetlov asvetlov mentioned this pull request Oct 3, 2017
Implement same-site cookies aio-libs/aiohttp#1049
Closed
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@Mariatta

Mariatta commented Nov 24, 2017

Copy link
Copy Markdown
Member

Please rebase.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon brettcannon added the awaiting merge label Feb 21, 2018
@alex alex mentioned this pull request Apr 7, 2018
bpo-29613: Added support for SameSite cookies #6413
Merged
@alex

alex commented Apr 7, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@alex alex closed this in #6413 Apr 7, 2018
@btharper 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
@jaraco
Avoid adapting the loader in 'no reader fallback' as it won't be adap…
00203eb 
…ted in CPython. Ref python#214.
jaraco added a commit to jaraco/cpython that referenced this pull request Feb 17, 2023
@jaraco
Add _adapters module, making SpecLoaderAdapter a first-class feature …
8207434 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag berkerpeksag requested changes
@alex alex alex approved these changes
@Mariatta Mariatta Mariatta approved these changes
+1 more reviewer
@timgraham timgraham timgraham left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

8 participants

@akash0x53 @the-knights-who-say-ni @alex @Mariatta @timgraham @berkerpeksag @kravietz @brettcannon
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.