Fix set cookie every request

[lepture]

Without this patch, the response will always set cookie, even the session is not changed.

For example:

@app.route('/')
def home():
    return 'home'


@app.route('/session')
def set_session():
    session['foo'] = 'bar'
    return 'session'

If you have visited /session, the next time when you visit / it will always send the Set-Cookie header.

[mitsuhiko]

This currently is intentional on a second check. Is there a reason this causes a problem for you?

[gregorynicholas]

👎 this pull request seems odd to me -- it makes much more sense to explicitly delete cookies and reset sessions rather than have to explicitly send them for every request handler

[lepture]

@mitsuhiko @gregorynicholas The server will send a Set-Cookie header, even session is not changed.

Let's take the example I gave in the issue:

1. You visit /, it will not send Set-Cookie
2. Then you visit /session, it will send Set-Cookie
3. Then you visit / again, it should not send a Set-Cookie since session is not changed, but it actually will send Set-Cookie

This is the problem!

@gregorynicholas Have you ever noticed that your server is always sending a Set-Cookie?

Let's have a look at the python code:

    def save_session(self, app, session, response):
        domain = self.get_cookie_domain(app)
        path = self.get_cookie_path(app)
        if not session:
            if session.modified:
                response.delete_cookie(app.session_cookie_name,
                                       domain=domain, path=path)
            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)

After step 2, session will not be None, and in this case, it will always go down to the end of the code, and the response will set the session cookie again.

[lepture]

@mitsuhiko I've created a test case for this issue. #809

[gregorynicholas]

i would defer to the RFC spec for answers on the specifics of the response / request definition. i haven't looked at the specs for this particular item -- hoping you've already done the research rather than just arbitrarily saying flask should or shouldn't send data.

[lepture]

@gregorynicholas I am pretty sure this is a bug, it has nothing to do with the RFCs, it is all about logic. Session is not changed, no need for refreshing cookie.

@mitsuhiko It will cause permanent session really permanent, not just 31 days, since it refreshed every request.

[gregorynicholas]

ok, cool. so you want to interpret and change the way browsers and servers communicate -- uhmm... thats what the RFC is about. you're going to break something you don't even know you're breaking with this request because you're not properly learning the technology.

[lepture]

@gregorynicholas Did you ever have a look at the changes? It is a little waste of time to explain it again and again. Take a little time, think about it.

Besides, I do know HTTP, I've read RFC2612 long time ago.

[lepture]

@gregorynicholas I've created a test case for this issue at #809, maybe you missed it. I'll just add it here, check out the test cases, please do have a look at the code.

[mitsuhiko]

@mitsuhiko It will cause permanent session really permanent, not just 31 days, since it refreshed every request.

That currently is intentional. I am happy to change that, but I want to know if it causes actual problems besides that it's something you don't expect.

[lepture]

@mitsuhiko There are no actual problems yet, since I can replace the session interface (https://gist.github.com/lepture/6093661).

The fact is that this is wrong, if it is really permanent, we should drop the configuration of permanent session life time, since it doesn't work the ways it should be (as I expected).

[mitsuhiko]

Maybe this should become an option then. I always envisioned it to work like this. The setting was supposed to be how long of inactivity it takes until the cookie gets deleted but auto refreshes otherwise. This is the behavior I expect as a user when I sign in with "remember me". Randomly getting logged out is not cool :)

[lepture]

@mitsuhiko I am not sure wether it is a good design or not. But other frameworks (rails and django) didn't do it this way, I think it is a little waste of bandwidth to send the header every request.

Let's keep this issue open until you have decided it. For now, I am replacing the session_interface.

[mitsuhiko]

I will think about this a bit. With regards to wasting bandwidth I think it's not too bad. Even Google sends Set-Cookie each response and they are super picky about bandwidth utilization.

[methane]

When user opens multiple tabs, "Lost update" problem may deletes some session data.
Sending Set-Cookie only when changes in session may minimize such risk.

[mitsuhiko]

That's a good point and something to consider. I might add some configuration flags to control that behavior.

[shahafabileah]

+1 on what @methane said.

Imagine a web page that issues two ajax requests simultaneously. One is fast and the other is slow. The fast one changes the session. The slow one doesn't. If the session cookie is written as part of every single response, then the fast ajax request will update the session and this update will get reverted when the slow request returns.

Setting SESSION_REFRESH_EACH_REQUEST = False fixes this problem, so I think this is a good change. Personally I think this should be the default, but that would be a breaking change so I understand the hesitation.

Looks like Flask 0.10.1 is the latest release in PyPi. If I wanted to use this feature, what's the cleanest way to get it? For now I just downloaded a copy of latest master code as a zip file and added it to my requirements.txt file.

[coolcoca]

@mitsuhiko suppose the case, I save some user info in session, then the Set-Cookie will appear on every response, for example: the static files. If I put CDN in front of it , the header Set-Cookie may cache by CDN, which will cause other people accessing wrong info.