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

Cookie value of more than 4096 bytes should not fail silently #780

Merged
merged 5 commits into from Apr 18, 2017

Conversation

Projects
None yet
4 participants
@Jaza
Copy link
Contributor

commented Oct 12, 2015

For more details on cookie size limits, see:

http://stackoverflow.com/questions/640938/what-is-the-maximum-size-of-a-web-browsers-cookies-key

A similar ticket was opened for Django - see:

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

The Django folks decided not to raise an exception when this happens, because they're using Python's standard Cookie library, so they'd have to duplicate the code to build / output the cookie value. Although they did update their documentation.

This is not an issue for Werkzeug, because it implements the raw encoding of the cookie by itself in dump_cookie(), rather than relying on Python Cookie. So, checking the size and raising an exception is a trivial change for Werkzeug.

Jaza added some commits Oct 12, 2015

throw an exception if trying to set a cookie with size of more than 4…
…096 bytes (the standard limit set by browsers); without this check, oversized cookie values get lost silently, which is hard to debug
@Jaza

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

Is anyone able to review and/or merge this, please? There has been no activity in this PR for over a year, and nobody has reviewed it since I created it a year and a half ago.

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

I'm going through all the PRs right now, you may have noticed the uptick in notifications if you're watching the repo. This is on the list.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

LGTM, please add a changelog entry. Also we should probably add docs.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Also we might consider adding a way to disable this restriction for very obscure usecases (custom clients?). After all this is not part an RFC afaict.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

(also @davidism feel free to request more reviews from me, I will get notified)

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Maybe this should behave like max_content_length, where the property is set on the request (so can be overridden by a Flask config) and passed as an argument. Right now, you'd have to change a global variable, which doesn't seem like good practice.

We can still merge this version in, and then make another patch that extends it.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Whichever route we choose, I'd be against releasing a version of Werkzeug that comes with this restriction without a good way to change/disable it.

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

It's possible to essentially disable it now, by setting the global to sys.maxsize. Not that that's particularly nice.

@ThiefMaster

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Monkeypatching is an awful way to do that :)

Maybe dump_cookie could take an argument with the default value being 4096. Since it's probably called from a method in the Response class it could be set to a class attribute - that way people could change the limit by subclassing Response (which is something that would work well in Flask too).

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Merging this now, I have a patch for part 2 that I'll submit in a minute.

@davidism davidism merged commit f6564aa into pallets:master Apr 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@untitaker untitaker removed the ready label Apr 18, 2017

@davidism

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

After some more discussion in #1109 I changed this to be a warning that describes how the size was being exceeded. It also moved the configuration into Response.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.