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

Allow configuring or disabling max cookie size check #1109

Merged
merged 3 commits into from Apr 18, 2017

Conversation

Projects
None yet
3 participants
@davidism
Copy link
Member

commented Apr 18, 2017

Continue #780

  • use arg instead of const for dump_cookie max_size
  • add max_cookie_size attr to BaseResponse
  • add changelog

@davidism davidism self-assigned this Apr 18, 2017

use arg instead of const for dump_cookie max_size
add max_cookie_size attr to BaseResponse
add changelog for #780
@@ -1072,15 +1083,11 @@ def set_cookie(self, key, value='', max_age=None, expires=None,
extension to the cookie standard and probably not
supported by all browsers.
"""
self.headers.add('Set-Cookie', dump_cookie(key,
value=value,

This comment has been minimized.

Copy link
@untitaker

untitaker Apr 18, 2017

Member

Can we keep the vertical alignment of all keys? IMO easier to read. Of course the linebreak before the first one is an improvement.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

LGTM, might need a testcase for custom values though

@davidism

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

The error is slightly confusing because you can pass 4093 bytes as the value and still get the error, since the options are added to the payload. I don't think we need to change it, just pointing it out.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

@davidism

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

Maybe this should be a warning? Could use a filter to turn it into an error in Flask debug mode.

@davidism

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2017

How about detecting the before and after version, then warning

The "{key}" cookie is too large: the value was {value_size} bytes but encoding the header required {header_size - value_size} extra bytes. The final size was {header_size} but the limit is {max} bytes. Browsers may silently ignore cookies larger than this.

@untitaker

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

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

1 check passed

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

@untitaker untitaker removed the in progress label Apr 18, 2017

@davidism davidism deleted the davidism:max-cookie-size branch Apr 18, 2017

@Jaza

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2017

Thanks @davidism for making this configurable, there should be need to override the limit of 4k per cookie in most cases, but yes, some people might want to change it.

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.