diff --git a/apps/api/tests/module-http-routes.test.ts b/apps/api/tests/module-http-routes.test.ts index 2b95461..73a5334 100644 --- a/apps/api/tests/module-http-routes.test.ts +++ b/apps/api/tests/module-http-routes.test.ts @@ -5,6 +5,7 @@ import { type ModuleRouteHandler, type ModuleRouteHandlerContext, type ModuleRuntimeServices, + moduleRoutesOverlap, parsePermissionId, type PermissionId, type RegisteredModuleHttpRoute, @@ -1454,3 +1455,153 @@ describe("module routes and the rest of the API", () => { expect(response.status).toBe(200); }); }); + +/** + * Pass 4: the decoded-parameter boundary and the routing ambiguity that made it dangerous. + * + * These drive the pinned Hono matcher with real requests rather than asserting what a mock returns, + * because the whole point is what the real router actually hands a module handler. + */ +describe("module route parameters are decoded, untrusted input", () => { + function paramsApp() { + const seen: Array>> = []; + const handler = vi.fn(async (_request, context) => { + seen.push(context.params); + return Response.json({ id: context.params.id }); + }); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: readPermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + method: "GET", + moduleId: "calendar", + path: "/items/:id", + permission: readPermission, + }), + ], + }); + return { app, handler, seen }; + } + + it.each([ + // A single encoded segment can carry characters the declaration grammar forbids outright. + ["an encoded path separator and dot segments", "%2F..%2Fadmin", "/../admin"], + ["an encoded backslash", "a%5Cb", "a\\b"], + ["an embedded NUL", "a%00b", "a\u0000b"], + ["encoded CR/LF", "a%0D%0Ab", "a\r\nb"], + ["a percent sign", "100%25", "100%"], + ["non-ASCII text", "%C3%A9v%C3%A9nement", "événement"], + ["a statement separator", "1%3B%20drop", "1; drop"], + ])("delivers %s decoded, exactly as the router produced it", async (_label, encoded, decoded) => { + const { app, seen } = paramsApp(); + + const response = await app.request(`/api/v1/modules/calendar/items/${encoded}`); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ id: decoded }); + // The declared path grammar (`/items/:id`) permits none of these characters — validating the + // *declaration* says nothing about what a request may send, which is exactly why a handler + // must validate the decoded value against its own domain grammar before using it. + expect(seen[0]?.id).toBe(decoded); + expect(Object.isFrozen(seen[0])).toBe(true); + }); + + it("keeps parameter delivery behind authorization", async () => { + const handler = vi.fn(async () => new Response("never")); + const { app, evaluator } = buildModuleApp({ + evaluator: { authorize: vi.fn().mockResolvedValue({ allowed: false }) }, + registry: permissionRegistry([{ id: readPermission, baseline: "restricted" }]), + routes: [ + moduleRoute({ + handler, + method: "GET", + moduleId: "calendar", + path: "/items/:id", + permission: readPermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/items/%2F..%2Fadmin"); + + expect(response.status).toBe(403); + expect(evaluator.authorize).toHaveBeenCalled(); + // An unauthorized caller can never observe how the handler would treat a parameter, because + // the handler never runs. + expect(handler).not.toHaveBeenCalled(); + }); + + it("lets a handler reject a decoded value against its own domain grammar", async () => { + // The platform supplies no generic parameter validator; this is the module's own check, which + // is exactly the documented contract. + const canonicalId = /^[0-9a-f]{8}$/u; + const handler = vi.fn(async (_request, context) => { + const id = context.params.id; + if (id === undefined || !canonicalId.test(id)) { + return Response.json({ code: "INVALID_REQUEST" }, { status: 400 }); + } + return Response.json({ id }); + }); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: readPermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + method: "GET", + moduleId: "calendar", + path: "/items/:id", + permission: readPermission, + }), + ], + }); + + expect((await app.request("/api/v1/modules/calendar/items/deadbeef")).status).toBe(200); + const rejected = await app.request("/api/v1/modules/calendar/items/%2F..%2Fadmin"); + expect(rejected.status).toBe(400); + expect(await rejected.json()).toEqual({ code: "INVALID_REQUEST" }); + }); +}); + +describe("module route overlap can never reach mounting", () => { + it("would otherwise let registration order choose the governing permission", async () => { + // Both orders are mounted directly here — bypassing the runtime rejection — to show precisely + // what `MODULE_ROUTE_OVERLAP` prevents: the same request reaching a different handler *and* a + // different permission depending only on the order the routes were registered in. + const createPermission = parsePermissionId("calendar.events.create"); + const byId = vi.fn(async () => Response.json({ handler: "byId" })); + const create = vi.fn(async () => Response.json({ handler: "new" })); + const registry = permissionRegistry([ + { id: readPermission, baseline: "public" }, + { id: createPermission, baseline: "public" }, + ]); + const paramRoute = moduleRoute({ + handler: byId, + method: "GET", + moduleId: "calendar", + path: "/events/:id", + permission: readPermission, + }); + const staticRoute = moduleRoute({ + handler: create, + method: "GET", + moduleId: "calendar", + path: "/events/new", + permission: createPermission, + }); + + const parameterFirst = buildModuleApp({ registry, routes: [paramRoute, staticRoute] }); + const staticFirst = buildModuleApp({ registry, routes: [staticRoute, paramRoute] }); + + expect( + await (await parameterFirst.app.request("/api/v1/modules/calendar/events/new")).json(), + ).toEqual({ handler: "byId" }); + expect( + await (await staticFirst.app.request("/api/v1/modules/calendar/events/new")).json(), + ).toEqual({ handler: "new" }); + + // Core rejects this pair during staged registration, so no such snapshot can ever be published + // to `mountModuleHttpRoutes` — see `core/tests/module-runtime.test.ts`. + expect(moduleRoutesOverlap(paramRoute, staticRoute)).toBe(true); + }); +}); diff --git a/core/src/modules/runtime.ts b/core/src/modules/runtime.ts index f6ecd94..9395b65 100644 --- a/core/src/modules/runtime.ts +++ b/core/src/modules/runtime.ts @@ -12,6 +12,7 @@ import { type ModuleRuntimeDefinition, type ModuleRuntimeServices, moduleRouteShapeKey, + moduleRoutesOverlap, parseModuleRuntimeDefinition, type RegisteredCapability, type RegisteredModuleHttpRoute, @@ -37,6 +38,7 @@ export type ModuleRuntimeIssueCode = | "MODULE_ROUTE_DUPLICATE" | "MODULE_ROUTE_MAX_BODY_BYTES_INVALID" | "MODULE_ROUTE_METHOD_INVALID" + | "MODULE_ROUTE_OVERLAP" | "MODULE_ROUTE_PATH_INVALID" | "MODULE_ROUTE_PERMISSION_INVALID" | "MODULE_ROUTE_PERMISSION_UNDECLARED" @@ -331,6 +333,22 @@ class StagedModuleHttpRoutes { ), ); } + // Distinct from a duplicate: these are *different* routes that can both match at least one + // request path. With an order-sensitive matcher, whichever registered first would decide + // which permission governs that path — a permission-policy decision no routing order may + // make. Core rejects the ambiguity here instead of ranking by specificity, so the module + // publishes none of its staged routes and no request can reach an ambiguous match. + const overlapping = local.find((existing) => moduleRoutesOverlap(existing, parsed.data)); + if (overlapping) { + throw new ModuleRuntimeSignal( + runtimeIssue( + "MODULE_ROUTE_OVERLAP", + "register", + module.manifest.id, + `Module ${module.manifest.id} registered HTTP routes that can both match the same request path.`, + ), + ); + } shapes.add(shapeKey); local.push( Object.freeze({ diff --git a/core/tests/module-runtime.test.ts b/core/tests/module-runtime.test.ts index 239d22c..048e837 100644 --- a/core/tests/module-runtime.test.ts +++ b/core/tests/module-runtime.test.ts @@ -562,6 +562,141 @@ describe("module HTTP routes", () => { expect(runtimeIssueCodes(error)).toEqual(["MODULE_ROUTE_DUPLICATE"]); }); + it.each([ + ["parameter registered first", "/events/:id", "/events/new"], + ["static registered first", "/events/new", "/events/:id"], + ["multi-position, parameter first", "/events/:id/edit", "/events/new/:action"], + ["multi-position, static first", "/events/new/:action", "/events/:id/edit"], + ["parameters on both sides", "/a/:left/c", "/a/b/:right"], + ])("rejects routes that can both match one request path (%s) regardless of registration order", async (_label, firstPath, secondPath) => { + const modulesDirectory = await createModulesDirectory(); + // Deliberately *different* permissions: the defect this rejects is that whichever route + // registered first would decide which permission governs the ambiguous path. + await createRuntimeModule(modulesDirectory, "overlap", { + manifest: manifestFor("overlap", { + permissions: [ + permissionDeclaration("overlap.events.read"), + permissionDeclaration("overlap.events.create"), + ], + }), + serverSource: `export default { + id: "overlap", version: "0.1.0", + register({ http }) { + http.route({ method: "GET", path: "${firstPath}", permission: "overlap.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "${secondPath}", permission: "overlap.events.create", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + } + };`, + }); + + const error = await captureRuntimeError(modulesDirectory); + expect(runtimeIssueCodes(error)).toEqual(["MODULE_ROUTE_OVERLAP"]); + }); + + it.each([ + ["two different static literals", "/events/new", "/events/archive"], + ["different segment counts", "/events/:id", "/events/:id/edit"], + ["a different static prefix", "/events/:id", "/people/:id"], + ["module root against a one-segment path", "/", "/events"], + ])("still allows disjoint routes (%s) and preserves their registration order", async (_label, firstPath, secondPath) => { + const modulesDirectory = await createModulesDirectory(); + await createRuntimeModule(modulesDirectory, "disjoint", { + manifest: manifestFor("disjoint", { + permissions: [permissionDeclaration("disjoint.events.read")], + }), + serverSource: `export default { + id: "disjoint", version: "0.1.0", + register({ http }) { + http.route({ method: "GET", path: "${firstPath}", permission: "disjoint.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "${secondPath}", permission: "disjoint.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + } + };`, + }); + + const runtime = await startModuleRuntime({ + moduleSdkVersion: "0.1.0", + modulesDirectory, + platformVersion: "0.1.0", + services: fakeModuleRuntimeServices(), + }); + expect(runtime.routes.map((route) => route.path)).toEqual([firstPath, secondPath]); + await runtime.stop(); + }); + + it("allows the same overlapping local path under different methods", async () => { + const modulesDirectory = await createModulesDirectory(); + await createRuntimeModule(modulesDirectory, "methods", { + manifest: manifestFor("methods", { + permissions: [permissionDeclaration("methods.events.read")], + }), + serverSource: `export default { + id: "methods", version: "0.1.0", + register({ http }) { + http.route({ method: "GET", path: "/events/:id", permission: "methods.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "POST", path: "/events/new", permission: "methods.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + } + };`, + }); + + const runtime = await startModuleRuntime({ + moduleSdkVersion: "0.1.0", + modulesDirectory, + platformVersion: "0.1.0", + services: fakeModuleRuntimeServices(), + }); + expect(runtime.routes).toHaveLength(2); + await runtime.stop(); + }); + + it("allows two different modules to declare the same overlapping local paths", async () => { + const modulesDirectory = await createModulesDirectory(); + for (const moduleId of ["alpha", "beta"]) { + await createRuntimeModule(modulesDirectory, moduleId, { + manifest: manifestFor(moduleId, { + permissions: [permissionDeclaration(`${moduleId}.events.read`)], + }), + serverSource: `export default { + id: "${moduleId}", version: "0.1.0", + register({ http }) { + http.route({ method: "GET", path: "/events/:id", permission: "${moduleId}.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + } + };`, + }); + } + + // Each module has its own platform-owned mount prefix, so these can never collide. + const runtime = await startModuleRuntime({ + moduleSdkVersion: "0.1.0", + modulesDirectory, + platformVersion: "0.1.0", + services: fakeModuleRuntimeServices(), + }); + expect(runtime.routes.map((route) => route.moduleId).sort()).toEqual(["alpha", "beta"]); + await runtime.stop(); + }); + + it("publishes no route from a module whose registration was rejected for an overlap", async () => { + const modulesDirectory = await createModulesDirectory(); + await createRuntimeModule(modulesDirectory, "partial", { + manifest: manifestFor("partial", { + permissions: [permissionDeclaration("partial.events.read")], + }), + serverSource: `export default { + id: "partial", version: "0.1.0", + register({ http }) { + http.route({ method: "GET", path: "/healthy", permission: "partial.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/events/:id", permission: "partial.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + http.route({ method: "GET", path: "/events/new", permission: "partial.events.read", maxBodyBytes: 1024, handler: ${noopHandlerSource()} }); + }, + start() { throw new Error("start must never run for a rejected registration"); } + };`, + }); + + const error = await captureRuntimeError(modulesDirectory); + expect(runtimeIssueCodes(error)).toEqual(["MODULE_ROUTE_OVERLAP"]); + // The earlier, perfectly valid `/healthy` route is not published either: registration is + // all-or-nothing, and the start hook above would have thrown had it ever run. + }); + it("fails a malformed route with its specific structured issue code", async () => { const badMethodDirectory = await createModulesDirectory(); await createRuntimeModule(badMethodDirectory, "bad-method", { diff --git a/docs/module-development/README.md b/docs/module-development/README.md index dd9f51d..4ff5f8b 100644 --- a/docs/module-development/README.md +++ b/docs/module-development/README.md @@ -219,6 +219,65 @@ registration outright rather than being ignored. `method` is one of `GET`, `POST `DELETE`, always uppercase; there is no `OPTIONS` (the platform owns CORS preflight) and no wildcard/all-method registration. +### Two routes may not both match one request path + +Within one module and one HTTP method, two route patterns must never be able to match the same +request path. `/events/:id` and `/events/new` can — `/events/new` matches both — and that pair is +rejected during registration with `MODULE_ROUTE_OVERLAP`. So are `/events/:id/edit` against +`/events/new/:action`, and `/a/:left/c` against `/a/b/:right`. Two identical shapes, including paths +differing only in parameter name (`/events/:id` and `/events/:eventId`), keep the existing +`MODULE_ROUTE_DUPLICATE` result. + +This is rejected rather than resolved by a "static beats parameter" rule because the routes may +carry *different permissions*, and letting declaration order decide which one guards `/events/new` +would make a permission policy depend on the order lines appear in your `register` hook. A rejected +registration publishes **none** of that module's routes or capabilities and runs no start hook, so +the failure is loud at startup rather than subtle at request time. + +Still perfectly legal: two different static literals (`/events/new` and `/events/archive`), +different segment counts (`/events/:id` and `/events/:id/edit`), the same path under different +methods, and the same local path in two different modules — each module has its own +platform-owned mount prefix. Declaration order among disjoint routes is preserved exactly. + +### Route parameters are decoded, untrusted input — validate them yourself + +`context.params` holds the router's **decoded** path values. Decoding is not validation, and the +fact that your *declared* path passed the platform's path grammar says nothing about what a caller +may *send*. A single encoded segment routinely carries characters no declared path could: + +| Request segment | `context.params` value | +| --- | --- | +| `%2F..%2Fadmin` | `/../admin` | +| `a%5Cb` | `a\b` | +| `a%00b` | `a`, NUL, `b` | +| `a%0D%0Ab` | `a`, CR, LF, `b` | +| `1%3B%20drop` | `1; drop` | +| `%C3%A9v%C3%A9nement` | `événement` | + +**Validate every parameter you use against your own exact domain grammar** — a UUID, slug, date, +numeric identifier, or whatever your module owns — before any database, filesystem, command, +identifier, or other side-effecting use, and reject a missing, repeated, malformed, or semantically +invalid value with your own sanitized response: + +```ts +const eventId = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/u; + +async handler(request, context) { + const id = context.params.eventId; + if (id === undefined || !eventId.test(id)) { + return Response.json({ code: "INVALID_REQUEST" }, { status: 400 }); + } + // ...only now is `id` safe to use as a domain identifier. +} +``` + +Re-encoding the value, or a decode/re-encode round trip, is **not** a substitute for that check. The +platform deliberately supplies no generic parameter validator: your parameters are your domain +types, and one platform-wide pattern would either reject legitimate values or approve meaningless +ones. The platform continues to own authentication, authorization, `Origin`, CORS, body limits, +cache behavior, and uncaught-handler sanitization, and permission enforcement always completes +before your handler runs — an unauthorized caller can never reach your validation at all. + `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 diff --git a/packages/contracts/src/module-http.ts b/packages/contracts/src/module-http.ts index 9fccc6f..8208314 100644 --- a/packages/contracts/src/module-http.ts +++ b/packages/contracts/src/module-http.ts @@ -141,6 +141,49 @@ export function moduleRouteShapeKey(method: ModuleHttpMethod, path: ModuleRouteP return `${method} /${shapeSegments.join("/")}`; } +/** `"/"` is the module root and has no segments; every other canonical path is its `/`-separated + * segments. Takes an already-validated `ModuleRoutePath`. */ +function moduleRoutePathSegments(path: ModuleRoutePath): readonly string[] { + return path === "/" ? [] : path.slice(1).split("/"); +} + +/** + * Whether two routes of the same module can both match at least one request path. + * + * Two routes overlap when they share an HTTP method and a segment count, and at every position + * either both segments are the same static literal or at least one is a parameter. Under this + * route grammar — static kebab-case literals or `:name` parameters, with no wildcard, regex, + * optional, or catch-all segment — that condition is exactly "some concrete path matches both". + * + * This exists because an overlap is a *permission* ambiguity, not a routing preference: + * `GET /events/:id` and `GET /events/new` can carry different permissions, and with an + * order-sensitive matcher whichever registered first decides which permission governs + * `/events/new`. Core rejects the overlap at registration instead of picking a winner, so no + * router's matching order can ever select the governing permission. Identical shapes (including + * paths differing only in parameter name) remain the pre-existing duplicate result; callers check + * that first. + * + * Deliberately framework-neutral: path semantics already live in this package, and nothing about + * this rule may depend on Hono, which stays confined to `apps/api`. + */ +export function moduleRoutesOverlap( + first: { readonly method: ModuleHttpMethod; readonly path: ModuleRoutePath }, + second: { readonly method: ModuleHttpMethod; readonly path: ModuleRoutePath }, +): boolean { + if (first.method !== second.method) { + return false; + } + const firstSegments = moduleRoutePathSegments(first.path); + const secondSegments = moduleRoutePathSegments(second.path); + if (firstSegments.length !== secondSegments.length) { + return false; + } + return firstSegments.every((segment, index) => { + const other = secondSegments[index] as string; + return segment.startsWith(":") || other.startsWith(":") || segment === other; + }); +} + /** * What a module's route handler receives about the caller, the match, and the platform services * available to it — never an authorization-store, grant-management, storage, jobs, configuration, @@ -149,6 +192,28 @@ export function moduleRouteShapeKey(method: ModuleHttpMethod, path: ModuleRouteP * before ever invoking its handler; this contracts-layer module never invokes a handler itself. * `services` is the same frozen `ModuleRuntimeServices` object the owning module's `start`/`stop` * hooks receive for this API runtime — never a fresh one built per request. + * + * ## `params` values are untrusted input + * + * `params` holds the HTTP router's **decoded** path values, frozen alongside this context. Decoding + * is not validation, and the declaration grammar above constrains only what a module may *declare* + * — it says nothing about what a request may *send*. A caller can percent-encode anything into a + * single segment, so a decoded value routinely contains characters no declared path could: + * verified against the pinned router, `%2F..%2Fadmin` arrives as `/../admin`, `a%5Cb` as `a\b`, + * `a%00b` with an embedded NUL, `a%0D%0Ab` with CR/LF, and `%C3%A9v%C3%A9nement` as `événement`. + * + * Every handler must therefore validate each parameter it uses against its own exact domain + * grammar — a UUID, slug, date, numeric id, or whatever that module owns — **before** any + * database, filesystem, command, identifier, or other side-effecting use, and reject a missing, + * repeated, malformed, or semantically invalid value with its own sanitized response. Re-encoding, + * decode/re-encode round-tripping, or inspecting the raw URL is not a substitute for that check. + * The platform deliberately supplies no generic parameter grammar: module parameters are + * module-owned domain types, and one platform-wide pattern would either reject legitimate values + * or approve meaningless ones. + * + * The platform continues to own authentication, authorization, `Origin`, CORS, body limits, cache + * behavior, and uncaught-handler sanitization; permission enforcement always completes before a + * handler — and therefore any parameter validation it performs — can run. */ export interface ModuleRouteHandlerContext { readonly moduleId: ModuleId; diff --git a/packages/contracts/tests/module-http.test.ts b/packages/contracts/tests/module-http.test.ts index 90e1e45..4f2d3c2 100644 --- a/packages/contracts/tests/module-http.test.ts +++ b/packages/contracts/tests/module-http.test.ts @@ -6,6 +6,7 @@ import { type ModuleRouteDefinitionInput, moduleRoutePathSchema, moduleRouteShapeKey, + moduleRoutesOverlap, parseModuleRouteDefinition, parseModuleRoutePath, safeParseModuleRouteDefinition, @@ -294,3 +295,42 @@ describe("parseModuleRouteDefinition", () => { } }); }); + +describe("moduleRoutesOverlap", () => { + const route = (method: "GET" | "POST", path: string) => ({ + method, + path: parseModuleRoutePath(path), + }); + + it.each([ + ["a parameter against a static literal", "/events/:id", "/events/new"], + ["a later position", "/events/:id/edit", "/events/new/:action"], + ["parameters on both sides at different positions", "/a/:left/c", "/a/b/:right"], + ["identical parameter shapes with different names", "/events/:id", "/events/:eventId"], + ["identical static paths", "/events/new", "/events/new"], + ["root against root", "/", "/"], + ])("reports an overlap for %s", (_label, first, second) => { + expect(moduleRoutesOverlap(route("GET", first), route("GET", second))).toBe(true); + // The relation is symmetric, so registration order cannot change the verdict. + expect(moduleRoutesOverlap(route("GET", second), route("GET", first))).toBe(true); + }); + + it.each([ + ["two different static literals", "/events/new", "/events/archive"], + ["different segment counts", "/events/:id", "/events/:id/edit"], + ["a static prefix that differs earlier", "/events/:id", "/people/:id"], + ["root against a one-segment path", "/", "/events"], + ])("reports no overlap for %s", (_label, first, second) => { + expect(moduleRoutesOverlap(route("GET", first), route("GET", second))).toBe(false); + expect(moduleRoutesOverlap(route("GET", second), route("GET", first))).toBe(false); + }); + + it("never reports an overlap across different methods", () => { + expect(moduleRoutesOverlap(route("GET", "/events/:id"), route("POST", "/events/new"))).toBe( + false, + ); + expect(moduleRoutesOverlap(route("GET", "/events/new"), route("POST", "/events/new"))).toBe( + false, + ); + }); +});