diff --git a/packages/control-plane/src/router.authorization-audit.test.ts b/packages/control-plane/src/router.authorization-audit.test.ts index 6495c9f3c..ec0eb3bc3 100644 --- a/packages/control-plane/src/router.authorization-audit.test.ts +++ b/packages/control-plane/src/router.authorization-audit.test.ts @@ -3,8 +3,8 @@ import { BUILT_IN_ROLE_REGISTRY } from "@open-inspect/shared/rbac"; import type * as AuthenticateModule from "./auth/authenticate"; import type { Principal } from "./auth/principal"; import type { SqlDatabase, SqlStatement } from "./db/sql-database"; +import { Hono } from "hono"; import { - defineRoute, json, GITHUB_SANDBOX_FALLBACK_ROUTE, permissionRequirement, @@ -12,8 +12,9 @@ import { requireAutomation, requirePermission, serviceAuthorized, - type Route, } from "./routes/shared"; +import { admit } from "./routing/admit"; +import type { ControlPlaneHonoEnv } from "./routing/hono-env"; import { createTestRequestHandler, TEST_BACKGROUND_TASK_CONTEXT } from "./router.test-support"; const mocks = vi.hoisted(() => ({ authenticate: vi.fn() })); @@ -23,109 +24,100 @@ vi.mock("./auth/authenticate", async (importOriginal) => ({ authenticate: mocks.authenticate, })); -const TEST_ROUTES: Route[] = [ - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/actorless-service", - authorization: requirePermission("sessions.lifecycle", { - actorlessGrants: [{ service: "github-bot" }], - }), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/user-only", - authorization: requirePermission("workspace.members.manage"), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/automations/:id/pause", - authorization: requireAutomation("manage"), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/managed", - authorization: requirePermission("workspace.members.manage"), - handler: async () => json({ handled: true }, 201), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "GET", - path: "/audit-test/managed", - authorization: requirePermission("workspace.members.manage"), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "GET", - path: "/audit-test/profiles", - authorization: requirePermission("skill_profiles.manage_own"), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "GET", - path: "/audit-test/read", - authorization: requirePermission("workspace.roles.read"), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/service-actor", - authorization: requirePermission("sessions.lifecycle"), - handler: async () => json({ handled: true }, 201), - } - ), - defineRoute( - { authentication: { kind: "service" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/service", - authorization: serviceAuthorized("github-bot", "required"), - handler: async () => json({ handled: true }), - } - ), - defineRoute( - { authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, - { - method: "POST", - path: "/audit-test/multi", - authorization: requireAll( - permissionRequirement("analytics.read"), - permissionRequirement("workspace.members.manage") - ), - handler: async () => json({ handled: true }), - } - ), - defineRoute(GITHUB_SANDBOX_FALLBACK_ROUTE, { - method: "POST", - path: "/audit-test/sessions/:id/upload", +const TEST_ROUTES = new Hono(); +TEST_ROUTES.post( + "/audit-test/actorless-service", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requirePermission("sessions.lifecycle", { + actorlessGrants: [{ service: "github-bot" }], + }), + }), + () => json({ handled: true }) +); +TEST_ROUTES.post( + "/audit-test/user-only", + admit({ + ...{ authentication: { kind: "user" }, supportedScmProviders: "all" }, + authorization: requirePermission("workspace.members.manage"), + }), + () => json({ handled: true }) +); +TEST_ROUTES.post( + "/audit-test/automations/:id/pause", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requireAutomation("manage"), + }), + () => json({ handled: true }) +); +TEST_ROUTES.post( + "/audit-test/managed", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requirePermission("workspace.members.manage"), + }), + () => json({ handled: true }, 201) +); +TEST_ROUTES.get( + "/audit-test/managed", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requirePermission("workspace.members.manage"), + }), + () => json({ handled: true }) +); +TEST_ROUTES.get( + "/audit-test/profiles", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requirePermission("skill_profiles.manage_own"), + }), + () => json({ handled: true }) +); +TEST_ROUTES.get( + "/audit-test/read", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requirePermission("workspace.roles.read"), + }), + () => json({ handled: true }) +); +TEST_ROUTES.post( + "/audit-test/service-actor", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requirePermission("sessions.lifecycle"), + }), + () => json({ handled: true }, 201) +); +TEST_ROUTES.post( + "/audit-test/service", + admit({ + ...{ authentication: { kind: "service" }, supportedScmProviders: "all" }, + authorization: serviceAuthorized("github-bot", "required"), + }), + () => json({ handled: true }) +); +TEST_ROUTES.post( + "/audit-test/multi", + admit({ + ...{ authentication: { kind: "user-or-service" }, supportedScmProviders: "all" }, + authorization: requireAll( + permissionRequirement("analytics.read"), + permissionRequirement("workspace.members.manage") + ), + }), + () => json({ handled: true }) +); +TEST_ROUTES.post( + "/audit-test/sessions/:id/upload", + admit({ + ...GITHUB_SANDBOX_FALLBACK_ROUTE, authorization: requirePermission("sessions.collaborate"), - handler: async () => json({ handled: true }, 201), }), -]; + () => json({ handled: true }, 201) +); interface AuditWrite { values: unknown[]; @@ -223,7 +215,7 @@ function auditRecord(write: AuditWrite) { }; } -const handleRequest = createTestRequestHandler(TEST_ROUTES); +const handleRequest = createTestRequestHandler([TEST_ROUTES]); beforeEach(() => { mocks.authenticate.mockReset(); diff --git a/packages/control-plane/src/router.test-support.ts b/packages/control-plane/src/router.test-support.ts index 85bd82ba5..98bc64cd3 100644 --- a/packages/control-plane/src/router.test-support.ts +++ b/packages/control-plane/src/router.test-support.ts @@ -9,13 +9,12 @@ import { buildServiceAuthHeaders, type ServiceName } from "@open-inspect/shared/service-auth"; import type { BackgroundTasks } from "./platform-ports"; import { createTestBackgroundTasks } from "./background-tasks.test-support"; -import { Hono } from "hono"; import { BUILT_IN_ROLE_REGISTRY, type PermissionId } from "@open-inspect/shared/rbac"; import type { SqlDatabase, SqlStatement } from "./db/sql-database"; -import { cloudflareHost, createControlPlaneApp, type RouteCatalogEntry } from "./routing/hono-app"; +import { cloudflareHost, createControlPlaneApp, type RouteModule } from "./routing/hono-app"; import { listRouteContracts, type RouteContract } from "./routing/route-contracts"; import { catalog } from "./routes/catalog"; -import type { Route, RouteParams } from "./routes/shared"; +import type { RouteParams } from "./routes/shared"; import type { Env } from "./types"; // The single contract-faithful double lives in background-tasks.test-support; @@ -40,15 +39,13 @@ function executionContextFromBackgroundTasks(tasks: BackgroundTasks): ExecutionC } /** - * Test-only adapter over an explicit catalog, through the production host. + * Test-only adapter over explicit route modules, through the production host. * Hono registers routes when the app is built, so fixtures that need - * synthetic routes construct their own handler instead of mutating the + * synthetic routes build their own module instead of mutating the * production catalog. */ -export function createTestRequestHandler( - entries: readonly RouteCatalogEntry[] -): TestRequestHandler { - const app = createControlPlaneApp(entries, cloudflareHost); +export function createTestRequestHandler(modules: readonly RouteModule[]): TestRequestHandler { + const app = createControlPlaneApp(modules, cloudflareHost); return (request, env, backgroundTasks) => Promise.resolve(app.fetch(request, env, executionContextFromBackgroundTasks(backgroundTasks))); } @@ -61,11 +58,6 @@ export const routeContracts: readonly RouteContract[] = listRouteContracts( createControlPlaneApp(catalog, cloudflareHost) ); -/** The catalog entries still registered through the legacy adapter. */ -export function legacyRoutes(entries: readonly RouteCatalogEntry[] = catalog): Route[] { - return entries.filter((entry): entry is Route => !(entry instanceof Hono)); -} - /** The production contract selected for a concrete method and path. */ export function contractFor(method: string, path: string): RouteContract | undefined { return routeContracts.find( @@ -143,21 +135,21 @@ export function ownerAuthorizationDatabase(userId = TEST_USER_ID): SqlDatabase { return authorizationDatabase({ userId }); } -/** Compile a catalog path into the legacy raw-path matcher, for handler-level fixtures. */ +/** Compile a route path into a matcher over a concrete pathname, for handler-level fixtures. */ export function routePathPattern(path: string): RegExp { return new RegExp(`^${path.replace(/:(\w+)/g, "(?<$1>[^/]+)")}$`); } -/** Select the catalog route for a concrete path and rebuild what the adapter hands its handler. */ +/** Select the first contract for a concrete method and path, with the raw parameters it binds. */ export function matchRoute( entries: readonly Entry[], method: string, path: string -): { route: Entry; match: RegExpMatchArray; params: RouteParams } | undefined { +): { route: Entry; params: RouteParams } | undefined { for (const route of entries) { if (route.method !== method) continue; const match = path.match(routePathPattern(route.path)); - if (match) return { route, match, params: { ...match.groups } }; + if (match) return { route, params: { ...match.groups } }; } return undefined; } diff --git a/packages/control-plane/src/routes/catalog.ts b/packages/control-plane/src/routes/catalog.ts index c0609254f..96c6d0abb 100644 --- a/packages/control-plane/src/routes/catalog.ts +++ b/packages/control-plane/src/routes/catalog.ts @@ -5,7 +5,7 @@ * and parameterized paths. */ -import type { RouteCatalogEntry } from "../routing/hono-env"; +import type { RouteModule } from "../routing/hono-env"; import { webhookRoutes } from "../webhooks"; import { analyticsRoutes } from "./analytics"; import { auditEventRoutes } from "./audit-events"; @@ -31,11 +31,8 @@ import { slackNotifyRoutes } from "./slack-notify"; import { signInProviderRoutes } from "./sign-in-providers"; import { skillRoutes } from "./skills"; -/** - * Registration order is the precedence order. A Hono sub-app is mounted where - * it appears; a legacy route is registered through the catalog adapter. - */ -export const catalog: RouteCatalogEntry[] = [ +/** Registration order is the precedence order: each module is mounted where it appears. */ +export const catalog: readonly RouteModule[] = [ healthRoutes, browserAuthRoutes, diff --git a/packages/control-plane/src/routes/repository-params.test.ts b/packages/control-plane/src/routes/repository-params.test.ts new file mode 100644 index 000000000..68251da24 --- /dev/null +++ b/packages/control-plane/src/routes/repository-params.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from "vitest"; +import { repositoryParams } from "./repository-params"; + +describe("repositoryParams", () => { + it("accepts a nested owner namespace Hono decoded from one segment", () => { + expect(repositoryParams({ owner: "group/subgroup", name: "web" })).toEqual({ + owner: "group/subgroup", + name: "web", + }); + }); + + it("rejects a slash in the repository name with the route's 400", async () => { + const result = repositoryParams({ owner: "group", name: "web/api" }); + + expect(result).toBeInstanceOf(Response); + expect((result as Response).status).toBe(400); + await expect((result as Response).json()).resolves.toEqual({ + error: "Owner and name must be valid repository path segments", + }); + }); +}); diff --git a/packages/control-plane/src/routes/shared.test.ts b/packages/control-plane/src/routes/shared.test.ts deleted file mode 100644 index b07f75b21..000000000 --- a/packages/control-plane/src/routes/shared.test.ts +++ /dev/null @@ -1,28 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { extractRepoParams } from "./shared"; -import { routePathPattern } from "../router.test-support"; - -describe("repository route parameters", () => { - it("decodes a nested owner namespace from one URL segment", () => { - const match = "/repos/group%2Fsubgroup/web/branches".match( - routePathPattern("/repos/:owner/:name/branches") - ); - - expect(match).not.toBeNull(); - expect(extractRepoParams(match!)).toEqual({ owner: "group/subgroup", name: "web" }); - }); - - it("rejects an encoded slash in the repository name", async () => { - const match = "/repos/group/web%2Fapi/branches".match( - routePathPattern("/repos/:owner/:name/branches") - ); - - const result = extractRepoParams(match!); - - expect(result).toBeInstanceOf(Response); - expect((result as Response).status).toBe(400); - await expect((result as Response).json()).resolves.toEqual({ - error: "Owner and name must be valid repository path segments", - }); - }); -}); diff --git a/packages/control-plane/src/routes/shared.ts b/packages/control-plane/src/routes/shared.ts index f1e240cba..05ab4be79 100644 --- a/packages/control-plane/src/routes/shared.ts +++ b/packages/control-plane/src/routes/shared.ts @@ -2,7 +2,6 @@ * Shared route primitives used by all route modules. */ -import { decodeRepositoryPathSegments } from "@open-inspect/shared/types/repositories"; import type { Principal } from "../auth/principal"; import type { RequestContext } from "../http/request-context"; import { error, HttpError } from "../http/responses"; @@ -37,23 +36,6 @@ export type ServiceActorClaimsResult = | { kind: "claims"; claims: ServiceActorProfileClaims } | { kind: "rejected"; response: Response }; -/** Route matching, authorization, and handler configuration. */ -export interface RouteDefinition { - method: string; - path: string; - /** Authorization policy enforced before the handler runs. */ - authorization: RouteAuthorization; - /** - * Extract profile claims asserted by the trusted service that owns this - * route. Authentication has already verified the exact request body before - * this hook runs. Invalid route input returns the route's own rejection so - * admission stops before any identity is written. - */ - serviceActorClaims?: (request: Request, ctx: RequestContext) => Promise; - cacheControl?: "no-store" | "private, no-store"; - handler: (request: Request, env: Env, match: RegExpMatchArray, ctx: Context) => Promise; -} - /** One permission or resource-admission requirement for an active user. */ export type RouteAuthorizationRequirement = | { kind: "permission"; permission: PermissionId } @@ -286,14 +268,18 @@ export interface RoutePolicy { supportedScmProviders: "all" | readonly SourceControlProviderName[]; } -/** Fully resolved route: a definition combined with the policy it was declared under. */ -export interface Route extends RouteDefinition, RoutePolicy {} - /** Framework-neutral policy consumed by request admission. */ -export type RouteAdmissionPolicy = Pick< - Route, - "authentication" | "authorization" | "serviceActorClaims" | "supportedScmProviders" ->; +export interface RouteAdmissionPolicy extends RoutePolicy { + /** Authorization policy enforced before the handler runs. */ + authorization: RouteAuthorization; + /** + * Extract profile claims asserted by the trusted service that owns this + * route. Authentication has already verified the exact request body before + * this hook runs. Invalid route input returns the route's own rejection so + * admission stops before any identity is written. + */ + serviceActorClaims?: (request: Request, ctx: RequestContext) => Promise; +} const SESSION_ID_BINDING: SandboxSessionBinding = { getSessionId: (params) => params.id ?? null, @@ -349,26 +335,6 @@ export const SCM_AGNOSTIC_SANDBOX_ROUTE = { supportedScmProviders: "all", } as const satisfies RoutePolicy; -export function defineRoutes( - policy: Policy, - routes: RouteDefinition>[] -): Route[] { - return routes.map((route) => defineRoute(policy, route)); -} - -export function defineRoute( - policy: Policy, - route: RouteDefinition> -): Route { - const handler: Route["handler"] = (request, env, match, ctx) => - route.handler(request, env, match, ctx as RouteContext); - return { - ...route, - ...policy, - handler, - }; -} - /** * Create a SourceControlProvider for use in Worker-level route handlers. * Cheap to construct (no I/O), so creating per-request is fine. @@ -403,25 +369,6 @@ export async function parseJsonBody(request: Request): Promise } } -/** - * Extract `owner` and `name` named groups from a route match, returning - * the pair or an error Response when either is missing. - */ -export function extractRepoParams( - match: RegExpMatchArray -): { owner: string; name: string } | Response { - const encodedOwner = match.groups?.owner; - const encodedName = match.groups?.name; - if (!encodedOwner || !encodedName) { - return error("Owner and name are required", 400); - } - const repository = decodeRepositoryPathSegments(encodedOwner, encodedName); - if (!repository) { - return error("Owner and name must be valid repository path segments", 400); - } - return { owner: repository.repoOwner, name: repository.repoName }; -} - /** * Resolve a repository via the SCM provider, returning the full * {@link RepositoryAccessResult} or raising an HttpError. diff --git a/packages/control-plane/src/routing/admit.ts b/packages/control-plane/src/routing/admit.ts index b9378e8d2..c62aab57a 100644 --- a/packages/control-plane/src/routing/admit.ts +++ b/packages/control-plane/src/routing/admit.ts @@ -10,7 +10,6 @@ import type { RouteAdmissionPolicy, RouteAuthentication, RouteContext, - RouteDefinition, RouteParams, } from "../routes/shared"; import type { Env } from "../types"; @@ -19,7 +18,9 @@ import { admitRoute, type RouteAdmissionResult } from "./route-admission"; import { rawRouteParams } from "./route-params"; /** Everything admission and response policy need to know about a route. */ -export type AdmissionPolicy = RouteAdmissionPolicy & Pick; +export type AdmissionPolicy = RouteAdmissionPolicy & { + cacheControl?: "no-store" | "private, no-store"; +}; /** The evaluated policy for the current request, read by the lifecycle. */ export interface RouteAdmission { diff --git a/packages/control-plane/src/routing/hono-app.test.ts b/packages/control-plane/src/routing/hono-app.test.ts index fb0c04a9a..e510d7723 100644 --- a/packages/control-plane/src/routing/hono-app.test.ts +++ b/packages/control-plane/src/routing/hono-app.test.ts @@ -2,17 +2,25 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { HttpError, json } from "../http/responses"; import { TEST_SERVICE_SECRETS } from "../router.test-support"; import { createTestBackgroundTasks } from "../background-tasks.test-support"; -import { defineRoute, NO_AUTHORIZATION, type Route } from "../routes/shared"; +import { NO_AUTHORIZATION } from "../routes/shared"; import type { RequestContext } from "../http/request-context"; import type { Env } from "../types"; import { Hono } from "hono"; -import { admit, dispatch } from "./admit"; -import { createControlPlaneApp, type ControlPlaneHonoEnv, type ControlPlaneHost } from "./hono-app"; +import { admit, type AdmissionPolicy, dispatch } from "./admit"; +import { + createControlPlaneApp, + type ControlPlaneHonoEnv, + type ControlPlaneHost, + type RouteModule, +} from "./hono-app"; const PUBLIC = { authentication: { kind: "public" }, supportedScmProviders: "all" } as const; -function publicRoute(path: string, handler: Route["handler"]): Route { - return defineRoute(PUBLIC, { method: "GET", path, authorization: NO_AUTHORIZATION, handler }); +/** A module with one public GET route, for lifecycle tests. */ +function publicRoute(path: string, handler: () => Promise): RouteModule { + const module = new Hono(); + module.get(path, admit({ ...PUBLIC, authorization: NO_AUTHORIZATION }), handler); + return module; } const tasks = createTestBackgroundTasks(); @@ -111,17 +119,23 @@ describe("control-plane Hono app lifecycle", () => { it("finalizes a failure inside admission with the route's response policy", async () => { const errors = vi.spyOn(console, "error").mockImplementation(() => {}); - const route: Route = { - ...publicRoute("/sessions/:id/tunnel-urls", async () => json({ ok: true })), - authentication: { - kind: "sandbox", - getSessionId: () => { - throw new Error("identity lookup failed"); + const module = new Hono(); + module.get( + "/sessions/:id/tunnel-urls", + admit({ + authentication: { + kind: "sandbox", + getSessionId: () => { + throw new Error("identity lookup failed"); + }, }, - }, - cacheControl: "no-store", - }; - const app = createControlPlaneApp([route], host); + supportedScmProviders: "all", + authorization: NO_AUTHORIZATION, + cacheControl: "no-store", + }), + () => json({ ok: true }) + ); + const app = createControlPlaneApp([module], host); const response = await app.fetch(new Request("https://cp.test/sessions/s-1/tunnel-urls"), env); expect(response.status).toBe(500); @@ -243,18 +257,12 @@ describe("control-plane Hono app lifecycle", () => { }); it("refuses to build a principal-less route that requires authorization", () => { - const route = { - ...publicRoute("/broken", async () => json({})), + const policy = { + ...PUBLIC, authorization: { kind: "authenticated", auditAllowed: false }, - } as Route; - expect(() => createControlPlaneApp([route], host)).toThrow( + } as AdmissionPolicy; + expect(() => admit(policy)).toThrow( "Route without a verified principal cannot require authorization" ); }); - - it("refuses a route path outside the literal-or-parameter grammar", () => { - expect(() => - createControlPlaneApp([publicRoute("/files/*", async () => json({}))], host) - ).toThrow("outside the supported grammar"); - }); }); diff --git a/packages/control-plane/src/routing/hono-app.ts b/packages/control-plane/src/routing/hono-app.ts index 7ee64e075..f6168730c 100644 --- a/packages/control-plane/src/routing/hono-app.ts +++ b/packages/control-plane/src/routing/hono-app.ts @@ -1,6 +1,6 @@ /** Hono application for ordinary control-plane HTTP requests. */ -import { Hono, type Handler } from "hono"; +import { Hono } from "hono"; import type { RouterRoute } from "hono/types"; import { TrieRouter } from "hono/router/trie-router"; import { @@ -13,15 +13,12 @@ import type { RequestContext } from "../http/request-context"; import { error, HttpError } from "../http/responses"; import { createLogger } from "../logger"; import { catalog } from "../routes/catalog"; -import type { Route, RouteParams } from "../routes/shared"; import type { Env } from "../types"; -import { admit } from "./admit"; -import { rawRouteParams } from "./route-params"; import type { ControlPlaneHonoEnv, ControlPlaneHost, PlatformExecutionContext, - RouteCatalogEntry, + RouteModule, } from "./hono-env"; import { finalizeRouteResponse, logRequest, withCorsAndTraceHeaders } from "./request-lifecycle"; @@ -29,7 +26,6 @@ export type { ControlPlaneHonoEnv, ControlPlaneHost, PlatformExecutionContext, - RouteCatalogEntry, RouteModule, } from "./hono-env"; @@ -43,9 +39,9 @@ export type ControlPlaneHttpHandler = ( const logger = createLogger("router"); /** - * Hono gives `*`, `?`, `{...}` and `.` routing meaning, and raw parameter - * segments are read back from the pathname by position, so a path may hold - * only literal and `:param` segments. + * Hono gives `*`, `?`, `{...}` and `.` routing meaning, and admission reads + * raw parameter segments back from the pathname by position, so a path may + * hold only literal and `:param` segments. */ const ROUTE_PATH_GRAMMAR = /^(\/([A-Za-z0-9_-]+|:\w+))+$/; @@ -86,15 +82,10 @@ function assertModuleAdmits(module: Hono): void { } } -/** Mount a module or register a legacy route, refusing either before it can serve a request. */ -function register(app: Hono, entry: RouteCatalogEntry): void { - if (entry instanceof Hono) { - assertModuleAdmits(entry); - app.route("/", entry); - return; - } - assertRoutePath(entry.method, entry.path); - app.on(entry.method, entry.path, admit(entry), legacy(entry)); +/** Mount a module, refusing it before it can serve a request. */ +function mount(app: Hono, module: RouteModule): void { + assertModuleAdmits(module); + app.route("/", module); } /** The execution context the platform passed to `app.fetch`, if any. */ @@ -108,31 +99,6 @@ function executionContextOf(c: { } } -/** Rebuild the legacy `RegExpMatchArray` handlers still read from raw parameters. */ -function legacyMatch(pathname: string, params: RouteParams): RegExpMatchArray { - const match = [pathname, ...Object.values(params)] as unknown as RegExpMatchArray; - match.index = 0; - match.input = pathname; - match.groups = { ...params }; - return match; -} - -/** Run a catalog handler with the request and context admission produced. */ -function legacy(route: Route): Handler { - return (c) => { - const admission = c.get("admission"); - if (admission?.result.kind !== "admitted") { - throw new Error(`Handler reached without admission: ${route.method} ${route.path}`); - } - return route.handler( - admission.result.handlerRequest, - c.env, - legacyMatch(c.req.path, rawRouteParams(c.req.routePath, c.req.path)), - c.get("requestContext") - ); - }; -} - /** * Replace the response once the handler chain has finished. Hono's setter * merges the previous response's headers into the new one, so clear it @@ -147,7 +113,7 @@ function replaceResponse( } /** - * Build the Hono application over a route catalog. + * Build the Hono application over route modules, mounted in precedence order. * * The lifecycle middleware owns everything around a route: the DB and HEAD * guards, the request context, the request log, authorization audit, and @@ -155,7 +121,7 @@ function replaceResponse( * handler that answers without admission having run is refused. */ export function createControlPlaneApp( - entries: readonly RouteCatalogEntry[], + modules: readonly RouteModule[], host: ControlPlaneHost ): Hono { const app = new Hono({ @@ -265,7 +231,7 @@ export function createControlPlaneApp( }); }); - for (const entry of entries) register(app, entry); + for (const module of modules) mount(app, module); app.notFound((c) => { c.set("admissionExempt", true); @@ -312,11 +278,11 @@ export const cloudflareHost: ControlPlaneHost = { }, }; -/** Build the Worker's ordinary HTTP entrypoint over a route catalog. */ +/** Build the Worker's ordinary HTTP entrypoint over route modules. */ export function createControlPlaneHttpHandler( - entries: readonly RouteCatalogEntry[] + modules: readonly RouteModule[] ): ControlPlaneHttpHandler { - const app = createControlPlaneApp(entries, cloudflareHost); + const app = createControlPlaneApp(modules, cloudflareHost); return (request, env, executionCtx) => Promise.resolve(app.fetch(request, env, executionCtx)); } diff --git a/packages/control-plane/src/routing/hono-env.ts b/packages/control-plane/src/routing/hono-env.ts index 16f09eac3..976cea1dd 100644 --- a/packages/control-plane/src/routing/hono-env.ts +++ b/packages/control-plane/src/routing/hono-env.ts @@ -3,7 +3,6 @@ import type { Hono } from "hono"; import type { RequestContext } from "../http/request-context"; import type { BackgroundTasks } from "../platform-ports"; -import type { Route } from "../routes/shared"; import type { Env } from "../types"; import type { AdmissionPolicy, RouteAdmission } from "./admit"; @@ -34,6 +33,3 @@ export interface ControlPlaneHost { /** A route module: a Hono sub-app whose every route is registered behind `admit()`. */ export type RouteModule = Hono; - -/** What the catalog lists, in precedence order: converted modules and, until they convert, legacy routes. */ -export type RouteCatalogEntry = Route | RouteModule; diff --git a/packages/control-plane/src/routing/request-lifecycle.test.ts b/packages/control-plane/src/routing/request-lifecycle.test.ts index 302749ef5..1f1b74414 100644 --- a/packages/control-plane/src/routing/request-lifecycle.test.ts +++ b/packages/control-plane/src/routing/request-lifecycle.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import type { Principal } from "../auth/principal"; import type { RequestContext } from "../http/request-context"; -import type { Route } from "../routes/shared"; +import type { AdmissionPolicy } from "./admit"; import { logPrincipal, logRequest, @@ -22,8 +22,10 @@ function requestContext(metrics: Record = {}): RequestContext { } as unknown as RequestContext; } -function route(cacheControl?: Route["cacheControl"]): Route { - return { cacheControl } as Route; +function route( + cacheControl?: AdmissionPolicy["cacheControl"] +): Pick { + return { cacheControl }; } function loggedEvents(spy: ReturnType): Array> { diff --git a/packages/control-plane/src/routing/request-lifecycle.ts b/packages/control-plane/src/routing/request-lifecycle.ts index 39f3e341e..c1f36fcc5 100644 --- a/packages/control-plane/src/routing/request-lifecycle.ts +++ b/packages/control-plane/src/routing/request-lifecycle.ts @@ -1,7 +1,7 @@ import type { Principal } from "../auth/principal"; import type { RequestContext } from "../http/request-context"; import { createLogger } from "../logger"; -import type { Route } from "../routes/shared"; +import type { AdmissionPolicy } from "./admit"; const logger = createLogger("router"); @@ -31,7 +31,7 @@ export function withCorsAndTraceHeaders(response: Response, ctx: RequestContext) /** Apply all matched-route response policy in one body-preserving reconstruction. */ export function finalizeRouteResponse( response: Response, - route: Pick, + route: Pick, ctx: RequestContext ): Response { return withCommonHeaders( diff --git a/packages/control-plane/src/routing/route-contracts.test.ts b/packages/control-plane/src/routing/route-contracts.test.ts index 513ce7b44..ec5c9aef2 100644 --- a/packages/control-plane/src/routing/route-contracts.test.ts +++ b/packages/control-plane/src/routing/route-contracts.test.ts @@ -1,7 +1,7 @@ import { Hono } from "hono"; import { describe, expect, it } from "vitest"; import { createTestBackgroundTasks } from "../background-tasks.test-support"; -import { defineRoute, json, NO_AUTHORIZATION, requirePermission } from "../routes/shared"; +import { json, NO_AUTHORIZATION, requirePermission } from "../routes/shared"; import { admit } from "./admit"; import { createControlPlaneApp, type ControlPlaneHonoEnv, type ControlPlaneHost } from "./hono-app"; import { listRouteContracts } from "./route-contracts"; @@ -10,7 +10,7 @@ const PUBLIC = { authentication: { kind: "public" }, supportedScmProviders: "all const host: ControlPlaneHost = { backgroundTasks: () => createTestBackgroundTasks() }; describe("listRouteContracts", () => { - it("lists modules and legacy routes in registration order with their policies", () => { + it("lists module routes in registration order with their policies", () => { const module = new Hono(); module.get("/module/:id", admit({ ...PUBLIC, authorization: NO_AUTHORIZATION }), () => json({}) @@ -25,22 +25,14 @@ describe("listRouteContracts", () => { }), () => json({}) ); - const legacy = defineRoute(PUBLIC, { - method: "GET", - path: "/legacy", - authorization: NO_AUTHORIZATION, - handler: async () => json({}), - }); - - const app = createControlPlaneApp([legacy, module], host); + const app = createControlPlaneApp([module], host); const contracts = listRouteContracts(app); expect(contracts.map((contract) => `${contract.method} ${contract.path}`)).toEqual([ - "GET /legacy", "GET /module/:id", "POST /module/:id", ]); - expect(contracts[2]).toMatchObject({ + expect(contracts[1]).toMatchObject({ authentication: { kind: "user" }, supportedScmProviders: ["github"], authorization: { kind: "active-user" }, diff --git a/packages/control-plane/src/routing/route-params.ts b/packages/control-plane/src/routing/route-params.ts index 9e3617e6b..0ffa4cd77 100644 --- a/packages/control-plane/src/routing/route-params.ts +++ b/packages/control-plane/src/routing/route-params.ts @@ -3,11 +3,12 @@ import type { RouteParams } from "../routes/shared"; /** * Path parameters as the raw, undecoded segments of the request pathname. * - * Hono decodes `c.req.param()`, but handlers decode repository and member - * segments themselves, so the segments are read back from the pathname by - * position. The route is already selected, so the parameter and pathname - * segments line up as long as the path grammar admits only literal and - * `:param` segments; a mismatch is an invariant violation, not data. + * Hono decodes `c.req.param()` and leaves a segment it cannot decode as it + * arrived, so admission reads the raw segments back from the pathname by + * position to refuse malformed encoding. The route is already selected, so + * the parameter and pathname segments line up as long as the path grammar + * admits only literal and `:param` segments; a mismatch is an invariant + * violation, not data. */ export function rawRouteParams(routePath: string, pathname: string): RouteParams { const names = routePath.split("/"); diff --git a/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts b/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts index de2e8664b..0320a4c73 100644 --- a/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts +++ b/packages/control-plane/test/integration/hono-route-catalog-conformance.test.ts @@ -1,7 +1,11 @@ import { createExecutionContext, env } from "cloudflare:test"; import { describe, expect, it } from "vitest"; import { catalog } from "../../src/routes/catalog"; -import { NO_AUTHORIZATION, type Route } from "../../src/routes/shared"; +import { Hono } from "hono"; +import { NO_AUTHORIZATION } from "../../src/routes/shared"; +import { admit } from "../../src/routing/admit"; +import type { ControlPlaneHonoEnv } from "../../src/routing/hono-env"; +import { rawRouteParams } from "../../src/routing/route-params"; import { cloudflareHost, createControlPlaneApp, @@ -49,19 +53,24 @@ describe("Hono route catalog conformance", () => { // without thousands of snapshot-only formatting lines. expect(manifest.map((entry) => JSON.stringify(entry))).toMatchSnapshot(); - // A shadow catalog keeps the production method/path/order and replaces - // each policy with a public echo handler, so selection and raw captures - // are observed without mutating the production route objects. - const shadow: Route[] = routes.map((route, routeIndex) => ({ - ...route, + // A shadow module keeps the production method/path/order and replaces + // each policy with a public echo handler, so selection and the raw + // parameter read-back are observed without the production policies. + const shadow = new Hono(); + const ECHO = admit({ authentication: { kind: "public" }, - authorization: NO_AUTHORIZATION, - serviceActorClaims: undefined, supportedScmProviders: "all", - handler: async (_request, _env, match) => - Response.json({ identity: manifest[routeIndex].identity, groups: match.groups ?? {} }), - })); - const handle = createControlPlaneHttpHandler(shadow); + authorization: NO_AUTHORIZATION, + }); + for (const [routeIndex, route] of routes.entries()) { + shadow.on(route.method, route.path, ECHO, (c) => + Response.json({ + identity: manifest[routeIndex].identity, + groups: rawRouteParams(c.req.routePath, c.req.path), + }) + ); + } + const handle = createControlPlaneHttpHandler([shadow]); for (const [routeIndex, route] of routes.entries()) { const { identity: expectedIdentity, pathname, groups } = manifest[routeIndex]; diff --git a/packages/control-plane/test/integration/route-admission-matrix.test.ts b/packages/control-plane/test/integration/route-admission-matrix.test.ts index 50a6b6df2..0b3be7c67 100644 --- a/packages/control-plane/test/integration/route-admission-matrix.test.ts +++ b/packages/control-plane/test/integration/route-admission-matrix.test.ts @@ -21,7 +21,9 @@ import { listRouteContracts, type RouteContract } from "../../src/routing/route- import type { Env } from "../../src/types"; import { AutomationStore, type AutomationRow } from "../../src/db/automation-store"; import { catalog } from "../../src/routes/catalog"; -import type { Route } from "../../src/routes/shared"; +import { Hono } from "hono"; +import { admit } from "../../src/routing/admit"; +import type { ControlPlaneHonoEnv } from "../../src/routing/hono-env"; import { cleanD1Tables } from "./cleanup"; import { initSession, @@ -385,11 +387,15 @@ describe("route admission sentinel", { timeout: MATRIX_TIMEOUT_MS }, () => { sandboxSessionId: "", automationId: "", }; - const shadow: Route[] = routes.map((route) => ({ - ...route, - handler: async () => Response.json({ sentinel: `${route.method} ${route.path}` }), - })); - const handle = createControlPlaneHttpHandler(shadow); + // Every production contract, admitted by its own policy, in front of a + // sentinel handler. + const shadow = new Hono(); + for (const route of routes) { + shadow.on(route.method, route.path, admit(route), () => + Response.json({ sentinel: `${route.method} ${route.path}` }) + ); + } + const handle = createControlPlaneHttpHandler([shadow]); beforeAll(async () => { await cleanD1Tables(); @@ -514,27 +520,31 @@ describe("route admission sentinel", { timeout: MATRIX_TIMEOUT_MS }, () => { } }); - it("delivers raw path segments to handlers", async () => { - const echo: Route[] = routes.map((route) => ({ - ...route, - handler: async (_request, _env, match) => Response.json({ groups: { ...match.groups } }), - })); - const handleEcho = createControlPlaneHttpHandler(echo); + it("delivers path segments to handlers decoded exactly once", async () => { + // Every production contract, admitted by its own policy, in front of a + // handler that echoes the parameters Hono decoded. + const echo = new Hono(); + for (const route of routes) { + echo.on(route.method, route.path, admit(route), (c) => + Response.json({ groups: c.req.param() }) + ); + } + const handleEcho = createControlPlaneHttpHandler([echo]); const cases: Array<{ method: string; url: string; groups: Record }> = [ { method: "GET", url: `${BASE}/sessions/abc%2Fdef`, - groups: { id: "abc%2Fdef" }, + groups: { id: "abc/def" }, }, { method: "GET", url: `${BASE}/repos/group%2Fsubgroup/web%252Fapp/secrets`, - groups: { owner: "group%2Fsubgroup", name: "web%252Fapp" }, + groups: { owner: "group/subgroup", name: "web%2Fapp" }, }, { method: "PUT", url: `${BASE}/members/${"1".repeat(31)}%2531/role`, - groups: { id: `${"1".repeat(31)}%2531` }, + groups: { id: `${"1".repeat(31)}%31` }, }, ];