server: inject CORS headers on Fastify-internal errors (#376)#421
Merged
Conversation
Surfaced during #374 review. When Fastify itself rejects a request before any handler runs (most commonly FST_ERR_BAD_URL for malformed percent-encoding like `%g1`, truncated `%E0%`, invalid UTF-8 sequences), the 400 response carries no `Access-Control-Allow-*` headers — browsers surface it as a generic CORS / network error instead of the real status, which is confusing for debugging and undermines the per-handler CORS work in #371 / #374. ## Root cause `FST_ERR_BAD_URL` is thrown by Fastify's URL parser BEFORE the request enters the normal handler chain. Confirmed by direct probe: neither `setErrorHandler` nor `onSend` hooks fire for this code path. Fastify writes the 400 response directly via `res.writeHead({...})` / `res.end(body)` in `onBadUrl()` (node_modules/fastify/fastify.js:752–771) when no `frameworkErrors` option is configured. ## Fix Configure Fastify's `frameworkErrors` option in `createServer`. This is the explicit hook Fastify provides for framework-level errors (FST_ERR_BAD_URL and FST_ERR_ASYNC_CONSTRAINT). When set, Fastify routes the error through this function instead of the direct-write path, giving us a Reply object to attach headers to. The handler: - Reads `request.headers.origin`; if present, sets the full JSS CORS header set via `getCorsHeaders(origin)` (ACAO mirrors the request Origin, plus Allow-Methods / Allow- Headers / Expose-Headers / Credentials / Max-Age). - Sends a JSON body matching the prior shape (`error`, `code`, `message`, `statusCode`) — minimal change for clients that were parsing the old format. ## Test plan Three new integration tests against a live JSS server: 1. `%g1` with Origin → 400, ACAO mirrors Origin, full CORS set 2. `%g1` without Origin → 400, JSON body still well-shaped (CORS not enforced when no Origin was sent) 3. Normal 404 path (no FST_ERR) → CORS still injected by the existing wildcard handler — confirms the new hook didn't displace existing behavior Test count: 777 → 780 in full suite.
There was a problem hiding this comment.
Pull request overview
This PR addresses missing CORS headers on Fastify “framework-level” error responses (notably FST_ERR_BAD_URL) that occur before JSS’s normal hook/handler chain runs, so browser clients can see the real HTTP error instead of a generic CORS/network failure.
Changes:
- Add a
frameworkErrorshandler increateServer()to inject JSS’s standard CORS headers and return a JSON error body for Fastify-internal errors. - Add integration tests to verify CORS headers are present for malformed percent-encoding requests and that normal 4xx paths still include CORS.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/server.js |
Adds frameworkErrors handling to attach CORS headers and send a JSON response for Fastify-internal errors. |
test/error-handler.test.js |
Adds integration coverage for FST_ERR_BAD_URL CORS behavior and a non-regression check for normal 404/401 paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+181
to
+189
| if (origin) { | ||
| // getCorsHeaders sets Access-Control-Allow-Origin to the | ||
| // request's Origin (or `*` when absent). Allow-Methods / | ||
| // Headers / Expose-Headers / Credentials / Max-Age are the | ||
| // standard JSS set from src/ldp/headers.js, so this error | ||
| // path advertises the same surface as a happy-path response. | ||
| const cors = getCorsHeaders(origin); | ||
| for (const [k, v] of Object.entries(cors)) reply.header(k, v); | ||
| } |
Comment on lines
+190
to
+196
| const statusCode = err.statusCode ?? 400; | ||
| reply.code(statusCode).type('application/json').send({ | ||
| error: err.name || 'Bad Request', | ||
| code: err.code, | ||
| message: err.message, | ||
| statusCode, | ||
| }); |
Comment on lines
+126
to
+131
| it('also returns CORS headers for the same bad URL even without an Origin (ACAO defaults to *)', async () => { | ||
| // Non-browser clients without an Origin still get a JSON 400 | ||
| // body — the CORS guard is "only set Allow-* when Origin was | ||
| // present" (browsers won't enforce CORS in this case anyway). | ||
| // Confirm the response is still well-shaped JSON and the status | ||
| // is correct. |
Three findings, all valid.
1. The `if (origin)` guard made the frameworkErrors path
inconsistent with the rest of the server, which sets CORS
on EVERY response via the global onRequest hook (with ACAO
defaulting to `*` when no Origin was sent). Removed the
guard — getCorsHeaders is now called unconditionally.
2. `error: err.name` produced "FastifyError" (unhelpful),
while Fastify's default error body uses the HTTP status
text ("Bad Request"). Switched to
`STATUS_CODES[statusCode] || 'Error'` from node:http —
400 → "Bad Request", 500 → "Internal Server Error", etc.
Matches Fastify's default body shape so any pre-fix client
that was parsing `error` keeps working.
3. The "no Origin" test name implied CORS was set with
ACAO=* but the assertions only checked the body. Now
asserts ACAO=*, ACAM contains GET, ACAH is set, and the
body's `error` field is "Bad Request". The "with Origin"
test also gained a body-shape assertion on the new
`error: "Bad Request"` field so any future regression
(like reverting to err.name) is caught.
Test count: same 780.
Comment on lines
+91
to
+92
| // to attach the CORS header set whenever the bad request carried | ||
| // an `Origin` header. |
Comment on lines
+188
to
+195
| reply.code(statusCode).type('application/json').send({ | ||
| // Use the HTTP status text (e.g. "Bad Request" for 400) | ||
| // rather than err.name (which for FastifyError is the | ||
| // unhelpful string "FastifyError"). Matches Fastify's | ||
| // default error-body shape that pre-fix clients were | ||
| // parsing. | ||
| error: STATUS_CODES[statusCode] || 'Error', | ||
| code: err.code, |
Two stale-docs findings, both real.
1. The test file's block comment said the fix attaches CORS
"whenever the bad request carried an Origin header" — that
was the pass-0 behavior. The pass-1 review (correctly)
removed that guard so CORS is now set unconditionally (with
ACAO=* when no Origin was sent). Updated the comment to
match the actual behavior.
2. The PR description's "After" example body still showed
`error: "FastifyError"` from my initial draft, before the
pass-1 fix that switched to `STATUS_CODES[statusCode]`
("Bad Request" for 400). Updated the PR body via REST
patch so the example matches the shipped response shape.
No code or test changes — pure documentation alignment.
Test count: same 780.
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.
Closes #376.
Before
Browser-side: surfaces as a generic CORS / network error instead of the real 400, masking the actual failure mode (malformed percent-encoding).
After
Browser sees the real 400 and the actual
FST_ERR_BAD_URLreason. Theerrorfield uses the HTTP status text ("Bad Request") viaSTATUS_CODES[statusCode], matching Fastify's default body shape so any pre-fix client that was parsingerrorkeeps working.For non-browser requests (no
Originheader), CORS headers are still set withAccess-Control-Allow-Origin: *— consistent with the rest of the server, where the globalonRequesthook sets CORS unconditionally.Root cause
FST_ERR_BAD_URLis thrown by Fastify's URL parser BEFORE the request enters the normal handler chain. Direct probe confirmed neithersetErrorHandlernoronSendhooks fire for this code path. Fastify writes the 400 directly viares.writeHead()/res.end()inonBadUrl()(node_modules/fastify/fastify.js:752–771) — bypassing every JSS CORS-injecting handler.Fix
Configure Fastify's
frameworkErrorsoption increateServer. This is the explicit hook Fastify provides for framework-level errors (FST_ERR_BAD_URL,FST_ERR_ASYNC_CONSTRAINT). When set, Fastify routes through this function instead of the direct-write path, giving us a Reply object to attach headers to.The handler:
getCorsHeaders(request.headers.origin)and applies the full JSS CORS set — Allow-Origin mirrors the request Origin if present, falls back to*if absenterror: STATUS_CODES[statusCode](HTTP status text, e.g. "Bad Request"),code(the Fastify error code, e.g. "FST_ERR_BAD_URL"),message, andstatusCode— same shape as Fastify's default error bodyTest plan
Three new integration tests in
test/error-handler.test.jsagainst a live JSS server:%g1with Origin → 400, ACAO mirrors Origin, full CORS set, bodyerror: "Bad Request"%g1without Origin → 400, ACAO=*, full CORS set, bodyerror: "Bad Request"FST_ERR) → CORS still injected by the existing wildcard handler — confirms the new hook didn't displace existing behaviorRefs
git-protocolheader in CORS preflight responses #371 — Git-Protocol CORS headerGIT_CORS_HEADERSconst + 4xx return paths (the PR that surfaced this issue)frameworkErrorsoption