Merge pull request #2288 from davidism/vary-cookies

Set "Vary: Cookie" header when session is accessed

flask/sessions.py

@@ -49,6 +49,13 @@ def _set_permanent(self, value):
     #: The default mixin implementation just hardcodes ``True`` in.
     modified = True
 
+    #: the accessed variable indicates whether or not the session object has
+    #: been accessed in that request. This allows flask to append a `Vary:
+    #: Cookie` header to the response if the session is being accessed. This
+    #: allows caching proxy servers, like Varnish, to use both the URL and the
+    #: session cookie as keys when caching pages, preventing multiple users
+    #: from being served the same cache.
+    accessed = True
 
 def _tag(value):
     if isinstance(value, tuple):
@@ -117,8 +124,23 @@ class SecureCookieSession(CallbackDict, SessionMixin):
     def __init__(self, initial=None):
         def on_update(self):
             self.modified = True
-        CallbackDict.__init__(self, initial, on_update)
+            self.accessed = True
+
+        super(SecureCookieSession, self).__init__(initial, on_update)
         self.modified = False
+        self.accessed = False
+
+    def __getitem__(self, key):
+        self.accessed = True
+        return super(SecureCookieSession, self).__getitem__(key)
+
+    def get(self, key, default=None):
+        self.accessed = True
+        return super(SecureCookieSession, self).get(key, default)
+
+    def setdefault(self, key, default=None):
+        self.accessed = True
+        return super(SecureCookieSession, self).setdefault(key, default)
 
 
 class NullSession(SecureCookieSession):
@@ -290,22 +312,20 @@ def get_expiration_time(self, app, session):
             return datetime.utcnow() + app.permanent_session_lifetime
 
     def should_set_cookie(self, app, session):
-        """Indicates whether a cookie should be set now or not.  This is
-        used by session backends to figure out if they should emit a
-        set-cookie header or not.  The default behavior is controlled by
-        the ``SESSION_REFRESH_EACH_REQUEST`` config variable.  If
-        it's set to ``False`` then a cookie is only set if the session is
-        modified, if set to ``True`` it's always set if the session is
-        permanent.
-
-        This check is usually skipped if sessions get deleted.
+        """Used by session backends to determine if a ``Set-Cookie`` header
+        should be set for this session cookie for this response. If the session
+        has been modified, the cookie is set. If the session is permanent and
+        the ``SESSION_REFRESH_EACH_REQUEST`` config is true, the cookie is
+        always set.
+        
+        This check is usually skipped if the session was deleted.
 
         .. versionadded:: 0.11
         """
-        if session.modified:
-            return True
-        save_each = app.config['SESSION_REFRESH_EACH_REQUEST']
-        return save_each and session.permanent
+
+        return session.modified or (
+            session.permanent and app.config['SESSION_REFRESH_EACH_REQUEST']
+        )
 
     def open_session(self, app, request):
         """This method has to be implemented and must either return ``None``
@@ -371,29 +391,35 @@ def save_session(self, app, session, response):
         domain = self.get_cookie_domain(app)
         path = self.get_cookie_path(app)
 
-        # Delete case.  If there is no session we bail early.
-        # If the session was modified to be empty we remove the
-        # whole cookie.
+        # If the session is modified to be empty, remove the cookie.
+        # If the session is empty, return without setting the cookie.
         if not session:
             if session.modified:
-                response.delete_cookie(app.session_cookie_name,
-                                       domain=domain, path=path)
+                response.delete_cookie(
+                    app.session_cookie_name,
+                    domain=domain,
+                    path=path
+                )
+
             return
 
-        # Modification case.  There are upsides and downsides to
-        # emitting a set-cookie header each request.  The behavior
-        # is controlled by the :meth:`should_set_cookie` method
-        # which performs a quick check to figure out if the cookie
-        # should be set or not.  This is controlled by the
-        # SESSION_REFRESH_EACH_REQUEST config flag as well as
-        # the permanent flag on the session itself.
+        # Add a "Vary: Cookie" header if the session was accessed at all.
+        if session.accessed:
+            response.headers.add('Vary', 'Cookie')
+
         if not self.should_set_cookie(app, session):
             return
 
         httponly = self.get_cookie_httponly(app)
         secure = self.get_cookie_secure(app)
         expires = self.get_expiration_time(app, session)
         val = self.get_signing_serializer(app).dumps(dict(session))
-        response.set_cookie(app.session_cookie_name, val,
-                            expires=expires, httponly=httponly,
-                            domain=domain, path=path, secure=secure)
+        response.set_cookie(
+            app.session_cookie_name,
+            val,
+            expires=expires,
+            httponly=httponly,
+            domain=domain,
+            path=path,
+            secure=secure
+        )

tests/test_basic.py

@@ -529,6 +529,48 @@ def run_test(expect_header):
     run_test(expect_header=False)
 
 
+def test_session_vary_cookie():
+    app = flask.Flask(__name__)
+    app.secret_key = 'testkey'
+
+    @app.route('/set')
+    def set_session():
+        flask.session['test'] = 'test'
+        return ''
+
+    @app.route('/get')
+    def get():
+        return flask.session.get('test')
+
+    @app.route('/getitem')
+    def getitem():
+        return flask.session['test']
+
+    @app.route('/setdefault')
+    def setdefault():
+        return flask.session.setdefault('test', 'default')
+
+    @app.route('/no-vary-header')
+    def no_vary_header():
+        return ''
+
+    c = app.test_client()
+
+    def expect(path, header=True):
+        rv = c.get(path)
+
+        if header:
+            assert rv.headers['Vary'] == 'Cookie'
+        else:
+            assert 'Vary' not in rv.headers
+
+    expect('/set')
+    expect('/get')
+    expect('/getitem')
+    expect('/setdefault')
+    expect('/no-vary-header', False)
+
+
 def test_flashes():
     app = flask.Flask(__name__)
     app.secret_key = 'testkey'