fix: enforce route-scoped auth for realtime RPC - #123
Conversation
Co-authored-by: codex <codex@openai.com>
|
Website preview: https://pr-123-eclipsa.xiarenda61.workers.dev Commit: 6e0dedc |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1166918490
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const routeMatch = getRpcCurrentRoute(appHooks, c); | ||
| if (!routeMatch) { | ||
| return c.text("Bad Request", 400); |
There was a problem hiding this comment.
This new pre-upgrade check makes realtime upgrades fail whenever getRpcCurrentRoute(...) is null, but browser clients cannot provide x-eclipsa-route-url during new WebSocket(...) connects (the runtime client in packages/eclipsa/core/realtime.ts opens a socket URL only). In practice, normal realtime().connect() calls will now hit 400 Bad Request for production (and the same logic exists in dev), so this change breaks legitimate browser realtime usage instead of only blocking cross-route ID probing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR closes an authorization gap where realtime WebSocket RPC handlers could be invoked globally by id, bypassing route-scoped access checks and route middleware protections.
Changes:
- Adds
realtimeIdsto route server access metadata and scopes realtime handlers to the reachable route graph (dev + build). - Enforces realtime RPC route resolution via
getRpcCurrentRoute(...)and validates the requested realtimeidagainst route access. - Runs realtime execution through
composeRouteMiddlewares(...), and caches pre-upgrade route matches via a per-requestWeakMapto keep upgrade checks consistent.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/eclipsa/vite/dev-app/mod.ts | Adds route-scoped realtime access IDs, enforces current-route resolution for realtime, caches route matches, and runs realtime via route middleware composition. |
| packages/eclipsa/vite/dev-app/mod.test.ts | Updates websocket adapter test to include route RPC header; adds test asserting realtime calls are blocked outside the current route graph. |
| packages/eclipsa/vite/build/mod.ts | Extends build-time route server access entries with realtime IDs and emits runtime checks + middleware composition for realtime WS routes. |
| packages/eclipsa/vite/build/mod.test.ts | Asserts emitted server bundle source includes realtime auth/middleware checks and route-match caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return composeRouteMiddlewares( | ||
| routeMatch.route, | ||
| requestContext, | ||
| routeMatch.params, | ||
| async () => { | ||
| await executeRealtime(id, requestContext, socket) | ||
| return requestContext.body(null, 204) | ||
| }, | ||
| ) as Promise<Response> |
There was a problem hiding this comment.
createRealtimeHonoUpgradeHandler ignores the return value of its connect callback (see packages/eclipsa/core/realtime.ts:404-439), so the Response produced by resolveRequest(...)/composeRouteMiddlewares(...) here is never sent to the client. If a route middleware short-circuits by returning a Response (e.g. auth failure), the WebSocket upgrade will still succeed and the connection may remain open but inert. Consider running the route middlewares/auth checks in the pre-upgrade middleware (before await next()), or have the connect path explicitly close the socket (e.g. 1008) when middlewares short-circuit (use a non-Response sentinel return from the handler to detect success).
| await resolveRequest(c, async (requestContext, appHooks) => { | ||
| const id = requestContext.req.param("id"); | ||
| await executeRealtime(id, requestContext, socket); | ||
| return requestContext.body(null, 204); | ||
| const routeMatch = realtimeRouteMatches.get(requestContext.req.raw) ?? getRpcCurrentRoute(appHooks, requestContext); | ||
| if (!routeMatch) { | ||
| return requestContext.text("Bad Request", 400); | ||
| } | ||
| return composeRouteMiddlewares( | ||
| routeMatch.route, | ||
| requestContext, | ||
| routeMatch.params, | ||
| async () => { | ||
| await executeRealtime(id, requestContext, socket); | ||
| return requestContext.body(null, 204); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
createRealtimeHonoUpgradeHandler does not use the Response returned by its connect callback (see packages/eclipsa/core/realtime.ts:404-439), so the Response returned from resolveRequest(...)/composeRouteMiddlewares(...) here is effectively discarded. This means route middlewares that deny access by returning a Response won't prevent the WebSocket upgrade and may leave an idle connection open. Consider moving middleware execution/auth denial into the pre-upgrade middleware (before await next()), or explicitly close the socket when middlewares short-circuit (use a non-Response sentinel from the handler so you can distinguish “authorized” vs “middleware returned a Response”).
Carry browser route context through realtime WebSocket URLs and authorize route-scoped realtime handlers before the WebSocket upgrade proceeds. Co-authored-by: codex <codex@openai.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
Motivation
Description
realtimeIdsto the route server access metadata and include realtimes when buildingrouteServerAccessEntriesso realtime handlers are scoped to reachable route files.getRpcCurrentRoute(...)and check membership of the requested realtimeidagainstrouteAccess.realtimeIdsin both the build-time app (packages/eclipsa/vite/build/mod.ts) and dev app (packages/eclipsa/vite/dev-app/mod.ts).composeRouteMiddlewares(...)so route middlewares and server hooks run beforeexecuteRealtime(...).WeakMapand reuse it during the websocket handler execution to keep checks consistent across the upgrade boundary.Testing
bunx tsc -p tsconfig.json --noEmitand it completed successfully.bun run test vite/build/mod.test.ts vite/dev-app/mod.test.ts(frompackages/eclipsa) and the updated test suite passed (48tests passed across both files).Codex Task