Address P0/P1 code-quality findings + auto-refresh on token expiry#80
Merged
Conversation
Introduce small per-app helpers that the route refactors depend on:
- utils/errors.ts: FDX-compliant error builder + curated set of
reusable error responses ({ code, message, debugMessage? }).
- utils/asyncHandler.ts: wraps async route handlers so rejected
promises propagate to Express's error handler.
- utils/auth.ts: AuthenticatedRequest type, getSubject helper, and
requireScope middleware for selectively enforcing OAuth scopes
per-route. Augments Express's Request via global declaration so
req.user is typed everywhere without per-route casts.
…okup
getCurrentCustomer now requires the authenticated user's id (the JWT
sub claim) instead of always returning the first record. This removes
the cross-customer data leak where any authenticated user resolved to
"customer-123" regardless of identity.
The first customer fixture is updated so its customerId matches the
demo user's sub ("user_123" - see apps/auth/src/index.ts USERS map),
allowing the demo flow to continue working end-to-end after the lookup
becomes identity-driven.
Add a customerId field to every account record and update the
repository so account access is always scoped by the authenticated
user. getAccounts now requires a customerId and filters to that
customer; getAccountForCustomer pairs a lookup with an ownership
check and treats not-owned identically to not-found so we don't leak
the existence of other customers' accounts.
All demo accounts are assigned to the demo user ("user_123") so the
existing happy-path flow keeps working after the route handlers move
to the ownership-aware repository helpers.
P0: protect all interaction POST routes (login/confirm/cancel) with the maintained `csrf-csrf` double-submit cookie middleware and embed a hidden CSRF token in every interaction form. The CSRF session is bound to the interaction UID so each login flow gets a uniquely-scoped token. P1: rate-limit POST /interaction/:uid/login at 10 attempts per 15 minutes per IP via `express-rate-limit`. The limiter renders a friendly EJS error in production and exposes `LOGIN_RATE_LIMIT_*` env vars for tuning. P1: replace the five eslint-disabled `any` annotations in apps/auth with narrow types from a new minimal `oidc-provider` module declaration, and swap the response interception body/chunk over to `unknown` with a typed parser. P1: hide the development credential block in views/interaction.ejs unless NODE_ENV !== 'production' (exposed via `app.locals.isProduction`). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Route handlers now derive the caller from req.user.sub and reject
account access where the caller does not own the account, fixing the
IDOR on every /accounts/:accountId, /contact, /statements,
/statements/:statementId, /transactions, /payment-networks, and
/asset-transfer-networks endpoint. The list endpoint is filtered to
the authenticated user.
Also:
- Wrap every async handler in asyncHandler so rejected promises
propagate to Express's error middleware instead of falling through
silently.
- Replace the ad-hoc { error, details } and mixed { code, error }
shapes with the centralized FDX { code, message, debugMessage? }
helpers so all routes return a consistent error contract.
- Type handlers as AuthenticatedRequest instead of bare Request.
Apply requireScope("accounts:read") in front of the accounts router
so the JWT's scope claim is checked once, centrally, before any
account/data route runs. /customers/current is left without the
extra scope guard because resolving the authenticated user only
needs the openid scope (already implicit in any authenticated
session).
Also small cleanups:
- Pull AuthenticatedRequest in from utils/auth so the JWT middleware
uses the canonical type and Express Request is augmented globally.
- Rewrite the 404 handler to return the FDX error shape.
- Add the missing NextFunction parameter to the global error handler
so Express recognizes its 4-arg signature.
P0 fixes:
* Sign httpOnly cookies (`signed: true`) and read tokens via
`req.signedCookies`. Cookie-parser was already initialized with
COOKIE_SECRET; the cookies just weren't being signed.
* Mask access/refresh/id tokens to first/last 4 chars on /debug/tokens
and gate the route behind `NODE_ENV !== 'production'`. Decoded payload
is still shown for development debugging.
* Harden the JWKS link in the token inspector: pass an absolute, validated
URL from the route handler instead of rendering `process.env.OP_ISSUER`
directly into an `href`. Add `rel="noopener noreferrer"`.
P1 fixes:
* Generate an OIDC `nonce` in /login, persist it alongside state and PKCE
verifier in the signed `oidc` cookie, and validate it via
`expectedNonce` on `authorizationCodeGrant` in /callback.
* Replace the local-only logout TODO with proper RP-initiated logout:
redirect to the auth server's `end_session_endpoint` with
`id_token_hint`, `client_id`, `state`, and `post_logout_redirect_uri`.
Local cookies are still cleared up front so the local session is gone
even if the auth server is unreachable.
Auto-refresh feature:
* New `getValidAccessToken(req, res)` helper:
- reads tokens from the signed cookie,
- decodes the access token (`exp` claim) with a 30s clock-skew buffer,
- if expired, calls openid-client's `refreshTokenGrant` with the
resource indicator,
- atomically writes the new token set back to the signed cookie before
returning the new access token.
* /api-call uses the helper proactively, then falls back to one
refresh-and-retry when the API returns 401 despite a non-expired token
(covers clock skew, server-side revocation, etc.).
Verified by:
* `pnpm --filter @apps/app build` and `npx eslint apps/app` both pass.
* Standalone test script for the expiry detection logic confirms
fresh / stale / within-skew / malformed / no-exp tokens all classify
correctly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the P0 and P1 findings from the auth/api/app code-quality review and adds automatic access-token refresh in the relying party.
apps/auth
csrf-csrf(double-submit, scoped per interaction UID); hidden_csrfinput added to login/confirm/cancel forms.express-rate-limit(default 10 attempts / 15 min / IP, configurable throughLOGIN_RATE_LIMIT_*env vars).anys inapps/auth/src/index.tswith a typed localoidc-providermodule declaration and a typedparseTokenBodyhelper.views/interaction.ejsis now gated to non-production.apps/api
requireScopemiddleware enforcesaccounts:readon the FDX accounts router.customerId; routes resolvereq.user.suband 404 on not-owned (no existence leak); list endpoint filters to the authenticated user.getCurrentCustomer(userId)derives from the JWT subject —customer-123hardcode removed. Demo fixtures rebound touser_123to match the authsub.{ code, message, debugMessage? }) viautils/errors.ts; newasyncHandlerwrapper; routes typed asAuthenticatedRequest.apps/app
signed: true+req.signedCookies)./debug/tokensmasks tokens to first/last 4 chars and 404s in production./tokenview receives a validated absolutejwksUrlinstead of rendering rawOP_ISSUER.noncegenerated, persisted in the signedoidccookie, and validated on callback.end_session_endpoint(withid_token_hint,state,post_logout_redirect_uri); local cookies cleared up front.getValidAccessToken(req, res)decodesexpwith a 30s skew buffer, refreshes viaclient.refreshTokenGrantwhen expired, and atomically writes new tokens to the signed cookie before returning the access token./api-callis proactive + reactive (one 401-retry-after-refresh fallback).Test plan
pnpm lintpassespnpm buildpasses for all four workspace projects/api-call, confirm cookies rotate and call returns 200accounts:read/debug/tokensreturns 404 with NODE_ENV=production🤖 Generated with Claude Code