test: pin raw path segments and scaffold route-policy completeness - #1720
Merged
Conversation
Guardrails ahead of the Hono shim retirement. The matrix suite now pins that percent-encoded segments reach admission and handlers undecoded, and that repository and RBAC handlers decode exactly once. A route-policy registry with a build-time completeness walk lands with its own tests; the production check is marked as an expected failure until admit() tags policies. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe routing API now exports its Hono app factory and environment type. A route-policy registry checks admission coverage. Integration tests cover encoded path matching and single-decoding validation. ChangesControl plane routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR adds path-segment regression coverage and a route-policy completeness scaffold without changing production request behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
PR #1720, test: pin raw path segments and scaffold route-policy completeness, by @ColeMurray changes 4 files (+256/-3). The encoded-segment coverage is strong, but the new completeness registry can accept a route whose admission middleware is unreachable, so the guardrail needs one correctness fix before it is relied on.
Critical Issues
- [Security/Correctness]
packages/control-plane/src/routing/route-policy-registry.ts:54- Policy presence is aggregated by method/path without considering execution order. An untagged handler can return before a later tagged middleware, while the route is still reported complete. Record policy as part of atomic route registration or otherwise verify that admission precedes every terminal handler.
Suggestions
None beyond the blocking inline finding.
Nitpicks
None.
Positive Feedback
- The integration cases pin raw, single-decoded, and double-encoded path behavior through both the real Worker and sentinel handlers.
- The registry tests cover mounted sub-apps,
app.use()exclusion, exemptions, and method/path isolation clearly. - Focused unit and integration tests pass, including the intentional expected failure, and control-plane typechecking passes.
Questions
None.
Verdict
Request Changes: Ensure the completeness check cannot credit admission middleware that is unreachable behind an earlier handler.
Contributor
There was a problem hiding this comment.
The encoded-path guardrails are focused and the changed files remain reasonably sized, but the proposed policy-completeness architecture is not sound enough to become a security boundary. Hono's public route list contains flattened handler records, not atomic route-registration chains. Aggregating those records by method and path can therefore certify an admission middleware that is not on the executable path, while handler wrapping can also erase the WeakMap identity. The cleaner move is to make policy part of the canonical native-route registration API so a route, its admission middleware, and its terminal handler are registered atomically instead of trying to infer that invariant afterward. The production check should also be a real, specific assertion rather than an expected failure that accepts any exception.
Requesting changes because the central guardrail can report an unprotected route as complete.
The first handler registered for a method+path must be the tagged policy, since Hono stops at the first handler that answers without calling next. Wrapped sub-app handlers are unwrapped before lookup, and the production check now asserts the exact untagged list instead of expecting any failure. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Completeness will be enforced at request time by the lifecycle middleware refusing any response that admission did not precede, so the registry and its build-time walk over app.routes are not needed. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
ColeMurray
added a commit
that referenced
this pull request
Sep 2, 2026
## Summary Second step of the series retiring the routing adapter left by #1716 (guardrails landed in #1720). This PR replaces the mechanism around every route while leaving all 171 route handlers untouched; after it, the remaining shim is one `legacy()` adapter function that later PRs delete module by module. **Admission is Hono middleware.** `admit(policy)` (`src/routing/admit.ts`) evaluates the route's declarative policy through the unchanged `admitRoute()` pipeline and sets `c.var.admission`. Denials answer from the middleware, with an authorization denial audited before anything is logged, as before. A principal-less policy that requires authorization is refused when the middleware is built. **The lifecycle is one `app.use("*")`** (`src/routing/hono-app.ts`) that owns what `dispatchMatchedRoute` and the outer handler used to: the DB guard (undecorated 503), the HEAD guard (404, never Hono's implicit GET), the request context and start time, the request log, the audit of allowed decisions after the handler including the 500 path, and the common response headers with the route's `Cache-Control`. `app.onError` maps `HttpError` to the `{ error }` envelope and logs anything else as the sanitized 500; a non-`Error` throw takes the same path. **Default deny.** If a handler answers without `admit()` having run ahead of it, the lifecycle replaces the response with a sanitized 500, drops the handler's headers, and logs `router.unadmitted_response`. Every policy, including `public`, sets the admission variable, so a route registered without `admit()` fails closed on its first request; the matrix suite drives every route in CI. This is the request-time check chosen in review of #1720 over the build-time registry. **Host-injected entrypoint.** `createControlPlaneApp(catalog, host)` takes a host whose one job is to build the `BackgroundTasks` port from whatever the platform passed as the execution context. `cloudflareHost` wraps `waitUntil`; the unit-test host hands the port straight through, which deletes the fake `ExecutionContext` in `router.test-support.ts`. `handleControlPlaneHttp` and `createControlPlaneHttpHandler` keep their signatures, so `index.ts` is unchanged. **Deleted:** `Route.pattern`, `parsePattern`, the raw-path re-match and its `router.match_mismatch` branch, `route-dispatch.ts`, the start-time `WeakMap`. Admission now takes `params: RouteParams` (raw segments read back from the pathname by position, `src/routing/route-params.ts`) instead of a `RegExpMatchArray`, and `legacyMatch()` rebuilds the array handlers still read from those params. **Test changes.** Unit tests that selected routes by `route.pattern` use `matchRoute()` / `routePathPattern()` from test support instead; no assertions changed. The conformance snapshot drops the `pattern` field from each of its 171 rows (identities, groups, policies, and order verified identical before and after). New `hono-app.test.ts` covers the default deny (including header non-leakage), preflight and 404 exemptions, HEAD, `HttpError` mapping, unexpected and non-`Error` throws, and both build-time refusals. ## Behavior notes - No status, body, header, or log-ordering change on any route. The four route-matrix snapshots are byte-identical. - The request-duration clock now starts in the lifecycle middleware, after Hono selects the route, rather than before `app.fetch`. The difference is the router lookup. - Parameter values handlers see are still raw. Decoding by default arrives with the module conversions. ## Verification - Unit: 232 files, 3,464 passed. - Integration (workerd, real D1): 96 files, 1,128 passed. - Typecheck (`src`, `tsconfig.test.json`, `test/integration`), ESLint, and Prettier clean. https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Updated request routing and authorization handling for more consistent policy enforcement. * Improved handling of denied requests, unexpected errors, unsupported methods, and route-related failures. * Strengthened route validation, including authorization requirements and duplicate parameter detection. * Improved request lifecycle behavior, including logging, trace identifiers, response finalization, and CORS preflight handling. * **Bug Fixes** * Improved extraction and handling of route parameters, including sandbox session-related requests. * Ensured unexpected handler failures return a consistent server error response with the appropriate headers and response policy. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ColeMurray
added a commit
that referenced
this pull request
Sep 3, 2026
## Summary Third step of the series retiring the routing adapter (#1720 guardrails, #1721 mechanism). This PR converts the first six route modules to native Hono sub-apps and establishes the pattern the remaining module PRs follow. 12 routes convert; the other 159 still go through the catalog adapter. **A converted module** is a `Hono` sub-app whose routes register with `admit(policy)` as their first middleware and a native handler behind it: ```ts export const keyboardShortcutRoutes = new Hono<ControlPlaneHonoEnv>(); keyboardShortcutRoutes.get( "/keyboard-shortcuts", admit({ ...SCM_AGNOSTIC_HUMAN_USER_ROUTE, authorization: ACTIVE_SELF }), async (c) => { const { ctx } = c.var.admitted; // ctx.principal is typed UserPrincipal by the policy ... } ); ``` `admit()` is now generic over its policy and sets `c.var.admitted` as `{ request, ctx }`, where `request` is the request as authentication returned it (sig1 verification consumes the original body) and `ctx` is the request context narrowed by the policy's authentication kind, exactly as `RouteContext<Authentication>` narrowed the legacy handler's fourth argument. Handlers use `c.req.param()` for path parameters; none of the six modules has one, so no decoding question arises in this PR. **The catalog mounts modules in place.** `catalog` (renamed from `routes`) is an ordered list of `Route | RouteModule`. `createControlPlaneApp` mounts a sub-app with `app.route("/", module)` where it appears and registers a legacy route through the adapter where it appears, so registration order, and therefore precedence, is unchanged. The path grammar and duplicate-parameter checks run for module routes too. **Route contracts come from Hono's own route list.** `listRouteContracts(app)` walks `app.routes` and yields `{ method, path, ...policy }` for every entry whose handler is the `admit()` middleware, which carries the policy it enforces. The policy tests, the route-admission matrix, its sentinel suite, and the conformance suite iterate that list instead of the catalog array, so they cover converted and legacy routes alike. Both integration snapshots are byte-identical to `main`. **Handler-level tests become request-level.** `analytics.test.ts` went through `createTestRequestHandler([analyticsRoutes])` with `authenticate` mocked to return a user and `ownerAuthorizationDatabase()` answering the effective-authorization lookup, which is the pattern `router.authorization-audit.test.ts` already used. Same assertions, plus a denial case that proves no store is touched. The remaining 26 handler-level test files convert with their modules. Converted: `health` (moved out of `catalog.ts` into its own module), `keyboard-shortcuts`, `model-preferences`, `sign-in-providers`, `audit-events`, `analytics`. ## Verification - Unit: 234 files, 3,474 passed. - Integration (workerd, real D1): 96 files, 1,128 passed. Route-matrix and conformance snapshots unchanged. - Typecheck (`src`, `tsconfig.test.json`, `test/integration`), ESLint, and Prettier clean. https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added a health check endpoint reporting service availability. - Expanded support for modular route handling while preserving existing endpoint access policies. - Added route contract reporting for endpoint methods, paths, and access requirements. - **Bug Fixes** - Preserved authentication, authorization, caching, and request-context behavior across updated endpoints. - Improved validation for unsupported route patterns and improperly configured routes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ColeMurray
added a commit
that referenced
this pull request
Sep 3, 2026
## Summary Fourth step of the series retiring the routing adapter (#1720, #1721, #1723). This PR converts the sessions cluster, the largest and most policy-diverse group, to native Hono sub-apps: 42 routes across 13 session modules plus the Slack notification route. It is the design proof for path parameters and for the sandbox-fallback and exact-sandbox policies. **Modules.** Each session module is a `Hono` sub-app registering `admit(policy)` first, and `sessions.ts` nests them under one sub-app in the old precedence order (`/sessions/inbox` still registers before `/sessions/:id`). The catalog mounts that sub-app and the Slack notification module where the spread and the inline route used to sit. **Handlers** keep their bodies and take a typed params object in place of the match array: ```ts export async function handleGetChild( request: Request, env: Env, params: { id: string; childId: string }, ctx: SessionRouteContext ): Promise<Response> ``` Registration passes `c.req.param()`, which Hono types from the path literal, so a handler declaring `childId` cannot be wired to a route without one. `withSessionRuntime(env, ctx)` replaces the `sessionRoute()` wrapper: it attaches the runtime client at the call site, and the handler-level tests use the same function to build their context. **Runtime proxy.** The three factories (`simpleProxy`, `legacyTokenRefresh`, `lifecycleProxy`) now build handlers rather than catalog entries, collected in `sessionRuntimeProxyHandlers` and registered one route each. The tests select handlers from that map by the production contract instead of scanning a route array. **Decode once, in Hono.** Admission now reads `c.req.param()`, so the session id the sandbox binding verifies, the automation id the ownership check loads, and any actorless-grant path parameter are Hono's decoded values, and admission's own `decodeURIComponent` calls are removed. `rawRouteParams` survives only inside the legacy adapter, for the modules not yet converted, and goes with them. ## Behavior change An encoded session id now resolves the same session as its plain form: `GET /sessions/%74est-…` returns the session rather than 404, and a sandbox token on `/sessions/%74est-…/tunnel-urls` verifies rather than 401. The guardrail tests from #1720 record this as their new expectation. No other route observed a different status: both integration snapshots are byte-identical to `main`. ## Tests Handler-level tests pass params objects and get their session runtime from `withSessionRuntime`; assertions are unchanged. `router.create-session.test.ts` calls the exported `handleCreateSession` directly. No test lost coverage; the route-matrix suite covers every converted route per credential class as before. ## Verification - Unit: 234 files, 3,476 passed. - Integration (workerd, real D1): 96 files, 1,128 passed. Route-matrix and conformance snapshots unchanged. - Typecheck (`src`, `tsconfig.test.json`, `test/integration`), ESLint, and Prettier clean. ## Review follow-up (71224c4) - **Malformed path segments are refused at admission.** Hono leaves a segment it cannot decode as it arrived, so the automation-id requirement's 400 had become a 404. `admit()` now checks every raw `:param` segment once and answers `400 { error: "Invalid path encoding" }` on every route before any lookup. This replaces the three route-specific messages `main` produced for this input (`Invalid automation route`, `Invalid user ID` on malformed member ids, and the session routes' 404); the two RBAC integration expectations and the routing-compatibility probe record the uniform envelope. Both matrix snapshots are still byte-identical. - **One call-site adapter.** `dispatch(c, handler)` in `routing/admit.ts` is the only place a Hono context is unpacked for a handler; `dispatchSession(c, handler)` adds the runtime client. A route reads `admit(policy), (c) => dispatchSession(c, handleX)`. The context type is checked against the policy, so a user-only handler behind a user-or-service policy fails to compile (a curried `handle(handler)` form was tried and rejected because Hono infers the environment from the returned function and that check disappears). - **Proxy tests dispatch through the production sub-app.** The test-only handler map is gone; the runtime proxy registers its handlers directly, and `session-runtime-proxy.test.ts` sends real requests through `sessionRuntimeProxyRoutes`, with a 17-row wiring table that fails if a route is bound to the wrong proxy. https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Improvements** * Standardized routing and authorization across session, attachment, media, child-session, prompt, pull-request, skills, WebSocket token, and notification endpoints. * Preserved existing endpoint paths, permissions, upload validation, streaming, and error handling. * Improved handling of percent-encoded path values so valid requests resolve correctly. * **Bug Fixes** * Malformed percent-encoded path segments now return a clear 400 error before processing. * **Tests** * Expanded coverage for routing, authorization, encoded paths, and request dispatch. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ColeMurray
added a commit
that referenced
this pull request
Sep 3, 2026
Last PR of the Hono series (#1720, #1721, #1723, #1724, #1726, #1727, #1728, #1729). Every route module is a Hono sub-app, so the adapter that carried catalog routes through admission has nothing left to serve. ## Removed - `legacy()` and `legacyMatch()` in `routing/hono-app.ts`; `createControlPlaneApp(modules, host)` mounts modules only. - `RouteDefinition`, `Route`, `defineRoute()`, `defineRoutes()`, and `extractRepoParams()` in `routes/shared.ts`. `RouteAdmissionPolicy` is now an interface over `RoutePolicy` plus `authorization` and `serviceActorClaims`; `cacheControl` lives on `AdmissionPolicy` in `routing/admit.ts`. - `RouteCatalogEntry`; the catalog is `readonly RouteModule[]`. - `legacyRoutes()` in test support; `matchRoute()` no longer fabricates a `RegExpMatchArray`. - `routes/shared.test.ts`, replaced by `routes/repository-params.test.ts` over the decoded pair. ## Kept on purpose - `RouteParams`: admission's decoded-parameter dictionary. Hono exports no such type. - `rawRouteParams()`: the malformed-encoding guard's raw read-back (added in #1724). ## Tests Fixtures that need synthetic routes build a `Hono` module and pass it to `createTestRequestHandler([module])`: the lifecycle suite, the contract lister's suite, and the authorization-audit suite's eleven routes. The two Worker-boundary suites build their shadow catalogs the same way: conformance echoes the raw read-back through `rawRouteParams()`, and the matrix's "raw path segments" probe from #1720 is now the decode-once assertion on what handlers receive (`abc%2Fdef` arrives as `abc/def`, `web%252Fapp` as `web%2Fapp`). ## Verification | Check | Result | |---|---| | Typecheck (src, test, integration) | clean | | ESLint, Prettier | clean | | Unit | 234 files, 3,512 passed | | Integration (workerd, real D1) | 96 files, 1,129 passed | | Matrix and conformance snapshots | byte-identical | https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Repository paths now support nested owner namespaces, such as `group/subgroup`. - Repository names containing slash characters are rejected with a clear validation message. - URL path parameters are decoded consistently, improving handling of encoded repository and member identifiers. - **Refactor** - Control-plane routing now uses the current route-module system, providing consistent admission, route matching, and request handling behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First step of the follow-up series that retires the routing adapter left by #1716. This PR adds only tests; no production behavior changes.
Encoded path segments are pinned (
test/integration/route-admission-matrix.test.ts). Two new tests through the real Worker and the sentinel catalog:group%2Fsubgroup) is one segment, a slash in the name (web%2Fapp) is refused after that decode, and a doubly-encoded slash (web%252Fapp) survives because nothing decodes it a second time.match.groups, proving the adapter deliversabc%2Fdef,group%2Fsubgroup/web%252Fapp, and a doubly-encoded member id untouched.Why these matter: the next PR moves parameter access to Hono, whose
c.req.param()decodes values, so these tests are the red-then-green story for keeping raw segments where handlers decode themselves.Dropped during review: an earlier revision carried a route-policy registry with a build-time walk over
app.routes. Reconstructing handler chains from Hono's flat route list needed positional rules and an unwrap for mounted sub-apps, which is more machinery than the guarantee deserves. The "every route is admitted" invariant will instead be enforced at request time in the next PR: the lifecycle middleware refuses any response that admission did not precede, and the matrix test exercises every route in CI so a route registered withoutadmit()fails before deploy.Verification
src,tsconfig.test.json,test/integration), ESLint, and Prettier clean.https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1