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

[server] Add warning when a cookie's length would exceed the RFC 6265 minimum user-agent support #5634

Closed
panagiks opened this issue Apr 20, 2021 · 3 comments · Fixed by #5959
Labels
enhancement good first issue Good for newcomers Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/

Comments

@panagiks
Copy link
Contributor

🐣 Is your feature request related to a problem? Please describe.
We had a report in aiohttp-session (see aio-libs/aiohttp-session#574) that a user submitting a 'large' cookie had the cookie dropped by the receiving browser.

According to RFC 6265 - section 6.1:

General-use user agents SHOULD
provide each of the following minimum capabilities:
o At least 4096 bytes per cookie (as measured by the sum of the
length of the cookie's name, value, and attributes).

This means that cookies that exceed 4096 bytes in storage (not transmission) have a chance that the user-agent will drop them which is a case that is hard to 'guess' for a developer.

💡 Describe the solution you'd like
A warning log should be emitted if a cookie that would exceed the RFC minimum support is created in aiohttp server.

A challenge here is the definition of 'exceed' because aiohttp knows the transmitted size but the RFC refers to (and the user-agents seem to implement) a check on the final stored size of the cookie string, including all field names and values regardless of whether they were transmitted or filled in as defaults by the user-agent.

Describe alternatives you've considered
It was briefly considered implementing this check in aiohttp-session but was rejected since this is a general issue with cookies and not specific to aiohttp-session so such a check could benefit all aiohttp users.

📋 Additional context
None.

@Dreamsorcerer Dreamsorcerer added good first issue Good for newcomers Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/ labels Apr 20, 2021
@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Apr 20, 2021

The warning would go somewhere around:

self._cookies[name] = value

Maybe requires a little trial and error with a browser forcing this limit, to help figure out exactly what is included in the size limit (the linked issue mentions this being reproducible in Chrome 89).

But, at a guess, this might look something like: len(name) + len(value) + sum(len(k) + len(v) for k, v in c.items())

@anesabml
Copy link
Contributor

anesabml commented Aug 9, 2021

@Dreamsorcerer I came across a similar bug report in Django, eventually, they didn't implement a warning or anything like that (they added a warning to the documentation), because of performance reasons (calculating the whole length of every cookie in the cookies set can introduce a performance overhead).

https://code.djangoproject.com/ticket/22242

@Dreamsorcerer
Copy link
Member

Well, that's what dev mode is for, then atleast it will catch errors for people during development.
Just wrap it in if sys.flags.dev_mode:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement good first issue Good for newcomers Hacktoberfest We think it's good for https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants