Only touch sessions in the store when rolling sessions are enabled

[dtymon]

• Currently, disabling rolling sessions suppresses unmodified
  cookies being sent back to the client (line 447). However, the
  session is "touched" in the store regardless of whether rolling
  sessions are enabled (line 339).

• This results in the expiry time in the cookie and in the
  persistent session deviating. Specifically, the expiry of the
  persistent session is pushed back on each and every request
  whereas the cookie expiry is only updated if the request
  changes the session.

• This prevents the use case of a "session ping" request that is
  sent by the client to test if their session is still valid. We
  have achieved this by turning off rolling sessions and placing
  the handling of the ping early in the express stack. A stack
  entry is then placed after the ping handling to make a benign
  change to the session to cause it to be stored, in effect,
  touching the session much like rolling sessions. An example
  router might look like (using passport with disabled rolling
  sessions):

    router.get('/ping', function (req, res, next) {
        if (!req.isAuthenticated()) {
            res.sendStatus(403);
        }
        else {
            res.sendStatus(200);
        }
    });

    router.use(function (req, res, next) {
        // Push back session expiry and make benign change to
        // force persistent storage.
        req.session.cookie.maxAge = 3600;
        req.session._dummy = req.session._dummy ? (req.session._dummy + 1) : 1;
        next();
    });

• This appears to work from a client perspective as the cookie
  expires at the appropriate time. However, the session expiry
  in the store is always pushed back, even for ping requests.
  This means that from a server perspective the session is
  still valid for some period after the cookie expiry. The
  store expiry therefore cannot be trusted or used to identify
  expired sessions in the store for purging or clean up.

[dougwilson]

Hi and thanks for your PR! Many users have different expirations for the server and the client and this would be a breaking change. We can either (a) hold this PR until a new major version is released of (b) the behavior can be opt-in such that current users do not get this new behavior.

Also, please add tests which test the new behavior. They should fail without your patch and pass with your patch. Thanks!

[dougwilson]

Needs tests to bring the coverage back to 100%.

[dougwilson]

I see you got the coverage back up, but can you still (a) let me know which option you are going to choose and (b) add test(s) for your change?

[dtymon]

Hi Doug. I'm fine with either option a) or b). The purist in me thinks it should be the default behaviour as I think the expiry times in the cookie and the store should match at all times. But the pragmatist in me concedes that it might break others so perhaps an option would be a better way to go.

I'll get started on adding some new tests for it.

Thanks,
David

[dougwilson]

Right, I haven't looked into it close enough, but on the surface it does feel like it should be the default behavior, though the change itself breaks tests which is definitely not a good sign for what will happen to existing apps using 1.x. We can of course land in 2.x but also can put as opt-in in 1.x and switch the default behavior in 2.x (or even remove the switching all together and it's the only behavior) which would let folks get ready right away.

[dtymon]

Hi Doug, I have added a new option alwaysTouchUnmodified so that the current behaviour is maintained. Not sure about the option name, so happy to change it if you have something better.

[dougwilson]

Awesome! Thr name seems fine to me. I assume that since this is opt-in, the changes made to the existing tests can be reverted now, right?

[dtymon]

Ah yes, good pick up. I'll revert those changes now.

[dtymon]

Hi Doug, I have reverted my changes to the existing tests now that the existing behaviour is the default. Let me know if there is anything else that you would like done.

Thanks,
David

[dougwilson]

The changes look good, but there is some kind of flaky test included:

  1) session() alwaysTouchUnmodified option should touch unmodified non-rolling sessions when set:

      Uncaught AssertionError: false == true
      + expected - actual

      -false
      +true

That will fail sometimes and pass other times. Not sure what the issue is yet, but I'm looking into it to see if there is a change to be made. Would need to be fixed in order to merge.

I haven't seen it happen on Travis CI but it happens around 80% on all three of my machines. Does it happen on your machine? @gabeio can you pull this PR and run the tests a few times to see if you also see the test failure?

flaky test

[dougwilson]

Should this extra logic be inside the shouldTouch function? Or if not, why not? I would expect that function is all that would be called to determine if a touch occurs, and this is controlling the touching.

[dougwilson]

