acme/web · pull request

Refactor session auth to short-lived tokens #247

main ← feature/short-lived-tokens

@dmitri-k wants to merge 14 commits Reviewed by agent

Request changes

The migration to short-lived access tokens is well structured, but two blocking issues remain: refresh tokens are not rotated on use, and a token-expiry check uses the wrong clock. Address the bug-level findings below before merge.

Files changed

5 files changed +248 −96

src/auth/tokenService.ts +132−41

src/auth/middleware/requireSession.ts +44−18

src/auth/refreshStore.ts +39−7

src/config/auth.ts +21−9

test/auth/tokenService.test.ts +12−21

Findings

Bug src/auth/refreshStore.ts:58

Refresh tokens are validated but never invalidated after use. A leaked refresh token stays valid for its full 30-day TTL and can be replayed indefinitely, defeating the point of short-lived access tokens.

src/auth/refreshStore.ts — issueAccessToken()

  const record = await store.find(refreshToken);
  if (!record || record.revoked) throw new AuthError("invalid_grant");
- // token reused as-is on every refresh
- return signAccessToken(record.userId);
+ // rotate: revoke old, mint a fresh refresh token
+ await store.revoke(record.id);
+ const next = await store.issue(record.userId);
+ return { access: signAccessToken(record.userId), refresh: next };

Suggested fix: Rotate the refresh token on every exchange and revoke the prior token; reject reuse of a revoked token (replay detection).

Bug src/auth/tokenService.ts:91

Expiry is compared against Date.now() (milliseconds) while the JWT exp claim is in seconds. Every token reads as expired ~1000× too early, so sessions drop almost immediately in production.

src/auth/tokenService.ts — verify()

- if (claims.exp < Date.now()) throw new TokenExpiredError();
+ if (claims.exp < Math.floor(Date.now() / 1000)) throw new TokenExpiredError();

Suggested fix: Convert to seconds for the comparison, or use a single time helper shared by signing and verification.

Warning src/auth/middleware/requireSession.ts:27

The middleware swallows verification errors and falls through to an anonymous session on any failure, including a tampered signature. A malformed token should be a 401, not a silent downgrade.

Suggested fix: Distinguish "no token" (continue anonymous) from "invalid token" (respond 401) and log the verification error.

Warning src/config/auth.ts:14

Access-token TTL is hard-coded to "15m" as a string and parsed ad hoc in two places. The two parsers disagree on whitespace handling, which is fragile.

Suggested fix: Store the TTL as a number of seconds in config and parse once at load time.

Nit src/auth/tokenService.ts:140

Two removed tests covered the old long-lived-token path; their rotation replacements are not yet added, so refresh rotation is currently untested.

Suggested fix: Add a test asserting that a reused refresh token is rejected after rotation.

Nit src/auth/refreshStore.ts:12

Imports are no longer ordered after the refactor and there is one unused crypto import left over.

Suggested fix: Run the linter's auto-fix; drop the unused import.

Summary: Solid direction and clean structure. Fix the two bug-level findings (refresh rotation and the seconds/milliseconds expiry comparison), tighten the middleware error path, and restore rotation test coverage. Happy to re-review once those are in — this should be close after that.

2 bugs · 2 warnings · 2 nits | reviewed automatically · 2026-06-07