Repository navigation
fix: send Set-Cookie when only the session cookie changes - #1150
Open
saadkhantareen wants to merge 1 commit into
Open
saadkhantareen wants to merge 1 commit into
saadkhantareen wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1002. When a handler extends an existing session by changing only the cookie (for example setting
req.session.cookie.maxAgeto a larger value, then callingreq.session.touch()andreq.session.save()), the response contains noSet-Cookieheader, so the browser keeps the old expiry and the session is never actually extended client-side.The session data hash used for change detection (
hash()) deliberately ignores thecookieproperty, which is correct for save/resave decisions, but it also makes cookie-only changes invisible toshouldSetCookie(). For an existing session withrolling: false, the cookie was therefore only re-sent when the session data itself changed.Changes
index.js: snapshot a hash of the session cookie when the session is generated or inflated, and treat the cookie as modified when that snapshot no longer matches.shouldSetCookie()now also returnstruefor an existing session whose cookie changed during the request.cookie.datawithexpiresexcluded, because the expiry is refreshed by the automatic touch on every request and cannot take part in change detection. ChangingmaxAgestill registers, since setting it updatesoriginalMaxAgethrough theexpiressetter insession/cookie.js.test/session.js: regression tests covering the reported flow, plus a control test that an untouched session still receives noSet-Cookie.Tests
npm test— 175 passing, 0 failing (includes 2 new tests under "cookie option > .maxAge changed without modifying the session": "should set-cookie when only the cookie is extended and saved" and "should not set-cookie when nothing changed").index.jschange (AssertionError: should have a cookie header) and pass with it.npm run lint— clean.cookie.maxAgeto 24h, touches and saves): before the fix the second response had noSet-Cookie; after the fix it carriesSet-Cookiewith an expiry ~24 hours out.