Skip to content

server: inject CORS headers on Fastify-internal errors (#376)#421

Merged
melvincarvalho merged 3 commits into
gh-pagesfrom
issue-376-cors-on-fastify-errors
May 12, 2026
Merged

server: inject CORS headers on Fastify-internal errors (#376)#421
melvincarvalho merged 3 commits into
gh-pagesfrom
issue-376-cors-on-fastify-errors

Conversation

@melvincarvalho

@melvincarvalho melvincarvalho commented May 11, 2026

Copy link
Copy Markdown
Contributor

Closes #376.

Before

$ curl -i -H 'Origin: https://example.com' 'https://example.test/foo%g1'
HTTP/2 400
content-type: application/json
content-length: 116
# (no access-control-* headers)
{"error":"Bad Request","code":"FST_ERR_BAD_URL", ...}

Browser-side: surfaces as a generic CORS / network error instead of the real 400, masking the actual failure mode (malformed percent-encoding).

After

$ curl -i -H 'Origin: https://example.com' 'https://example.test/foo%g1'
HTTP/2 400
access-control-allow-origin: https://example.com
access-control-allow-methods: GET, HEAD, POST, PUT, DELETE, PATCH, OPTIONS
access-control-allow-headers: Accept, Authorization, Content-Type, DPoP, ...
access-control-expose-headers: Accept-Patch, Accept-Post, ...
access-control-allow-credentials: true
access-control-max-age: 86400
content-type: application/json; charset=utf-8
{"error":"Bad Request","code":"FST_ERR_BAD_URL","message":"'/foo%g1' is not a valid url component","statusCode":400}

Browser sees the real 400 and the actual FST_ERR_BAD_URL reason. The error field uses the HTTP status text ("Bad Request") via STATUS_CODES[statusCode], matching Fastify's default body shape so any pre-fix client that was parsing error keeps working.

For non-browser requests (no Origin header), CORS headers are still set with Access-Control-Allow-Origin: * — consistent with the rest of the server, where the global onRequest hook sets CORS unconditionally.

Root cause

FST_ERR_BAD_URL is thrown by Fastify's URL parser BEFORE the request enters the normal handler chain. Direct probe confirmed neither setErrorHandler nor onSend hooks fire for this code path. Fastify writes the 400 directly via res.writeHead() / res.end() in onBadUrl() (node_modules/fastify/fastify.js:752–771) — bypassing every JSS CORS-injecting handler.

Fix

Configure Fastify's frameworkErrors option in createServer. 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:

  • Always calls getCorsHeaders(request.headers.origin) and applies the full JSS CORS set — Allow-Origin mirrors the request Origin if present, falls back to * if absent
  • Sends a JSON body with error: STATUS_CODES[statusCode] (HTTP status text, e.g. "Bad Request"), code (the Fastify error code, e.g. "FST_ERR_BAD_URL"), message, and statusCode — same shape as Fastify's default error body

Test plan

Three new integration tests in test/error-handler.test.js against a live JSS server:

  • %g1 with Origin → 400, ACAO mirrors Origin, full CORS set, body error: "Bad Request"
  • %g1 without Origin → 400, ACAO=*, full CORS set, body error: "Bad Request"
  • Normal 404 path (no FST_ERR) → CORS still injected by the existing wildcard handler — confirms the new hook didn't displace existing behavior
  • Full suite: 777 → 780 pass

Refs

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 frameworkErrors handler in createServer() 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 thread src/server.js Outdated
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 thread src/server.js
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 thread test/error-handler.test.js Outdated
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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comment thread test/error-handler.test.js Outdated
Comment on lines +91 to +92
// to attach the CORS header set whenever the bad request carried
// an `Origin` header.
Comment thread src/server.js
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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@melvincarvalho
melvincarvalho merged commit 1ee75c7 into gh-pages May 12, 2026
4 checks passed
@melvincarvalho
melvincarvalho deleted the issue-376-cors-on-fastify-errors branch May 12, 2026 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CORS headers missing on Fastify-level error responses (FST_ERR_BAD_URL et al)

2 participants