Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
rowanmanning opened this issue Mar 22, 2022 · 2 comments · Fixed by #395
Closed

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

rowanmanning opened this issue Mar 22, 2022 · 2 comments · Fixed by #395
Labels
enhancement New feature or request

Comments

@rowanmanning
Copy link

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:

await this._set(req[REGENERATED_SESSION_ID] || id, {
header: { iat, uat, exp },
data: req[sessionName],
});

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

@adamjmcgrath
Copy link
Contributor

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 adamjmcgrath added the enhancement New feature or request label Mar 29, 2022
@wfjsw
Copy link

wfjsw commented May 30, 2022

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants