Files
Ekklesia/docs/architecture/decisions/0017-public-request-perimeter-hardening.md
ekklesiaandClaude Opus 5 3d6c85b0f8 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]>
2026-07-26 15:59:33 -05:00

19 KiB
Raw Permalink Blame History

0017: Public request perimeter hardening — client address, signup gating, body limits, timeouts

  • Status: Accepted
  • Date: 2026-07-26
  • Amends: 0011
  • Extends: 0015

Context

An independent security audit against main (Phase 5C checkpoint, commit 5e093300) found that apps/api's public HTTP perimeter had four related gaps before any Surface work or additional module-platform capability could responsibly proceed:

  1. Better Auth's rate limiter resolved client identity from a caller-controlled X-Forwarded-For header by default (ADR 0011 explicitly left trusted-proxy configuration unset, but the pinned release's own fallback still trusts a single-value header with no proxy policy at all). A caller could rotate the header value on every request to make each attempt look like a new, unlimited client, or collapse unrelated callers into one bucket when no header was present.
  2. Public registration (ADR 0011) had no verified email, CAPTCHA, invitation, or durable distributed limiter, and — more immediately — no way to disable it at all. Every deployment was permanently open to unlimited account creation.
  3. Neither Better Auth's endpoints, Core's grant-mutation endpoint, nor any module route enforced a request-body size limit. core/src/grants-routes.ts's c.req.json() and any module handler reading a body could be driven with an arbitrarily large or indefinitely streamed payload.
  4. The Node HTTP server had no configured headersTimeout/requestTimeout, leaving Node's own generous defaults (60s/300s) as the only bound on a slow-header or slow-body connection.

This ADR records the pass-1 fix for all four, as one bounded slice (Phase 5D pass 1). Passes 2–5 (identity/database transport hardening, authorization integrity, module/runtime/migration correctness, and deployment/supply-chain closure) remain separate, later work.

Decision

One platform-owned client-address resolution boundary

core/src/identity/client-address.ts defines a framework-agnostic policy — ClientAddressConfig ({mode: "direct"} or {mode: "trusted-proxy", header, trustedProxies}), createClientAddressConfig (strict validation, safe default), and createClientAddressResolver (the actual per-request resolution algorithm) — built only on node:net and the standard Headers type, so it stays importable by Core without pulling Hono into it. It never reads a request header for "direct" mode: it trusts only the value it is given as the actual immediate socket peer address. For "trusted-proxy" mode, a forwarded header is consulted only when the actual immediate peer matches a configured trusted-proxy address/CIDR; otherwise the peer address itself is used, exactly as if no header existed. A forwarded chain (comma-separated) is walked right to left: trailing hops matching a trusted proxy are skipped, and the first non-matching hop from the right is the resolved address. A missing, empty, malformed, or entirely-trusted-hops chain from an otherwise-trusted peer fails closed (PEER_ADDRESS_UNAVAILABLE / FORWARDED_HEADER_MISSING / FORWARDED_HEADER_MALFORMED / FORWARDED_CHAIN_EXHAUSTED) rather than guessing or falling back to a shared bucket.

Node's own runtime cannot be told about the real socket peer from inside Hono — only the raw IncomingMessage (@hono/node-server's HttpBindings.incoming) has it. apps/api/src/ client-address.ts therefore adds exactly one Hono-adjacent piece, wrapFetchWithClientAddress, which wraps the fetch callback passed to @hono/node-server's serve() in apps/api/src/ server.ts: it resolves the trusted address from env.incoming.socket.remoteAddress before the wrapped callback (and therefore every route, including Better Auth's own handler) ever sees the request, then unconditionally overwrites — never appends to — any externally supplied copy of a fixed internal header (TRUSTED_CLIENT_ADDRESS_HEADER = "x-ekklesia-client-address", core/src/identity/runtime.ts) with the resolved value. A resolution failure returns a sanitized 503 {"code":"CLIENT_ADDRESS_UNRESOLVED"} directly from this wrapper — the wrapped callback, Better Auth, Core, and every module handler are all unreachable in that case.

Better Auth is configured with advanced.ipAddress.ipAddressHeaders: [TRUSTED_CLIENT_ADDRESS_HEADER] and no trustedProxies of its own: the pinned release's own trustedProxies option only inspects header content for proxy-shaped addresses and cannot verify who actually sent the request (its own doc comment says as much), so the real trust decision is made exactly once, at the transport boundary, using the genuine socket peer — never asked of Better Auth a second time. Core never reinterprets an untrusted proxy header itself; it only ever receives the one already-resolved, already-sanitized header apps/api produced.

This closes the original reproduction directly: rotating a spoofed X-Forwarded-For on every request no longer changes the rate-limit bucket in direct mode (the default), because the header is never consulted at all. The in-memory rate-limit store remains per-process (core/src/identity/ runtime.ts, unchanged from ADR 0011) — this pass does not add distributed storage; a genuinely multi-instance deployment still needs a shared or edge limiter, reserved for pass 5.

Public signup is disabled by default, and cannot be enabled in production at all

IdentityConfig.publicSignupEnabled (core/src/identity/config.ts) is a strict boolean: undefined input means false (disabled); only an explicit boolean or the exact strings "true"/"false" are accepted, so an absent, misspelled, or loosely parsed environment variable can never enable it.

Production refuses enablement outright. validatePublicSignupEnabled takes the same IdentityEnvironment the rest of this config already validates against, and throws the stable PUBLIC_SIGNUP_UNSUPPORTED_IN_PRODUCTION error whenever the parsed value is true in production. The durable signup-abuse prerequisites a public registration endpoint requires — verified email or an invitation requirement, plus durable shared/edge throttling rather than this release's per-process in-memory limiter — do not exist in this repository, so there is no production configuration in which the flag could be honored safely; permitting a deliberate opt-in would be offering a switch whose safe position does not exist yet. Parsing is kept separate from this policy so a malformed value still reports PUBLIC_SIGNUP_ENABLED_INVALID in every environment, and an operator who typo'd the flag is never told the misleading "unsupported in production" story instead. Because apps/api's executable entry point calls createApiRuntimeConfig before startApi, such a deployment fails before any PostgreSQL pool, Better Auth instance, module runtime, or listener exists. pnpm doctor surfaces the same state as a blocking NOT READY failure (via its existing IdentityConfigError path), never a warning. Development and test composition may still enable it explicitly through the identical typed path — that is how this repository's own integration suite creates accounts over HTTP. When the prerequisites are actually implemented and proven, this one function is the single place that changes.

core/src/identity/runtime.ts wires the validated flag directly to Better Auth's own emailAndPassword.disableSignUp flag: when disabled, POST /sign-up/email rejects every call with a fixed 400 {"code":"EMAIL_PASSWORD_SIGN_UP_DISABLED"} before Better Auth ever looks up the email, so a disabled deployment cannot leak whether an address is already registered, and apps/api/src/app.ts now forces Cache-Control: no-store on every /api/auth/* response (previously unset on the success path). Sign-in, session resolution, and sign-out are entirely unaffected — this is Better Auth's own native, already-audited gate, not a second, competing platform-owned check, since the pinned release's own mechanism already satisfies every required property (no email-existence leak, no business logic invoked, a stable code). Since email/password is the only enabled provider (ADR 0011), there is no second signup entry point to separately guard.

This amends ADR 0011's framing ("Anyone may create an Ekklesia account") for production entirely — public registration remains the intended eventual product policy, but is currently reachable only in development/test composition (publicSignupEnabled: true with environment: "development"); ADR 0011's architectural description of what a registered account is (an authentication identity, not a membership record) is otherwise unchanged and still authoritative. This pass adds no email verification, CAPTCHA, invitation, or durable abuse control — those remain explicitly deferred prerequisites for a real future public-enrollment mode.

One platform-owned request-body policy, and a module route's own conscious body contract

apps/api/src/body-limits.ts centralizes three named, typed values built on Hono's pinned hono/body-limit middleware (verified directly against its source: it rejects an oversized declared Content-Length synchronously from the header alone, and rejects an oversized declared-length-free body by reading and counting the stream incrementally, aborting as soon as the running total exceeds the limit — never buffering the whole body first):

  • AUTH_REQUEST_MAX_BODY_BYTES = 8 KiB, applied to every /api/auth/* request before Better Auth's handler ever runs.
  • CORE_MUTATION_MAX_BODY_BYTES = 16 KiB, applied to POST /api/v1/grants and POST /api/v1/grants/:grantId/revoke after requireTrustedOrigin (so a rejected cross-origin mutation's body is never read) but before permission evaluation.
  • MODULE_ROUTE_MAX_BODY_BYTES_CEILING = 1 MiB (re-exported from @ekklesia/contracts), the hard ceiling no module route may exceed and the platform's overall body-size ceiling.

Every hono/body-limit instance shares one sanitized public contract, requestBodyTooLargeResponse — a stable 413 {"code":"REQUEST_BODY_TOO_LARGE"} with Cache-Control: no-store — so auth, Core, and module rejections are indistinguishable in shape.

A module route's body contract becomes a fifth, required field on the existing route-definition contract (packages/contracts/src/module-http.ts): maxBodyBytes: number, where 0 means the route accepts no body at all (a non-empty declared or streamed body is rejected) and any other value is the exact byte maximum, validated as a non-negative, finite integer no greater than MODULE_ROUTE_MAX_BODY_BYTES_CEILING (MODULE_ROUTE_MAX_BODY_BYTES_INVALID otherwise). This is a security contract, not a manifest capability: it does not touch manifestVersion or the static manifest at all, only the executable http.route(...) call every module already makes during register. apps/api/src/module-http-routes.ts applies platformBodyLimit(route.maxBodyBytes) to every mounted route, positioned identically to the Core mutation routes — after requireTrustedOrigin for a state-changing method, before requirePermission — so an oversized request never reaches authorization work, a module handler, or module database behavior, and a rejected cross-origin mutation's body is never read for size at all.

This is a deliberate, minimal public-contract change to an existing executable contract (not the manifest): every existing module fixture, test, and the template (templates/module/src/server/ index.ts) are updated in this same pass to declare it explicitly, per module-authoring convention documented in Module development.

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 keepAliveTimeout: 5s (Node's own default, made explicit and typed). Node's own documented behavior for requestTimeout/headersTimeout — verified directly against the pinned Node 24 runtime with a standalone reproduction — is to respond 408 and close the connection without ever invoking a request listener, so a timed-out connection cannot invoke a handler or receive a second response by construction; this pass does not reimplement that behavior.

That same reproduction surfaced a non-obvious operational requirement: Node's http.Server only checks for a headersTimeout/requestTimeout 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 before Node ever closes it, on top of the configured value. NodeServerTimeouts 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.

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.

Alternatives

  • Setting Better Auth's advanced.ipAddress.trustedProxies alone was rejected: that option only inspects the forwarded header's own content for proxy-shaped addresses and can never verify who actually connected, so any direct caller could still choose its own bucket by crafting a header that merely looks trusted-proxy-shaped.
  • Resolving the client address inside Core's identity runtime directly from headers was rejected: Core has no access to the real socket peer (it only ever sees a Fetch Request), and duplicating proxy-trust logic inside Core would create a second, divergent implementation of exactly the policy this ADR centralizes once, at the transport boundary.
  • A separate CAPTCHA, email-verification, or invitation system to gate signup in this pass was rejected as out of scope: pass 1 closes the default-open exposure; the deeper abuse-control prerequisites for a real public-enrollment mode are reserved, honestly, for later work.
  • Adding maxBodyBytes to the static module manifest instead of the executable route definition was rejected, matching ADR 0015's own reasoning for keeping routes executable data: a manifest field can't be enforced without also trusting the manifest as a security boundary the platform doesn't otherwise treat that way, and it would create a second, potentially-drifting place to declare the same route.
  • A single blanket global body-limit middleware applied before per-route logic was rejected in favor of positioning platformBodyLimit individually within each route's own middleware chain: a single global instance mounted ahead of requireTrustedOrigin would read (or begin reading) a rejected cross-origin mutation's body before the origin check ever ran, which this ADR's ordering requirement forbids.
  • Leaving Node's connectionsCheckingInterval at its default was rejected once the reproduction showed it would silently widen both configured timeouts by up to 30 additional seconds — a security-relevant gap, not merely a latency one.

Consequences

Direct mode (the default) trusts only the real socket peer for every request; explicit trusted-proxy mode requires a header name and a concrete, non-empty trusted-proxy list, validated the same way in core/src/identity/client-address.ts and surfaced by Doctor (identity.client-address), which distinguishes safe direct mode, an explicitly configured proxy mode, and an invalid configuration. Public signup is closed by default everywhere, and production cannot open it at all until the durable abuse-control prerequisites exist — only development and test composition may enable it. Every body-consuming public route — auth, Core, and every module route — now has an explicit, enforced byte ceiling with one sanitized 413 contract. The Node server itself now bounds a slow-header or slow-body connection instead of relying on Node's generous multi-minute defaults. None of this constitutes distributed rate limiting, a durable signup-abuse control, or a claim that the origin is unreachable except through a configured proxy — the deployment's own reverse proxy and firewall remain responsible for that, and durable/shared throttling for a multi-instance or public-signup deployment remains explicitly reserved for pass 5.