-
Notifications
You must be signed in to change notification settings - Fork 342
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
touch does not update session.cookie.expires #351
Comments
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Activity! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Unstale |
So what is the expected behavior? The inner expires should sync with the outer one? If it is a bug. What is the current impact? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Unstale |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
unstale Ping @mingchuno |
As cookie expiration may be taken from the Lines 350 to 352 in 6c62235
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
unstale |
Ping @YC |
Lines 350 to 354 in 6c62235
connect-mongo/test/legacy-tests.test.js Lines 525 to 532 in 6c62235
connect-mongo/test/legacy-tests.test.js Lines 540 to 547 in 6c62235
The tests seem to be inconsistent as to what should be passed in the |
@YC do you recon what I described in my original post is the correct thing? |
If the cookie is updated due to |
@YC great, looking forward to the fix :) |
@thernstig @YC I have just rewrite the code using TS and published a new version! Can you please help me try it out to see of it fix your issue? https://www.npmjs.com/package/connect-mongo |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
unstale |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
unstale |
@YC What still need to be follow up on this issue? |
Current Behavior
Expected Behavior (my opinion)I performed a quick search on
Reference: https://expressjs.com/en/resources/middleware/session.html Happy to have a look at code if everyone agrees. |
Here are the logs from the different cases.
|
unstale..... |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Since it's not perhaps very clear how this mechanism works, it might be beneficial to write some integration tests against |
When
resave: false
androlling: true
then the store will update the top-levelexpires
in the session database on each request. express-session will also update the cookie and send it with every response.The problem is that the stored document at the path
session.cookie.expires
does not get updated, so it is out-of-sync with both the top level expires as well as the expires in the cookie.Since the
touch()
implementation already sends a call to the database to update the top-levelexpires
should it not also updatesession.cookie.expires
?This is the configuration of express-session:
The actual document saved to the store looks something like this:
The text was updated successfully, but these errors were encountered: