fix: fall back to 500 when error status codes are out of range - #7024
fix: fall back to 500 when error status codes are out of range#7024ndycode wants to merge 3 commits into
Conversation
Errors thrown or rejected with a status/statusCode outside the 400-599 range were applied to the response without validation: values between 600 and 999 were written to the wire as-is, and larger values made res.writeHead throw ERR_HTTP_INVALID_STATUS_CODE from inside the error pipeline, escaping as an unhandled rejection and leaving the response hanging. With a tracing:fastify.request.handler subscriber present, setErrorStatusCode threw FST_ERR_BAD_STATUS_CODE before publishing the error event, crashing the async path and silently dropping the event on the sync path. Validate error-provided status codes with the same 400-599 band already used by the default error handler, falling back to the reply's current status code or 500, matching reply.code()'s documented 100-599 contract.
github-actions
Bot
added
the
documentation
| if (!reply[kReplyHasStatusCode] || reply.statusCode === 200) { | ||
| const statusCode = err && (err.statusCode || err.status) | ||
| reply.code(statusCode >= 400 ? statusCode : 500) | ||
| reply.code(isValidErrorStatusCode(statusCode) ? statusCode : 500) |
There was a problem hiding this comment.
Falling back to 500 is correct for the HTTP response, but an out-of-range status code is still a server-side programming error. We should surface it with a dedicated FST_ERR_BAD_ERROR_STATUS_CODE, preserving the original error as its cause.
The following is me thinking out loud, I recommend to wait for further review.
We create a new Fastify Internal Error:
FST_ERR_BAD_ERROR_STATUS_CODE: createError(
'FST_ERR_BAD_ERROR_STATUS_CODE',
'Invalid status code on Error: %s'
)We update the signature of setErrorStatusCode
function setErrorStatusCode (reply, err) {
if (!reply[kReplyHasStatusCode] || reply.statusCode === 200) {
const statusCode = err && (err.statusCode || err.status)
if (statusCode !== undefined && !isValidErrorStatusCode(statusCode)) {
reply.code(500)
// err is replaced by `FST_ERR_BAD_ERROR_STATUS_CODE` keeping the orginal
// error reference.
return new FST_ERR_BAD_ERROR_STATUS_CODE(statusCode, {
cause: err
})
}
reply.code(statusCode || 500)
}
return err
}We then can use it for diagnostics so monitoring tools can catch it.:
// Set status code before publishing so subscribers see the correct value
const withErrorWrongStatus = setErrorStatusCode(reply, err)
channels.error.publish(withErrorWrongStatus)See:
Line 193 in d266f83
The previous https://owasp.org/www-community/... URL now returns a 308 redirect to community.owasp.org, which the External Link Checker (redirects: error) reports as broken. Link to the canonical article directly.
The setErrorStatusCode path keeps a valid status code already set on the reply instead of forcing 500, so say that in the setErrorHandler and reply.send(error) notes, matching the Errors.md wording.
|
The broken OWASP link is fixed (commit 35a9655) and the full suite, including the link checker, was green on that head after approval. The latest head (6d308ee) only adjusts two wording lines in Server.md and Reply.md to state that an already-set valid status code is preserved rather than forced to 500, matching the implemented behavior. Could a maintainer re-approve the workflow runs on the updated head? Thanks! |
2 participants
Summary
Errors thrown or rejected with a
status/statusCodeoutside the 400-599 range were previously applied to the response without validation, in two places that bypassreply.code()'s 100-599 check (FST_ERR_BAD_STATUS_CODE, #2078 / #2169):setErrorHeaders(lib/error-handler.js) wroteerror.status/error.statusCodestraight tores.statusCodewhenever the value was>= 400setErrorStatusCode(lib/error-status.js) passed the unvalidated value toreply.code(), which throws from inside the error machineryObserved failures on
main@d266f833(all reproduced before the fix; the new tests fail red on the base):err.statusCode = 600HTTP/1.1 600on the wire with anFST_ERR_BAD_STATUS_CODEbody500err.statusCode = 69420RangeError [ERR_HTTP_INVALID_STATUS_CODE]escapes the error pipeline as an unhandled rejection; the response never completes500tracing:fastify.request.handlersubscriber (e.g. OTel-style tracing)FST_ERR_BAD_STATUS_CODEthrown fromwrap-thenable.jsbefore the error event is published: unhandled rejection on the async path, silently dropped tracing event on the sync path500, response completesreply.code(503)set, then throw withstatusCode = 600503overridden to600on the wire503preservedThe
wrap-thenable.js/handle-request.jscall sites were introduced by #6412, which fixed a different bug (the diagnostics channel reporting status 200) and did not account for out-of-range codes.Changes
lib/error-status.js: addisValidErrorStatusCode(400-599 band, same lower bound the default error handler already enforced);setErrorStatusCodefalls back to 500 for anything elselib/error-handler.js:setErrorHeadershonorserror.status/error.statusCodeonly within 400-599; a previously set valid status code is preserveddocs/Reference/Errors.md,docs/Reference/Server.md(setErrorHandler), and thereply.send(error)note indocs/Reference/Reply.mdtest/reply-error.test.js(600 / 69420 /'600', async + sync,err.status, user-set code preservation) andtest/diagnostics-channel/error-status.test.js(async + sync with a channel subscriber)Validation
d266f833with only the test files applied, all 10 new tests fail with the mechanisms above; all 90 pre-existing tests in those files passnpm run unit— 2350 tests, 0 failures (Windows 11, Node 24.18.0); the two affected files 100/100 across 6 runsnpm run lint,npm run lint:markdown,npm run test:types(tstyche, 1282 assertions),npx borp --coverage --check-coverage --lines 100— all pass locallynpm run test(unit + types) andnpm run benchmark --if-present(1.46M requests in 30s) both pass, including the OWASP link correction and thesetErrorHandler/reply.send(error)preservation wording.The affected code paths also exist on
5.x(#6412 was backported); happy to prepare a backport if wanted.This change was developed with AI assistance (opencode / GLM). The analysis, reproduction, red/green test evidence, and final diff were reviewed by the branch author before opening this pull request.
Checklist
npm run test && npm run benchmark --if-presentand the Code of conduct