Nacodex — Sample audit: expressjs/session

Audited	expressjs/session at commit 8947e169ae838b22cc1af198e50e370e6fc09f96
Date	11 October 2026
How it ran	API run on the Nacodex server, full audit, Standard review
Verdict after review	Pass with notes (rule: Fail if a High finding remains after review, otherwise Pass with notes)

High after review

Medium after review

Low after review

Info after review

excluded on review

How to read this page

Each finding keeps the number it has in the audit report. The rating shown first is the one after review; the first automated rating is listed with it. 5 findings were first rated Medium or High; 1 of them are Medium or High after review. Text marked "From the report" is quoted from the audit report; fixes are suggestions and were not tested. Code locations link to the file and line at the audited commit.

Medium after review (1)

8. Medium Response headers go out before the session store write finishes, so store errors cannot change the response

First rating: Medium · Reviewed rating: Medium · Review: confirmed

Location: index.js:294, index.js:329 (same pattern for save at index.js:354 and touch at index.js:366)

From the report

Evidence

if (!res._header) {
  res._implicitHeader()
}

Why it matters

When the response ends, the middleware starts the store call (save, touch or destroy) and immediately writes the status line, the headers and all but the last byte of the body. Only the final byte waits for the store. If the store then fails, the error goes to next() after the client has already received a success status. For example, with unset: 'destroy' a logout can look completed to the user while the session is still valid on the server. A login can also appear to succeed while its session was never stored.

Suggested fix, not tested

For flows where it matters, call req.session.save() or destroy() explicitly and send the response only in the callback, and document this. In the middleware, consider delaying the header write until the store call has succeeded, so a store error can still produce a 5xx response.

Review: Quotes: Trace: each branch registers the store call, then return writetop(); writetop forces the implicit header (294-296) and, with a body and a Content-Length, writes all but the last byte (314-316), otherwise writes the whole chunk (320). The store callback calls defer(next, err) on error but then still runs writeend() (330-336, 355-359, 367-372), so the response completes and the error reaches next after the status line is out. The client sees the original status although the store write failed. Claim correct. Small imprecision: "all but the last byte" applies only when Content-Length > 0; without it the whole chunk goes out (line 320). With no chunk, headers are fixed but not yet flushed. The error is not silent server side: it goes to next(err) and is logged by the error handler. Mitigations: this is the long-standing design (split-response trick), and README (Session.save section) tells users to call save explicitly for redirects. Impact needs a store failure. I keep MEDIUM (borderline LOW-MEDIUM) because the logout/destroy case leaves a live session while the user sees success. Library code. Matches: #538 "How to handle session store error" (closed; user gets 200 when sessionStore.set fails, headers already sent), #360 "Chrome/FF follow redirect before session is fully saved" (open, 28 comments), #1037 "Simplify res.end() proxy logic" (open). Publicly known.

Upstream: Already reported upstream: #538, #360, #1037.

Low after review (7)

1. Low With rolling sessions, the store's expiry is not extended

First rating: Medium · Reviewed rating: Low · Review: rated too high

Location: index.js:363 (decision at index.js:491)

From the report

Evidence

} else if (storeImplementsTouch && shouldTouch(req)) {
...
: rollingSessions || req.session.cookie.expires != null && isModified(req.session);

Why it matters

This applies with rolling: true, resave: false and a store that has no touch(). Every response sends a refreshed Set-Cookie with a later expiry. The session data has not changed, so it is not saved, and there is no touch() to call. The browser's expiry moves forward on each request, but the record in the store keeps its old expiry. Stores that use the cookie expiry as a TTL then drop the session while the user is still active, and the user is logged out unexpectedly.

Suggested fix, not tested

Detect this combination when the middleware is built, and either throw or warn. Alternatively, fall back to a full save() whenever the cookie is rolled and the store cannot touch(), so the stored expiry and the browser expiry always move together.

Review: Quotes: Trace confirmed. With a store lacking touch, resave:false (savedHash = originalHash set in inflate, lines 395-397) and rolling:true: shouldSetCookie is true (491), req.session.touch() runs (349) and moves cookie.expires in memory, shouldSave is false (hash ignores cookie, see item 4), the touch branch (363) is not entered, so end() runs _end at 378. Browser cookie extended, stored record not. Mechanism correct. Why OVERSTATED: README.md:290-293 (resave section) says exactly this: "If it does not implement the touch method and your store sets an expiration date on stored sessions, then you likely need resave: true." The default MemoryStore implements touch (memory.js:147), so the default setup is fine. It needs a specific option combination plus a third-party store without touch, and the README names the remedy. Fair LOW (a startup warning would help). Library code. Related, not identical: #891 "session.touch is called on every request when resave: false" (closed), #1002 "Set cookie is not being passed in header when session is extended" (open), #624 "Session TTL is auto-reset on each request even after session expiry (resave: false)" (closed). No exact match.

Upstream: No exact upstream report; related items: #891, #1002, #624.

2. Low After reload(), an unchanged session is written back and can overwrite a concurrent update

First rating: Medium · Reviewed rating: Low · Review: rated too high

Location: index.js:417, index.js:450

From the report

Evidence

function reload(callback) {
  _reload.call(this, rewrapmethods(this, callback))
...
return originalId === sess.id && savedHash === hash(sess);

Why it matters

The "has it changed?" baseline (originalHash, and savedHash when resave is false) is taken once, when the session is first loaded. reload() replaces the data but does not reset that baseline. Fresh data from the store no longer matches the old fingerprint, so the end-of-request check treats the session as modified and writes it back. Suppose a second request changes the session after this one has reloaded it. This request then writes its stale copy over that change, even though its handler never modified anything.

Suggested fix, not tested

After a successful reload, recompute originalHash from the reloaded session, and also savedHash when resave is false. The skip-if-unchanged check then compares against what was actually loaded.

Review: Quotes: Session.reload (session/session.js:88-99) calls store.createSession(req, sess); the baselines originalHash/savedHash are set only in generate()/inflate() (384-397) and the save wrapper (424). After a reload with changed store data, the hash differs from the baseline, isSaved (450) is false, and the reloaded copy is saved again at the end. Mechanism correct. Why OVERSTATED: it matters only with resave:false (with the default resave:true the session is saved at the end anyway, savedHash is undefined). The write-back happens only if store data really changed between first load and reload, and the data written is the freshly reloaded copy, so it can only clobber changes made by another request in the window between the reload and the end of this request. Read-modify-write races exist for any handler that modifies the session. The report's "stale copy" wording overstates: the copy is fresh as of the reload. Fair LOW. Library code. Upstream: repo:expressjs/session reload overwrite resave 1 hit (#64 closed, unrelated 2013 PR); reload hash 2 hits (#1145 about regenerate, #543 unrelated); reload concurrent 1 hit (#1145). No match found.

Upstream: No matching upstream report found (11 October 2026).

3. Low Sessions created by regenerate() skip the save tracking

First rating: Low · Reviewed rating: Low · Review: confirmed

Location: session/store.js:52 (line corrected on review; the report cites :53) (generator at index.js:158)

From the report

Evidence

this.destroy(req.sessionID, function(err){
  self.generate(req);
  fn(err);

Why it matters

The middleware wraps save() and reload() so that it can track whether the session has already been saved. It does this only in its private generate() and inflate(). The store.generate used by regenerate() creates a plain Session, so a req.session.save() inside a regenerate callback is not tracked, and the session is saved a second time when the response ends. A reload() on that session also skips the re-wrapping. We saw no data loss, but this is inconsistent and causes redundant store writes.

Suggested fix, not tested

Apply the same wrapping to every new session object. For example, wrap inside the generator that regenerate() uses, or after regenerate() completes.

Review: Quote (store.js:52-54): this.destroy(req.sessionID, function(err){ then self.generate(req); then fn(err);. store.generate is the middleware closure at index.js:158, which does not call wrapmethods (only the local generate() at 382-387 does). After regenerate, a manual req.session.save() does not update savedHash, so the end-of-request check saves again. Redundant write only (originalId !== sess.id makes shouldSave true anyway). Upstream: issue #935 "Regenerated session is re-saved even if not modified since save" (open; notes a possible race); PR #1145 "fix: avoid duplicate save after regenerate" (open, created 2026-08-02). LOW is fair. Library code.

Upstream: Already reported upstream: #935, #1145.

4. Low Cookie-only changes are invisible to the change check

First rating: Low · Reviewed rating: Low · Review: confirmed

Location: index.js:655

From the report

Evidence

// ignore sess.cookie property
if (this === sess && key === 'cookie') {
  return

Why it matters

The fingerprint that decides whether to save ignores session.cookie, but the cookie is part of the stored record. If a handler changes only cookie settings, for example req.session.cookie.maxAge for "remember me", the session is not saved. The change reaches the store only through touch(), which some stores do not implement. This is intended behaviour but it is not documented, and it is easy to trip over.

Suggested fix, not tested

Document that cookie-only changes are persisted only through touch() or an explicit save(). Alternatively, include the persisted cookie fields in the fingerprint when the store has no touch().

Review: Quote (index.js:654-656): // ignore sess.cookie property, if (this === sess && key === 'cookie') {, return. Confirmed; also shouldSetCookie (491) uses isModified, so a cookie-only change does not even re-send Set-Cookie unless rolling. Persisted only via touch() on stores that have it (MemoryStore.touch copies session.cookie, memory.js:152). README (Cookie.maxAge section) does not mention this. Fair LOW. Library code. Related: #452 (closed).

Upstream: No exact upstream report; related items: #452.

5. Low Store event listeners accumulate

First rating: Low · Reviewed rating: Low · Review: confirmed

Location: index.js:179

From the report

Evidence

store.on('disconnect', function ondisconnect() {
  storeReady = false
})

Why it matters

Each call to session() adds a disconnect listener and a connect listener to the store and never removes them. Applications or test suites that build the middleware repeatedly against one long-lived store pile up listeners, and Node eventually prints a max-listeners warning. The closures also keep the old middleware instances in memory.

Suggested fix, not tested

Register the listeners once per store, for example behind a flag on the store. Alternatively, expose a way to detach them.

Review: Quote (index.js:179-184): store.on('disconnect', function ondisconnect() { and store.on('connect', function onconnect() {. Confirmed: two listeners per session() call, never removed, closures keep storeReady and options alive. Triggers only when session() is called repeatedly on one store (tests, per-request construction). Upstream issue #949 reports exactly the MaxListenersExceededWarning (closed). Fair LOW. Library code.

Upstream: Already reported upstream: #949.

9. Low The middleware overwrites generate on the shared store object

First rating: Medium · Reviewed rating: Low · Review: rated too high

Location: index.js:158

From the report

Evidence

store.generate = function(req){
  req.sessionID = generateId(req);
  req.session = new Session(req);

Why it matters

Each session() call writes its own generate function, with its own genid and cookie options, onto the store object it is given. If two middleware instances share one store (for example, different cookie settings for two sub-apps), the last one registered wins. New sessions and regenerate() calls in the first instance then silently use the second instance's ID generator and cookie options (path, domain, secure, sameSite).

Suggested fix, not tested

Keep the generator inside the middleware closure, and pass it to regenerate() through the request or as an argument. Do not mutate the caller's store object.

Review: Quotes: 158: store.generate = function(req){ and 383: store.generate(req);. Confirmed: each session() call assigns its closure to the store object, and both new-session creation (383) and Store.prototype.regenerate (store.js:53) call store.generate. With two instances on one store the last registered wins, including genid and cookie options (path, domain, secure, sameSite). Silent. Narrow: needs two session() instances sharing one store object. Fair LOW-MEDIUM (the effect on cookie flags is what matters). Library code. Upstream: issue #521 "Sharing session store between multiple session instances doesn't work" (open since 2017-11-04, includes a failing test showing the later instance's cookie options being applied); related #613 "Multiple session configurations getting wrong cookie path" (closed). Publicly known.

Upstream: Already reported upstream: #521, #613.

10. Low The default in-memory store grows without limit

First rating: Medium · Reviewed rating: Low · Review: rated too high

Location: session/memory.js:119 (line corrected on review; the report cites :120) (defaults at index.js:100 and index.js:128)

From the report

Evidence

MemoryStore.prototype.set = function set(sessionId, session, callback) {
  this.sessions[sessionId] = JSON.stringify(session)

Why it matters

MemoryStore deletes an expired session only when that session is read again, and it never sweeps or caps the rest. The default cookie has no expiry, so such sessions never expire at all. If saveUninitialized is left unset, it defaults to true, and every request without a cookie (bots, scanners, health checks) adds a permanent entry. An anonymous client can grow the process memory until it crashes. The risk is documented, but the console warning appears only when NODE_ENV is production.

Suggested fix, not tested

Add a periodic, unref'd sweep and a maximum-entries cap to MemoryStore. Warn in every environment. Consider refusing the default store when NODE_ENV is production.

Review: Quotes: memory.js:119-120: MemoryStore.prototype.set = function set(sessionId, session, callback) { then this.sessions[sessionId] = JSON.stringify(session); index.js:100: var store = opts.store || new MemoryStore(); index.js:126-129 saveUninitialized undefined defaults to true (with a deprecate call). Expired entries are deleted only when read (memory.js:164-184); no sweep or cap. The default cookie has no expiry (Cookie maxAge null). The warning appears only when NODE_ENV === 'production' (index.js:33, 153). All factual claims verified. Why OVERSTATED: the store is "purposely not designed for a production environment. It will leak memory under most conditions" (README.md:36-39), the code warns at runtime in production, and maintainers treat it as non-production by design. The report itself says "the risk is documented". A memory-growth DoS exists only for deployments that ignore both warnings. Fair LOW for a development-only store; MEDIUM only if viewed as a deployment hazard. Library code. Upstream: MemoryStore memory leak unbounded 0 hits; MemoryStore expired sessions cleanup 1 hit (#220); MemoryStore memory leak 8 hits.

Upstream: Already reported upstream: #128, #220, #487, #556, #871.

Info after review (2)

6. Info The same defer helper is copied into several files

First rating: Low · Reviewed rating: Info · Review: rated too high

Location: index.js:65, also session/memory.js:25 and test/support/smart-store.js

From the report

Evidence

var defer = typeof setImmediate === 'function'
  ? setImmediate
  : function(fn){ process.nextTick(fn.bind.apply(fn, arguments)) }

Why it matters

Three copies of the same compatibility shim have to be fixed together. If one copy changes, the other modules quietly keep the old behaviour.

Suggested fix, not tested

Move the shim into one internal module, for example session/defer.js, and require it everywhere. Since modern Node always has setImmediate, you could also drop the shim entirely.

Review: Quote (index.js:65-67): var defer = typeof setImmediate === 'function' ? setImmediate : function(fn){ process.nextTick(fn.bind.apply(fn, arguments)) }. Copies at index.js:65, session/memory.js:25 and test/support/smart-store.js:7 (test file). package.json engines is node >= 0.8.0, which is why the shim exists. Accurate but trivial: a 3-line shim marked istanbul ignore next, no behaviour risk. Fair INFO. One of the three copies is test code.

7. Info A known-better default is postponed to a future major release

First rating: Low · Reviewed rating: Info · Review: rated too high

Location: index.js:135

From the report

Evidence

// TODO: switch to "destroy" on next major
var unsetDestroy = opts.unset === 'destroy'

Why it matters

Today's default is keep. When a handler deletes or nulls req.session, the stored session is left in place, so the server keeps a session the application thought it had removed. The resave and saveUninitialized defaults at least produce a warning when they are left unset. This default produces none.

Suggested fix, not tested

Warn when unset is omitted, the same way the other two defaults do. Also state the current behaviour clearly in the README rather than relying on the next major release.

Review: Quote (index.js:135-136): // TODO: switch to "destroy" on next major and var unsetDestroy = opts.unset === 'destroy'. Accurate that the default is keep and no warning is emitted (resave/saveUninitialized do call deprecate, lines 121-129). The report omits that README.md:366-375 documents the default and its meaning ("The session in the store will be kept, but modifications made during the request are ignored and not saved"). Changing a default or adding a warning is a maintainer API decision, not a defect. Fair INFO. Library code.

Want this for your code?

Upload a ZIP or link a public GitHub repo, pick the areas and the depth, and get findings by severity with fixes.