pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
488 stars 236 forks source link

Session Management Issue Causing Unexpected User Logouts #221

Closed Emong closed 3 months ago

Emong commented 3 months ago

version 0.6.0 use '‎should_set_cookie' to prevent send cookie in every response. but the expiration on the server side not refresh also. Caused users to log out even when there was activity.

the begin code of 'save_session' function

if not self.should_set_cookie(app, session):
            return

should move to the end for refresh expiraton but not send cookie?

if self.should_set_cookie(app, session):
            self.set_cookie_to_response(app, session, response, expiration_datetime)
Lxstr commented 3 months ago

Can you please confirm what settings you have for SESSION_REFRESH_EACH_REQUEST and SESSION_PERMANENT?

Emong commented 3 months ago
PERMANENT_SESSION_LIFETIME = timedelta(seconds=3600)
SESSION_TYPE = "filesystem" 
SESSION_PERMANENT = False
SESSION_USE_SIGNER = True

SESSION_REFRESH_EACH_REQUEST not set.

I've tested SESSION_REFRESH_EACH_REQUEST=True. it make every response make SetCookie and refresh the server side expiration.

Lxstr commented 3 months ago

Ok so the non permanent session is what's causing it. should_set_cookie which comes from flask) evaluates

session.modified or (
            session.permanent and app.config["SESSION_REFRESH_EACH_REQUEST"]
        )

This logic makes sense to reduce unneeded setting of client side cookies that have no expire and therefore no need to update every request.

I guess you could make the case that this should be different for updating the stored session but ultimately non permanent sessions don't really fit that well in server side sessions. In the upcoming releases I've added documentation that explains that we use the PERMANENT_SESSION_LIFETIME to set the expiry in storage even for non permanent sessions. (edit: This is to prevent lots of un-expirable sessions in storage)

If you are using a non permanent session effectively normally you are saying you don't care about expiration but just want to only have a session while the tab is open. However, perhaps there is a case where server side sessions could actually provide something more restrictive: a session that only exists in an open tab AND expires.

In that case we would need to modify the logic either in the save_session or overwrite the should_set_cookie.

May I ask your use for non permanent session and if it is indeed this edge case I'm suggesting

Lxstr commented 3 months ago

If it is, it may be an option to add a method for should_set_storage which would limit the storage aspects separately, given the should_set_cookie still effectively prevents unneeded cookie sets. Would the below function make sense?

    def should_set_storage(self, app: Flask, session: SessionMixin) -> bool:
        """Used by session backends to determine if session in storage
        should be set for this session cookie for this response. If the session
        has been modified, the session is set to storage. If 
        the ``SESSION_REFRESH_EACH_REQUEST`` config is true, the session is
        always set to storage. In practice, this means refreshing the expiry.

        .. versionadded:: 0.7.0
        """

        return session.modified or app.config["SESSION_REFRESH_EACH_REQUEST"]

If this is false, we can be confident that should_set_cookie is also false. Therefore, we could check this first and return if false.