From d3c6bd73d31aad8bb2aae56c9ae39a05a9f11cd1 Mon Sep 17 00:00:00 2001 From: ekklesia Date: Sun, 26 Jul 2026 12:29:24 -0500 Subject: [PATCH] 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 --- README.md | 30 +- apps/api/src/app.ts | 4 +- apps/api/src/body-limits.ts | 48 ++++ apps/api/src/grants-routes.ts | 90 +++--- apps/api/src/module-http-routes.ts | 16 +- apps/api/src/server.ts | 81 +++++- apps/api/tests/api-http.integration.ts | 134 ++++++++- apps/api/tests/app.test.ts | 111 ++++++++ apps/api/tests/grants-routes.test.ts | 134 +++++++++ apps/api/tests/lifecycle.test.ts | 113 +++++++- apps/api/tests/module-http-routes.test.ts | 266 +++++++++++++++++- core/src/modules/runtime.ts | 2 + .../db-facade/server/index.js | 7 + core/tests/module-runtime.test.ts | 61 ++-- ...0017-public-request-perimeter-hardening.md | 208 ++++++++++++++ docs/architecture/decisions/README.md | 1 + docs/architecture/platform-architecture.md | 17 +- docs/development/api.md | 49 ++++ docs/module-development/README.md | 25 +- packages/contracts/src/module-http.ts | 67 ++++- packages/contracts/tests/module-http.test.ts | 41 +++ .../tests/define-module-runtime.test.ts | 1 + templates/module/src/server/index.ts | 6 + templates/module/tests/manifest.test.ts | 3 + 24 files changed, 1419 insertions(+), 96 deletions(-) create mode 100644 apps/api/src/body-limits.ts create mode 100644 docs/architecture/decisions/0017-public-request-perimeter-hardening.md diff --git a/README.md b/README.md index 9780bc2..38f9544 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,10 @@ surfaces. Phase 3 is complete, and Phase 4A (Core identity foundation), Phase 4B (identity HTTP transport), Phase 4C (Core authorization), Phase 5A (module authorization integration), Phase 5B (module HTTP -transport), and Phase 5C (module database access) have landed. The repository includes +transport), and Phase 5C (module database access) have landed. Phase 5D (public request perimeter +hardening against an independent security audit) is in progress; pass 1 of 5 has landed — see +below. **Phase 5D is not complete, and Phase 6 (Surface work) remains blocked until every audit +finding is resolved and a final re-audit passes.** The repository includes public module contracts, independently versioned capabilities, static module manifests, deterministic automatic discovery, compiled server loading, a staged capability registry, dependency-ordered lifecycle with rollback, module-owned Drizzle table contracts, deterministic @@ -32,9 +35,10 @@ in `@ekklesia/contracts`, and an explicit Better Auth construction boundary in C Phase 4B adds identity HTTP transport: a minimal Hono-based `apps/api` composition root (`pnpm api:start`) mounts Better Auth at `/api/auth/*` and exposes `GET /api/v1/principal` behind Core's own Fetch-compatible identity runtime — Core still does not depend on Hono, and modules -still cannot reach any of this. Public email/password registration is intentional; every account -receives identical lowest-privilege baseline status and signup order never grants elevation. See -[API](docs/development/api.md) and +still cannot reach any of this. Public email/password registration is the intended eventual +product policy — every account receives identical lowest-privilege baseline status and signup +order never grants elevation — but is now **disabled by default** and requires an explicit opt-in +(Phase 5D pass 1, below). See [API](docs/development/api.md) and [ADR 0011](docs/architecture/decisions/0011-identity-http-transport-and-public-enrollment.md). **No email verification, password recovery, or Dashboard exists yet** — that, along with production approval-provider composition, jobs, physical storage provisioning, surfaces, and @@ -91,6 +95,24 @@ startup. See [API](docs/development/api.md#module-database-access), [Module development](docs/module-development/README.md#database-access), and [ADR 0016](docs/architecture/decisions/0016-module-database-access.md). +Phase 5D pass 1 of 5 hardens the public request perimeter against an independent security audit +of the Phase 5C checkpoint, without adding a Surface, a product module, or any new capability. +Better Auth's authentication rate limiter now resolves client identity from a server-owned +client-address boundary (`core/src/identity/client-address.ts`, wired in at the Node/API transport +boundary) — direct mode trusts only the real socket peer by default, and an explicit trusted-proxy +mode is required before any forwarded header is ever consulted, closing a rotating-header +rate-limit bypass. Public sign-up is now **disabled by default** and requires an explicit typed +opt-in (`EKKLESIA_PUBLIC_SIGNUP_ENABLED=true`). Every public body-consuming route — Better Auth, +Core's grant-mutation endpoint, and every module route (via a new, required `maxBodyBytes` field on +the executable route contract) — now has an explicit, platform-enforced byte ceiling with one +sanitized `413` contract. The Node HTTP server now has finite header/request timeouts. See +[API](docs/development/api.md), [Module development](docs/module-development/README.md#http-routes), +and [ADR 0017](docs/architecture/decisions/0017-public-request-perimeter-hardening.md). **This pass +does not add distributed rate limiting, durable public-signup abuse control, or any deployment/TLS +guarantee, and it does not complete Phase 5D** — passes 2–5 (identity/database transport hardening, +authorization integrity, module/runtime/migration correctness, and deployment/supply-chain closure +plus a final independent re-audit) remain outstanding before Phase 6 (Surface work) may begin. + ## Quick start Prerequisites: Node.js 24 LTS, pnpm 11.12, and Docker with Compose. diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 4fe5787..bf24cf6 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -12,6 +12,7 @@ import { type EkklesiaIdentityRuntime, IdentityRuntimeError } from "@ekklesia/co import { Hono } from "hono"; import type { Context, MiddlewareHandler } from "hono"; import { cors } from "hono/cors"; +import { AUTH_REQUEST_MAX_BODY_BYTES, platformBodyLimit } from "./body-limits.js"; import { createGrantsRoutes } from "./grants-routes.js"; import { canonicalizeVaryHeader, mountModuleHttpRoutes } from "./module-http-routes.js"; @@ -117,7 +118,8 @@ export function createApiApp(options: CreateApiAppOptions): Hono { app.use("/api/auth/*", corsMiddleware); app.use("/api/v1/*", corsMiddleware); - app.on(["POST", "GET"], "/api/auth/*", async (c) => { + const authBodyLimit = platformBodyLimit(AUTH_REQUEST_MAX_BODY_BYTES); + app.on(["POST", "GET"], "/api/auth/*", authBodyLimit, async (c) => { try { const response = await runtime.handleAuthRequest(c.req.raw); // Every Better Auth response — including a session cookie — must never be cached by an diff --git a/apps/api/src/body-limits.ts b/apps/api/src/body-limits.ts new file mode 100644 index 0000000..12e4ea9 --- /dev/null +++ b/apps/api/src/body-limits.ts @@ -0,0 +1,48 @@ +import { MODULE_ROUTE_MAX_BODY_BYTES_CEILING } from "@ekklesia/contracts"; +import type { Context } from "hono"; +import { bodyLimit } from "hono/body-limit"; + +/** + * The platform's one request-body policy, centralized here rather than scattered as magic numbers + * across every route file. Every limit below is deliberately small relative to the actual payload + * shapes it guards: + * + * - `AUTH_REQUEST_MAX_BODY_BYTES` (8 KiB): every enabled Better Auth route + * (`/api/auth/sign-up/email`, `/sign-in/email`, `/sign-out`, ...) accepts only an email, a + * password (bounded to 128 characters — `core/src/identity/runtime.ts`), and an optional short + * display name; the pinned release adds no other enabled provider or field (ADR 0011). A real + * payload is well under 200 bytes; 8 KiB leaves generous headroom for a verbose email address or + * name while remaining two orders of magnitude below the module ceiling. + * - `CORE_MUTATION_MAX_BODY_BYTES` (16 KiB): the one Core JSON mutation body, explicit-grant + * creation (`POST /api/v1/grants`), accepts `subjectUserId` (<=128 chars), `permission` (a bounded + * identifier), an optional `scope` (<=256 chars), and an optional ISO `expiresAt` string — every + * field individually bounded well under 1 KiB combined; 16 KiB leaves headroom without inviting + * abuse. + * - `MODULE_ROUTE_MAX_BODY_BYTES_CEILING` (1 MiB, re-exported from `@ekklesia/contracts` — the same + * constant a module's own route declaration is validated against at registration) is the hard + * ceiling no module route may exceed and doubles as the platform's overall request-body ceiling: + * nothing in this platform ever declares, or enforces, a larger single request body than this. + * Large media transfer is explicitly out of scope for ordinary module HTTP bodies — it belongs to + * a future dedicated storage design, not to widening this ceiling. + */ +export const AUTH_REQUEST_MAX_BODY_BYTES = 8 * 1024; +export const CORE_MUTATION_MAX_BODY_BYTES = 16 * 1024; +export { MODULE_ROUTE_MAX_BODY_BYTES_CEILING }; + +/** The one stable, sanitized public contract for every request-body-size rejection across the + * platform — auth, Core, and module routes alike. Never leaks the configured limit, the offending + * size, a declared Content-Length, or any body content. */ +export function requestBodyTooLargeResponse(c: Context): Response { + c.header("Cache-Control", "no-store"); + return c.json({ code: "REQUEST_BODY_TOO_LARGE" }, 413); +} + +/** A single shared `hono/body-limit` factory so every call site uses the identical sanitized 413 + * contract rather than redefining `onError` per route. Covers both a declared oversized + * `Content-Length` and an oversized chunked/streamed body with no declared length — see + * `hono/body-limit`'s pinned implementation, which checks `Content-Length` synchronously before + * ever reading the stream, and otherwise reads and counts the stream incrementally, rejecting as + * soon as the running total exceeds `maxSize` rather than buffering the whole body first. */ +export function platformBodyLimit(maxSize: number) { + return bodyLimit({ maxSize, onError: requestBodyTooLargeResponse }); +} diff --git a/apps/api/src/grants-routes.ts b/apps/api/src/grants-routes.ts index 2f3d0c0..45ee0aa 100644 --- a/apps/api/src/grants-routes.ts +++ b/apps/api/src/grants-routes.ts @@ -21,6 +21,7 @@ import { requirePermission, requireTrustedOrigin, } from "./authorization-middleware.js"; +import { CORE_MUTATION_MAX_BODY_BYTES, platformBodyLimit } from "./body-limits.js"; export interface CreateGrantsRoutesOptions extends AuthorizationMiddlewareOptions { readonly store: AuthorizationStore; @@ -129,6 +130,10 @@ export function createGrantsRoutes(options: CreateGrantsRoutesOptions): Hono { const routes = new Hono(); const requireManagePermission = requirePermission(options, CORE_AUTHORIZATION_GRANTS_MANAGE); const requireOrigin = requireTrustedOrigin(options.trustedOrigins); + // Origin is checked first (a rejected cross-origin mutation must never have its body read), then + // the body-size limit runs before permission evaluation, so an oversized mutation never reaches + // authorization work or the store. + const mutationBodyLimit = platformBodyLimit(CORE_MUTATION_MAX_BODY_BYTES); routes.get("/api/v1/grants", requireManagePermission, async (c) => { c.header("Cache-Control", "no-store"); @@ -146,52 +151,59 @@ export function createGrantsRoutes(options: CreateGrantsRoutesOptions): Hono { } }); - routes.post("/api/v1/grants", requireOrigin, requireManagePermission, async (c) => { - c.header("Cache-Control", "no-store"); - const principal = getAuthorizedPrincipal(c); - if (!isAuthenticatedPrincipal(principal)) { - return c.json({ code: "AUTHORIZATION_DENIED" }, 403); - } + routes.post( + "/api/v1/grants", + requireOrigin, + mutationBodyLimit, + requireManagePermission, + async (c) => { + c.header("Cache-Control", "no-store"); + const principal = getAuthorizedPrincipal(c); + if (!isAuthenticatedPrincipal(principal)) { + return c.json({ code: "AUTHORIZATION_DENIED" }, 403); + } - let rawBody: unknown; - try { - rawBody = await c.req.json(); - } catch { - return c.json({ code: "INVALID_REQUEST" }, 400); - } - const parsed = parseCreateGrantBody(rawBody); - if (!parsed) { - return c.json({ code: "INVALID_REQUEST" }, 400); - } + let rawBody: unknown; + try { + rawBody = await c.req.json(); + } catch { + return c.json({ code: "INVALID_REQUEST" }, 400); + } + const parsed = parseCreateGrantBody(rawBody); + if (!parsed) { + return c.json({ code: "INVALID_REQUEST" }, 400); + } - // A well-formed PermissionId is not necessarily a *registered* one. Only the trusted registry - // (never the request) decides what may be granted — the same registry `requireManagePermission` - // already consults for the caller's own `core.authorization.grants.manage` check — so a caller - // cannot pre-create a dormant grant for an invented identifier that later becomes effective the - // moment someone registers it. Sanitized identically to any other denial: this must never reveal - // whether the identifier was malformed (400, above) or simply unregistered (403, here). - if (!options.registry.has(parsed.permission)) { - return c.json({ code: "AUTHORIZATION_DENIED" }, 403); - } + // A well-formed PermissionId is not necessarily a *registered* one. Only the trusted registry + // (never the request) decides what may be granted — the same registry `requireManagePermission` + // already consults for the caller's own `core.authorization.grants.manage` check — so a caller + // cannot pre-create a dormant grant for an invented identifier that later becomes effective the + // moment someone registers it. Sanitized identically to any other denial: this must never reveal + // whether the identifier was malformed (400, above) or simply unregistered (403, here). + if (!options.registry.has(parsed.permission)) { + return c.json({ code: "AUTHORIZATION_DENIED" }, 403); + } - try { - const grant = await options.store.createGrant({ - actor: createUserActor(principal.userId), - ...(parsed.expiresAt === undefined ? {} : { expiresAt: parsed.expiresAt }), - permission: parsed.permission, - ...(parsed.scope === undefined ? {} : { scope: parsed.scope }), - sourceKind: "explicit", - userId: parsed.subjectUserId, - }); - return c.json({ grant: serializeGrant(grant) }, 201); - } catch (error) { - return sanitizedStoreErrorResponse(c, error); - } - }); + try { + const grant = await options.store.createGrant({ + actor: createUserActor(principal.userId), + ...(parsed.expiresAt === undefined ? {} : { expiresAt: parsed.expiresAt }), + permission: parsed.permission, + ...(parsed.scope === undefined ? {} : { scope: parsed.scope }), + sourceKind: "explicit", + userId: parsed.subjectUserId, + }); + return c.json({ grant: serializeGrant(grant) }, 201); + } catch (error) { + return sanitizedStoreErrorResponse(c, error); + } + }, + ); routes.post( "/api/v1/grants/:grantId/revoke", requireOrigin, + mutationBodyLimit, requireManagePermission, async (c) => { c.header("Cache-Control", "no-store"); diff --git a/apps/api/src/module-http-routes.ts b/apps/api/src/module-http-routes.ts index 52c373d..c8ce3f9 100644 --- a/apps/api/src/module-http-routes.ts +++ b/apps/api/src/module-http-routes.ts @@ -11,6 +11,7 @@ import { requirePermission, requireTrustedOrigin, } from "./authorization-middleware.js"; +import { platformBodyLimit } from "./body-limits.js"; const MODULE_ROUTE_PREFIX = "/api/v1/modules"; const STATE_CHANGING_METHODS: ReadonlySet = new Set(["DELETE", "PATCH", "POST", "PUT"]); @@ -187,6 +188,10 @@ export function mountModuleHttpRoutes( } const requireRoutePermission = requirePermission(middlewareOptions, route.permission); + // The route's own conscious body contract (`route.maxBodyBytes` — 0 means no body accepted at + // all) is enforced before permission evaluation ever runs, so an oversized request never + // reaches authorization work, let alone the handler or the module database. + const routeBodyLimit = platformBodyLimit(route.maxBodyBytes); const path = moduleRoutePath(route.moduleId, route.path); const finalHandler = async (c: Context): Promise => { const context: ModuleRouteHandlerContext = Object.freeze({ @@ -212,9 +217,16 @@ export function mountModuleHttpRoutes( // array — each branch below is therefore a distinct, fixed-arity call rather than one call // spreading a variable-length handlers array. if (STATE_CHANGING_METHODS.has(route.method)) { - app.on(route.method, path, requireOrigin, requireRoutePermission, finalHandler); + app.on( + route.method, + path, + requireOrigin, + routeBodyLimit, + requireRoutePermission, + finalHandler, + ); } else { - app.on(route.method, path, requireRoutePermission, finalHandler); + app.on(route.method, path, routeBodyLimit, requireRoutePermission, finalHandler); } } } diff --git a/apps/api/src/server.ts b/apps/api/src/server.ts index 095cedb..3141ab6 100644 --- a/apps/api/src/server.ts +++ b/apps/api/src/server.ts @@ -27,6 +27,47 @@ import { prepareStartupModuleInventory } from "./platform-permissions.js"; const connectionTimeoutMs = 10_000; const operationTimeoutMs = 10_000; +export interface NodeServerTimeouts { + readonly headersTimeoutMs: number; + readonly requestTimeoutMs: number; + readonly keepAliveTimeoutMs: number; + /** + * How often Node re-scans open connections for a header/request timeout violation + * (`http.Server`'s `connectionsCheckingInterval`). This is not merely a performance knob: Node + * enforces `headersTimeout`/`requestTimeout` only when this periodic check runs, and its own + * default (30s) is verified (`node --version` 24.15.0, direct `http.createServer` reproduction) + * to let a violating connection sit for up to a full additional interval past its actual + * timeout before Node ever closes it — so leaving this at Node's default would silently widen + * both timeouts above by up to 30s in the worst case. Kept small relative to `headersTimeoutMs` + * so a violation is detected promptly after it actually elapses. + */ + readonly connectionsCheckingIntervalMs: number; +} + +/** + * Finite Node HTTP server timeouts for this JSON-only API. Node's own defaults + * (`headersTimeout`: 60s, `requestTimeout`: 300s) are generous enough to let a slow-header or + * slow-body connection hold a socket for minutes with no reverse proxy in front; these values bound + * that exposure while remaining comfortable for the small JSON payloads this API ever accepts (see + * `./body-limits.js`). `headersTimeoutMs` must not exceed `requestTimeoutMs` — Node validates this + * relationship itself at server construction — so headers alone may take up to 20s, and the + * complete request (headers plus body) up to 30s, before Node itself responds `408` and closes the + * connection without ever invoking a handler (subject to `connectionsCheckingIntervalMs`'s own + * detection latency, above). `keepAliveTimeoutMs` matches Node's own default (5s) made explicit and + * typed rather than left implicit; it bounds only the idle window between requests on a reused + * connection, never an in-flight one. + */ +const PRODUCTION_NODE_SERVER_TIMEOUTS: NodeServerTimeouts = Object.freeze({ + connectionsCheckingIntervalMs: 5_000, + headersTimeoutMs: 20_000, + keepAliveTimeoutMs: 5_000, + requestTimeoutMs: 30_000, +}); + +/** Never carries the remote address, request path, headers, or any other connection detail. */ +const NODE_SERVER_TIMEOUT_LOG_MESSAGE = + "[server] A connection was closed for exceeding a configured header/request timeout."; + export interface RunningApi { readonly app: Hono; readonly server: ServerType; @@ -45,6 +86,10 @@ export interface StartApiOptions { * runtime startup. Never set by the executable entry point. See * `prepareStartupModuleInventory`. */ readonly discoveredModulesOverride?: readonly DiscoveredModule[]; + /** Test-only escape hatch: narrows the Node header/request/keep-alive timeouts so a transport + * test can prove real timeout behavior deterministically without waiting out the production + * values. Never set by the executable entry point. */ + readonly nodeServerTimeoutOverride?: Partial; } async function safeCloseDatabase(handle: ApiDatabaseHandle): Promise { @@ -68,21 +113,36 @@ async function safeStopModuleRuntime(moduleRuntime: StartedModuleRuntime): Promi } /** - * Binds the Node HTTP server. `wrapFetchWithClientAddress` resolves and sanitizes the caller's + * Binds the Node HTTP server. Two platform-owned transport concerns are applied at this exact + * boundary, below Hono entirely: `wrapFetchWithClientAddress` resolves and sanitizes the caller's * address from the real socket peer before any route (including Better Auth's own rate limiter) - * ever sees the request — see `./client-address.js` for the platform-owned client-address - * resolution policy this depends on. + * ever sees the request (see `./client-address.js`), and `serverOptions` gives Node finite + * header/request/keep-alive timeouts so a slow-header or slow-body connection cannot hold a socket + * indefinitely. A connection Node itself closes for exceeding one of these timeouts never reaches + * `app.fetch` and never receives a second response — both are Node's own documented behavior for + * `headersTimeout`/`requestTimeout`, not something this function re-implements. */ function bindServer( app: Hono, host: string, port: number, clientAddressResolver: ClientAddressResolver, + timeouts: NodeServerTimeouts, ): Promise { return new Promise((resolve, reject) => { let settled = false; const server = serve( - { fetch: wrapFetchWithClientAddress(app.fetch, clientAddressResolver), hostname: host, port }, + { + fetch: wrapFetchWithClientAddress(app.fetch, clientAddressResolver), + hostname: host, + port, + serverOptions: { + connectionsCheckingInterval: timeouts.connectionsCheckingIntervalMs, + headersTimeout: timeouts.headersTimeoutMs, + keepAliveTimeout: timeouts.keepAliveTimeoutMs, + requestTimeout: timeouts.requestTimeoutMs, + }, + }, () => { if (!settled) { settled = true; @@ -90,6 +150,13 @@ function bindServer( } }, ); + // A sanitized operational observation only — Node's own headersTimeout/requestTimeout + // enforcement already closes the connection (and, for requestTimeout, sends a 408) regardless + // of whether anything is listening here; this never carries the remote address, headers, or any + // other request detail. + server.on("timeout", () => { + console.error(NODE_SERVER_TIMEOUT_LOG_MESSAGE); + }); server.once("error", () => { if (!settled) { settled = true; @@ -199,10 +266,14 @@ export async function startApi( } const clientAddressResolver = createClientAddressResolver(config.clientAddress); + const timeouts: NodeServerTimeouts = { + ...PRODUCTION_NODE_SERVER_TIMEOUTS, + ...options.nodeServerTimeoutOverride, + }; let server: ServerType; try { - server = await bindServer(app, config.host, config.port, clientAddressResolver); + server = await bindServer(app, config.host, config.port, clientAddressResolver, timeouts); } catch (error) { await safeStopModuleRuntime(moduleRuntime); await safeCloseDatabase(databaseHandle); diff --git a/apps/api/tests/api-http.integration.ts b/apps/api/tests/api-http.integration.ts index 1d5e579..cdad99a 100644 --- a/apps/api/tests/api-http.integration.ts +++ b/apps/api/tests/api-http.integration.ts @@ -1,5 +1,6 @@ import { randomBytes } from "node:crypto"; import { request as httpRequest } from "node:http"; +import net from "node:net"; import { fileURLToPath } from "node:url"; import path from "node:path"; import { parseEkklesiaUserId, parsePermissionId, SYSTEM_ACTOR } from "@ekklesia/contracts"; @@ -20,7 +21,7 @@ import { Client, type QueryResultRow } from "pg"; import { afterAll, describe, expect, it, vi } from "vitest"; import type { ClientAddressConfigInput } from "../src/client-address.js"; import { createApiRuntimeConfig } from "../src/config.js"; -import { startApi, type RunningApi } from "../src/server.js"; +import { startApi, type NodeServerTimeouts, type RunningApi } from "../src/server.js"; const integrationDatabaseUrlEnvironment = "EKKLESIA_MIGRATION_TEST_DATABASE_URL"; const coreMigrationsDirectory = path.join( @@ -275,6 +276,7 @@ async function createFixtureModule( method: "GET", path: "/events/:eventId", permission: "calendar.events.read", + maxBodyBytes: 0, async handler(request, context) { return Response.json({ eventId: context.params.eventId, principal: context.principal }); }, @@ -283,6 +285,7 @@ async function createFixtureModule( method: "POST", path: "/events", permission: "calendar.events.read", + maxBodyBytes: 1024, async handler(request, context) { return Response.json({ created: true, principal: context.principal }, { status: 201 }); }, @@ -329,6 +332,7 @@ function apiOrigin(running: RunningApi): string { interface StartConfiguredApiOptions { readonly clientAddress?: ClientAddressConfigInput; readonly modulesDirectoryOverride?: string; + readonly nodeServerTimeoutOverride?: Partial; /** Defaults to `true`: almost every existing scenario in this suite signs accounts up over HTTP * and needs it enabled. Pass `false` explicitly for the dedicated signup-gating scenarios. */ readonly publicSignupEnabled?: boolean; @@ -364,6 +368,9 @@ async function startConfiguredApi( ...(options.modulesDirectoryOverride === undefined ? {} : { modulesDirectoryOverride: options.modulesDirectoryOverride }), + ...(options.nodeServerTimeoutOverride === undefined + ? {} + : { nodeServerTimeoutOverride: options.nodeServerTimeoutOverride }), }); } @@ -1929,3 +1936,128 @@ describe.sequential("live API — client-address resolution and auth rate-limit await dropDatabase(database.name); }); }); + +describe.sequential("live API — Node header/request timeouts", () => { + it("closes a connection whose headers never complete within the configured headersTimeout, never invoking a handler", async () => { + const database = await createDisposableDatabase("node_timeouts"); + await applyCoreMigrations(database.url); + const config = createApiRuntimeConfig( + { + databaseUrl: database.url, + host: "127.0.0.1", + identity: { + baseUrl: identityBaseUrl, + publicSignupEnabled: false, + secret: "integration-test-secret-0123456789abcdefgh", + trustedOrigins: [identityBaseUrl], + }, + port: 0, + }, + { environment: "production" }, + ); + // Deliberately tiny, test-only timeouts — production composition (server.ts's `main`) never + // sets this override — so this test observes the real transport-event close deterministically, + // in well under a second, rather than waiting out the production values. + const running = await startApi(config, { + nodeServerTimeoutOverride: { + connectionsCheckingIntervalMs: 100, + headersTimeoutMs: 200, + keepAliveTimeoutMs: 200, + requestTimeoutMs: 300, + }, + }); + try { + const address = running.server.address(); + if (!address || typeof address !== "object") { + throw harnessError("The API integration server has no bound address."); + } + + const socket = net.connect({ host: "127.0.0.1", port: address.port }); + await new Promise((resolve, reject) => { + socket.once("connect", () => resolve()); + socket.once("error", reject); + }); + const receivedChunks: Buffer[] = []; + // A `net.Socket` stays paused (and never surfaces the peer's eventual FIN as `close`) until + // something actively consumes its readable side — attaching this listener is what makes the + // socket start flowing, not merely a way to capture the response for the assertion below. + socket.on("data", (chunk) => receivedChunks.push(chunk as Buffer)); + const closed = new Promise((resolve) => socket.once("close", resolve)); + + // An incomplete request: a request line and one header, deliberately never terminated by the + // blank line that would mark the end of the headers, and no further data ever sent. + socket.write("GET /api/v1/principal HTTP/1.1\r\nHost: 127.0.0.1\r\n"); + + await closed; + expect(socket.destroyed).toBe(true); + const received = Buffer.concat(receivedChunks).toString("utf8"); + expect(received).toContain("408"); + } finally { + await running.shutdown().catch(() => undefined); + await dropDatabase(database.name); + } + }); + + it("closes a connection whose body never completes within the configured requestTimeout, never invoking a handler or writing a second response", async () => { + const database = await createDisposableDatabase("node_timeouts_body"); + await applyCoreMigrations(database.url); + const config = createApiRuntimeConfig( + { + databaseUrl: database.url, + host: "127.0.0.1", + identity: { + baseUrl: identityBaseUrl, + publicSignupEnabled: false, + secret: "integration-test-secret-0123456789abcdefgh", + trustedOrigins: [identityBaseUrl, dashboardOrigin], + }, + port: 0, + }, + { environment: "production" }, + ); + const running = await startApi(config, { + nodeServerTimeoutOverride: { + connectionsCheckingIntervalMs: 100, + headersTimeoutMs: 300, + keepAliveTimeoutMs: 200, + requestTimeoutMs: 400, + }, + }); + try { + const address = running.server.address(); + if (!address || typeof address !== "object") { + throw harnessError("The API integration server has no bound address."); + } + + const socket = net.connect({ host: "127.0.0.1", port: address.port }); + await new Promise((resolve, reject) => { + socket.once("connect", () => resolve()); + socket.once("error", reject); + }); + const receivedChunks: Buffer[] = []; + socket.on("data", (chunk) => receivedChunks.push(chunk as Buffer)); + const closed = new Promise((resolve) => socket.once("close", resolve)); + + // Complete, well-formed headers declaring a body that never actually arrives. + socket.write( + "POST /api/auth/sign-in/email HTTP/1.1\r\n" + + "Host: 127.0.0.1\r\n" + + "Content-Type: application/json\r\n" + + "Content-Length: 1000\r\n" + + "\r\n" + + '{"partial":', + ); + + await closed; + const received = Buffer.concat(receivedChunks).toString("utf8"); + // Node's own documented requestTimeout behavior: a 408, never a real sign-in attempt (which + // would need the complete body to ever reach Better Auth), and never a second response. + expect(received).toContain("408"); + expect(received).not.toContain("Set-Cookie"); + expect(received.match(/^HTTP\/1\.1 \d{3}/gmu)?.length ?? 0).toBeLessThanOrEqual(1); + } finally { + await running.shutdown().catch(() => undefined); + await dropDatabase(database.name); + } + }); +}); diff --git a/apps/api/tests/app.test.ts b/apps/api/tests/app.test.ts index 253d4c4..b9d1be1 100644 --- a/apps/api/tests/app.test.ts +++ b/apps/api/tests/app.test.ts @@ -14,6 +14,7 @@ import type { EkklesiaIdentityRuntime } from "@ekklesia/core/identity"; import { IdentityRuntimeError } from "@ekklesia/core/identity"; import { describe, expect, it, vi } from "vitest"; import { createApiApp, type CreateApiAppAuthorizationOptions } from "../src/app.js"; +import { AUTH_REQUEST_MAX_BODY_BYTES } from "../src/body-limits.js"; const trustedOrigin = "https://dashboard.example.test"; const untrustedOrigin = "https://evil.example.test"; @@ -226,6 +227,116 @@ describe("Ekklesia API Hono application", () => { expect(response.headers.get("cache-control")).toBe("no-store"); }); + describe("auth request body limit", () => { + it("accepts a request exactly at the auth body limit", async () => { + const runtime = fakeRuntime(); + const app = createApiApp({ + moduleRoutes: Object.freeze([]), + moduleServices: fakeModuleServices(), + authorization: fakeAuthorization({}), + runtime, + trustedOrigins: [trustedOrigin], + }); + + const response = await app.request("/api/auth/sign-in/email", { + body: "a".repeat(AUTH_REQUEST_MAX_BODY_BYTES), + headers: { "content-length": String(AUTH_REQUEST_MAX_BODY_BYTES) }, + method: "POST", + }); + + expect(response.status).toBe(200); + expect(runtime.handleAuthRequest).toHaveBeenCalledTimes(1); + }); + + it("rejects a request one byte over the auth body limit with a sanitized 413, never invoking Better Auth", async () => { + const runtime = fakeRuntime(); + const app = createApiApp({ + moduleRoutes: Object.freeze([]), + moduleServices: fakeModuleServices(), + authorization: fakeAuthorization({}), + runtime, + trustedOrigins: [trustedOrigin], + }); + + const response = await app.request("/api/auth/sign-in/email", { + body: "a".repeat(AUTH_REQUEST_MAX_BODY_BYTES + 1), + headers: { "content-length": String(AUTH_REQUEST_MAX_BODY_BYTES + 1) }, + method: "POST", + }); + + expect(response.status).toBe(413); + expect(await response.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(response.headers.get("cache-control")).toBe("no-store"); + expect(runtime.handleAuthRequest).not.toHaveBeenCalled(); + }); + + it("rejects an oversized declared Content-Length from its header alone, without invoking Better Auth", async () => { + // A declared-oversized Content-Length is rejected synchronously from the header value itself + // (`hono/body-limit`'s pinned implementation never reads the stream for this case) — proven + // here by the outcome (413, Better Auth never invoked) rather than by instrumenting the + // stream, since this in-memory test harness's own `Request`/body plumbing may touch the + // stream for reasons unrelated to `bodyLimit`'s own logic. + const runtime = fakeRuntime(); + const app = createApiApp({ + moduleRoutes: Object.freeze([]), + moduleServices: fakeModuleServices(), + authorization: fakeAuthorization({}), + runtime, + trustedOrigins: [trustedOrigin], + }); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("a")); + controller.close(); + }, + }); + + const response = await app.request("/api/auth/sign-in/email", { + body: stream, + duplex: "half", + headers: { "content-length": String(AUTH_REQUEST_MAX_BODY_BYTES * 10) }, + method: "POST", + } as RequestInit); + + expect(response.status).toBe(413); + expect(await response.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(runtime.handleAuthRequest).not.toHaveBeenCalled(); + }); + + it("rejects an oversized streamed body with no declared Content-Length", async () => { + const runtime = fakeRuntime(); + const app = createApiApp({ + moduleRoutes: Object.freeze([]), + moduleServices: fakeModuleServices(), + authorization: fakeAuthorization({}), + runtime, + trustedOrigins: [trustedOrigin], + }); + const chunk = new Uint8Array(1024).fill(97); + let enqueued = 0; + const stream = new ReadableStream({ + pull(controller) { + if (enqueued > AUTH_REQUEST_MAX_BODY_BYTES) { + controller.close(); + return; + } + enqueued += chunk.length; + controller.enqueue(chunk); + }, + }); + + const response = await app.request("/api/auth/sign-in/email", { + body: stream, + duplex: "half", + method: "POST", + } as RequestInit); + + expect(response.status).toBe(413); + expect(await response.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(runtime.handleAuthRequest).not.toHaveBeenCalled(); + }); + }); + it("has deterministic 404 behavior for an unknown route", async () => { const app = createApiApp({ moduleRoutes: Object.freeze([]), diff --git a/apps/api/tests/grants-routes.test.ts b/apps/api/tests/grants-routes.test.ts index 07012a5..5b4b976 100644 --- a/apps/api/tests/grants-routes.test.ts +++ b/apps/api/tests/grants-routes.test.ts @@ -15,6 +15,7 @@ import { import type { EkklesiaIdentityRuntime } from "@ekklesia/core/identity"; import { Hono } from "hono"; import { describe, expect, it, vi } from "vitest"; +import { CORE_MUTATION_MAX_BODY_BYTES } from "../src/body-limits.js"; import { createGrantsRoutes } from "../src/grants-routes.js"; const trustedOrigin = "https://dashboard.example.test"; @@ -378,3 +379,136 @@ describe("POST /api/v1/grants/:grantId/revoke", () => { expect(await response.json()).toEqual({ code: "AUTHORIZATION_GRANT_REVOKE_FAILED" }); }); }); + +describe("Core grant mutation request body limit", () => { + it("accepts a comfortably-under-limit grant-creation body and rejects one byte over the exact limit", async () => { + const store = fakeStore(); + const { app } = buildApp({ store }); + + const overSizedBody = "a".repeat(CORE_MUTATION_MAX_BODY_BYTES + 1); + const overResponse = await app.request("/api/v1/grants", { + body: overSizedBody, + headers: { + "content-length": String(overSizedBody.length), + "content-type": "application/json", + origin: trustedOrigin, + }, + method: "POST", + }); + expect(overResponse.status).toBe(413); + expect(await overResponse.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(store.createGrant).not.toHaveBeenCalled(); + + // A well-formed, comfortably-under-limit payload still succeeds through the same route. + const underResponse = await app.request("/api/v1/grants", { + body: JSON.stringify({ permission: registeredPermission, subjectUserId }), + headers: { "content-type": "application/json", origin: trustedOrigin }, + method: "POST", + }); + expect(underResponse.status).not.toBe(413); + }); + + it("rejects an oversized declared Content-Length on grant creation without invoking permission evaluation", async () => { + const evaluatorAuthorize = vi.fn().mockResolvedValue({ allowed: true }); + const evaluator: AuthorizationEvaluator = { authorize: evaluatorAuthorize }; + const store = fakeStore(); + const app = new Hono(); + app.route( + "/", + createGrantsRoutes({ + evaluator, + registry: CORE_PERMISSION_REGISTRY, + runtime: fakeRuntime(), + store, + trustedOrigins: new Set([trustedOrigin]), + }), + ); + + const response = await app.request("/api/v1/grants", { + body: "a", + headers: { + "content-length": String(CORE_MUTATION_MAX_BODY_BYTES * 10), + "content-type": "application/json", + origin: trustedOrigin, + }, + method: "POST", + }); + + expect(response.status).toBe(413); + expect(evaluatorAuthorize).not.toHaveBeenCalled(); + expect(store.createGrant).not.toHaveBeenCalled(); + }); + + it("rejects an oversized streamed grant-creation body with no declared Content-Length", async () => { + const store = fakeStore(); + const { app } = buildApp({ store }); + const chunk = new Uint8Array(1024).fill(97); + let enqueued = 0; + const stream = new ReadableStream({ + pull(controller) { + if (enqueued > CORE_MUTATION_MAX_BODY_BYTES) { + controller.close(); + return; + } + enqueued += chunk.length; + controller.enqueue(chunk); + }, + }); + + const response = await app.request("/api/v1/grants", { + body: stream, + duplex: "half", + headers: { "content-type": "application/json", origin: trustedOrigin }, + method: "POST", + } as RequestInit); + + expect(response.status).toBe(413); + expect(await response.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(store.createGrant).not.toHaveBeenCalled(); + }); + + it("does not read an oversized body from an untrusted Origin: the trusted-origin rejection wins first", async () => { + const store = fakeStore(); + const { app } = buildApp({ store }); + const chunk = new Uint8Array(1024).fill(97); + let pullCount = 0; + const stream = new ReadableStream({ + pull(controller) { + pullCount += 1; + if (pullCount > 3) { + controller.close(); + return; + } + controller.enqueue(chunk); + }, + }); + + const response = await app.request("/api/v1/grants", { + body: stream, + duplex: "half", + headers: { "content-type": "application/json", origin: untrustedOrigin }, + method: "POST", + } as RequestInit); + + expect(response.status).toBe(403); + expect(await response.json()).toEqual({ code: "UNTRUSTED_ORIGIN" }); + expect(store.createGrant).not.toHaveBeenCalled(); + }); + + it("also bounds the revoke mutation body", async () => { + const store = fakeStore(); + const { app } = buildApp({ store }); + + const response = await app.request(`/api/v1/grants/${grantId}/revoke`, { + body: "a".repeat(CORE_MUTATION_MAX_BODY_BYTES + 1), + headers: { + "content-length": String(CORE_MUTATION_MAX_BODY_BYTES + 1), + origin: trustedOrigin, + }, + method: "POST", + }); + + expect(response.status).toBe(413); + expect(store.revokeGrant).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/api/tests/lifecycle.test.ts b/apps/api/tests/lifecycle.test.ts index 3628d25..48d1e51 100644 --- a/apps/api/tests/lifecycle.test.ts +++ b/apps/api/tests/lifecycle.test.ts @@ -155,6 +155,7 @@ async function createWorkingFixtureModule( method: "GET", path: "/ping", permission: ${JSON.stringify(permissionId)}, + maxBodyBytes: 0, handler: async () => new Response("pong", { status: 200 }), });` : ""; @@ -214,6 +215,7 @@ function fakeDiscoveredModule( * `triggerError` simulates the server's own `'error'` event (e.g. a bind failure). */ function fakeServer() { let errorHandler: ((error: Error) => void) | undefined; + let timeoutHandler: (() => void) | undefined; const server = { address: () => ({ address: "127.0.0.1", family: "IPv4", port: 4_000 }), close: vi.fn((callback?: () => void) => { @@ -221,6 +223,12 @@ function fakeServer() { return server; }), listening: true, + on: vi.fn((event: string, handler: () => void) => { + if (event === "timeout") { + timeoutHandler = handler; + } + return server; + }), once: vi.fn((event: string, handler: (error: Error) => void) => { if (event === "error") { errorHandler = handler; @@ -231,6 +239,7 @@ function fakeServer() { return { server, triggerError: (error: Error) => errorHandler?.(error), + triggerTimeout: () => timeoutHandler?.(), }; } @@ -239,12 +248,12 @@ function fakeServer() { * resolving synchronously would reference `bindServer`'s own `server` const before it is * initialized. */ function mockSuccessfulServe() { - const { server, triggerError } = fakeServer(); + const { server, triggerError, triggerTimeout } = fakeServer(); serveMock.mockImplementation((_options: unknown, listeningListener?: () => void) => { queueMicrotask(() => listeningListener?.()); return server; }); - return { server, triggerError }; + return { server, triggerError, triggerTimeout }; } function mockFailingServe(error: Error) { @@ -465,6 +474,105 @@ describe("Ekklesia API shutdown lifecycle", () => { }); }); +describe("Ekklesia API Node server timeouts", () => { + beforeEach(() => { + poolConstructor.mockReset(); + betterAuthMock.mockReset(); + drizzleAdapterMock.mockClear(); + serveMock.mockReset(); + }); + + it("configures finite, typed header/request/keep-alive timeouts on the underlying Node server", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + mockSuccessfulServe(); + + const running = await startApi(validConfig()); + + expect(serveMock).toHaveBeenCalledTimes(1); + const [options] = serveMock.mock.calls[0] as [ + { + serverOptions?: { + connectionsCheckingInterval?: number; + headersTimeout?: number; + keepAliveTimeout?: number; + requestTimeout?: number; + }; + }, + ]; + expect(options.serverOptions?.headersTimeout).toBeGreaterThan(0); + expect(options.serverOptions?.requestTimeout).toBeGreaterThan(0); + expect(options.serverOptions?.keepAliveTimeout).toBeGreaterThan(0); + // Node only enforces headersTimeout/requestTimeout when this periodic check runs (verified + // directly against the pinned Node runtime — its own default, 30s, would otherwise let a + // violating connection sit for up to an additional 30s past its actual timeout). + expect(options.serverOptions?.connectionsCheckingInterval).toBeGreaterThan(0); + expect(options.serverOptions?.connectionsCheckingInterval).toBeLessThan(30_000); + // Node requires headersTimeout not to exceed requestTimeout. + expect(options.serverOptions?.headersTimeout).toBeLessThanOrEqual( + options.serverOptions?.requestTimeout ?? 0, + ); + + await running.shutdown(); + }); + + it("allows a test-only override to narrow the timeouts without changing the production defaults", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + mockSuccessfulServe(); + + const running = await startApi(validConfig(), { + nodeServerTimeoutOverride: { headersTimeoutMs: 50, requestTimeoutMs: 100 }, + }); + + const [options] = serveMock.mock.calls[0] as [ + { + serverOptions?: { + headersTimeout?: number; + keepAliveTimeout?: number; + requestTimeout?: number; + }; + }, + ]; + expect(options.serverOptions?.headersTimeout).toBe(50); + expect(options.serverOptions?.requestTimeout).toBe(100); + // The un-overridden keep-alive timeout keeps its production default. + expect(options.serverOptions?.keepAliveTimeout).toBeGreaterThan(0); + + await running.shutdown(); + }); + + it("logs a sanitized operational event, and nothing else, when the underlying server reports a timeout", async () => { + const poolEnd = vi.fn().mockResolvedValue(undefined); + poolConstructor.mockImplementation(function fakePool() { + return { end: poolEnd, on: vi.fn() }; + }); + betterAuthMock.mockReturnValue({ api: { getSession: vi.fn() }, handler: vi.fn() }); + const { triggerTimeout } = mockSuccessfulServe(); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => undefined); + + try { + const running = await startApi(validConfig()); + triggerTimeout(); + + expect(errorSpy).toHaveBeenCalledTimes(1); + const [message] = errorSpy.mock.calls[0] as [string]; + expect(message).toContain("timeout"); + expect(message).not.toMatch(/\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/u); + + await running.shutdown(); + } finally { + errorSpy.mockRestore(); + } + }); +}); + describe("Ekklesia API module permission composition at startup", () => { afterEach(async () => { await Promise.all( @@ -688,6 +796,7 @@ describe("Ekklesia API module runtime startup and shutdown", () => { method: "GET", path: "/ping", permission: "db-identity.ping.read", + maxBodyBytes: 0, handler: async (_request, context) => { ${refs.expression}.push(context.services.database); return new Response(null, { status: 204 }); diff --git a/apps/api/tests/module-http-routes.test.ts b/apps/api/tests/module-http-routes.test.ts index d0fbdbb..ffe4f5c 100644 --- a/apps/api/tests/module-http-routes.test.ts +++ b/apps/api/tests/module-http-routes.test.ts @@ -49,8 +49,14 @@ function fakeStore(): AuthorizationStore { }; } -function moduleRoute(fields: RegisteredModuleHttpRoute): RegisteredModuleHttpRoute { - return Object.freeze({ ...fields }); +/** Defaults `maxBodyBytes` to a generous value for tests that are not themselves exercising the + * body-size contract, so the many pre-existing routing/authorization/CORS scenarios below do not + * each need to restate an unrelated body-size choice; the dedicated "module route body limits" + * suite further down overrides it explicitly to prove the contract itself. */ +function moduleRoute( + fields: Omit & { maxBodyBytes?: number }, +): RegisteredModuleHttpRoute { + return Object.freeze({ maxBodyBytes: 1024, ...fields }); } /** A small typed fake `ModuleRuntimeServices` — these tests exercise route mounting, @@ -1147,6 +1153,262 @@ describe("module route HEAD semantics", () => { }); }); +describe("module route body limits", () => { + it("rejects a non-empty declared body on a route declaring maxBodyBytes: 0", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: 0, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + body: "a", + headers: { "content-length": "1", origin: trustedOrigin }, + method: "POST", + }); + + expect(response.status).toBe(413); + expect(await response.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(handler).not.toHaveBeenCalled(); + }); + + it("rejects a non-empty streamed body on a route declaring maxBodyBytes: 0", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: 0, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + const stream = new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("a")); + controller.close(); + }, + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + body: stream, + duplex: "half", + headers: { origin: trustedOrigin }, + method: "POST", + } as RequestInit); + + expect(response.status).toBe(413); + expect(handler).not.toHaveBeenCalled(); + }); + + it("accepts a genuinely empty body on a route declaring maxBodyBytes: 0", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: 0, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + headers: { origin: trustedOrigin }, + method: "POST", + }); + + expect(response.status).toBe(200); + expect(handler).toHaveBeenCalledTimes(1); + }); + + it("accepts a body exactly at an explicit maxBodyBytes limit and rejects one byte over", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const limit = 32; + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: limit, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + + const atLimit = "a".repeat(limit); + const atLimitResponse = await app.request("/api/v1/modules/calendar/events", { + body: atLimit, + headers: { "content-length": String(limit), origin: trustedOrigin }, + method: "POST", + }); + expect(atLimitResponse.status).toBe(200); + + const overLimit = "a".repeat(limit + 1); + const overLimitResponse = await app.request("/api/v1/modules/calendar/events", { + body: overLimit, + headers: { "content-length": String(limit + 1), origin: trustedOrigin }, + method: "POST", + }); + expect(overLimitResponse.status).toBe(413); + expect(await overLimitResponse.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(handler).toHaveBeenCalledTimes(1); + }); + + it("rejects an oversized streamed body on a route with an explicit limit, with no declared Content-Length", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const limit = 64; + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: limit, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + const chunk = new Uint8Array(16).fill(97); + let enqueued = 0; + const stream = new ReadableStream({ + pull(controller) { + if (enqueued > limit) { + controller.close(); + return; + } + enqueued += chunk.length; + controller.enqueue(chunk); + }, + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + body: stream, + duplex: "half", + headers: { origin: trustedOrigin }, + method: "POST", + } as RequestInit); + + expect(response.status).toBe(413); + expect(handler).not.toHaveBeenCalled(); + }); + + it("enforces the body limit before authorization runs, so an oversized denied request never reaches the evaluator", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const evaluator: AuthorizationEvaluator = { + authorize: vi.fn().mockResolvedValue({ allowed: false }), + }; + const { app } = buildModuleApp({ + evaluator, + registry: permissionRegistry([{ id: writePermission, baseline: "restricted" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: 8, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + body: "a".repeat(9), + headers: { "content-length": "9", origin: trustedOrigin }, + method: "POST", + }); + + expect(response.status).toBe(413); + expect(await response.json()).toEqual({ code: "REQUEST_BODY_TOO_LARGE" }); + expect(evaluator.authorize).not.toHaveBeenCalled(); + expect(handler).not.toHaveBeenCalled(); + }); + + it("never reads an oversized body from an untrusted Origin: the trusted-origin rejection wins first", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: 8, + method: "POST", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + const chunk = new Uint8Array(16).fill(97); + let pullCount = 0; + const stream = new ReadableStream({ + pull(controller) { + pullCount += 1; + if (pullCount > 3) { + controller.close(); + return; + } + controller.enqueue(chunk); + }, + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + body: stream, + duplex: "half", + headers: { origin: untrustedOrigin }, + method: "POST", + } as RequestInit); + + expect(response.status).toBe(403); + expect(await response.json()).toEqual({ code: "UNTRUSTED_ORIGIN" }); + expect(handler).not.toHaveBeenCalled(); + }); + + it("applies the route's own declared limit to a GET route too", async () => { + const handler = vi.fn(async () => Response.json({ ok: true })); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: readPermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + maxBodyBytes: 0, + method: "GET", + moduleId: "calendar", + path: "/events", + permission: readPermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events"); + expect(response.status).toBe(200); + expect(handler).toHaveBeenCalledTimes(1); + }); +}); + describe("module routes and the rest of the API", () => { it("does not invoke a handler for an unmatched module path", async () => { const handler = vi.fn(async () => Response.json({ ok: true })); diff --git a/core/src/modules/runtime.ts b/core/src/modules/runtime.ts index 60a63fd..f6ecd94 100644 --- a/core/src/modules/runtime.ts +++ b/core/src/modules/runtime.ts @@ -35,6 +35,7 @@ export type ModuleRuntimeIssueCode = | "MODULE_ROLLBACK_FAILED" | "MODULE_ROUTE_DEFINITION_INVALID" | "MODULE_ROUTE_DUPLICATE" + | "MODULE_ROUTE_MAX_BODY_BYTES_INVALID" | "MODULE_ROUTE_METHOD_INVALID" | "MODULE_ROUTE_PATH_INVALID" | "MODULE_ROUTE_PERMISSION_INVALID" @@ -334,6 +335,7 @@ class StagedModuleHttpRoutes { local.push( Object.freeze({ handler: parsed.data.handler, + maxBodyBytes: parsed.data.maxBodyBytes, method: parsed.data.method, moduleId: module.manifest.id, path: parsed.data.path, diff --git a/core/tests/fixtures/module-database-integration/db-facade/server/index.js b/core/tests/fixtures/module-database-integration/db-facade/server/index.js index bf04f57..212061d 100644 --- a/core/tests/fixtures/module-database-integration/db-facade/server/index.js +++ b/core/tests/fixtures/module-database-integration/db-facade/server/index.js @@ -37,6 +37,7 @@ export default { method: "GET", path: "/records/:id", permission: "db-facade.records.read", + maxBodyBytes: 0, async handler(_request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), @@ -62,6 +63,7 @@ export default { // fixture's one authenticated-baseline GET route, used to prove HEAD cannot bypass // authorization the way it could if HEAD skipped the same middleware chain GET uses. permission: "db-facade.records.write", + maxBodyBytes: 0, async handler(_request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), @@ -81,6 +83,7 @@ export default { method: "POST", path: "/records", permission: "db-facade.records.write", + maxBodyBytes: 1024, async handler(request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), @@ -98,6 +101,7 @@ export default { method: "PATCH", path: "/records/:id", permission: "db-facade.records.write", + maxBodyBytes: 1024, async handler(request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), @@ -122,6 +126,7 @@ export default { method: "POST", path: "/records/transactional", permission: "db-facade.records.write", + maxBodyBytes: 1024, async handler(request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), @@ -152,6 +157,7 @@ export default { method: "GET", path: "/boom", permission: "db-facade.records.read", + maxBodyBytes: 0, async handler(_request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), @@ -170,6 +176,7 @@ export default { method: "GET", path: "/safe-boom", permission: "db-facade.records.read", + maxBodyBytes: 0, async handler(_request, context) { recordEvent({ contextKeys: Object.keys(context).sort(), diff --git a/core/tests/module-runtime.test.ts b/core/tests/module-runtime.test.ts index 51a1f78..239d22c 100644 --- a/core/tests/module-runtime.test.ts +++ b/core/tests/module-runtime.test.ts @@ -444,6 +444,7 @@ describe("module HTTP routes", () => { method: "GET", path: "/items", permission: "single.items.read", + maxBodyBytes: 1024, handler: ${noopHandlerSource()}, }); } @@ -473,8 +474,8 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "alpha", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/first", permission: "alpha.first.read", handler: ${noopHandlerSource()} }); - http.route({ method: "GET", path: "/second", permission: "alpha.second.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/first", permission: "alpha.first.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/second", permission: "alpha.second.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -485,7 +486,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "beta", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/third", permission: "beta.third.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/third", permission: "beta.third.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -509,8 +510,8 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "same-path", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "same-path.items.read", handler: ${noopHandlerSource()} }); - http.route({ method: "POST", path: "/items", permission: "same-path.items.write", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "same-path.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "POST", path: "/items", permission: "same-path.items.write", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -532,8 +533,8 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "dup", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "dup.items.read", handler: ${noopHandlerSource()} }); - http.route({ method: "GET", path: "/items", permission: "dup.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "dup.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "dup.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -551,8 +552,8 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "param-shape", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/events/:eventId", permission: "param-shape.events.read", handler: ${noopHandlerSource()} }); - http.route({ method: "GET", path: "/events/:id", permission: "param-shape.events.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/events/:eventId", permission: "param-shape.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/events/:id", permission: "param-shape.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -570,7 +571,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "bad-method", version: "0.1.0", register({ http }) { - http.route({ method: "get", path: "/items", permission: "bad-method.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "get", path: "/items", permission: "bad-method.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -586,7 +587,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "bad-path", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "items", permission: "bad-path.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "items", permission: "bad-path.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -594,6 +595,22 @@ describe("module HTTP routes", () => { "MODULE_ROUTE_PATH_INVALID", ]); + const badMaxBodyBytesDirectory = await createModulesDirectory(); + await createRuntimeModule(badMaxBodyBytesDirectory, "bad-max-body-bytes", { + manifest: manifestFor("bad-max-body-bytes", { + permissions: [permissionDeclaration("bad-max-body-bytes.items.read")], + }), + serverSource: `export default { + id: "bad-max-body-bytes", version: "0.1.0", + register({ http }) { + http.route({ method: "GET", path: "/items", permission: "bad-max-body-bytes.items.read", maxBodyBytes: -1, handler: ${noopHandlerSource()} }); + } + };`, + }); + expect(runtimeIssueCodes(await captureRuntimeError(badMaxBodyBytesDirectory))).toEqual([ + "MODULE_ROUTE_MAX_BODY_BYTES_INVALID", + ]); + const badDefinitionDirectory = await createModulesDirectory(); await createRuntimeModule(badDefinitionDirectory, "bad-shape", { manifest: manifestFor("bad-shape", { @@ -624,7 +641,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "undeclared", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "undeclared.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "undeclared.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -644,6 +661,7 @@ describe("module HTTP routes", () => { method: "GET", path: "/items", permission: "core.authorization.grants.manage", + maxBodyBytes: 1024, handler: ${noopHandlerSource()}, }); } @@ -667,7 +685,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "beta", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "alpha.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "alpha.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -689,9 +707,9 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "baselines", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/public", permission: "baselines.public.read", handler: ${noopHandlerSource()} }); - http.route({ method: "GET", path: "/authenticated", permission: "baselines.authenticated.read", handler: ${noopHandlerSource()} }); - http.route({ method: "GET", path: "/restricted", permission: "baselines.restricted.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/public", permission: "baselines.public.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/authenticated", permission: "baselines.authenticated.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/restricted", permission: "baselines.restricted.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -701,6 +719,7 @@ describe("module HTTP routes", () => { for (const route of runtime.routes) { expect(Object.keys(route).sort()).toEqual([ "handler", + "maxBodyBytes", "method", "moduleId", "path", @@ -724,6 +743,7 @@ describe("module HTTP routes", () => { method: "GET", path: "/items", permission: "never-invoked.items.read", + maxBodyBytes: 1024, handler: () => { ${recorder.expression}.push("handler:called"); return new Response(null, { status: 204 }); }, }); } @@ -745,7 +765,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "frozen", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "frozen.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "frozen.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -772,6 +792,7 @@ describe("module HTTP routes", () => { method: "GET", path: "/items", permission: "mutate.items.read", + maxBodyBytes: 1024, handler: ${noopHandlerSource()}, }; http.route(definition); @@ -800,7 +821,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "first-ok", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "first-ok.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "first-ok.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -809,7 +830,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "second-bad", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "second-bad.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "second-bad.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); } };`, }); @@ -829,7 +850,7 @@ describe("module HTTP routes", () => { serverSource: `export default { id: "routed", version: "0.1.0", register({ http }) { - http.route({ method: "GET", path: "/items", permission: "routed.items.read", handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/items", permission: "routed.items.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); }, start() { ${recorder.expression}.push("routed:start"); }, stop() { ${recorder.expression}.push("routed:stop"); } diff --git a/docs/architecture/decisions/0017-public-request-perimeter-hardening.md b/docs/architecture/decisions/0017-public-request-perimeter-hardening.md new file mode 100644 index 0000000..2e8bd69 --- /dev/null +++ b/docs/architecture/decisions/0017-public-request-perimeter-hardening.md @@ -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. diff --git a/docs/architecture/decisions/README.md b/docs/architecture/decisions/README.md index a2eca9f..a4aea95 100644 --- a/docs/architecture/decisions/README.md +++ b/docs/architecture/decisions/README.md @@ -25,3 +25,4 @@ ADRs capture durable choices that affect more than one package or phase. - [0014: Module permission declarations and platform permission registry](0014-module-permission-declarations-and-platform-registry.md) — Accepted - [0015: Static module HTTP routing and production runtime lifecycle](0015-static-module-http-routing-and-runtime-lifecycle.md) — Accepted - [0016: Module database access](0016-module-database-access.md) — Accepted +- [0017: Public request perimeter hardening — client address, signup gating, body limits, timeouts](0017-public-request-perimeter-hardening.md) — Accepted diff --git a/docs/architecture/platform-architecture.md b/docs/architecture/platform-architecture.md index f4e1772..bc23498 100644 --- a/docs/architecture/platform-architecture.md +++ b/docs/architecture/platform-architecture.md @@ -90,8 +90,9 @@ established, ordered before module migrations in the same aggregate. See Phase 4B adds identity HTTP transport: a minimal Hono-based `apps/api` composition root mounts Better Auth at `/api/auth/*` and exposes one Ekklesia-owned route, `GET /api/v1/principal`, behind Core's own Fetch-compatible identity runtime (`@ekklesia/core/identity`) — Core still does not -depend on Hono. Public email/password registration is intentional; every account receives -identical lowest-privilege baseline status, and signup order never grants elevation. See +depend on Hono. Public email/password registration is the intended eventual product policy — every +account receives identical lowest-privilege baseline status, and signup order never grants +elevation — but is disabled by default as of Phase 5D pass 1, below. See [ADR 0011](decisions/0011-identity-http-transport-and-public-enrollment.md) and [the API guide](../development/api.md), and [the identity development guide](../development/identity.md) for the underlying schema and @@ -116,6 +117,18 @@ pool. See [ADR 0014](decisions/0014-module-permission-declarations-and-platform- [ADR 0016](decisions/0016-module-database-access.md), and the repository root [README](../../README.md#current-status) for the complete, current status of every phase. +Phase 5D pass 1 of 5 hardens the public request perimeter against an independent security audit of +the Phase 5C checkpoint: a server-owned client-address resolution boundary (direct mode by default, +explicit trusted-proxy mode otherwise) replaces trusting a caller-controlled forwarded header for +the authentication rate limiter; public sign-up is disabled by default behind an explicit typed +opt-in; every public route — Better Auth, Core's grant mutation, and every module route via a new +required `maxBodyBytes` field on the executable route contract — has an explicit, enforced +request-body ceiling with one sanitized `413` contract; and the Node HTTP server has finite +header/request timeouts. See +[ADR 0017](decisions/0017-public-request-perimeter-hardening.md). This pass does not add +distributed rate limiting or durable signup-abuse control, and Phase 5D itself is not complete — +passes 2–5 and a final independent re-audit remain before Phase 6 may begin. + The project continues finishing the module platform's backend foundations before Phase 6 Surface work begins. Target architecture not yet implemented includes production approval-provider composition, physical storage-bucket provisioning, jobs/workflow automation, richer public resource diff --git a/docs/development/api.md b/docs/development/api.md index c18bd47..dcd68b1 100644 --- a/docs/development/api.md +++ b/docs/development/api.md @@ -278,6 +278,50 @@ bucket, and unrelated real peers are never collapsed into one shared bucket mere header was present. This closes the rate-limit-identity gap; it does not add distributed storage — a genuinely multi-instance deployment still needs a shared or edge limiter, reserved for pass 5. +## Request body limits + +Every public body-consuming route has an explicit, platform-enforced byte ceiling +(`apps/api/src/body-limits.ts`, built on Hono's pinned `hono/body-limit` middleware — verified +directly against its source to reject both an oversized declared `Content-Length` and an oversized +declared-length-free streamed body without ever buffering the whole payload first): + +| Surface | Limit | +| --- | --- | +| `/api/auth/*` (every Better Auth route) | 8 KiB | +| `POST /api/v1/grants`, `POST /api/v1/grants/:grantId/revoke` | 16 KiB | +| A module route (`http.route(...)`'s own `maxBodyBytes`) | Up to 1 MiB, the platform ceiling (`MODULE_ROUTE_MAX_BODY_BYTES_CEILING`, `@ekklesia/contracts`) | + +An oversized request always receives the identical stable, sanitized contract — +`413 {"code":"REQUEST_BODY_TOO_LARGE"}` with `Cache-Control: no-store` — and never invokes Better +Auth's business logic, a Core mutation, permission evaluation (where avoidable), a module handler, +or module database behavior. For a state-changing route the body limit is enforced after +`requireTrustedOrigin` but before permission evaluation, so a rejected cross-origin mutation's body +is never read for size at all, while an oversized same-origin request still fails before any +authorization work runs. See [Module HTTP routes](#module-http-routes) for the module-route body +contract itself, and [Module development](../module-development/README.md#http-routes) for the +module-authoring side of it. Large media transfer is explicitly out of scope for ordinary module +HTTP bodies — that belongs to a future dedicated storage design, never to widening this ceiling. + +## Node request and header timeouts + +`apps/api/src/server.ts` configures the underlying Node HTTP server (via `@hono/node-server`'s +`serverOptions`, passed straight through to `http.createServer`) with finite +`headersTimeout` (20s), `requestTimeout` (30s), and `keepAliveTimeout` (5s, Node's own default, +made explicit). Node's own documented behavior responds `408` and closes the connection without +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 +[ADR 0017](../architecture/decisions/0017-public-request-perimeter-hardening.md) for the full +account, including the standalone reproduction that found the `connectionsCheckingInterval` gap. + ## Module discovery, permission composition, and module runtime startup `startApi()` composes production in one fixed order: @@ -330,6 +374,11 @@ are mounted under `/api/v1/modules/`, in the module runtime's publish one. `OPTIONS` remains entirely platform-owned: a module cannot register it, and a CORS preflight never reaches authorization or a handler. `GET` inherits Hono's own implicit `HEAD` handling (same permission check, response body stripped); a module cannot separately author `HEAD`. +- **Body limit.** A route's own `maxBodyBytes` (see [Module development](module-development/README.md#http-routes) + and [Request body limits](#request-body-limits)) is enforced immediately after the trusted-origin + check and before permission evaluation — `0` means no body is accepted at all. An oversized + request receives the platform's stable `413 {"code":"REQUEST_BODY_TOO_LARGE"}` and never reaches + authorization, the handler, or the module database. - **Handler input.** A handler receives the raw Fetch `Request` (so `Request.url`, headers, and body all behave normally, and the full query string remains available) plus a frozen context of exactly `moduleId`, the authorized `EkklesiaPrincipal`, frozen, Hono-decoded path parameters (an diff --git a/docs/module-development/README.md b/docs/module-development/README.md index 2c2dbdb..dd9f51d 100644 --- a/docs/module-development/README.md +++ b/docs/module-development/README.md @@ -199,6 +199,8 @@ export default defineModuleRuntime({ method: "GET", path: "/events/:eventId", permission: "calendar.events.read", + // This route accepts no request body. See the body-size contract below. + maxBodyBytes: 0, async handler(request, context) { return Response.json({ eventId: context.params.eventId, @@ -210,11 +212,23 @@ export default defineModuleRuntime({ }); ``` -A route definition has exactly four fields — `method`, `path`, `permission`, `handler` — and nothing -else; an unknown field (a baseline, a mount prefix, CORS/origin policy, middleware, audit metadata, -authentication behavior, or another module's namespace) fails registration outright rather than -being ignored. `method` is one of `GET`, `POST`, `PUT`, `PATCH`, `DELETE`, always uppercase; there is -no `OPTIONS` (the platform owns CORS preflight) and no wildcard/all-method registration. +A route definition has exactly five fields — `method`, `path`, `permission`, `maxBodyBytes`, +`handler` — and nothing else; an unknown field (a baseline, a mount prefix, CORS/origin policy, +middleware, audit metadata, authentication behavior, or another module's namespace) fails +registration outright rather than being ignored. `method` is one of `GET`, `POST`, `PUT`, `PATCH`, +`DELETE`, always uppercase; there is no `OPTIONS` (the platform owns CORS preflight) and no +wildcard/all-method registration. + +`maxBodyBytes` is a conscious, required request-body contract, not an optional or defaulted field: +`0` means the route accepts no body at all, and a non-empty declared (`Content-Length`) or streamed +body is rejected before the handler ever runs; any other value is the exact maximum number of bytes +accepted. It must be a non-negative integer no greater than the platform ceiling, +`MODULE_ROUTE_MAX_BODY_BYTES_CEILING` (`@ekklesia/contracts`, currently 1 MiB) — a negative, +non-integer, non-finite, or over-ceiling value fails registration with +`MODULE_ROUTE_MAX_BODY_BYTES_INVALID`. The platform enforces this limit itself, ahead of permission +evaluation and the handler, using the identical sanitized `413 {"code":"REQUEST_BODY_TOO_LARGE"}` +contract every other body-consuming route in the platform uses — see +[API](../development/api.md#request-body-limits). `path` is local to the module, never the path a caller actually sends. The platform alone owns the production mount prefix, `/api/v1/modules/`: `"/"` is the module's own root, `"/events"` @@ -370,6 +384,7 @@ export default defineModuleRuntime({ method: "GET", path: "/events", permission: "calendar.events.read", + maxBodyBytes: 0, async handler(_request, context) { const rows = await context.services.database .select({ id: events.id, title: events.title }) diff --git a/packages/contracts/src/module-http.ts b/packages/contracts/src/module-http.ts index 9dd9749..9fccc6f 100644 --- a/packages/contracts/src/module-http.ts +++ b/packages/contracts/src/module-http.ts @@ -21,6 +21,23 @@ export function isModuleHttpMethod(value: unknown): value is ModuleHttpMethod { return typeof value === "string" && moduleHttpMethods.has(value); } +/** + * The platform-owned hard ceiling on any single module route's declared request-body allowance, in + * bytes. No module route may declare a `maxBodyBytes` above this value — validated at registration + * (`safeParseModuleRouteDefinition`, below), not merely documented. This is a security contract, + * not a manifest capability: it does not gate on `manifestVersion` and is enforced identically for + * every installed module regardless of its declared compatibility range. Large media transfer + * belongs to a future dedicated storage design, never to widening this ceiling. + */ +export const MODULE_ROUTE_MAX_BODY_BYTES_CEILING = 1_048_576; + +const moduleRouteMaxBodyBytesSchema = z + .number() + .int() + .finite() + .min(0) + .max(MODULE_ROUTE_MAX_BODY_BYTES_CEILING); + const MODULE_ROUTE_PATH_MAX_LENGTH = 256; const staticSegmentPattern = /^[a-z0-9]+(?:-[a-z0-9]+)*$/u; const parameterSegmentPattern = /^:[a-z][a-zA-Z0-9]*$/u; @@ -151,15 +168,21 @@ export type ModuleRouteHandler = ( /** * What a module passes to `http.route(...)` during `register`. `permission` is a plain string here * (never the branded `PermissionId`) so a module can write a literal permission id without an - * explicit parse call first; `http.route` validates and brands it. The type has exactly these four - * fields — a module cannot specify a baseline, mount prefix, absolute path, CORS/origin policy, - * authentication behavior, grant/platform-owner state, middleware, audit metadata, another module's - * namespace, or an unprotected/no-permission route. + * explicit parse call first; `http.route` validates and brands it. `maxBodyBytes` is the route's + * conscious request-body contract — `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 maximum number of bytes + * accepted, capped at `MODULE_ROUTE_MAX_BODY_BYTES_CEILING`. This is a security contract the + * platform enforces before a handler ever runs, not a manifest capability: it is not optional and + * has no implicit default, so a module author must make an explicit choice for every route. The + * type has exactly these five fields — a module cannot specify a baseline, mount prefix, absolute + * path, CORS/origin policy, authentication behavior, grant/platform-owner state, middleware, audit + * metadata, another module's namespace, or an unprotected/no-permission route. */ export interface ModuleRouteDefinitionInput { readonly method: ModuleHttpMethod; readonly path: string; readonly permission: string; + readonly maxBodyBytes: number; readonly handler: ModuleRouteHandler; } @@ -169,6 +192,7 @@ export interface ModuleRouteDefinition { readonly method: ModuleHttpMethod; readonly path: ModuleRoutePath; readonly permission: PermissionId; + readonly maxBodyBytes: number; readonly handler: ModuleRouteHandler; } @@ -187,18 +211,29 @@ export interface RegisteredModuleHttpRoute { readonly method: ModuleHttpMethod; readonly path: ModuleRoutePath; readonly permission: PermissionId; + readonly maxBodyBytes: number; readonly handler: ModuleRouteHandler; } -const moduleRouteDefinitionKeys = new Set(["method", "path", "permission", "handler"]); +const moduleRouteDefinitionKeys = new Set([ + "method", + "path", + "permission", + "maxBodyBytes", + "handler", +]); function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null && !Array.isArray(value); } -function hasModuleRouteDefinitionShape( - value: unknown, -): value is { method: unknown; path: unknown; permission: unknown; handler: unknown } { +function hasModuleRouteDefinitionShape(value: unknown): value is { + method: unknown; + path: unknown; + permission: unknown; + maxBodyBytes: unknown; + handler: unknown; +} { if (!isRecord(value)) { return false; } @@ -214,7 +249,8 @@ export type ModuleRouteDefinitionIssueCode = | "MODULE_ROUTE_DEFINITION_INVALID" | "MODULE_ROUTE_METHOD_INVALID" | "MODULE_ROUTE_PATH_INVALID" - | "MODULE_ROUTE_PERMISSION_INVALID"; + | "MODULE_ROUTE_PERMISSION_INVALID" + | "MODULE_ROUTE_MAX_BODY_BYTES_INVALID"; export type ModuleRouteDefinitionParseResult = | { readonly success: true; readonly data: ModuleRouteDefinition } @@ -222,10 +258,10 @@ export type ModuleRouteDefinitionParseResult = /** * Validates every field-level rule a route definition must satisfy on its own — exact shape, - * canonical method, canonical path, well-formed permission id — but never whether the permission is - * actually declared by the registering module. That ownership check needs the module's own manifest - * and belongs to the caller (Core's module runtime), not this framework-neutral contract. Never - * echoes the rejected input in the returned code. + * canonical method, canonical path, well-formed permission id, well-formed body-size contract — but + * never whether the permission is actually declared by the registering module. That ownership check + * needs the module's own manifest and belongs to the caller (Core's module runtime), not this + * framework-neutral contract. Never echoes the rejected input in the returned code. */ export function safeParseModuleRouteDefinition(value: unknown): ModuleRouteDefinitionParseResult { if (!hasModuleRouteDefinitionShape(value)) { @@ -242,12 +278,17 @@ export function safeParseModuleRouteDefinition(value: unknown): ModuleRouteDefin if (!parsedPermission.success) { return { success: false, code: "MODULE_ROUTE_PERMISSION_INVALID" }; } + const parsedMaxBodyBytes = moduleRouteMaxBodyBytesSchema.safeParse(value.maxBodyBytes); + if (!parsedMaxBodyBytes.success) { + return { success: false, code: "MODULE_ROUTE_MAX_BODY_BYTES_INVALID" }; + } return { success: true, data: Object.freeze({ method: value.method, path: parsedPath.data, permission: parsedPermission.data, + maxBodyBytes: parsedMaxBodyBytes.data, handler: value.handler as ModuleRouteHandler, }), }; diff --git a/packages/contracts/tests/module-http.test.ts b/packages/contracts/tests/module-http.test.ts index ee281b1..90e1e45 100644 --- a/packages/contracts/tests/module-http.test.ts +++ b/packages/contracts/tests/module-http.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { isModuleHttpMethod, MODULE_HTTP_METHODS, + MODULE_ROUTE_MAX_BODY_BYTES_CEILING, type ModuleRouteDefinitionInput, moduleRoutePathSchema, moduleRouteShapeKey, @@ -22,6 +23,7 @@ function validDefinition( method: "GET", path: "/events/:eventId", permission: "calendar.events.read", + maxBodyBytes: 0, handler: noopHandler, ...overrides, }; @@ -153,6 +155,7 @@ describe("safeParseModuleRouteDefinition", () => { method: "GET", path: "/events/:eventId", permission: "calendar.events.read", + maxBodyBytes: 0, handler: noopHandler, }); expect(Object.isFrozen(result.data)).toBe(true); @@ -216,6 +219,44 @@ describe("safeParseModuleRouteDefinition", () => { }); }); + it("accepts a maxBodyBytes of exactly zero (no body accepted) and exactly the platform ceiling", () => { + expect(safeParseModuleRouteDefinition(validDefinition({ maxBodyBytes: 0 })).success).toBe(true); + const atCeiling = safeParseModuleRouteDefinition( + validDefinition({ maxBodyBytes: MODULE_ROUTE_MAX_BODY_BYTES_CEILING }), + ); + expect(atCeiling.success).toBe(true); + if (atCeiling.success) { + expect(atCeiling.data.maxBodyBytes).toBe(MODULE_ROUTE_MAX_BODY_BYTES_CEILING); + } + }); + + it("rejects a negative, non-integer, non-finite, or over-ceiling maxBodyBytes", () => { + for (const maxBodyBytes of [ + -1, + 1.5, + Number.NaN, + Number.POSITIVE_INFINITY, + Number.NEGATIVE_INFINITY, + MODULE_ROUTE_MAX_BODY_BYTES_CEILING + 1, + ]) { + expect(safeParseModuleRouteDefinition(validDefinition({ maxBodyBytes }))).toEqual({ + success: false, + code: "MODULE_ROUTE_MAX_BODY_BYTES_INVALID", + }); + } + }); + + it("rejects a non-numeric or missing maxBodyBytes", () => { + expect( + safeParseModuleRouteDefinition({ ...validDefinition(), maxBodyBytes: "0" }), + ).toMatchObject({ success: false, code: "MODULE_ROUTE_MAX_BODY_BYTES_INVALID" }); + const { maxBodyBytes: _maxBodyBytes, ...withoutMaxBodyBytes } = validDefinition(); + expect(safeParseModuleRouteDefinition(withoutMaxBodyBytes)).toEqual({ + success: false, + code: "MODULE_ROUTE_DEFINITION_INVALID", + }); + }); + it("does not retain the caller's mutable definition object: mutating it afterward cannot change the parsed result", () => { const source = { ...validDefinition() }; const result = safeParseModuleRouteDefinition(source); diff --git a/packages/module-sdk/tests/define-module-runtime.test.ts b/packages/module-sdk/tests/define-module-runtime.test.ts index 4fd4106..fd004ee 100644 --- a/packages/module-sdk/tests/define-module-runtime.test.ts +++ b/packages/module-sdk/tests/define-module-runtime.test.ts @@ -48,6 +48,7 @@ describe("defineModuleRuntime", () => { method: "GET", path: "/records", permission: "example.records.read", + maxBodyBytes: 0, handler: () => new Response(null, { status: 204 }), }); }, diff --git a/templates/module/src/server/index.ts b/templates/module/src/server/index.ts index 0b00625..a9f84e9 100644 --- a/templates/module/src/server/index.ts +++ b/templates/module/src/server/index.ts @@ -21,6 +21,12 @@ export default defineModuleRuntime({ method: "GET", path: "/records", permission: "example.records.read", + // This route accepts no request body at all — a GET route never should. Every route must + // make this choice consciously: 0 means "no body accepted" (a non-empty declared or streamed + // body is rejected before this handler ever runs); a route that does accept a body declares + // its own explicit byte maximum instead, up to the platform's ceiling + // (`MODULE_ROUTE_MAX_BODY_BYTES_CEILING`, `@ekklesia/contracts`). + maxBodyBytes: 0, async handler(_request, context) { const rows = await context.services.database .select({ id: records.id, value: records.value }) diff --git a/templates/module/tests/manifest.test.ts b/templates/module/tests/manifest.test.ts index 4af5b8a..7564609 100644 --- a/templates/module/tests/manifest.test.ts +++ b/templates/module/tests/manifest.test.ts @@ -106,6 +106,9 @@ describe("module template", () => { method: "GET", path: "/records", permission: "example.records.read", + // The template's example route demonstrates the explicit no-body contract: a GET route + // accepts no request body at all. + maxBodyBytes: 0, }); });