I forget looking through the comments: is there an intention to make alwaysTouchUnmodifed the default behavior in the next major? I ask because all default options that are Booleans should be false such that undefined and null are stand in values. The only options that default to true are deprecated options, but this doesn't have a deprecation warning on it.

[dtymon]

Regarding the flakey test, I can see how it might fail on a fast machine since there is no delay between capturing the first expiry time and executing the second request. So there is a chance that the expiry times are equal due to the millisecond resolution. I'm running on a VM which appears to be slow enough that it never fails. I will put a small delay in there to ensure the expiry times resulting from the two requests should always differ.

I'll move the logic into shouldTouch(). I hadn't appreciated that this was the only place shouldTouch() was called and wanted to limit the impact to just this location.

I'll look into renaming the option so I can change the default value to false.

Thanks David

[dtymon]

Hi Doug,

Regarding the default for this Boolean option being true, personally I would like to see this change to false in the next major as I think the current behaviour is not what you might expect given the cookie and persisted session can have differing expiry times when rolling sessions are disabled.

If you agree with this plan, does that mean the option should have a similar deprecation warning to the other Booleans defaulting to true? If so, I will add that.

If you don't think the default behaviour should change in the next major, then I will rename the option so I can flip the default value to false.

Hopefully the intermittent test failures should be fixed and I've moved the logic into shouldTouch().

Thanks,
David

[dougwilson]

Hi @dtymon I have never had any issue with the existing behavior, and it seems no one else has mentioned this in all these years of very heavy use. Without a larger set of opinions on this, I'm not sure we can say the current behavior is unexpected, seeing as no one has seemed to stumble into whatever the issue is here.

I, personally, expect the existing behavior as I mentioned in my first repose :) That means at this point we have 1 in favor and 1 not. Not only is that a tie, but no where near enough opinions to really say one way or the other. We can leave this PR open for a few months to collect additional opinions 👍

[dtymon]

Oh well, I guess we will have to agree to disagree on this one. I would have thought that the session expiry time should be essentially identical on client and server regardless of how the middleware is configured but perhaps others think differently.

[dougwilson]

Why not just not update the session expiration when it's touched on the server? The touch just indicated the session was accessed, and the underlying store chooses what it should do with that information. It can certainly decide to update the expiration on the server, but it can also decide not to do that.

[dougwilson]

The entire reason this module started calling touch instead of always just save was to serve the exact purpose you're asking about -- so the store can determine to, for example, not update the session if it wasn't modified or anything else it wanted to do; it would depend on the underlying storage mechanism that is used. Your comment has made me realize you're likely running into a store issue, not an issue with this module. What store module are you using?

[dtymon]

I'm using the MySQL store that updates the 'expires' column when touch() is called. It looks like the memcached store doesn't store an explicit session expiry, only the cookie and session object, and so does not suffer the issue. But the MySQL store has an explicit 'expires' column which is updated in the touch() implementation.

[dougwilson]

Gotcha. I mean, it sounds like you should just add an option to the store module to allow you to choose if you want session access ("touch") to keep bumping that column or not, right? That sounds like a feature request to the store module.

[dougwilson]

