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

Append Vary: Cookies header to response when session is accessed #1026

Closed
wants to merge 2 commits into from

Conversation

citruspi
Copy link
Contributor

To prevent caching proxies like Varnish from providing multiple users with the same cache, you can set a Vary: Cookie header.

This pull request appends the header to each response when the session is accessed.

I came across this issue while working on Warehouse, PyPi 2.0. @dstufft wanted something that added the header to the response anytime the session was being used.

@citruspi + @abadger

@citruspi
Copy link
Contributor Author

At this point, the tests fail because you check for cookie headers in this test and expect none, but we're adding them because flask.session is accessed and we need to make sure that the cookie is available for the proxy server to add to the key.

@citruspi + @abadger

@DasIch
Copy link
Contributor

DasIch commented Apr 28, 2014

I didn't know exactly how Vary: Cookie worked before and I've read up on it a little bit. If I understand correctly the idea is, that a cache should generally ignore cookies entirely in his consideration on whether or not to send a cached response. If a server responds to a request with the Vary: Cookie header, the response depends on the cookie and therefore, the cache has to consider the existance and value of the cookie in the request to determine whether or not to send a response.

Please correct me, if anything about that is wrong. Anyway based on this I don't see why the cookie has to be set, if we respond with Vary: cookie, any cookie set in the response should be irrelevant to the cache, given the mechanism described earlier.

@abadger
Copy link

abadger commented Apr 30, 2014

Note: Sorry, github made it look like I double posted this so I deleted one of them. Looks like it actually wasn't a double post so I'm reposting now. :-/

Vary: Cookie allows caching the response but the cache has to use the values in the cookie as part of its key. That way the cache can save separate entries for a request for /user/profile/ when there's a cookie for username=abadger and a request for /user/profile/ when the cookie is for username=citruspi

@danielchatfield
Copy link

As per the spec the header fields listed in Vary should be used by the cache to determine whether a cached response is valid or not and thus two requests with different cookies but otherwise identical headers would not match.

It should be noted that not all caches (including varnish and Google's page speed service) implement what is described by @abadger and instead just drop caching for that request altogether with this header.

Despite this, I've got a great big 👍 for this PR, this is a very clean and nice way of ensuring that whenever a request relies upon the data stored in the session then the cache will not serve a cached response if the session data has changed.

@mitsuhiko
Copy link
Contributor

I'm not so sure myself yet. I would love to get more input on this. For me this sounds like a neat idea but I need to think more about it.

@jeffwidman
Copy link
Contributor

As of Nginx 1.7.7, the Nginx proxy cache (commonly used alongside uWSGI) also supports using the Vary header to manage what gets cached/returned--see the final FAQ question here: https://www.nginx.com/blog/nginx-caching-guide/

I didn't dig into the implementation much, but I'm +1 on the general idea here.

# whole cookie.
if not session:
if session.modified:
response.delete_cookie(app.session_cookie_name,

This comment was marked as off-topic.

@untitaker
Copy link
Contributor

untitaker commented Apr 23, 2017

This appears to be the only way for Flask itself to do this automatically, but I'm not sure whose responsibility we want caching config to be.

Perhaps this should be part of something like Flask-Login (e.g.: everything decorated with login_required sends with header).

I'd much rather see an issue opened about this before we discuss a concrete implementation.

@davidism
Copy link
Member

davidism commented May 2, 2017

Django adds Vary: Cookie based on session access: https://github.com/django/django/blob/1ce04bcce0076360623ae164afd3541a5c031af2/django/contrib/sessions/middleware.py#L45. It also checks to delete the cookie before adding the header.

@untitaker
Copy link
Contributor

Then I suppose I am fine with it conceptually.

@davidism
Copy link
Member

Continued in #2288.

@davidism davidism closed this May 20, 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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants