fix(auth): register the error handler before routes so it actually runs - #388
Merged
Merged
Conversation
The app-level `setErrorHandler` was registered after both `@fastify/autoload`
calls, under a comment claiming the last position "catches all unhandled
errors". That is backwards. A route does not resolve its error handler at
request time — Fastify snapshots it while closing the route's enclosing plugin
(`context.errorHandler = this[kErrorHandler]` in the `after()` callback of
`lib/route.js`), and avvio runs queued plugins in registration order. The
handler was therefore unreachable from every route, and Fastify's built-in
`{statusCode, code, error, message}` envelope answered every error.
What that silently reverted:
- The F-01 register-enumeration fix (PR #212). `UniqueConstraintError` is
genericised so the DB constraint name never reaches the wire; the built-in
envelope echoes `error.message`, which embeds it.
- The RFC 6750 section 3 `WWW-Authenticate: Bearer` challenge that a
bearer-protected resource such as /oauth/userinfo MUST return on a 401.
- The RFC 6749 section 5.2 OAuth error shapes.
- `error_description` sanitisation for that challenge header.
The handler now registers ahead of `cors` rather than merely ahead of the two
AutoLoads: `@fastify/cors` registers an `options('*')` route of its own, and
`@fastify/rate-limit` throws its 429 from an earlier `onRequest` hook, so a
rate-limited OPTIONS request would otherwise still render the built-in shape.
Also corrects the surrounding ordering comments, which asserted guarantees
Fastify does not provide, and records the two that are genuinely load-bearing:
- `rateLimitPlugin` MUST precede the routes AutoLoad. It applies per-route
limits through an `onRoute` hook, and `fastify.js` routes `onRoute` past the
`kChildren` recursion in `_addHook`, so it is not retroactive. Moving it
would silently drop the /auth/login brute-force ceiling with no error.
- `securityHeaders` / `requestId` positions are NOT load-bearing, but via
`_addHook`'s recursion into existing child scopes — not via `preReady`, which
reads a snapshot. The distinction is what separates them from `onRoute`.
- `main.ts`'s validator/serializer compilers MUST precede `register(app)`. A
child scope snapshots them at creation with no propagation, so moving them
would return every route to ajv validation and reactivate the fast-uri
host-confusion advisories the SECURITY INVARIANT comment says are inert.
Regression coverage registers the real `app` plugin and injects over HTTP,
rather than the stubbed-Fastify pattern that let this ship: the existing route
tests build mock instances, and `error-handler.test.ts` exercises the handler in
isolation where it works correctly. Nothing asserted it was reachable. Three of
the four cases fail if the registration is moved back below the AutoLoads.
`vitest.config.ts` inlines `@fastify/autoload` into Vite's module graph, without
which the assembled app cannot boot under Vitest — autoload resolves route
modules by absolute path at runtime. It is set at the workspace root because the
root `qauth:test` target globs every project's specs, auth-server's included.
Fixes #365
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DWdz6Yd57TV48dTbVCej6j
…-unreachable-from-routes
#365 made `plugins/error-handler.ts` reachable from routes for the first time. Three of its outputs were wrong in ways that only mattered once something could see them. **RFC 6749 §5.2 error codes.** `InvalidClientError` and `UnauthorizedClientError` took their detail as a MESSAGE, and the handler put the message in `error` — so a described failure shipped `{"error": "CIMD document is not valid JSON"}` where the specification requires the registered token plus a separate `error_description`. The response shape depended on which call site threw, and nothing branching on `error === 'invalid_client'` could read it. Both classes now carry the token as their message and the prose as `errorDescription`, matching `InvalidGrantError` and the three §5.2 classes that already did, and both join the handler's OAuth branch. Call sites are unchanged: the single constructor argument is now the description. A bare throw still answers with the token alone, which is what `helpers/client-auth.ts` relies on — an authentication failure that describes itself is an enumeration oracle. **Log level and logger.** Every error was logged at `error` on `fastify.log`. That is the SERVER logger, which carries no `reqId` — the one line an operator needs to correlate with a caller's report was the one line that could not be correlated, defeating the request-id propagation of #128. And a mistyped `grant_type`, an expired token and a 404 all arrived at the severity of an unhandled exception: on an authentication server, an `error` stream whose volume an unauthenticated stranger controls, with real 500s buried in it. Now `request.log`, with 5xx at `error` and 4xx at `warn`, and the status decided once up front so the log line and the response can never disagree. **Validation details.** `error.validation` was forwarded verbatim. Under the Zod compiler that is the compiler's own issue objects: on `POST /oauth/token` each carries a `keyword`, a `schemaPath` (`#/grant_type/invalid_union`) and a `params` bag holding the discriminator name and full option list — the internal shape of the schema, handed to an unauthenticated caller. Projected to `path` and `message`, which is what a client needs to fix its request. The message keeps the constraint it quotes: the schemas are published in `openapi.json`, so that is documentation, not disclosure — the distinction is structural-vs-contractual, not verbose-vs-terse. **The invariants this PR asserts in prose.** `app.ts` now carries an order guard for the two registrations whose violation produces NO runtime signal: `rateLimitPlugin` before the routes AutoLoad (`onRoute` is not retroactive, so a later position silently drops every per-route limit), and `errorHandler` before `cors` (which registers `options('*')` of its own). Neither is observable from the assembled-app suite — the limiter is disabled there because it is Redis-backed and the suite runs without one, and cors answers preflights from a hook ahead of the route body. A source-order assertion is blunt and deliberate: for a silent invariant, a guard that reads the declaration beats no guard. Verified by moving the registration and watching it fail. `resolveClient`'s diagnostic `reason` reads `errorDescription` before `message`, or every distinct CIMD failure would collapse into the string `invalid_client` in the audit log. It reaches the audit trail and the operator's log, never a client. Not addressed here, and worth its own change: schema-validation failures on OAuth endpoints still render `VALIDATION_ERROR` rather than §5.2's `invalid_request`. That is a per-route contract change with an OpenAPI update behind it, not an error-handler fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TKAC2F5PayPstKTrF2NLp9
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
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.
Fixes #365
The app-level
setErrorHandlerwas registered after both@fastify/autoloadcalls, under a comment claiming the last position "catches all unhandled errors". It is backwards — a route does not resolve its error handler at request time. Fastify snapshots it while closing the route's enclosing plugin (context.errorHandler = this[kErrorHandler]in theafter()callback offastify/lib/route.js:376-379), and avvio runs queued plugins in registration order. The handler was unreachable from every route, and Fastify's built-in{statusCode, code, error, message}envelope answered every error.What was silently reverted
UniqueConstraintErroris genericised so the DB constraint name never reaches the wire; the built-in envelope echoeserror.message, which embeds it.WWW-Authenticate: Bearerchallenge a bearer-protected resource such as/oauth/userinfoMUST return on a 401.error_descriptionsanitisation for that challenge header.Changes
errorHandlerregistration ahead ofcors, not merely ahead of the two AutoLoads.@fastify/corsregisters anoptions('*')route of its own (index.js:79), and@fastify/rate-limitthrows its 429 from an earlieronRequesthook, so a rate-limited OPTIONS request would otherwise still render the built-in shape.rateLimitPluginMUST precede the routes AutoLoad — it applies per-route limits through anonRoutehook, andfastify.js:599routesonRoutepast thekChildrenrecursion in_addHook, so it is not retroactive. Moving it would silently drop the/auth/loginbrute-force ceiling with no error, warning or boot check. This carried no comment before.securityHeaders/requestIdpositions are not load-bearing — but via_addHook's recursion into existing child scopes (fastify.js:613-616), not viapreReady, which reads a snapshot. The old comments credited the wrong mechanism; the distinction is exactly what separates them fromonRoute.main.ts:59-60's validator/serializer compilers MUST precederegister(app). A child scope snapshots them at creation with no propagation, so moving them intostart()would return every route to ajv validation and reactivate thefast-urihost-confusion advisories (CVE-2026-13676, CVE-2026-16221) that the SECURITY INVARIANT comment says are inert because no request touches ajv. Same failure mode as this issue, with a CVE behind it. Code was already correct; the position requirement is now stated.Testing
The reason this shipped is in the issue:
apps/auth-server's route tests build stubbed Fastify objects, anderror-handler.test.tsexercises the handler in isolation where it works correctly. Nothing asserted it was reachable.apps/auth-server/src/app/error-handler-wiring.test.tsregisters the realappplugin and injects over HTTP:(a)POST /oauth/tokeninvalid body returns the custom validation shape, notFST_ERR_VALIDATION(b)unauthenticatedGET /oauth/userinfocarries an RFC 6750 §3WWW-Authenticate: Bearerchallenge(c)aUniqueConstraintErroris genericised — the constraint name never reaches the wire (F-01)(d)no autoloaded route answers with the built-in envelope (11-route sweep)Verified in both directions: with the fix,
74 files / 1397 testspass. With the registration moved back below the AutoLoads, (a), (b) and (d) fail with exactlycode: 'FST_ERR_VALIDATION'and a missingWWW-Authenticate.(c)is deliberately not an ordering guard and stays green under the reintroduced bug — any route added from outsideapp()inherits the final handler either way. The guard is (a)/(b)/(d) against the real autoloaded routes. This is documented in the file so it is not later misread as the F-01 regression guard.Reviewer notes
apps/auth-server. The rootvitest.config.tsgains oneserver.deps.inline: ['@fastify/autoload']entry — autoload resolves route modules by absolute path at runtime, which Vite cannot otherwise resolve. It is at the workspace root rather than project-local because the rootqauth:testtarget runs a barevitest runthat globs every project's specs, auth-server's included.nx run-many -t testpasses for all 18 projects.RATE_LIMIT_ENABLED=false— the limiter's store is Redis, and an unreachable Redis kills every request in itsonRequesthook with a uniform 500 that hides every response under test.DATABASE_URL/REDIS_URLpoint at127.0.0.1:1deliberately so the suite can never touch a live database.Follow-ups (not in this PR)
mcp-guard'ssendBearerChallengeJSDoc@exampletells host apps to callsetErrorHandlerwith no ordering warning — the same defect shipped as library documentation./docsroutes and the default 404 context live on the root instance inmain.ts, so they resolve the root's error handler and never reach this one. Not a regression; recorded so this fix is not over-read as "every response now goes througherror-handler.ts".🤖 Generated with Claude Code
https://claude.ai/code/session_01DWdz6Yd57TV48dTbVCej6j