Repository navigation
fix: avoid duplicate save after regenerate - #1145
deepakganesh78 wants to merge 1 commit into
Conversation
Wrap regenerated sessions so manual saves update the request save tracking. Track the explicitly saved session id separately from the original id so modification and Set-Cookie semantics remain unchanged after session id rotation. Fixes expressjs#935 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
9700baa to
b1e56d4
Compare
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
I found one API compatibility regression in the new regenerate wrapper.
| function regenerate(callback) { | ||
| debug('regenerating %s', this.id) | ||
| var sess = this | ||
| _regenerate.call(this, function () { |
There was a problem hiding this comment.
The delegated call needs to be returned here. Session.prototype.regenerate() is documented and implemented to return this for chaining, but this wrapper replaces the method and currently returns undefined. I reproduced the change with the same middleware request against base d209c1f (ret === currentSession) and head b1e56d4 (ret === undefined). Please return _regenerate.call(...) and add a regression assertion for the return-value contract.
yxshee
left a comment
There was a problem hiding this comment.
I checked out b1e56d4 (it merges cleanly onto current master). Results: 175 passing and eslint clean. With index.js reverted to master, both new tests fail with 2 !== 1 store writes, so they do catch #935.
I also wrote a small script that counts MemoryStore#set calls and Set-Cookie headers for one request in each scenario (resave: false, saveUninitialized: false unless noted):
| Scenario | master | this PR |
|---|---|---|
regenerate → set value → save (the README login example) |
2 writes | 1 |
same, resave: true |
2 | 1 |
same, saveUninitialized: true |
2 | 1 |
regenerate twice, then save |
2 | 1 |
regenerate → save → change value after the save callback |
2 (final value stored) | 2 (final value stored) |
regenerate → save → reload → change value |
2 | 2 |
regenerate with no manual save |
1 | 1 |
Set-Cookie was sent once in every case on both branches, and the store always ended up with the last value. So the change only removes the redundant write, and a change made after the manual save is still persisted.
I can confirm the return-value point from the earlier review. var r = s.regenerate(cb) gives r === s on master and undefined here, which goes against the @return {Session} for chaining in session/session.js. Note that the existing reload and save wrappers already drop the return value on master (_reload.call(...) and _save.apply(...) aren't returned). So return _regenerate.call(...) fixes the regression this PR introduces, and doing the same for the other two would make all three consistent.
With that return added, this looks correct to me.
Fixes #935
Reproduction
On current
master, a request that callsreq.session.regenerate(), mutates the new session, and then callsreq.session.save()writes the same regenerated session twice. A minimal supertest reproduction countedMemoryStore#setcalls and showedtotal saves after response: 2.Root cause
regenerate()replacesreq.sessionwith a newSession, but the middleware only wrapped the original session's methods. As a result, a manualsave()on the regenerated session did not update the request's saved-session tracking hash, so the response finalizer treated it as unsaved and wrote it again.Fix
Wrap
regenerate()likereload()so the replacement session gets the middleware method wrappers. Track the explicitly saved session id in a separatesavedIdclosure variable alongsidesavedHash, and use that pair only forisSaved().Cookie-setting and modification semantics are unchanged:
originalIdis not mutated bysave(), so session id rotation still makesisModified()true and still emitsSet-Cookiefor the new id even when the regenerated session has no data changes.Compatibility / security notes
This is a conservative behavior fix: explicit session regeneration and saving still persists and sends the new session cookie, but avoids an extra store write that could overwrite concurrent updates. No dependencies or public API are changed.
Validation
0 passing, 1 failing, with2 !== 1store writes.0 passing, 1 failing, missing the expectedSet-Cookieheader.2 passing.175 passing (3s).npm run lint: passed.npm teston Windows fails before running tests because npm/cmd cannot execute./test/support/gencert.sh('.' is not recognized...); reran the same script/test command through Git Bash withMSYS_NO_PATHCONV=1successfully.