fix: enforce API request limits
Add a single platform-owned request-body policy (hono/body-limit) with small, named limits for Better Auth and Core's grant-mutation endpoint, and a new required maxBodyBytes field on the executable module route contract so every module route makes a conscious no-body-or-bounded-body choice, capped at a platform ceiling. Every oversized request gets one sanitized 413 contract and never reaches business logic, authorization, or a handler. Also configure finite Node header/request/keep-alive timeouts, including the non-obvious connectionsCheckingInterval needed for them to take effect promptly, so a slow-header or slow-body connection cannot hold a socket indefinitely. Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
cafd513019
commit
d3c6bd73d3
@@ -0,0 +1,208 @@
|
||||
# 0017: Public request perimeter hardening — client address, signup gating, body limits, timeouts
|
||||
|
||||
- Status: Accepted
|
||||
- Date: 2026-07-26
|
||||
- Amends: [0011](0011-identity-http-transport-and-public-enrollment.md)
|
||||
- Extends: [0015](0015-static-module-http-routing-and-runtime-lifecycle.md)
|
||||
|
||||
## 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
|
||||
|
||||
`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. `core/src/identity/runtime.ts` wires this 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 the production
|
||||
default only — public registration remains the intended eventual product policy, and development
|
||||
or test composition may still enable it explicitly (`publicSignupEnabled: true`); 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](../../module-development/README.md#http-routes).
|
||||
|
||||
### Finite Node header/request timeouts, and a non-obvious operational detail
|
||||
|
||||
`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.
|
||||
|
||||
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).
|
||||
|
||||
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 except where a
|
||||
deployment explicitly opts in. 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.
|
||||
Reference in New Issue
Block a user