Fix #251 Add disableTTLResetOnTouch option

[nfadili]

@wavded I believe this is all that would be needed to fix the issue addressed in #251. I have tested it locally and it is behaving how I initially expected express-session to behave with this store.

If I am understanding the purpose of the 'touch' function correctly, connect-redis might not need to implement 'touch' at all. But the flag i'm proposing in this PR leaves the code backwards compatible to its current behavior while also allowing the caller to specifically have it on or off.

[wavded]

Appreciate you looking into this @nfadili, it appears this is functionally equivalent to the the disableTTL option that already exists, so perhaps some more context. Also @alex996 and @knoxcard since you have poked and prodded at this issue more than I, could you comment on this pull request as well?

[nfadili]

I appreciate the quick eyes on this @wavded.

There's a slight difference between the two options. I'll try my best to explain:

• The disableTTL option affects both the set and and touch methods. If disableTTL is set to true then a TTL will not be set when a session is initially set (using set) as well as when a session is touched (using touch).
• The disableTTLResetOnTouch option affects only the touch method. If disableTTLResetOnTouch is set to true then a TTL will not be set when a session is touched (using touch) but has no effect on when a session is initially set.

I believe that when touch was initially implemented it should not have reset the TTL when called. This is the bug that #251 is referring to. The only reason I am proposing this second separate option is to maintain backwards compatibility. If the library maintainers feel comfortable with changing this within the touch method itself, my proposed flag is not needed.

[wavded]

Ahh thanks for the clarification @nfadili.

My question now is should we make this default. In other words, is this a bug fix or breaking. It seems there is an argument for not disabling TTL on touch. In fact, our current TTL option seems to go along with the rolling option in the express-session:

https://github.com/expressjs/session#rolling

It appears if that is set, we should be updating the TTL in Redis, but if not, we shouldn't. Does that make sense to you?

[nfadili]

That makes sense! The current TTL option does seem to be 'rolling' by default. My intuition says this would likely want to be in sync with whatever express-session's defaults are (which is rolling: false).

My assumption when using this library was that it would not extend sessions. The touch method's description is kind of vague, but it sounds like its existence is for store implementations that delete idle sessions. I believe that connect-redis does not really have a concept of 'idle' sessions (the TTL is the only authority on deleting the session key) and thus might not need to implement touch at all.

All that being said, I am not very familiar with the problem space or codebases! So I would defer judgement to you and the other maintainers. Though I think we can agree that there is a valid need for some way to not extend sessions 😄

[knoxcard]

@wavded - I did look at the code a second time a few days ago and came up with the same assumption as you. As in it being equivalent to the the disableTTL option. Now I am reading over everything posted and catching up.

[wavded]

At this point, my gut is to make a breaking change that conforms to the express-session option for rolling. If we go this route, we'd want to clean up any other express-session options we are not using or not conforming to correctly and then cut a major release. Thoughts? Or do you think we need a temporary workaround for now that isn't breaking first?

[alex996]

It's worth to note that the current default of resave: true and rolling: false leads to stale session cookies, i.e. the session rolls on the server, but not on the client. The resave fallback is deprecated though, so it seems express-session does lean towards not extending sessions by default.

As far as always calling store.touch, from expressjs/session#531 I gather that it's meant to merely signal to the store that a given session was accessed. Whether its TTL gets reset is completely up to the store. Having touch is definitely useful though, as in most apps, you'd want to prolong session lifetime while the user is still browsing the site. So, it's definitely important to bump TTL, but perhaps it should only be reset if the session was modified?

In the original issue I reported, the session "expires" was out of sync with TTL because touch() would always reset TTL unconditionally. Turning off TTL reset with the proposed flag would solve the problem, but so would a check against the expiry date. If I'm not mistaken, touch receives an updated session object, so comparing sess.cookie._expires against expires of the session stored in Redis should reveal if the session changed. TTL can then be reset accordingly, but only if the session was indeed updated. This check itself could be behind a flag as well.

---

Regarding indefinite lifetime, it still remains an issue if you do want to roll sessions. With resave: true, rolling: true, and disableTTLResetOnTouch: false, the user can still extend their session by pinging the server, because express-session does not offer an option for absolute timeout yet. (There's a PR expressjs/session#595 but it's been up for almost a year now). Nonetheless, I realize this last scenario is best handled in express-session anyway, and you might be better off forking the repo and implementing the timeout yourself.

[knoxcard]

@alex996 - so this exactly what is stated here...#259 Seems like a major bug that has been lingering for quite some time.

Connect-Redis vs Express-Session in regards to handling a .touch() call

connect-redis

  RedisStore.prototype.touch = function (sid, sess, fn) {
    var store = this;
    var psid = store.prefix + sid;
    if (!fn) fn = noop;
    if (store.disableTTL) return fn();

    var ttl = getTTL(store, sess);

    debug('EXPIRE "%s" ttl:%s', sid, ttl);
    store.client.expire(psid, ttl, function (er) {
      if (er) return fn(er);
      debug('EXPIRE complete');
      fn.apply(this, arguments);
    });
  };

function getTTL(store, sess, sid) {
  if (typeof store.ttl === 'number' || typeof store.ttl === 'string') return store.ttl;
  if (typeof store.ttl === 'function') return store.ttl(store, sess, sid);
  if (store.ttl) throw new TypeError('`store.ttl` must be a number or function.');

  var maxAge = sess.cookie.maxAge;
  return (typeof maxAge === 'number'
    ? Math.floor(maxAge / 1000)
    : oneDay);
}

express-session

MemoryStore.prototype.touch = function touch(sessionId, session, callback) {
  var currentSession = getSession.call(this, sessionId)

  if (currentSession) {
    // update expiration
    currentSession.cookie = session.cookie
    this.sessions[sessionId] = JSON.stringify(currentSession)
  }

  callback && defer(callback)
}

[knoxcard]

@nfadili @wavded - yes, if express session is set to rolling: true, then the current behavior should persist, otherwise default behavior should work as @nfadili has stated. Nice job @nfadili!

This comment stated by @iandvt got to me a few days ago, in regards to this issue...

At the moment I would suggest that this module is unsafe and unusable, I am going to revert to a mongo store

This module has been nothing but good news to me, but a failed session means account security compromise, sensitive information potentially falling into the wrong hands. If anything would require a major version change, a security issue would be it.

[alex996]

@knoxcard I believe the issue you linked could be easily circumvented with resave: true. If you intend to roll cookies, you must re-save sessions. Otherwise, store.touch needs to be more configurable in connect-redis, and expose behaviors like this to the user.

[wavded]

I like @alex996 suggestion of checking the cookie expire time on touch and keeping the TTL in sync with that. Whether it needs to have another flag do enable that... I can't think of any scenarios where a user wouldn't want their cookie and Redis TTL to be in sync, can any of you? @nfadili care to modify the PR to support?

[nfadili]

I've spent some time digging through the express-session codebase and I've come to the conclusion that the root of the issue (mine at least) is located there and not in connect-redis. It seems that express-session does not keep cookie expirations in sync with store expirations regardless of how you configure it. This can be clearly seen by having console.log(sid, store.ttl, sess.cookie.maxAge, sess.cookie.expires) in connect-redis's touch method! express-session passes the full expiration values each time. So regardless of how connect-redis implements touch (using ttl, maxAge, or expires) it would be extending the session by the initial value of the configuration.

I hope this explanation makes sense ^^ I absolutely agree with @alex996's point on keeping them in sync, but it doesn't seem to be possible based on what express-session is providing. Any sanity checks on my findings would be great! 😄

[knoxcard]

@alex996 - in regards to my web platform, i've had resave and rolling both set to true ever since I've installed this package. Hence, maybe that's why I've never noticed this issue?

[knoxcard]

@xapasnon - posted this in a different ticket, thoughts?

While not critical for our specific use-case, it seems strange that the sid is not passed to getTTL in the touch prototype, when it is in set.

I.e. row 248 is:
var ttl = getTTL(store, sess);

Should be:
var ttl = getTTL(store, sess, sid);

Bug? Feature? Misunderstood?

[wavded]

I poked a little bit and here is what I am seeing. Per @nfadili comment, running the latest express-session, I am seeing that expires is always getting updated regardless if its rolling or not, making that unreliable to use. set or touch could be called depending on what options you have on, like resave: true.

To me it seems like change needed is to ignore setting the expiration after initially saving if rolling: false in both the set and touch methods. However, there is no way for this library to know that rolling was set as as those options are not exposed to stores.

We already have an option that satisfies most of this, disableTTL. It prevents TTLs from being set in both set and touch methods. However, it doesn't set an initial TTL which is problematic. Since we don't know if a session is new or not, we'd have to make an additional call to Redis on every set to check which is not ideal. Plus the user would have to remember to set this option.

Ideally, we could get express-session to expose the options used to the stores in some way so we can just make it work correctly by default without the user having to "match settings".

@dougwilson what do you think about that?

[knoxcard]

@dougwilson - get a chance to look at @wavded's comment above?

[wavded]

I have a PR for V4 which I believe will provide the required behavior here, instead of reinventing the wheel with how we handle TTL, we will directly depend on express-session to manage and set the cookie.expires field in which we will reflect Redis. This is similar to how other notable stores do it, (e.g. connect-mongo) and I think simplifies a lot of stuff. The V4 PR also introduces a disableTouch which is very similar to what this PR set out to do, which will prevent touch calls from express-session from doing anything.

[nfadili]

@wavded sounds great to me. Thank you for incorporating this in v4! I think the decision to offload responsibility to express-session while also providing disable flags is a good call 👍 Looking forward to its release.

[wavded]

V4 has been released, see the readme for more details. But essentially the new disableTouch option does the same thing this pull request set out to do but it may not be necessary depending because we now depend on cookie.expires.