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

Support for setting partitioned cookies #9870

Open
1 task done
FeldrinH opened this issue Nov 14, 2024 · 12 comments
Open
1 task done

Support for setting partitioned cookies #9870

FeldrinH opened this issue Nov 14, 2024 · 12 comments
Labels
enhancement waiting-for-upstream We are waiting for an upstream change

Comments

@FeldrinH
Copy link

Is your feature request related to a problem?

I need to use partitioned cookies to set cookies in contexts where third-party cookies are otherwise restricted.

Describe the solution you'd like

A recent addition to the Set-Cookie header is the ability to mark cookies as partitioned (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#partitioned, https://developer.mozilla.org/en-US/docs/Web/Privacy/Privacy_sandbox/Partitioned_cookies).

StreamResponse.set_cookie should support setting partitioned cookies, e.g. using a partitioned=True keyword argument.

Describe alternatives you've considered

I can create a 'Set-Cookie' header manually, but would have to figure out edge cases with encoding and escaping special characters myself. This would be inconvenient and probably error-prone.

Related component

Server

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@Dreamsorcerer
Copy link
Member

To be clear, you only need to set the flag when sending cookies? You're not asking for partitioned storage when receiving them (which would be a lot more work)?

@bdraco
Copy link
Member

bdraco commented Nov 14, 2024

I'm pretty sure this is a problem in Python itself and is fixed in Python 3.13+

Will need to check when I'm back at my desk

@bdraco
Copy link
Member

bdraco commented Nov 14, 2024

if sys.version_info[:2] < (3, 14):
    from http import cookies

    # See: https://github.com/python/cpython/issues/112713
    cookies.Morsel._reserved["partitioned"] = "partitioned"  # type: ignore[attr-defined]
    cookies.Morsel._flags.add("partitioned")  # type: ignore[attr-defined]

That should work as a workaround if you can't upgrade to Python 3.13

@bdraco
Copy link
Member

bdraco commented Nov 14, 2024

Looks like python/cpython#112714 didn't actually merge into 3.13 so the guard might need to be (3, 14) instead

@Dreamsorcerer
Copy link
Member

For setting them, we'd need to add the parameters though:

def set_cookie(

Should be a nice easy PR for anyone wanting to volunteer.

@Dreamsorcerer
Copy link
Member

Yeah, so it is rejected currently:

>>> m["partitioned"] = "1"
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.9/http/cookies.py", line 312, in __setitem__
    raise CookieError("Invalid attribute %r" % (K,))
http.cookies.CookieError: Invalid attribute 'partitioned'

So, will need that PR merged in cpython before we can do anything here.

@Dreamsorcerer Dreamsorcerer added the waiting-for-upstream We are waiting for an upstream change label Nov 14, 2024
@FeldrinH
Copy link
Author

Would it be possible for aiohttp to provide some kind of polyfill/backport of this feature? Waiting for Python 3.14, which comes out next year and will take potentially years to roll out to production servers does not seem ideal.

@Dreamsorcerer
Copy link
Member

Seems like we could include the hack as shown above: #9870 (comment)

I think we'd atleast want to know that the upstream PR has been merged and which version it will be released in first though.

@asvetlov
Copy link
Member

I'm strongly against monkey-patching stdlib for adding new features.
The monkeypatching could be acceptable after careful thinking for fixing stdlib bugs; we did it in aiohttp several times.
IIRC check_hostname from SSL had a bug for example.
But for a new feature this approach is much more questionable.

Plus, if aiohttp user really need CHIPS we provide low-level resp.cookies property already.
If an application patches stdlib to make a particular web service work -- it's totally ok.
But if aiohttp wants the same -- please don't.

I can imagine a workaround for aiohttp itself though.
We can copy ./Lib/http/cookies.py from stdlib to aiohttp, patch it and ship as a private vendored aiohttp file. Maybe tests for it are not required; we trust CPython.
By this, we are avoiding potential breaking problems from wild http.cookies change in upcoming python releases. Who knows the future CPython changes?

@Dreamsorcerer
Copy link
Member

We can also just add the parameter without a version check. Then users can do the monkeypatch on older releases if they need it.

@asvetlov
Copy link
Member

Works for me as well, and much less code to maintain

@jinnatar
Copy link

jinnatar commented Dec 6, 2024

Relatedly (too lazy right now to open a separate issue) a response Set-Cookie header with partitioned fails to get stored into response.cookies and the session cookie_jar, I assume because SimpleCookie just silently doesn't actually load invalid cookies. So as these headers start to be out there in the wild, you may start getting confusing support requests when cookies are just silently not getting stored into sessions. Some amount of monkeypatching is thus probably a good idea around there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement waiting-for-upstream We are waiting for an upstream change
Projects
None yet
Development

No branches or pull requests

5 participants