fix: observe request timeout failures
The sanitized operational line promised for a header/request timeout was never actually emitted. Driving a real slow-header and slow-body connection through a real socket against the pinned Node 24 runtime shows the general server 'timeout' event never fires for headersTimeout/requestTimeout — that event belongs to server.setTimeout()'s idle-socket timer, which this server never arms. Both surface as 'clientError' carrying err.code === "ERR_HTTP_REQUEST_TIMEOUT". bindServer now observes 'clientError'. Because registering any such listener suppresses Node's own default response entirely, the handler reproduces that default byte-for-byte — 408 for a timeout, 431 for a header overflow, 400 otherwise — writing it only when the socket is still writable and no response has already begun, then destroying the socket exactly as Node would. Exactly one fixed sanitized line is logged per event, never the caught error's own message, which for a parse failure derives from attacker-controlled input. Proof moves to the real transport boundary: raw-socket scenarios assert the exact response bytes, exactly one status line, socket closure, handler non-invocation, and exactly one sanitized log for a slow-header connection, a slow-body connection, a malformed request line, and an oversized header block. The prior unit test, which manually emitted 'timeout', is removed as misleading. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5
parent
1161f5a076
commit
3d6c85b0f8
@@ -159,7 +159,7 @@ manifest): every existing module fixture, test, and the template (`templates/mod
|
||||
index.ts`) are updated in this same pass to declare it explicitly, per module-authoring
|
||||
convention documented in [Module development](../../module-development/README.md#http-routes).
|
||||
|
||||
### Finite Node header/request timeouts, and a non-obvious operational detail
|
||||
### Finite Node header/request timeouts, and two non-obvious operational details
|
||||
|
||||
`apps/api/src/server.ts` configures `@hono/node-server`'s `serverOptions` (passed straight through
|
||||
to `http.createServer`) with `headersTimeout: 20s`, `requestTimeout: 30s`, and
|
||||
@@ -177,11 +177,35 @@ configured timeout before Node ever closes it, on top of the configured value. `
|
||||
therefore also sets `connectionsCheckingIntervalMs: 5s` explicitly, so a violation is detected
|
||||
promptly after it actually elapses rather than after an unrelated, much longer default interval.
|
||||
|
||||
A sanitized `server.on("timeout", ...)` listener logs a fixed, generic operational message — never
|
||||
the remote address, headers, or request path — purely for observability; it does not participate
|
||||
in, and cannot suppress, Node's own `408`/close behavior for `headersTimeout`/`requestTimeout`
|
||||
(confirmed directly: only overriding the low-level `'clientError'` event, which this pass does
|
||||
not touch, would do that).
|
||||
The second detail concerns *observing* those timeouts, and corrects a claim this ADR originally
|
||||
made. Node's general server `'timeout'` event belongs to `server.setTimeout()`'s idle-socket timer,
|
||||
which this server never arms — it does **not** fire for `headersTimeout`/`requestTimeout`. Driving
|
||||
a real slow-header and a real slow-body connection through a real socket against the pinned runtime
|
||||
shows the only server-level event either produces is `'clientError'`, carrying
|
||||
`err.code === "ERR_HTTP_REQUEST_TIMEOUT"`. A `server.on("timeout", ...)` listener therefore never
|
||||
fires in production, and the sanitized operational line it was meant to emit was never actually
|
||||
emitted.
|
||||
|
||||
`apps/api/src/server.ts` consequently observes `'clientError'` instead. That carries an obligation:
|
||||
registering *any* `'clientError'` listener suppresses Node's own default response entirely
|
||||
(`_http_server.js` falls back only when `server.emit("clientError", ...)` returns `false`), so a
|
||||
listener that merely watched would silently turn every `408`/`400`/`431` into a bare connection
|
||||
reset. The handler therefore reproduces Node's default byte-for-byte — `408 Request Timeout` for
|
||||
`ERR_HTTP_REQUEST_TIMEOUT`, `431 Request Header Fields Too Large` for `HPE_HEADER_OVERFLOW`, `400
|
||||
Bad Request` otherwise — writing it only when the socket is still writable and no response has
|
||||
already begun for the in-flight request (mirroring Node's own `_httpMessage`/`headersSent` guard, so
|
||||
a handler that already responded before an unread body timed out never receives a second, corrupting
|
||||
status line), then destroys the socket exactly as Node would.
|
||||
|
||||
Exactly one fixed, sanitized line is logged per event — one message for a timeout, one for any other
|
||||
malformed or oversized request. Neither ever contains the request path, query, body, cookies,
|
||||
authorization value, client-address header, forwarding chain, raw peer address, stack, or the caught
|
||||
error's own message, since a parse failure's message is derived directly from attacker-controlled
|
||||
input. This is proven at the real transport boundary rather than by manual event emission: raw-socket
|
||||
scenarios in `apps/api/tests/api-http.integration.ts` drive a genuine slow-header connection, a
|
||||
genuine slow-body connection, a malformed request line, and an oversized header block, each
|
||||
asserting the exact response bytes, exactly one status line, socket closure, non-invocation of any
|
||||
handler, and exactly one sanitized log.
|
||||
|
||||
Bounded graceful-shutdown deadlines for module `stop()` hooks are a related but distinct concern,
|
||||
explicitly deferred to pass 4 and not touched here.
|
||||
|
||||
+30
-9
@@ -324,16 +324,37 @@ made explicit). Node's own documented behavior responds `408` and closes the con
|
||||
ever invoking a request listener when either timeout is exceeded, so a timed-out connection cannot
|
||||
invoke a handler or receive a second response by construction.
|
||||
|
||||
One non-obvious detail, found only by reproducing it directly against the pinned Node runtime:
|
||||
Node's `http.Server` only *checks* for a timeout violation when its periodic
|
||||
`connectionsCheckingInterval` fires, and that option's own default is 30 seconds — leaving it
|
||||
unset would silently let a violating connection sit for up to a further 30 seconds past its
|
||||
configured timeout. `connectionsCheckingIntervalMs` is therefore also set explicitly (5s). A
|
||||
sanitized, fixed operational log line (never the remote address, headers, or request path) is
|
||||
emitted on a server-level `'timeout'` event purely for observability; it does not participate in,
|
||||
and cannot suppress, Node's own `408`/close behavior. See
|
||||
Two non-obvious details, both found only by reproducing them directly against the pinned Node 24
|
||||
runtime through a real socket:
|
||||
|
||||
1. **Detection latency.** Node's `http.Server` only *checks* for a timeout violation when its
|
||||
periodic `connectionsCheckingInterval` fires, and that option's own default is 30 seconds —
|
||||
leaving it unset would silently let a violating connection sit for up to a further 30 seconds
|
||||
past its configured timeout. `connectionsCheckingIntervalMs` is therefore also set explicitly
|
||||
(5s).
|
||||
2. **The observable event is `'clientError'`, not `'timeout'`.** Node's general server `'timeout'`
|
||||
event belongs to `server.setTimeout()`'s idle-socket timer, which this server never arms; it
|
||||
does **not** fire for `headersTimeout`/`requestTimeout`. Those surface as `'clientError'` with
|
||||
`err.code === "ERR_HTTP_REQUEST_TIMEOUT"`. `apps/api/src/server.ts` therefore observes
|
||||
`'clientError'`, and — because registering any `'clientError'` listener *suppresses* Node's own
|
||||
default response entirely (`_http_server.js` only falls back when the emit returns `false`) —
|
||||
that handler reproduces Node's default byte-for-byte rather than merely watching: `408 Request
|
||||
Timeout` for a timeout, `431 Request Header Fields Too Large` for a header overflow, and `400
|
||||
Bad Request` otherwise, each followed by destroying the socket, and each written only when the
|
||||
socket is still writable and no response has already begun (so a handler that already responded
|
||||
before an unread body timed out never receives a second, corrupting status line).
|
||||
|
||||
Exactly one fixed, sanitized operational line is logged per transport-level client failure — one
|
||||
message for a timeout, one for any other malformed/oversized request. Neither ever contains the
|
||||
request path, query, body, cookies, authorization value, client-address header, forwarding chain,
|
||||
raw peer address, stack, or the caught error's own message (a parse failure's message is derived
|
||||
from attacker-controlled input). This is proven at the real transport boundary by raw-socket
|
||||
scenarios in `apps/api/tests/api-http.integration.ts` — a real slow-header connection, a real
|
||||
slow-body connection, a malformed request line, and an oversized header block — each asserting the
|
||||
exact response bytes, exactly one status line, socket closure, no handler invocation, and exactly
|
||||
one sanitized log. See
|
||||
[ADR 0017](../architecture/decisions/0017-public-request-perimeter-hardening.md) for the full
|
||||
account, including the standalone reproduction that found the `connectionsCheckingInterval` gap.
|
||||
account.
|
||||
|
||||
## Module discovery, permission composition, and module runtime startup
|
||||
|
||||
|
||||
Reference in New Issue
Block a user