Modifying session.cookie.expires does not send an updated Set-Cookie header

[kblomster]

This is, as far as I can tell, essentially the same as issue #189 from way back in 2015.

One would think that if session.cookie.expires (or maxAge, for that matter) is modified, a Set-Cookie header would be set on the response with the new expiration time. The documentation has an example that strongly implies that this is supposed to be possible. Unless you have rolling set to true, though, that doesn't actually happen. If the session store supports touch, the stored session will be touched properly with the modified expiration time, but the session cookie will never be updated, neither in the store nor in the browser.

The browser cookie will never get updated because shouldSetCookie ends up checking for req.session.cookie.expires != null && isModified(req.session) (index.js line 447), and isModified will always return false if the only thing you changed was the cookie, because the hash function ignores the cookie part of the session data (index.js line 582). Changing anything else (other than the expiration) in the cookie also has the same issue, of course, but that's a more exotic use case.

The same root cause (hash ignoring the entire cookie part of the session) also causes another annoyance. If you're using a touch-supporting session store that separates the storage expiration from the expiration timestamp inside the session data itself (probably most of them; connect-session-sequelize does it and I believe connect-redis does as well), rolling doesn't work properly without resave, as pointed out in the issue referenced above, because shouldSave ends up checking for !isSaved, which does the same thing - checks if the hash has changed, and it hasn't because the cookie part is ignored. The session store is touched properly because shouldTouch does the right thing, so the data storage timeout is updated, but not the actual session data which contains the cookie expiration. Unless you change something else in the session that makes the store resave it, req.session.cookie will always retain its original expiration timestamp and maxAge will keep counting down.

[dougwilson]

Hi @kblomster thanks for the informative post! It was unfortunate that the issue you referenced turned into too many things going on without anything really concrete. I'd love to take a look into this issue. If you're not able to provide a patch with test case for the issue, then can you provide all the following?

1. The exact version of this module
2. The exact version of Node.js
3. The exact versions of all other modules needs to reproduce.
4. Full, complete source code we can copy and paste into a file and run.
5. Full, complete instructions on how to reproduce. Please leave nothing implied, as everything you just leave to implied makes it that much more unlikely for us to reproduce. For example, saying "make a request to the server" is not enough information. What does the cookie jar in the browser contain when the request is made, for example?

[kblomster]

I'll see about providing a test case.

However, I think a question that would be far more interesting to ask than what Node version I'm using (come on now, really? I pointed out exactly where in your source code the logic I'm discussing is) is what the correct behavior actually is. As discussed above, with the current implementation there's a range of quite realistic scenarios in which the touching logic effectively does nothing except waste a bunch of write ops on some storage service, because while it keeps the session data in the store from expiring, it does absolutely nothing about the session cookie that links the browser to the data. Once the cookie expires it doesn't actually matter that the session store's data hasn't expired, because you can't find it anymore.

Setting rolling to true is one way of fixing this, but I think it's rather confusing behavior that session.touch() resets one expiration timestamp (in the store) but not the other (in the cookie). If it is indeed the intended behavior though (and I can certainly see that there are arguments that could be made in favor of separating the two expiration mechanisms), it should be documented.

[dougwilson]

AFAIK the code is operating exactly as intended, and I understand you described it, but it was just describing how it is supposed to be working. req.session.cookie can only be hand-altered before the session is saved, and after that it is just a reflection of what the cookie is set to, not something you can modify. It is exposed for complex use-cases right now, and can probably have more docs if you feel so inclined.

As far as I can tell, the only issue I can figure out here is just a misunderstanding you have of how the module works, which is why I asked to see what you're trying to do, to better understand what you're trying to accomplish and what the best approach is to solve this.

[dougwilson]

Sorry, clicked close on the wrong issue.

[dougwilson]

And to add, imo those modules are simply buggy or perhaps you're not using then correctly, not sure. That's another reason I am asking for example code to actually run through it and figure out what is happening. If you are confident in a solution, please feel free to make a pull request, otherwise I'm not sure what I need to do without being able to dig into the issue just like you have done.

[dougwilson]

I'm still just working off assumptions here, but I think the ignoring of the cookie in the hash function may no longer be necessary, since the automatic alteration of this object got moved around semi-recently. I'm taking a look. Originally it was excluded because as soon as the request came in, that object was altered, meaning the session would always get saved, no matter what, even if nothing changed. I believe at the time the ignoring of cookie for hashing was probably an easy way out (not 100% sure, since that was before I was a part of this project).

[kblomster]

Sorry if I came off as rude, I understand what you're saying and the need for test cases.

req.session.cookie can only be hand-altered before the session is first saved, and after that it is just a reflection of what the cookie is set to, not something you can modify.

Well, okay, that does clarify things and makes it far clearer to me why I'm supposed to be using rolling. However, in that case I think the section of README.md that talks about assigning to various cookie properties could be clarified to point out these limitations.

Really, as far as setting the cookie expiration vs setting the store expiration goes, I think that it could certainly be argued that it's not actually a bug and could be fixed by clarifying the documentation to say that if you want session.touch() to refresh both cookie and store, then you should be using rolling: true. I would certainly like if I could get the cookie automatically reset for me by manipulating req.session.cookie, but that's more of a feature request.

The resave/rolling combination though is another thing and I may have to return to that later. For now, it's Friday evening and I'll get back to you. Thanks for your time and your explanations :)

[slavab89]

I just stumbled upon this issue as i've noticed something similar on my end (Not exactly the same as i'm not using touch)
I think the main point is

I think the ignoring of the cookie in the hash function may no longer be necessary

Using rolling : true as a workaround will set the cookie on every request, am i correct? Not sure if its the intention.

[dougwilson]

Closing since I never got a test case and I believe all the outstanding questions were answered. If clarifications are needed in the README, please feel free to make a pull request with the suggested changes :)