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

ERR invalid expire time in set #292

Closed
paton opened this issue Nov 29, 2019 · 11 comments
Closed

ERR invalid expire time in set #292

paton opened this issue Nov 29, 2019 · 11 comments

Comments

@paton
Copy link

paton commented Nov 29, 2019

We're getting the ERR invalid expire time in set error and having some trouble resolving it.

  • Running latest versions of all modules
  • Resave, rolling, and saveUninitialized are false.
  • No ttl is explicitly set (see below).
  • Cookie's maxAge is set to 14 days

The ERR invalid expire time in set begins to happens exactly 14 days (the maxAge) after the session was created.

I included some code below to illustrate our connect-redis and express-session config.

const session = require('express-session');
const RedisStore = require('connect-redis')(session);
const redis = require('redis');
const redisClient = redis.createClient(...);
const store = new RedisStore({ client: redisClient });

app.use(
  session({
      secret: 'secret here',
      store: store,
      resave: false,
      rolling: false,
      saveUninitialized: false,
      proxy: true,
      name: 'xyz',
      cookie: {
        maxAge: 1000 * 60 * 60 * 24 * 14,
        path: '/',
        secure: true,
        httpOnly: true,
      },
    });
);

We have an API that modifies and saves the session...

req.session.someData = '123';
req.session.save((err) => {
  if (err) {
    return res.status(400).json({});
  }
 res.status(200).json({});
});

The above code in our controller throws the ERR invalid expire time in set error when attempting to save the session exactly 14 days (same as maxAge) after the session was created.

I also noticed that, when the request completes with the 400 error as shown above, there's a Set-Cookie header on the 400 response with an Expires date equal to 14 days in the future. However, if we add console.log(req.session.cookie._expires) right before req.session.save(), the date that's logged is the (correct) date, which is 14 days after the session was first created (yet, on the response, the expires in Set-Cookie is 14 days in the future).

If I'm understanding the internal workings of connect-redis and express-session correctly, with the above configuration, a session created should be valid for only 14 days (and the cookie should also expires after 14 days. Is this incorrect, seeing that the Set-Cookie header on the response is giving the cookie a new expires of 14 days in the future whenever the session data is changed?

If you could help understand what might be happening here, it would be greatly appreciated! Our solution as of now is to rotate the cookie name every 13 days to force all users to login again, thereby creating a new session (which works, until 14 days later when the issue reoccurs).

@wavded
Copy link
Collaborator

wavded commented Dec 2, 2019

My guess is since this has a fixed max age, it is trying to set a time in the past after 14 days which will be an error in Redis. If rolling is false, then the maxAge never resets in this case I believe. I'm not sure if this is an issue in this library or express-session (parent lib). It seems that if a cookie has expired, it shouldn't be attempted to be stored (resulting in the error).

@paton
Copy link
Author

paton commented Dec 2, 2019

It sounds like the Expires date on the cookie is incorrectly being set to the current date + maxAge whenever the session is modified despite rolling: false, which is extending the age of the session cookie beyond the life of the session.

The expected behavior (I think) would be that the session cookie expires maxAge after the session was created, which isn't happening.

Not sure if this is a scenario that should be handled by connect-redis (ie. rejecting session cookies older than maxAge, which seems to be possible since the correct expiration date can be seen in req.session.cookie._expires) or if the behavior should be addressed in express-session by fixing whatever is causing the cookie to not expire after maxAge.

@wavded
Copy link
Collaborator

wavded commented Dec 3, 2019

It seems like the fix would be best suited upstream in the express-session repo as this could affect all store implementations. This store, similar to other popular stores, always uses the expires set from express-session.

@paton
Copy link
Author

paton commented Dec 16, 2019

I raised the issue with express-session. Please see Dough's response. Is this something that should be fixed in connect-redis?

expressjs/session#718 (comment)

The expected behavior (I believe) is that, even when the session data is modified, the session length should never exceed maxAge from the time the session was first created.

This us not how it works. The module works like you are describing as it is intended to work that way. It just seems you are expecting to opposite of how it is designed to work is all.

This is causing connect-redis to break because the cookie set by express-session is living beyond the length of the maxAge of the original session.

This session store should be keeping the session alive.

Remember, the values under the "cookie" section are the values the cookie takes, as specified by the RFC for cookie. The "maxAge" attributes sets the maxAge for the cookie. The only thing rolling does is set the cookie on every response instead of only on the responses where the session is modified.

I hope that helps. The behavior you are seeing is how this module has works since it was first created and is intended. If you want to see a method to set a maximum limit for the session as a whole, you're welcome to open a feature proposal for it, and maybe even a PR if you're up to it :)

@paton
Copy link
Author

paton commented Dec 16, 2019

Additional information: This issue first started happening after upgrading connect-redis from 3.4.2 to 4.0.3. Before upgrading from 3.4.x to 4.0.x, we were also setting the ttl setting to the same value as cookie.maxAge in the configuration options. We removed the ttl setting from our config when we upgraded the module to 4.x. Not sure if the issue started happening after the module upgrade or after removing the ttl option.

@paton
Copy link
Author

paton commented Jan 19, 2020

I have posted code to reproduce this issue here: expressjs/session#718 (comment)

@paton
Copy link
Author

paton commented Jan 20, 2020

Update:

This issue is likely caused by manually saving the session rather than allowing the session to automatically save on its own at the end of the HTTP response.

req.session.save(function(err) {
  // session saved
})

Possible fix is to not manually call req.session.save(). The downside is a lack in confirmation that the session data was successfully stored prior to closing the HTTP request.

@wavded
Copy link
Collaborator

wavded commented Jan 20, 2020

Per the last discussions in the other issue. I don't think we can reliably come up with a scheme that does the "right" thing when a user chooses to manually save. So this responsibility would fall on the user of the library (so the user manually updates the cookie.expires before saving). However, we probably should make a note of this in the readme perhaps as it was news to me (I never manually save).

If you want to just log errors you can set a 'error' handler on the client and capture it there as an option too.

@paton
Copy link
Author

paton commented Jan 21, 2020

Thanks @wavded. I really appreciate the quick response on this thread and also the other one. It's very much appreciated.

@vrg-success
Copy link

vrg-success commented Dec 29, 2020

I spend 48 hours, to get the error in production server, but the ease to fix!! You just don't need set maxAge or expired cookie in initialization session, you must set maxAge or expired in controller your app, for example:

app.use(
  session({
    store: new RedisStoreSession({ client: redisClient }),
    secret: process.env.SESSION_SECRET,
    cookie: {
      secure: process.env.NODE_ENV === 'production',
      httpOnly: true,
    },
    name: process.env.SESSION_COOKIE_NAME,
    resave: false,
    rolling: true,
    saveUninitialized: false,
  })
);

export async function usersLoginController(
  req: Request,
  res: Response,
  next: NextFunction
): Promise<Response | void> {
  req.session.cookie.expires = addDays(new Date(), 1);

  res.status(200).json({ csrfToken: req.session.csrfToken });
}

And yes, that was stupid from me, set expired cookie date in initialization... 😄

@NikitaSmithTheOne
Copy link

If you are using express + connect-redis you should use maxAge instead of expires in session setup.
Works well for me.

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

No branches or pull requests

4 participants