-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
gh-112713 : Add support for 'partitioned' attribute in http.cookies #112714
base: main
Are you sure you want to change the base?
Conversation
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Doc/library/http.cookies.rst
Outdated
The attribute :attr:`partitioned` indicates to user agents that these | ||
cross-site cookies *should* only be available in the same top-level context | ||
that the cookie was first set in. For this to be accepted by the user agent, | ||
you **must** also set both ``Secure`` and ``Path=/``. |
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 updated the wording here to clarify that Secure is required... but the CHIPS spec doesn't explicitly say anything about Path= though Path=/ appears in all of its examples. What wording should be used regarding Path, I'm not sure how I've phrased this is wholly accurate.
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.
Per privacycg/CHIPS#49, Path=/
is not needed. I'm going to push an update to this PR shortly.
@@ -121,6 +121,14 @@ def test_set_secure_httponly_attrs(self): | |||
self.assertEqual(C.output(), | |||
'Set-Cookie: Customer="WILE_E_COYOTE"; HttpOnly; Secure') | |||
|
|||
def test_set_secure_httponly_partitioned_attrs(self): |
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.
should set set Path=/ to also serve as a better example?
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.
Since Path=/
is no longer required for CHIPS, it seems like this is probably fine as-is.
I'm leaving this as a Draft PR as whether or not this is desirable isn't settled - it is not yet a standard. We normally wait until something sees actual accepted adoption. |
Understood that CHIPS is a draft and that it probably makes sense to wait until it's ratified before adding to the stdlib. My understanding from reading the RFCs is that it has tentative support from Mozilla and Apple, so hopefully it will be mergeable whenever the wheels of the working groups turn! :) I want to address this comment from #112713 by @gpshead:
We are planning to provide a custom patch which does something like this (figuratively): original_morsel_output = Cookie.Morsel.output
def patched_morsel_output(
self: _MorselType,
attrs: Optional[List[str]] = None,
header: str = "Set-Cookie:",
):
cookie_str = original_morsel_output(self, attrs, header)
if "samesite=none" in cookie_str.lower():
cookie_str += "; Partitioned"
return cookie_str
Cookie.Morsel.output = patched_morsel_output It's not ideal, but gives us space to also extend the stdlib in other ways since we'll be importing this instead of |
@giles-v Please note that discussions on PRs should be about the implementation; wider discussion about the request itself happens on the issue or on python-ideas. |
Lib/http/cookies.py
Outdated
"secure": "Secure", | ||
"httponly": "HttpOnly", | ||
"version": "Version", | ||
"samesite": "SameSite", |
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.
Let’s not touch existing lines (better for git history etc)
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 can't avoid it either way, because if we keep the spacing alignment we'll need to realign all the lines to partitioned
, being the longest key now.
I'm going to retain @gpshead's preference that we drop the alignment spacing in my rebase commit, and happy to follow whatever consensus emerges if you disagree.
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.
No, keep the existing lines as is and don’t follow the alignment for the new line. Easiest thing to do!
@merwok updated and marked Ready for Review. My rebasing skills are weak (sorry) but the changeset is correct. |
I don't think so. 3.13 won't accept new features per PEP 719: https://peps.python.org/pep-0719/#schedule (thus I'll remove the label). @merwok By the way, we use the "3.x" labels on issues and "needs backport 3.x" on PRs. |
This has missed the beta window, but I think an argument could be made to core-devs and RM on discuss that this change is useful to deal with an external standard. (I don’t have the time to be the one to do that) We’ve had such a policy for changes to mimetypes for example. On the other hand, this is not a simple addition to a data dictionary, but a code change which we would not backport to stable branches, so the reply could be negative. (Thanks for the gardening picnixz. GPS added the label, not I) |
I would be glad to support such an argument if it's public; given the timeline of Google's CHIPS adoption, waiting for the next release is going to be onerous for users. As someone who has not engaged with the Python community before however I don't think I have any standing to drive a discussion like that either. |
In this case, this could be categorized as type-security/bug fix (bugfixes would get backported to 3.12 and security fixes until 3.8) cc @Yhg1s |
I think arguing for this feature as a security fix would go too far. |
Fixes #112713.
This PR adds support for the new
Partitioned
attribute in the Morsel object inhttp.cookies
.📚 Documentation preview 📚: https://cpython-previews--112714.org.readthedocs.build/