fix: reject ambiguous module routes
A module can register both `GET /events/:id` and `GET /events/new`. Driven through the pinned router, `/events/new` reached the parameter handler under `demo.events.read` when the parameter route registered first, and the static handler under `demo.events.create` when it registered second. Nothing rejected the pair, so declaration order decided which permission guarded the path. That is a permission policy, not a routing preference, and it must not be inferred from array position. Two routes of one module now overlap when they share a 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 grammar — static literals or `:name` parameters, no wildcard, regex, optional, or catch-all — that is exactly "some concrete request path matches both". `moduleRoutesOverlap` implements it in the contracts package, where path semantics already live and Hono cannot reach. Core checks it during staged registration, after the existing duplicate check, so identical shapes (including paths differing only in parameter name) keep returning MODULE_ROUTE_DUPLICATE and a genuine intersection returns the new MODULE_ROUTE_OVERLAP. The rejected module publishes none of its staged routes or capabilities, no start hook runs, and no listener binds. Rejection rather than a specificity rule: ranking static above parameter would still silently pick which permission applies. Disjoint routes keep their order; different methods, different segment counts, and the same local path in two different modules (each has its own platform-owned mount prefix) all remain legal. The same requests also pinned down what handlers actually receive. Route declarations are validated; route *parameters* are not, and the two are unrelated. A single encoded segment arrives decoded as `/../admin`, `a\b`, an embedded NUL, CR/LF, `1; drop`, or non-ASCII text — none of which any declared path could contain. The contract now says so explicitly: decoded values are untrusted, module-owned input that a handler must validate against its own exact domain grammar before any database, filesystem, command, or identifier use, and re-encoding is not a substitute. No generic validator, path-normalization layer, or route field was added, because module parameters are module-owned domain types and one platform-wide pattern would either reject legitimate values or approve meaningless ones. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
ekklesia
co-authored by
Claude Opus 5
Claude Opus 5
parent
4a34a8203c
commit
800ecfdb92
@@ -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<Readonly<Record<string, string>>> = [];
|
||||
const handler = vi.fn<ModuleRouteHandler>(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<ModuleRouteHandler>(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<ModuleRouteHandler>(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<ModuleRouteHandler>(async () => Response.json({ handler: "byId" }));
|
||||
const create = vi.fn<ModuleRouteHandler>(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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user