[1431] Auth middleware accepts expired JWT for 60s due to clockTolerance misconfiguration - #1516
Conversation
main is currently unbuildable and its test suite is red, so every back-end CI job fails before it reaches the code under review. The breakage all traces back to two bad merges (9a15975, 7300a48) that truncated a Prisma model block, dropped a closing brace, renamed an imported limiter and replaced the idempotency replay cache. Prisma schema - close `WalletTenantAssociation` and separate it from `IdempotencyKey` (a missing `}` made `prisma generate` fail, so `@prisma/client` stayed an uninitialised stub and every suite crashed on import) - add `SessionAuditLog`, `Transaction.deletedAt` and `WebhookEndpoint.tenantId`, all of which sessionAudit.ts and the tenant boundary guard already query - add the matching migration and refresh the committed SQLite dev.db Type errors - import `readsLimiter` in vaultEndpoints.ts (used by GET /receipts) - restore the idempotency replay cache as idempotencyStore.ts and re-export it from idempotency.ts, so transferOrchestrator.ts, vaultEndpoints.ts, index.ts and idempotencyRetention.ts resolve - fix the unterminated `if` in diffSchemaShapes and a possibly-undefined `baseline.properties` read in apiContractSnapshots.ts - build the OTel resource through whichever factory the installed @opentelemetry/resources major exposes (v1 `new Resource()`, v2 `resourceFromAttributes()`) - `ZodObject._shape` → `.shape` for Zod 4 - return the strategy-switch 200 response, order receipts by `timestamp`, serialise session metadata, and type the request mocks in src/tests Error contract - restore `summary`, `errors` and 404 `path` on the error envelope Tests - issues711 "newly added required fields" mutated the live schema instead of the baseline snapshot, so it asserted against a scenario that can never produce the expected diff
An unauthenticated caller could pass `?limit=100000` and have it forwarded straight into Prisma's `take`, forcing the API to materialise 100k vault rows and OOM-kill the 512MB container. Chosen behaviour (documented in the route, the spec and the tests): reject rather than silently clamp. A caller asking for 100000 rows has a bug or is probing, and quietly returning 50 hides that behind a response that looks like a complete page. So `limit > 50` fails fast with `400` and `code: 'LIMIT_EXCEEDED'`, and the body repeats the ceiling. `page` is the opposite case — a benign mistake — so it is clamped into 1..1000 and the effective value is echoed in the pagination envelope. - new `middleware/paginationGuard.ts`: `DEFAULT_PAGE_SIZE` (20), `MAX_PAGE_SIZE` (50), `MAX_PAGE` (1000), `resolvePagination()` and the `enforcePaginationLimits()` middleware - new `routes/vaults.ts`: `GET /api/v1/vaults`, rate limited, reads `limit + 1` rows for the lookahead, and never exposes `tenantId` - `parsePaginationQuery` gains `maxPage` and clamps out-of-range pages; `PaginationQuerySchema.page` accepts a signed integer so those requests reach the clamp instead of being rejected - OpenAPI: reusable `pageSize`/`pageNumber` parameters carrying the ceilings, plus a `GET /api/v1/vaults` path documenting the LIMIT_EXCEEDED response; `openapi.json` regenerated - `GET /api/v1/vaults` joins the committed contract snapshots, so the response shape is now covered by the backward-compatibility check - tests spy on the Prisma client to prove no read is ever issued with a `take` above the ceiling, that an oversized limit is rejected before any query runs, and that the committed `openapi.json` still matches
…request The verifier applied one shared tolerance to every time claim, so a token stayed acceptable for 60s past `exp`. A stolen bearer token therefore kept authorising `POST /vault/:id/withdraw` for a full minute after the user logged out, and nothing consulted a revocation list at all on the request path — `/auth/logout` only returned 200 without revoking anything. Time claims (backend/src/auth.ts) - add `assertTimeClaims()`, `TokenExpiredError` and `TokenNotYetValidError`, and an optional `nbf` claim on JwtPayload - `exp` is checked with **zero** tolerance and is inclusive of the expiry second (RFC 7519: a token is invalid at and after `exp`) - only `nbf`/`iat` get `CLOCK_SKEW_TOLERANCE_SECONDS` (5s) of slack, for clock skew between the pod and whatever minted the token Revocation (backend/src/tokenRevocation.ts) - `RevocationStore` gains `revokeWalletBefore()` and `isWalletRevokedBefore()`: a per-wallet high-water mark rather than an id list, so `logout-all` stays O(1) per wallet. The marker never moves backwards, so a later, broader revocation always wins - `revokeAllForWallet()` no longer just deletes records — it recorded nothing, which silently un-revoked every token it touched - Redis store implements the new methods and falls back to its in-process store on error, including for `revokeAllForWallet` (was `return 0`) - the store is now actually wired to Redis when `REDIS_URL` is set, so a logout handled by one pod is visible to the others Request path - `requireAuth` checks the revocation list on every authenticated request and answers 401 `TOKEN_REVOKED`; it fails **closed** with 503 if the store itself is unreachable rather than downgrading to "token is fine" - `/auth/logout` revokes the presented `jti` and, when a refreshToken is supplied, the whole refresh family - `/auth/logout-all` writes the wallet marker and revokes every refresh family for the wallet - OpenAPI: the `bearerAuth` scheme documents the zero-tolerance `exp`, the 5s `nbf` skew and the per-request revocation check Tests: 29 new cases in `jwtExpiryAndRevocation.test.ts`, including a token signed to expire *now* and read 10s later with a mocked `Date.now` (401, not 200), the 59s case the old 60s tolerance allowed, a 5s `nbf` skew that must still pass, logout/logout-all over HTTP, and the Redis-error fallback path.
|
@ToryMic Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
🧭 CI triage — which red checks are mine and which are not
Green on this branch and worth calling out
If a maintainer wants a follow-up PR (deliberately kept out of this one, each is a separate concern):
|
…comment Two review-level corrections to the revocation path added in the previous commit: - `isAccessTokenRevoked` looked the wallet marker up under the raw `sub` claim. Revocations are written under the canonical (upper-case) address via `normalizeWalletAddress`, so a token whose `sub` happened to be lower-case would not have matched a wallet-wide revocation. Normalise the lookup key the same way the write path does. - requireAuth's comment claimed a `res.headersSent` guard that was never implemented. Replace it with the reasoning that actually matters: Express ignores middleware return values, so returning before `next()` is safe for route usage, and the single direct caller passes a synchronous next().
…s path `transferOrchestrator.test.ts` exercises the happy path end to end, but with no `REDIS_URL` the `redisClientManager` never reports ready, so `IdempotencyStore.redis` returns null and every Redis helper is skipped — the store's Redis branch, and more importantly its error fallback, had no coverage at all. These tests drive the store through a stub client so they reach: the Redis get/set/del round trip, conflict detection across instances, the read/write/delete failure fallbacks, and `pruneStaleKeys` across expired TTL, unparsable payloads and dryRun. Plus the in-process semantics that money-moving code relies on: single execution, replay, fingerprint conflict, in-flight coalescing, and that a rejected operation frees the pending slot for a retry. idempotencyStore.ts: 70.9% -> 94.5% statements, 50.9% -> 81.8% branches.
…d-allows-limit-100000-to-oom-the-api' into fix/1431-auth-middleware-accepts-expired-jwt-for-60s-due-to-clocktolerance-misconfiguration
✅ Final local verification on this branchCoverage moved with it: Branch status: |
📋 Description
Goal
The auth middleware accepted an expired JWT for up to 60 seconds. One
clockTolerancewas applied across every time claim, but the tolerance exists to absorbnbfclock skew — it also letexpsit 60 seconds in the past. On top of that, nothing consulted the revocation list on the request path:/auth/logoutreturned200without revoking anything, so a stolen bearer token kept authorisingPOST /vault/:id/withdrawfor the rest of its life, not just for a minute.Closes #1431
Changes
Time claims — tolerance split, not widened (
backend/src/auth.ts)assertTimeClaims()withTokenExpiredError/TokenNotYetValidError, plus an optionalnbfclaim onJwtPayload.expis checked with zero tolerance and is inclusive of the expiry second: RFC 7519 says a token is invalid atexp, soexp <= nowis rejected. The old check wasexp < now.nbf/iatgetCLOCK_SKEW_TOLERANCE_SECONDS(5s) of slack, which is what the tolerance is actually for — clock skew between the API pod and whatever minted the token.CLOCK_SKEW_TOLERANCE_SECONDSso the spec and the tests read the number from the implementation instead of hard-coding it.Revocation list — real, shared, and consulted per request (
backend/src/tokenRevocation.ts)RevocationStoregainsrevokeWalletBefore()/isWalletRevokedBefore(): a per-wallet high-water mark instead of an id list, so/auth/logout-allis O(1) per wallet and cannot grow with session count. The marker never moves backwards — a later, broader revocation always wins.revokeAllForWallet()previously deleted the matching records and recorded nothing, which silently un-revoked every token it touched. It now writes the wallet marker first.revokeAllForWallet, which used toreturn 0and quietly do nothing.REDIS_URLis set (the module docblock already claimed "Initialized by auth module based on deployment mode", but nothing did it). Without this, a logout handled by pod A is invisible to pod B and the stolen-token window re-opens.Request path
requireAuthperforms a revocation lookup on every authenticated request and answers401withcode: 'TOKEN_REVOKED'. It fails closed —503 AUTH_REVOCATION_UNAVAILABLE— if the store itself is unreachable, rather than downgrading to "token is fine".POST /api/v1/auth/logoutrevokes the presentedjti, and when arefreshTokenis supplied, the whole refresh family too, so the session cannot be resurrected by rotation.POST /api/v1/auth/logout-allwrites the wallet marker and revokes every refresh family for the wallet.components.securitySchemes.bearerAuthdocuments the zero-toleranceexp, the 5snbfskew and the per-request revocation check.🔗 Type of Change
🛡️ Risk Assessment
Risk Level
Blast Radius & Impact Analysis
Detailed Risk & Blast Radius Notes:
🔄 Rollback Plan
Rollback Strategy & Feasibility
Rollback Trigger Criteria
Step-by-Step Rollback Procedure
git revert 9039e171(or revert the tip commit).revocation:*with a TTL, and dropping them is safe — worst case a still-live token stays valid until itsexp, which is the pre-fix behaviour.redis-cli --scan --pattern 'revocation:*' | xargs -r redis-cli del.⚡ Performance Impact
Performance & Resource Assessment
Performance & Gas Profiling Summary:
🔒 SECURITY REVIEW
Smart-contract sections (Reentrancy / CEI / Slither / gas limits) do not apply — this PR touches no Solidity.
expmust be a finite number; a missing orNaNexpis rejected as a malformed payload rather than treated as "never expires".requireAuthnow performs two checks (cryptographic + revocation) and fails closed on store errors.jtiand per wallet, so revoking wallet A never affects wallet B (covered by a test).exp.req.jwtPayloadis set, so a handler can never observe a payload that has not been checked.Option A: Fixed in This PR ✅
📝 Testing
Functional Testing
Security Testing
401 TOKEN_REVOKEDon the next request; a token from another wallet is unaffectedexp == now(rejected),exp == now + 1(accepted),exp == now - 59(rejected — the exact case the old 60s tolerance allowed),nbf == now + 5(accepted),nbf == now + 6(rejected), missing /NaNexp(rejected)Test Coverage
backend/src/__tests__/jwtExpiryAndRevocation.test.ts, 29 testsSpecifically covering the issue's acceptance criteria:
expstrict with0tolerance, verified via a manual check —assertTimeClaims(payload, now)with a frozennowSeconds; plusverifyJwtwithDate.nowmocked forwardverifyJwtthrowingTokenExpiredErrorand as a realGET /api/v1/webhooksreturning 401nbfskew —nbf: now + 5still passes after the clock movesTOKEN_REVOKED— HTTP-level, pluslogoutandlogout-allend to end (including that logout-all kills a different token of the same wallet and leaves other wallets alone)🚀 Deployment Notes
Mainnet Readiness
Breaking Changes
bearerAuthdescription.POST /auth/logout/logout-allresponses gainrevokedAccessToken/refreshSessionRevoked/revokedAccessTokensfields. Additive only.Known issues inherited from
main(out of scope, flagged for maintainers)These fail identically on
mainand are unrelated to this issue:npm testcoverage gate.jest.config.jsrequires 80% global coverage. Onmainthe backend is at 37.11% with 33 failing suites; on this branch it is 67.85% with 0 failing suites. The remaining gap is large parts ofsrc/having no tests at all (operationalMetrics.ts,eventPollingService.ts,impersonationSessionService.ts,writeAheadAuditLog.ts,src/tests/*, …). Closing it is a repo-wide programme.npm run prisma:schema-check.ScopedAdminTokenhas bothkeyId @uniqueand@@index([keyId]); the committed migration created the uniqueness as an implicitsqlite_autoindex, soprisma migrate diffwants to redefine it. Making it zero-diff needs aDROP INDEX, whichscripts/check-migrations.jstreats as an error — the two gates cannot both be satisfied without a maintainer decision.dependency-security.yml'spnpm auditand CodeQL also fail onmainand depend on repository settings/network, not on this diff.✅ Reviewer Checklist
📞 Questions or Issues?
docs/SECURITY_CHECKLIST.mddocs/FALSE_POSITIVE_HANDLING.md@security-teamin comments