express-openid-connect icon indicating copy to clipboard operation
express-openid-connect copied to clipboard

Unable to set session expiry with express-session-store implementations

Open rowanmanning opened this issue 3 years ago • 2 comments

Describe the problem

The documentation for this module states you can use a custom session store and that it's compatible with express-session-store. I've found that this isn't quite true.

This is because, although it's not explicit in the documentation for the custom Store class, in that library the session object passed into a method like store.get(sid, session) has certain properties which can always be assumed to be present by a session store implementation.

The example I've come across quite a few times is that most of the session store implementations expect session.cookie to be an object with maxAge and expires properties. The presence of session.cookie is documented when it appears under the req object.

Without these properties, most session store implementations cannot have a session expiry configured, and most seem to default to one day. This is the best case, and some session store implementations just error if the cookie property is not present on the session object.

Here's what I've found when testing with a few common stores:

  • memorystore, which is used in your example, does not error when used. However it's impossible to set the expiry time on the session that's stored in memory. This is because the module gets the session max age from session.cookie.maxAge and otherwise defaults it to one day:

    var maxAge = (sess && sess.cookie) ? sess.cookie.maxAge : null
    return (typeof maxAge === 'number'
      ? Math.floor(maxAge)
      : oneDay) // set higher up in the file
    
  • connect-redis, also used in your documentation, does not error. However it also is impossible to set the session expiry time to more than a day. This is because it relies on the presence of session.cookie.expires:

    if (sess && sess.cookie && sess.cookie.expires) {
      let ms = Number(new Date(sess.cookie.expires)) - Date.now()
      ttl = Math.ceil(ms / 1000)
    } else {
      ttl = this.ttl // set to one day higher up in the file
    }
    
  • connect-session-knex does not get as far as setting a cookie. It errors immediately with Cannot destructure property 'maxAge' of 'sessObject.cookie' as it is undefined. Although this is a worse outcome, this is what alerted me to the issue. It also attempts to use session.cookie.maxAge but doesn't protect against the property not being present:

    const { maxAge } = sessObject.cookie;
    const now = new Date().getTime();
    const expired = maxAge ? now + maxAge : now + oneDay;
    

For the libraries which don't error, the session will be removed/expired within one day even if I have configured express-openid-connect to have an expiry time of one week.

What was the expected behavior?

I expected that these libraries would all work, as they all work as expected when used with express-session, and that I'd be able to configure the expiry time of the sessions via configuring absoluteDuration in this module.

Based on scanning through a few other express-session-store implementations, I think most can be resolved if express-openid-connect provides them with a session.cookie.maxAge and session.cookie.expires during setting of session data here:

https://github.com/auth0/express-openid-connect/blob/b850bfd410b78c9c7c0227de9b91c8025d91c597/lib/appSession.js#L223-L226

Reproduction

This is difficult to reproduce without having a full application set up with express-openid-connect configured. If you have that already for testing purposes, you can use any of the libraries above to replicate the issue. I'm outlining only the use of the auth0 middleware below:

const { auth } = require('express-openid-connect');
const redis = require('redis');
const RedisStore = require('connect-redis')(auth);

// ... other Express app setup

const redisClient = redis.createClient();

app.use(
  auth({
    session: {
      absoluteDuration: 432000, // 5 days
      store: new RedisStore({ client: redisClient }),
    },
  })
);

// ...set up routes, listen etc

With the above set up and running, and the absoluteDuration set to 5 days, authenticate with the app and then connect directly to Redis. Check the expiry time of the session you created, it will expire in 1 day.

Environment

rowanmanning avatar Mar 22 '22 22:03 rowanmanning

Thanks for raising this @rowanmanning

Ultimately, our session is not the same as express-session, so any store that relies on parts of express-session, or anything beyond the custom store implementation's required fields - may not work as expected.

That said, if it's just a case of adding cookie.maxAge and cookie.expires to the session data, we can look at doing this, will add something to the backlog.

adamjmcgrath avatar Mar 29 '22 13:03 adamjmcgrath

connect-memcached does not work with this as well:

TypeError: Cannot read property 'maxAge' of undefined
    at MemcachedStore.set (./node_modules/connect-memcached/lib/connect-memcached.js:118:32)
    at internal/util.js:297:30
    at new Promise (<anonymous>)
    at MemcachedStore.<anonymous> (internal/util.js:296:12)
    at CustomStore.set (./node_modules/express-openid-connect/lib/appSession.js:223:20)
    at ServerResponse.resEnd (./node_modules/express-openid-connect/lib/appSession.js:359:23)
    at ServerResponse.redirect (./node_modules/express/lib/response.js:978:10)
    at callbackStack (./node_modules/express-openid-connect/middleware/auth.js:168:25)
    at Layer.handle [as handle_request] (./node_modules/express/lib/router/layer.js:95:5)
    at next (./node_modules/express/lib/router/route.js:144:13)

wfjsw avatar May 30 '22 08:05 wfjsw