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

Sessions cookie changes to support persistent cookies and partitioned cookies #2527

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ToasterChicken
Copy link

  • Issues #2019 Added same_site for CHIPS cookie support
  • Discussion #2441 cookies are missing the partitioned option.

Updated Documentation for changes
Updated Unit Test to include both new SessionsMiddleware features.

Summary

Added cookie persistence to the sessions middleware so the cookie isn't refreshed on every request for issue #2019

Clarified the security regarding the session cookie, that it is not encrypted and not considered private but is not modifiable.

Added the partitioned cookies flag for the cookies to support CHIPS (Cookies Having Independent Partitioned State) to support cookies being set in iframes if the middleware being used to set a 3rd party cookie for a different top level site. Discussion #2441

Checklist

  • [ X] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • [ X] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • [ X] I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Feb 24, 2024

Too many changes here, and no previous discussions about this implementation.

We need a bit more granularity on those changes.

@ToasterChicken
Copy link
Author

@Kludex I linked an issue and a discussion for the two changes submitted for this PR. When I say there's two changes, there's two changes including their associated test updates and documentation.

First the sessions middleware generates a brand new cookie on every single request made so the large change adds cookie persistence. If a page makes multiple requests the middle ware will set a new cookie on every single response. #2019.

Second issue the cookies do not support CHIPS partitioned cookies that Chromium started enforcing in December for 3rd party cookies causing the cookies to be rejected when it's the 3rd party cookie. #2441

Would it be better to submit two PR's?

@Kludex
Copy link
Member

Kludex commented Feb 29, 2024

Yes, please.

@Kludex
Copy link
Member

Kludex commented Feb 29, 2024

@ToasterChicken
Copy link
Author

This PR #2501 appears to be for the responses module, the Sessions Middleware isn't currently using the responses module for cookie handling or http.cookies. It is somewhat unfortunate that there's an inconsistency here.

The sessions middleware is pretty basic building a string for the cookie's attributes and setting it using the datastructures MutableHeaders by way of appending a "Set-Cookie" header in the message. It is maybe unfortunate that there's an inconsistency here with cookie handling.

The discussion on python/cpython#112714 is rather interesting, the PR is on hold until the feature is ratified. Browser compatibility for the experimental feature looks to be just Chromium based browsers, WebKit and FireFox are not listed as supporting it, although it doesn't appear to be an issue if the value is present.

I'll split up the PR and submit the persistence first.

Having read the discussion on the cpython stdlib discussion, It makes sense they have opted to refrain from implementing a pending standard. The change to the sessions middleware to add it is minor if adding it as an option makes sense now, or if the sessions middleware should be updated to use the stdlib for its cookie handling and add the feature once it also supports it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants