fix(auth0-express): keep migrated sessions alive across the absoluteDuration gap - #46
Conversation
…uration gap A migrated express-openid-connect session keeps its original creation time (the legacy `iat`), and this SDK expires a session at `createdAt + absoluteDuration`. This SDK defaults absoluteDuration to 3 days while express-openid-connect defaults it to 7, so a legacy session already older than 3 days transformed fine but the write-back emitted a maxAge<=0 cookie the browser drops — a silent logout of the very sessions the feature exists to preserve, invisible to a fresh-login runbook. Document that a migrated session keeps its original createdAt and that absoluteDuration (and inactivityDuration) must be set to at least the old deployment's values, in both the zero-downtime guide and the `after` example. Add a regression test pinning both the default (cut short) and configured (kept alive) behaviour.
|
… store and docs Address code-review feedback on the migrated-session absoluteDuration fix: - Mirror the stateful regression tests into the stateless store spec, pinning both the default (cut short) and configured (kept alive) behaviour. The stateless store computes maxAge on set() rather than eagerly on get(), so the test drives get() then set() and asserts on the set cookie's maxAge. - Add a second Note callout in MIGRATION.md so the absoluteDuration caveat is not buried only inside the code fence. - Add an aged-session verification scenario to the example runbook, and wire a SESSION_ABSOLUTE_DURATION override into the after example so the step is runnable without editing code. - Clarify the inactivityDuration comment: 86400 already matches the SDK default and is kept only for symmetry with absoluteDuration.
916a06f to
b8ca586
Compare
…ation A migrated session keeps its original express-openid-connect creation time, so if the app does not raise absoluteDuration to at least the old deployment's value, a session already older than the default is dropped on its first write — an invisible misconfiguration that only surfaces as scattered "why was I logged out?" reports. Both migration stores now emit a console.warn on read when the base store's calculateMaxAge (its own private rolling/absolute durations) yields maxAge <= 0 for the transformed session — the exact condition under which the write-back drops the cookie. Delegating to calculateMaxAge keeps the check in lockstep with the real expiry decision and correctly stays silent when rolling is disabled (maxAge is then a constant absoluteDuration and the session is never dropped for age). The helper lives in legacy-session-transformer.ts, the shared migration-utilities module both stores already import from. Document the warning in MIGRATION.md and assert both the fires/does-not-fire cases in each store's spec.
b8ca586 to
763fa38
Compare
| `after` app: start it with `SESSION_ABSOLUTE_DURATION=1` (1 second), then reload | ||
| in the same browser. Expected: you are logged out on that first reload. In | ||
| DevTools the `appSession` cookie is dropped (stateless) or the Redis key is | ||
| written with an expired TTL (stateful). This is the failure the fix prevents. |
There was a problem hiding this comment.
For the stateless store this step does not reproduce as written. A reload only calls getUser/getSession, which are read only and never call set, so the appSession cookie is not dropped and the user stays logged in. The drop only happens on the first write, for example hitting /refresh-token.
For the stateful store there is also no expired Redis TTL. The example Redis set() writes no TTL at all, so the logout actually comes from the appSession cookie being written with Max-Age=0 on the write back.
Can we reword this step per store so it matches the real behaviour?
| if (iat !== undefined) { | ||
| stateData.internal.createdAt = iat; | ||
| if (this.calculateMaxAge(iat) <= 0) { | ||
| warnSessionDropped(iat); |
There was a problem hiding this comment.
This warns on every read. The stateless store has no write back, and reads (getUser/getSession) never call set, so for an aged session this fires on every request until some write finally drops the cookie. The stateful store only warns once. Could we emit it once (per process, or per createdAt) to avoid spamming the logs?
There was a problem hiding this comment.
Reworked this in ff780b4. Rather than warn on every read (or track per-session state to dedupe), the warning now fires once at store construction when sessionConfiguration.absoluteDuration is left unset — which is the actual misconfiguration. The store is built once per app startup, so it logs once, needs no bookkeeping, and surfaces the problem before any user is even affected. An app that explicitly set a value is not warned. The per-read behaviour is now unchanged from the modern-cookie path (aged session returned as-is, cookie dropped on next write).
…ion override Address review feedback on PR #46: - Scenario 3 reload does not reproduce the logout for the stateless store (reads never write back); reword per store — stateful drops the cookie on the reload's write-back, stateless needs a real write such as /refresh-token. Clarify the logout is the Max-Age=0 cookie, not an expired Redis TTL (the example store writes no TTL). - Guard SESSION_ABSOLUTE_DURATION with Number.isFinite so a non-numeric value falls back to the default instead of NaN, which would drop every session.
71bc406 to
5da3de1
Compare
…t config Address PR review on the migrated-session absoluteDuration handling: - Enforce this SDK's absoluteDuration on read. A migrated cookie still carries express-openid-connect's own Max-Age/exp (the old deployment's window), so unlike a modern cookie the browser keeps sending it past this SDK's cap and there is no modern exp to reject it. Both stores now return no session when calculateMaxAge(iat) <= 0, so an expired migrated session is ignored immediately and consistently rather than lingering on read-only traffic until the next write. Regression tests pin that read-rejection and write-drop agree on the same aged createdAt (no state the write keeps alive while the read logs out). - Warn once at store construction when sessionConfiguration.absoluteDuration is left unset in migration mode, instead of per read. The store is built once per app startup, so it logs once with no per-session bookkeeping; an app that set a value is not warned. Wording is honest that this is a heuristic (set vs unset), not a precise age check. - Reword runbook Scenario 3 per store and clarify read-time rejection; guard SESSION_ABSOLUTE_DURATION with Number.isFinite so a non-numeric value falls back to the default instead of NaN.
5da3de1 to
ddf2fdd
Compare
Summary
A migrated
express-openid-connectsession keeps its original creation time (the legacyiat), and this SDK expires a session atcreatedAt + absoluteDuration. This SDK defaultsabsoluteDurationto 3 days, whileexpress-openid-connectdefaults it to 7 days. So a legacy session already older than 3 days is transformed successfully, but the write-back emits aMax-Age=0cookie the browser drops — a silent logout of exactly the sessions the migration feature exists to preserve. It's invisible to a fresh-login runbook, since a new session is always younger than 3 days.Changes
createdAt, and thatabsoluteDuration(andinactivityDuration) must be set to at least the old deployment's values — in both the zero-downtime guide and theafterexample.Test plan
npm test— 266 passing (2 new)npm run build,eslint— cleanPart of a 3-PR split of the migration review findings (independent, not stacked).