(I'm just trying to think about this, is all. I keep reading and re-reading your initial post trying to understand the issue and I think I do, but not sure. It would have been nice to get an issue filed that explains what you're running into with full examples I can see myself to better understand the problem at hand here -- I'm not trying to be mean or anything, just hard for me to reason about without grasping the issue at hand and instead I just have a solution presented. I hope that makes sense).

[dtymon]

Yeah, it does seem to be a MySQL store issue. I hadn't appreciated that it was up to the store to decide what it wanted to do with a touch(). Instead I thought it was in line with the UNIX command touch, essentially asking the store to push the expiry time back. But I now see that touch is informing the store that the session has been accessed and the store can make use of the info any way it sees fit.

[dtymon]

I will try to explain what we are trying to achieve to hopefully better explain how we got to this point.

• For a logged in user, we would like the client to be able to ask the server if its session is still valid. If the session is invalid, then the user will be logged out automatically. This is important as what is shown on the screen is potentially sensitive so we need to be able to detect when a user has possibly left their terminal so we can log them out.
• Since the client is going to send a ping request to the server to test the session validity, we need to disable rolling sessions otherwise the ping will cause the session's expiry to be updated therefore defeating the whole purpose.
• This all works nicely and is not where the issue lies but might explain why rolling sessions have been turned off.
• We have noticed that there are cases where invalid sessions remain in the MySQL database. For example, when the cookie expires, it is not sent by the browser and so there is no stimulus on the server to cleanup the session associated with that cookie which in turn leads to that session being left in the database.
• Ideally, we would really like the session table to be empty when no one is logged on to the system so we would like to periodically purge expired sessions from the database and were looking at the expires column in the MySQL table as the best way to identify expired sessions.
• However, this is when we noticed quite large discrepancies between the cookie expiry and the persisted session expiry. When we looked into it, we found that even though the ping request was not updating the cookie expiry, it was causing the value in the expires column to be updated due to the session being touched.
• This led us to believe it was a problem with touching unmodified sessions when rolling sessions were disabled.

Hopefully this might give a better understanding on how we got to this point.

[dougwilson]

This is important as what is shown on the screen is potentially sensitive so we need to be able to detect when a user has possibly left their terminal so we can log them out.

Wouldn't this break if the user's machine was offline during this time? Or does the failure to reach the server cause an immediate log out?

[dtymon]

Yes. Failure to reach the server to do the ping will cause the app to log out the user. Due to the nature of the app, it is imperative that it is not abused and so anything other than a successful ping will result in the app shutting down.

[dougwilson]

We have noticed that there are cases where invalid sessions remain in the MySQL database. For example, when the cookie expires, it is not sent by the browser and so there is no stimulus on the server to cleanup the session associated with that cookie which in turn leads to that session being left in the database.

Typically this is the common use-case for the expirrations not matching on the server and the client (usually the server will have a shorter expiration), though of course a user can just disappear and come back a few days later on your 1 hour session. This concern is unfortinatley completely outside of this module and would be something up to the store. I did store session in MySQL before just like you and this is actually a problem that is hard to solve when the underlying storage itself doesn't eject (vs something like Redis).

[dtymon]

OK, cool. I'm thinking we will just write our own MySQL store as it is not very difficult and that way we will be able to implement whatever functionality we require.

[dougwilson]

But yea, I get what you're saying. It does indeed sound like you are describing an issue with your store, though.

[dtymon]

Thanks Doug, I agree it is a store issue. Perhaps we can reject this pull request then.

[dougwilson]

I mean, I'm still OK with adding the functionality; the tangent was just a discussion that stemmed from changing the default behavior, rather than just adding yet another optional behavior users can opt-in to. Usually for the ping case, I think most of the time, the ping is used to specifically case the session not to expire (for example, because the user is still on the site / page, so you don't want it to suddenly log them out). I haven't really heard the case to do the opposite case before (like you're doing), especially because I would certainly be miffed if my router rebooted and got kicked out of all the sites I was logged in to :) I'm not saying your use-case is not valid, but I think I understand it as the opposite of what I usually always see, which may also be why this has never come up before on here, for example.

[dougwilson]

Haha, that reminds me of all the times I've had really crappy WiFI at hotels and on air planes, when the connection would just be down for a few minutes every so often and you have to sit there waiting for it to come back to continue working. Also reminds me of back in the day where a common "security" mechanism was to pin your login session to a single IP and when you're at a school or similar that redirected traffic though multiple outbound IPs and you constantly keep getting booted out of web site because the IP that site would see keeps changing @_@

[tony-gutierrez]

So I ended up here because my store (connect-session-sequelize) is writing to the db every request, regardless of whether rolling is on or off in express session. I thought the touch on the store was to be dumb, and just do an update when requested. If I understand what you guys are saying, you want touch to just be a communication to the store that the session was accessed? So essentially a store can break the functionality of the "rolling" option?

[tony-gutierrez]

In looking into this I basically decided the way forward for me was to disable the touch() in my store, so that I could control when the db is accessed. Asking to store to control timing of touch() is hard because the current expiration of the session is not passed to the touch() method so it would have to do a db read before it could decide not to update the db. The expiration of the cookie is passed in some configs, but there is no guarantee that that will match the store value.

mweibel/connect-session-sequelize#67

[shahyar]

Hey Doug,

This PR still seems to be important. The store has no way of knowing if the session has changed. Right now, if a session changes, store.touch is not called; req.session.save is called instead. This also makes it cumbersome to conditionally extend a session.