Production configuration accepted publicSignupEnabled: true and started a working Identity/API runtime, even though Ekklesia has no verified email, invitation requirement, CAPTCHA, or durable shared/edge limiter — Doctor merely reported "Public signup is explicitly ENABLED" as a pass. validatePublicSignupEnabled now takes the same IdentityEnvironment the rest of this config validates against and throws the stable PUBLIC_SIGNUP_UNSUPPORTED_IN_PRODUCTION whenever the parsed value is true in production. Parsing stays separate from the policy, so a malformed value still reports PUBLIC_SIGNUP_ENABLED_INVALID everywhere. Because the executable entry point validates configuration before startApi, such a deployment fails before any pool, Better Auth instance, module runtime, or listener exists, and Doctor reports NOT READY via its existing IdentityConfigError path. Development and test composition still enable it explicitly through the identical typed path. Co-Authored-By: Claude Opus 5 <[email protected]>
231 lines
17 KiB
Markdown
231 lines
17 KiB
Markdown
# 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, 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](../../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, 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.
|