Skip to content

fix(auth): fail closed when a session's stored expiry is unparseable#1498

Open
nickmopen wants to merge 1 commit into
JSONbored:mainfrom
nickmopen:fix/auth-session-expiry-fail-closed
Open

fix(auth): fail closed when a session's stored expiry is unparseable#1498
nickmopen wants to merge 1 commit into
JSONbored:mainfrom
nickmopen:fix/auth-session-expiry-fail-closed

Conversation

@nickmopen

Copy link
Copy Markdown
Contributor

Summary

authenticateSessionToken (src/auth/security.ts) gates a session on:

if (session.revokedAt || Date.parse(session.expiresAt) <= Date.now()) return null;

A session whose stored expires_at is malformed or empty parses to NaN, and NaN <= Date.now() is false. Combined with a null revokedAt, the session is then treated as valid and never-expiring — a fail-open on the expiry check.

Problem

expires_at is normally written by our own session-creation path, but the auth check should not depend on that invariant holding for every row (DB drift, a partial write, a manually inserted row). An unparseable expiry must be rejected, not silently honored forever. This matches the project's fail-closed posture (e.g. #1368 "fail closed when … unreadable").

Fix

Parse once and guard with Number.isFinite, so an unparseable expiry fails closed exactly like an expired one. Revoked and past-dated behavior is unchanged.

Validation

Run from repo root (Node 24):

npm run test:ci   # GATE_EXIT=0 — 4607 passed / 4 skipped; typecheck, coverage, workers, MCP, UI all green

Added a regression assertion alongside the existing expired-session test in test/unit/auth.test.ts: a session with expires_at = "not-a-date" now authenticates to null (it would have returned a valid identity before this fix).

authenticateSessionToken rejected revoked and past-dated sessions, but a session whose stored
expires_at is malformed/empty parsed to NaN, and `NaN <= Date.now()` is false — so it
authenticated as if it never expired. Guard the parse with Number.isFinite so an unparseable
expiry fails closed (rejected), matching the expired-session path. Mirrors the existing
expired-session test with a malformed-expiry case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@nickmopen nickmopen requested a review from JSONbored as a code owner June 26, 2026 12:48
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 26, 2026
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.44%. Comparing base (e39566a) to head (0c28ef7).
⚠️ Report is 7 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1498   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files         194      194           
  Lines       21096    21097    +1     
  Branches     7630     7630           
=======================================
+ Hits        20136    20137    +1     
  Misses        383      383           
  Partials      577      577           
Files with missing lines Coverage Δ
src/auth/security.ts 98.68% <100.00%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant