refactor: convert the browser auth passthrough to a Hono sub-app - #1726
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughBrowser auth routes now use a Hono sub-app. The catalog mounts this sub-app directly. Tests dispatch requests through the complete route handler and cover proxying, authorization, errors, headers, and allowlisted paths. ChangesBrowser auth routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Browser authentication proxy routes now run through a Hono sub-application while retaining their allowlist, web-service access restriction, response behavior, and catalog precedence. Coverage includes the relevant proxy, authorization, error, and header behavior, with no concrete current-head merge risk identified. 🚥 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}
There was a problem hiding this comment.
Summary
PR #1726, refactor: convert the browser auth passthrough to a Hono sub-app, by @ColeMurray updates 3 files (+161/-16). The conversion preserves the six-route positive allowlist, web-service-only admission, registration precedence, Better Auth response handling, and runtime-misconfiguration behavior. No correctness, security, performance, or maintainability issues were found.
Critical Issues
None.
Suggestions
None.
Nitpicks
None.
Positive Feedback
- The production sub-app is tested through the real Hono/admission lifecycle rather than by invoking implementation details directly.
- Coverage includes every allowlisted route plus status/header passthrough, authorization denial, configuration failure, and route/method exclusion.
- Reusing the declarative admission policy across registrations keeps the converted module concise while retaining route-contract introspection.
Questions
None.
Verdict
Approve: Ready to merge. Focused unit tests (11), focused integration tests (27), route compatibility checks, TypeScript typechecking, and diff whitespace validation all pass.
There was a problem hiding this comment.
Deep review complete. This is a clean conversion to the branch's canonical Hono route-module boundary: admission remains centralized in admit, handler adaptation remains centralized in dispatch, the shared positive allowlist remains the single source of route registration, and catalog precedence is preserved. The production module grows only from 93 to 99 lines; no file-size or decomposition concern is introduced. The added tests exercise every allowlisted route through the production sub-app, denial and non-allowlisted behavior, runtime misconfiguration, and response/header passthrough. I found no structural regression, spaghetti growth, boundary leak, or credible code-judo simplification that would materially improve this change. Verified locally on the PR head: control-plane typecheck, all 234 unit files / 3,505 tests, ESLint on changed files, and Prettier all pass.
The six Better Auth proxy routes register from the shared allowlist in its order, each behind the web-service admission policy, and reach the one passthrough handler through `dispatch()`. The handler keeps owning its status codes and headers; nothing in its body changed. The module's tests now dispatch through the sub-app: one row per allowlisted route proves the wiring and which Better Auth entry point serves it, plus the status passthrough, the 503 for a missing runtime, the web-service-only refusal, and the 404 for a method outside the list. Claude-Session: https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
f7d5a9a to
3f5b946
Compare
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
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 -->
1 participant
Rebased onto
mainafter #1724 merged; the diff is this PR alone.PR 7 of the Hono follow-up series (plan:
docs/internal/2026-09-02-control-plane-hono-follow-ups.mdin prod).What changed
src/routes/browser-auth.tsis a Hono sub-app. The six routes inBROWSER_AUTH_PROXY_ROUTESregister in the allowlist's order, each behindadmit({ ...SCM_AGNOSTIC_WEB_SERVICE_ROUTE, authorization: NO_AUTHORIZATION }), and reachhandleBrowserAuththroughdispatch(). The handler body is unchanged: it still owns Better Auth's status codes and headers.routes/catalog.tsmounts the module where the spread was (after health, before sign-in providers), so precedence is unchanged.src/routes/browser-auth.test.tsdispatches through the production sub-app: one row per allowlisted route asserting which Better Auth entry point served it (directgetSessionfor the session read, the HTTP handler otherwise), the status and cookie passthrough withno-store/no-referrer, the 503 when the runtime is not configured, the 401 for a non-web caller, and the 404 for a method outside the allowlist.Routes converted: 6 (running total 60 of 171).
Verification
https://claude.ai/code/session_01KdDpTgGEjXpBA9SaGQVUH1
Summary by CodeRabbit