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.

Sign up for GitHub

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

Jump to bottom

Don't overwrite Vary header when setting for cookie access #2322

Merged
merged 2 commits into from
May 25, 2017
Merged

Don't overwrite Vary header when setting for cookie access #2322

merged 2 commits into from
May 25, 2017
+45 −11

Conversation

nnja
Copy link
Contributor

@nnja nnja commented May 23, 2017

Closes #2317

@nnja nnja changed the title Don't overwrite Vary header when setting for cookie access #2317 Don't overwrite Vary header when setting for cookie access May 23, 2017
ThiefMaster
ThiefMaster previously requested changes May 24, 2017
View reviewed changes
flask/sessions.py Outdated
vary_headers.append('Cookie')

updated_header = ', '.join(header for header in vary_headers if header)
response.headers.set('Vary', updated_header)

This comment was marked as off-topic.

Sign in to view

flask/sessions.py Outdated
@@ -348,6 +349,8 @@ class SecureCookieSessionInterface(SessionInterface):
"""The default session interface that stores sessions in signed cookies
through the :mod:`itsdangerous` module.
"""
#: the regular expression used to split the value of the 'Vary' header
comma_delim_header_re = re.compile(r'\s*,\s*')

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

davidism
davidism requested changes May 24, 2017
View reviewed changes
flask/sessions.py Outdated

The header should retain it's original ordering.
"""
vary_headers = self.comma_delim_header_re.split(response.headers.get('Vary', ''))

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

flask/sessions.py Outdated
The header should retain it's original ordering.
"""
vary_headers = self.comma_delim_header_re.split(response.headers.get('Vary', ''))
if 'Cookie' not in vary_headers:

This comment was marked as off-topic.

Sign in to view

@nnja nnja force-pushed the dont_overwrite_vary_header branch from 18e2106 to e7cd68b Compare May 25, 2017 03:06
@codecov
Copy link

codecov bot commented May 25, 2017 •
edited
Loading

Codecov Report

Merging #2322 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2322      +/-   ##
==========================================
+ Coverage   92.79%   92.82%   +0.02%     
==========================================
  Files          45       45              
  Lines        6333     6355      +22     
  Branches      496      498       +2     
==========================================
+ Hits         5877     5899      +22     
  Misses        335      335              
  Partials      121      121
Impacted Files Coverage Δ
flask/sessions.py 95.73% <100%> (+0.02%) ⬆️
flask/helpers.py 83.77% <100%> (+0.39%) ⬆️
tests/test_basic.py 96.5% <100%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ec1fbc...bdf31cd. Read the comment docs.

@nnja
Copy link
Contributor Author

nnja commented May 25, 2017

Requested changes submitted, ready for re-review.

@davidism
Copy link
Member

I extracted and renamed the method to flask.helpers.patch_vary_header since it could be useful to others modifying the Vary header. I'd also like to see this in werkzeug.http so that we can eventually import it from there instead, if you'd like to make another PR to Werkzeug.

@davidism davidism dismissed ThiefMaster’s stale review May 25, 2017 17:29

made requested changes

@davidism davidism merged commit cb94f4c into pallets:master May 25, 2017
@davidism davidism mentioned this pull request May 29, 2017
Closed
@davidism
Copy link
Member

😞 turns out there's already support for this in Werkzeug. #2345

response.vary.add('Cookie')

@davidism davidism modified the milestone: 1.0 Jul 31, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ThiefMaster ThiefMaster

@davidism davidism

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

Don't overwrite Vary header when setting for cookie access
3 participants
@nnja @davidism @ThiefMaster