Opened 6 weeks ago
Last modified 6 weeks ago
#36399 assigned New feature
Missing cookies when using ASGI and HTTP/2
Reported by: | Ingmar Stein | Owned by: | JaeHyuckSa |
---|---|---|---|
Component: | HTTP handling | Version: | 5.2 |
Severity: | Normal | Keywords: | |
Cc: | Carlton Gibson | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | yes | UI/UX: | no |
Description
I originally created the report here: https://forum.djangoproject.com/t/missing-cookies-when-using-asgi-and-http-2/40946
https://github.com/paperless-ngx/paperless-ngx/issues/9935 describes the issue in more detail. In a nutshell: when serving a Django app using ASGI and HTTP/2, cookies may get dropped. In case this hits the csrftoken
cookie, it might explain the various "CSRF verification failed" topics in this forum category.
I had a brief look at the coke and it looks like the ASGI module joins multiple values for the same header using commas but `parse_cookie` splits by semicolon.
Same same issue has also hit other ASGI frameworks: https://github.com/encode/starlette/discussions/2916
@carltongibson created this minimal repro:
from django.conf import settings from django.core.handlers.asgi import ASGIRequest settings.configure(DEBUG=True) scope = { "type": "http", "asgi": { "version": "3.0", "spec_version": "2.3", }, "http_version": "2.0", "method": "GET", "scheme": "http", "path": "/", "raw_path": b"/", "query_string": b"", "root_path": "", "headers": [ (b"cookie", b"a=abc;"), (b"cookie", b"b=def;"), (b"cookie", b"c=ghi;") ], "client": ("127.0.0.1", 10000), "server": ("127.0.0.1", 8000), "extensions": {} } request = ASGIRequest(scope, None) print(request.COOKIES) # Prints: {'a': 'abc', ',b': 'def', ',c': 'ghi'} assert request.COOKIES == {'a': 'abc', 'b': 'def', 'c': 'ghi'}
Change History (12)
comment:1 by , 6 weeks ago
Cc: | added |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 6 weeks ago
Cc: | removed |
---|---|
Easy pickings: | set |
Type: | Bug → New feature |
Yes, I confirm this.
ASGIRequest
will need to special case the cookie header in a similar way to how it handles content-length &co before entering the _generic headers_ loop.
Multiple cookie values need to be joined with semicolons — which are already present terminating the value — not commas.
Looks like HTTP/1 didn't allow multiple Cookie headers, and they had to be automatically grouped by the client, where HTTP/2 does allow this.
Since this has never been supported I'll mark it a new feature: it probably merits a line in the release notes.
comment:3 by , 6 weeks ago
Cc: | added |
---|
comment:4 by , 6 weeks ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 6 weeks ago
Has patch: | set |
---|
comment:6 by , 6 weeks ago
Patch needs improvement: | set |
---|
comment:7 by , 6 weeks ago
Patch needs improvement: | unset |
---|
comment:8 by , 6 weeks ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:11 by , 6 weeks ago
This doesn't qualify for a backport but you can easily do yourself by adjusting your asgi.py
file.
Something like...
# asgi.py import os # Restore when removing ASGIRequest backport. # from django.core.asgi import get_asgi_application # Imports for Backport import django from django.core.handlers.asgi import ASGIHandler # https://code.djangoproject.com/ticket/36399 assert django.VERSION < (6,0), "Remove ASGIRequest backport." # Inline ASGIRequest here. # You can copy/paste the whole class or just override __init__. Up to you. # Subclass ASGIHandler to use your request class. class BackportASGIHandler(ASGIHandler): request_class = # Your backported request class. os.environ.setdefault("DJANGO_SETTINGS_MODULE", "your_app.settings") # Restore when removing ASGIRequest backport. # application = get_asgi_application() django.setup(set_prefix=False) application = BackportASGIHandler()
That's just a sketch. Adjust to your needs.
comment:12 by , 6 weeks ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
Thank you, replicated