fix(proxy): carry proxy identity on the renderer WebSocket bridge - #3648
Conversation
The proxy terminates a browser WebSocket and opens a second socket to the shared renderer. That second hop was built with `new WebSocket(url)`, which cannot set request headers, so the tenant identity was moved into the query string. The renderer deliberately refuses to read identity from a WebSocket query (browser-controlled), and its `createProxyGuard` answers the headerless hop 502 "x-project-slug header is required in proxy mode". This is firing in production continuously: 1000-5000 rejections/hour over the last 24h, one per HMR reconnect attempt. Preview live-reload never connects; the client retries every second forever and no user-visible error names the cause. The proxy already holds the identity the guard demands -- the project `x-token` it minted from its own API client credentials, plus the resolved project, environment and branch identity it attaches to every other forwarded request. The fix carries that same header set on the bridge hop via `createProxyContextHeaders`, using a `WebSocketStream`-backed client that can present headers. The guard is unchanged: nothing is exempted, no credential is invented, and identity still cannot come from a path, a query param or anything a client chooses. - `buildRendererBridgeRequest` builds the hop and deletes the browser-supplied `x-project-slug` / `x-environment` query params instead of overwriting them. - `UpstreamWebSocket` presents headers on the handshake behind a `WebSocket`-shaped surface, so the bridge wiring is untouched. It fails loudly rather than falling back to a headerless socket. - Tests assert the hop through the real guard, and that the guard still rejects a look-alike hop that names itself only in the query, a hop with no upstream token, and a hop arriving at an untrusted boundary.
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9290cb074c
ℹ️ 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".
The renderer stand-in bound a fixed port 45913. Unit test modules run under --parallel, so a second module (or a developer/CI host already holding that port) makes Deno.serve throw AddrInUse before any assertion runs: SECOND BIND THREW: AddrInUse Address already in use (os error 48) Bind with port 0 and read the assigned port off server.addr, matching the other proxy tests (server-resolver.test.ts, token-manager.test.ts). The bridge target base now derives from the same address instead of the constant. Four concurrent runs of the module pass; src/proxy/ --parallel is 51 passed (498 steps) | 0 failed.
CI status: 1 blocking failure, and it is not this PR
Line 52 is Why it is not this PR. The diff is five files, all under Why re-running has not cleared it, and probably will not. Shard 7/8 passed on this same branch at This looks like shared-cache degradation that will hit other PRs on the same stack, not something to paper over here. Deliberately not fixed here. The obvious local patch — filtering Deno progress lines out of Everything else is green: Earlier in the run set, seven checks failed purely on |
The hole
createProxyGuard(src/server/runtime-handler/project-runtime-context.ts:477) rejects the platform's own proxy → renderer WebSocket bridge (src/proxy/main.ts:272-322).Mechanism. The bridge terminates the browser socket and opens a second socket to the shared renderer with
new WebSocket(targetUrl). That API cannot set request headers, so the bridge put the tenant identity in the query string:The renderer deliberately does not read tenant identity from a WebSocket query —
wsSlugOverridewas pinned toundefinedin #3290 because those params are browser-controlled. So the hop arrives with nox-project-slugand nox-token, the bridge target host (veryfront-server) carries no slug, andcreateProxyGuardanswers 502 before any handler runs.Locally this never shows up: the combined dev server runs the renderer with
PROXY_MODE=0, andcreateProxyGuardreturnsundefinedwhen!isProxyMode.It is firing in production right now
Loki,
veryfront-production, last 24h — the guard warn and the proxy's matching failure, one pair per reconnect:sum(count_over_time(... [1h]))holds between 1,000 and 5,000 rejections per hour for the entire window — never zero.User-visible symptom: preview HMR / live-reload is dead on
*.preview.veryfront.com. The browser's socket to the proxy succeeds (101), the upstream hop 502s, the client is closed1011 "Server connection error", and the HMR script reconnects on backoff — roughly once a second, forever. Silently retried, never surfaced: no error names the guard, the header, or the proxy.The fix
The proxy already holds the identity the guard demands. Its
x-tokenis a project-scoped bearer it minted from its own API client credentials (VERYFRONT_PROXY_API_CLIENT_ID/SECRET), and it attaches that plus the resolved project/environment/branch identity to every other forwarded request viacreateProxyContextHeaders. The bridge hop was the only request that dropped it, because of a transport limitation.So the hop now carries that same header set, over a
WebSocketStream-backed client that can present headers:buildRendererBridgeRequestbuilds the hop withcreateProxyContextHeaders(req.headers, context)and deletes the browser-suppliedx-project-slug/x-environmentquery params rather than overwriting them. Identity travels only in proxy-owned headers.UpstreamWebSocket(src/proxy/websocket-client.ts) presents headers on the handshake behind aWebSocket-shaped surface (readyState/send/close/onopen/onmessage/onerror/onclosewith real DOM events), so the bridge wiring inmain.tsis otherwise unchanged. It fails loudly ifWebSocketStreamis unavailable instead of falling back to a headerless socket, which would silently reproduce this bug.--unstable-netis already baked into the compiled proxy binary (scripts/build/compile-binary.ts:104).No security control is weakened.
createProxyGuardis not touched: no route is exempted, no header presence is trusted, no new credential is invented. The caller now holds the credential the gate always demanded. A prefix/path-keyed exemption was never on the table for exactly the reason recorded in #3641.Existing shared identity: what was looked for
Before inventing a credential I checked what the proxy and renderer actually share.
CHANNEL_DISPATCH_SIGNING_PUBLIC_KEYis public-key-only on both sides — neither can mint with it.VERYFRONT_PROXY_ROUTING_INVALIDATION_SECRETis proxy-only (veryfront-server-proxy-secret; the renderer'senvFromisveryfront-server-secret, disjoint). There is no shared symmetric secret, and there does not need to be: the proxy'sx-tokenis the existing, API-minted identity, and it now rides the hop.Tests
TDD. The red test failed with the exact production message:
The identity tests build the hop with the production builder and hand it to the production guard (
prepareProjectRequest), not to a hand-written header list. Adversarial coverage — all three must stay rejected:?x-project-slug=…&x-environment=…, no headers) → stillx-project-slug header is required in proxy modex-token header is required in proxy modeproject, environment, and branch identity headers require an operator-authenticated proxy boundaryx-project-slug=victim-projectin the/_wsquery still resolves as its own tenant, because the bridge deletes those paramsTransport tests run a real
Deno.serve+Deno.upgradeWebSocketrenderer stand-in and assert the headers arrive on the wire, including the full browser-derived set (cookie/origin/user-agent riding along), plus the 502-rejection path and the no-silent-fallback guarantee.No existing test was weakened or deleted.
Risk
The transport swap is the substantive risk:
WebSocketStreamis Deno-unstable and this path carries all preview HMR. It is compiled in, verified end-to-end against a real Deno WS server here, and the adapter keeps the bridge's event wiring identical. Worth watching after deploy:[WebSocket] Server connection errorshould go to zero and[WebSocket] Server connected, bridge establishedshould appear for preview projects.