fix(proxy): CORS on edge-synthesized sandbox errors - #3389
Conversation
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 3704240. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 6 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
When a sandbox is running, the CORS headers on its host come from envd inside the sandbox. When it is paused or killed there is no envd to answer, so the proxy synthesizes the error — and that response carried no `Access-Control-*` headers at all, which coupled CORS support to the sandbox being alive. For anything probing liveness that is inverted: the one answer the probe exists to deliver is the one a browser cannot receive. `Sandbox.isRunning()` maps a 502 to `false`, but in a browser `fetch` rejects before there is a status to read, so a bare `TypeError: Failed to fetch` reaches the caller, indistinguishable from being offline. Knock-on, `checkSandboxHealth` can never return `false`, so 24 SDK call sites downgrade an actionable "sandbox was killed" timeout to a generic network error. Adding the headers to the 502 alone would not have fixed it. Every envd request carries `E2b-Sandbox-Id` and `E2b-Sandbox-Port`, which are not CORS-safelisted, so a browser always preflights first, and a preflight response must have an ok status or the browser errors out no matter which headers it carries. So the fix is two parts: answer preflights with 204 on the paths that would otherwise answer with an error status, and set `Access-Control-Allow-Origin: *` (matching what envd sends) on the synthesized responses themselves. `TemplatedError.HandleError` is the choke point for all eight error templates, so it covers the whole family at once; the raw `http.Error` paths go through the new `cors.Error`. Both `handler()` and the pool's `ErrorHandler` — which owns `port_closed` and the unreachable-sandbox 502 — answer preflights. The healthy path is untouched: envd keeps answering its own preflights, and the proxy must not add a second `Access-Control-Allow-Origin`, since a browser rejects two values as readily as none. Also fixes content negotiation, found while investigating. `template.go` chose HTML over JSON by User-Agent sniffing, but a `fetch()` from page scripts carries the browser's User-Agent and cannot override it (UA is a forbidden header name), so an SDK call from a browser got the HTML error page where it expects JSON — `Accept` was ignored entirely. Intent now decides first: JSON when `Accept` prefers it, or when `Sec-Fetch-Mode` / `X-Requested-With` mark the request as script-initiated. UA sniffing stays as the fallback, where it only catches genuine top-level navigations, which is what the HTML pages are for. Verified with `go test -race ./packages/shared/pkg/proxy/...`, including socket-level tests that reproduce the browser's preflight-then-request exchange against a missing and an unreachable sandbox, plus `make fmt` and `make lint`. Both new behaviors were checked to have teeth by reverting the fix and watching the tests fail. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removes canObserveStoppedSandbox. The stopped-sandbox 502 arriving from the edge without CORS headers is fixed in e2b-dev/infra#3389, so isRunning() can report false in a browser again and checkSandboxHealth can return false, which restores the actionable TimeoutError on the 24 call sites that use it. Also un-gates host.test.ts 'ping server in non-running sandbox', which was under the wrong flag: it asserts the edge's own 502 JSON envelope rather than a response from a server inside the sandbox, so infra#3389 covers it too (the same PR's content-negotiation fix is what keeps the body JSON rather than the HTML error page). Its sibling 'ping server in running sandbox' does read the user's python server, so that one keeps the gate. canFetchSandboxServers stays for the four tests that read a response from a server the test started in the sandbox — no CORS headers there, and one of them sends a custom header that would preflight into python's 501. Its doc comment no longer cites the proxy 502 as an example, since that now carries the header. Until infra#3389 reaches production these 14 tests fail on the browser leg — 13 with 'TypeError: Failed to fetch' and one with the degraded SandboxError in place of TimeoutError, matching the two documented consequences exactly. The Node/Bun/Deno/workerd legs are unaffected (7 files / 26 tests green on Node). Co-Authored-By: Claude <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| HandleError(w, r) | ||
| if err != nil { | ||
| logger.L().Error(ctx, "failed to handle too many connections error", zap.Error(err), logger.WithSandboxID(d.SandboxId)) | ||
| http.Error(w, "Failed to handle too many connections error", http.StatusInternalServerError) | ||
| cors.Error(w, "Failed to handle too many connections error", http.StatusInternalServerError) | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
🔴 The connection-limiting branch in handler() (the too many connections path) never calls cors.HandlePreflight, so a CORS preflight OPTIONS request that happens to land while the sandbox's connection limit is exceeded gets answered with a 429 instead of a 204.
Extended reasoning...
The bug. handler() only short-circuits CORS preflights for the getDestination failure path: if err != nil && cors.HandlePreflight(w, r) { ... } at the top of the function. That covers every branch that follows from a non-nil err (not-found, port-closed-via-template, invalid host/ID/port, etc.), because all of those checks are gated on err.
The connection-limiting block, however, only runs when err == nil — i.e. the sandbox did resolve successfully. When connLimitConfig.Limiter.TryAcquire fails because the sandbox is already at its concurrent-connection limit, the code falls straight into template.NewSandboxTooManyConnectionsError(...).HandleError(w, r) (lines 195-203) without ever asking whether the incoming request was a CORS preflight. TemplatedError.HandleError does call cors.SetHeaders(w) so the response carries Access-Control-Allow-Origin: *, but it also calls w.WriteHeader(e.vars.StatusCode()), and sandboxTooManyConnectionsData.StatusCode() is http.StatusTooManyRequests (429) — not an "ok" status.
Why this matters despite the headers being present. Per the Fetch/CORS spec, a browser only proceeds with the real request after a preflight if the preflight response has an ok HTTP status (2xx). Headers are irrelevant if the status fails that check — the browser aborts the whole request and the page's fetch() promise rejects before there is a status or body for JS to read. That is precisely the failure mode the rest of this PR is fixing (opaque TypeError: Failed to fetch instead of an actionable error), but this one branch was left uncovered.
Concrete walk-through:
- A sandbox is running and reachable, and
connLimitConfigis configured with somemaxLimit. - The sandbox is already at
maxLimitconcurrent connections (e.g. legitimate load, or a burst of requests). - A browser SDK call adds a non-safelisted header (e.g.
E2b-Sandbox-Id), so the browser issues a CORS preflight:OPTIONSwithAccess-Control-Request-Method/Access-Control-Request-Headers. getDestination(r)succeeds (the sandbox is valid and routable) →err == nil→ the top-levelcors.HandlePreflightshort-circuit at the start ofhandler()is skipped, because it's gated onerr != nil.- Execution reaches the
connLimitConfigblock;TryAcquirereturnsacquired == falsebecause the limit is already exhausted. NewSandboxTooManyConnectionsError(...).HandleError(w, r)runs, writingAccess-Control-Allow-Origin: *plus a429status.- The browser evaluates the preflight response, sees a non-2xx status, and rejects the preflight per spec — it never sends the actual
OPTIONS-guarded request. The page'sfetch()throws a bareTypeError: Failed to fetch, with no way for the SDK to distinguish "too many connections" from being offline.
Fix. Mirror what the other two call sites (handler.go's top-level check and pool/client.go's ErrorHandler) already do: call cors.HandlePreflight(w, r) and return early before (or instead of) running the connection-limit check, so a preflight is always answered with 204 regardless of whether the underlying request would have been rate-limited. The real request that follows the preflight can then carry the informative 429/CORS-headers response as it does today.
| - **Sandbox proxy** (:5007, `pkg/proxy/`): reverse-proxies incoming traffic from client-proxy to | ||
| the sandbox's slot IP and requested port, enforcing per-sandbox traffic access tokens. | ||
| the sandbox's slot IP and requested port, enforcing per-sandbox traffic access tokens. A live | ||
| sandbox answers CORS itself (envd allows all origins); when the sandbox is missing or | ||
| unreachable the proxy synthesizes the response instead, so it carries its own CORS headers | ||
| and answers preflights (`pkg/proxy/cors/`) — otherwise a browser would hide the error from JS. |
There was a problem hiding this comment.
🟡 The new parenthetical (pkg/proxy/cors/) in the Sandbox proxy bullet doesn't resolve to an existing path under packages/orchestrator/pkg/proxy/ (which only has proxy.go/metrics.go) — the cors package lives at packages/shared/pkg/proxy/cors/. Suggest fully qualifying it as packages/shared/pkg/proxy/cors/, matching the packages/shared/pkg/proxy/host.go reference used elsewhere in this same doc for the same package family.
Extended reasoning...
The bug: The new sentence added to the Sandbox proxy bullet reads:
Sandbox proxy (:5007,
pkg/proxy/): ... and answers preflights (pkg/proxy/cors/) ...
packages/orchestrator/pkg/proxy/cors/ does not exist. Verified directly:
packages/orchestrator/pkg/proxy/ -> metrics.go, proxy.go (no cors/ subdir)
packages/shared/pkg/proxy/cors/ -> cors.go, cors_test.go (this PR's new package)
Why this happened: Elsewhere in the Orchestrator section, unqualified pkg/... paths are orchestrator-relative (pkg/sandbox/, pkg/server/, pkg/service/, pkg/volumes/ all resolve under packages/orchestrator/). The Sandbox proxy bullet's own header, (:5007, pkg/proxy/), appears to follow that same pattern, so a reader naturally expects pkg/proxy/cors/ to also be orchestrator-local — but it isn't.
The counter-argument (from a refutation) has real merit and I want to address it directly: packages/orchestrator/pkg/proxy/ itself is just a thin wrapper (proxy.go, metrics.go) that imports the real reverse-proxy engine from packages/shared/pkg/proxy (aliased reverseproxy) and packages/shared/pkg/proxy/pool. None of handler.go, pool/, template/, or the new cors/ exist under the orchestrator path — they're all under packages/shared/pkg/proxy/. So one could argue the bullet's own pkg/proxy/ was already loosely referring to "the proxy package" in the abstract, spanning both the orchestrator wrapper and the shared engine it wraps, and cors/ is consistent with that loose usage.
That reading is plausible, but it doesn't make the reference unambiguous or correct — it just means the bullet's pre-existing pkg/proxy/ header was already a little imprecise, and this PR's addition inherits and compounds that imprecision rather than resolving it. Critically, the same document already establishes the correct, unambiguous way to reference this exact package two sections below: packages/shared/pkg/proxy/host.go. That precedent exists precisely because unqualified pkg/proxy/ is not sufficient to locate the shared package unambiguously in this doc. Following that established precedent for the new cors/ reference would have avoided the ambiguity entirely.
Impact: Someone reading the doc and trying to jump to pkg/proxy/cors/ from the orchestrator package tree will not find it, costing them a grep/find round-trip. This is a pure documentation-navigation issue with no runtime or behavioral impact — the described CORS behavior itself is accurate.
Fix: Change (pkg/proxy/cors/) to (packages/shared/pkg/proxy/cors/) to match the fully-qualified packages/shared/pkg/proxy/host.go reference used elsewhere in the same document for the same package family.
Severity: This is a one-line, easily-fixed doc precision issue with no functional impact — nit, not blocking.
| HandleError(w, r) | ||
| if err != nil { | ||
| logger.L().Error(ctx, "failed to handle too many connections error", zap.Error(err), logger.WithSandboxID(d.SandboxId)) | ||
| http.Error(w, "Failed to handle too many connections error", http.StatusInternalServerError) | ||
| cors.Error(w, "Failed to handle too many connections error", http.StatusInternalServerError) | ||
|
|
||
| return | ||
| } |
There was a problem hiding this comment.
🟡 The connection-limit path in handler() (reached only when getDestination succeeds) calls template.NewSandboxTooManyConnectionsError(...).HandleError directly, so it never runs through the new preflight short-circuit added at the top of the function. A CORS preflight OPTIONS against a live sandbox that's at its connection limit gets a bare 429 with no preflight answer, so per the Fetch spec the browser rejects it and never sends the real request - JS sees an opaque network error instead of the too-many-connections status, the exact class of bug this PR sets out to fix.
Extended reasoning...
The gap: handler() in packages/shared/pkg/proxy/handler.go gates its new preflight short-circuit on err != nil && cors.HandlePreflight(w, r) (around line 24). Every error branch that follows is guarded by an errors.As/errors.Is check on that same err, so they're all reachable only when err is non-nil. The connection-limit block (connLimitConfig != nil { ... TryAcquire ... }, lines ~182-203) sits after the final if err != nil { ... } catch-all and is therefore only reached when err == nil - i.e. getDestination succeeded and resolved a live sandbox destination d. Because the preflight short-circuit never fires on this path, a CORS preflight OPTIONS request that happens to land while the sandbox is at its connection limit falls straight into TryAcquire, and on failure into template.NewSandboxTooManyConnectionsError(d.SandboxId, r.Host, maxLimit).HandleError(w, r).
Why that response breaks the browser flow: sandboxTooManyConnectionsData.StatusCode() returns http.StatusTooManyRequests (429). TemplatedError.HandleError does add cors.SetHeaders(w) (this PR's own fix), so the 429 does carry Access-Control-Allow-Origin: * - but per the Fetch/CORS spec, a preflight response is only accepted by the browser if its status is in the ok range (200-299). A non-2xx preflight response is rejected regardless of what headers it carries, and the browser never sends the real (GET/POST/etc.) request that was gated behind it. The result: JS gets a bare network error (TypeError: Failed to fetch in browsers) with no visibility into the 429 or its body - exactly the failure mode ('browser can't see the synthesized error status') that this PR was written to eliminate for the other seven error templates.
Why nothing else catches this: The PR's own approach for every other synthesized-error path (sandbox not found, still transitioning, resume denied, team limit, missing/invalid traffic token, port closed, unreachable sandbox) is to run cors.HandlePreflight before falling into the corresponding HandleError/http.Error call. The connection-limit block was simply never wired into that short-circuit, because it lives on the success side of getDestination (err == nil) rather than the error side the new check was written for. HandlePreflight/isPreflight in cors.go has no awareness of connection limiting at all, so there is no fallback that would catch this.
Concrete walkthrough:
- A sandbox is live and already has maxLimit concurrent connections open (a legitimate, load-dependent state - not a bug on its own).
- Browser JS calls fetch() against the sandbox URL with the SDK's custom headers (E2b-Sandbox-Id, E2b-Sandbox-Port). Because these are non-safelisted headers, the browser first sends a preflight: OPTIONS with Access-Control-Request-Method: GET and Access-Control-Request-Headers listing those headers.
- handler() runs: getDestination(r) succeeds (err == nil, sandbox resolves), so the top-of-function
err != nil && cors.HandlePreflight(...)check is false and falls through. - Execution reaches the connLimitConfig block; TryAcquire fails because the sandbox is already at maxLimit. NewSandboxTooManyConnectionsError(...).HandleError(w, r) writes a 429 (JSON or HTML body, plus Access-Control-Allow-Origin: *, but with Access-Control-Allow-Methods/Access-Control-Allow-Headers never set and no 2xx status).
- The browser evaluates the preflight response: status 429 is not ok, so per the Fetch standard's CORS-preflight-fetch algorithm the whole request is failed. The actual GET is never sent.
- fetch()'s promise rejects with a generic network error - the caller (and the E2B SDK) cannot distinguish 'sandbox is overloaded' from 'network is down' or 'CORS misconfigured', losing exactly the signal (429 / too-many-connections) the backend tried to communicate.
Fix: extend the top-of-handler() short-circuit (or add an equivalent cors.HandlePreflight(w, r) check) so it also runs for the err == nil / connection-limit path - e.g. check cors.HandlePreflight(w, r) unconditionally near the top of handler(), before branching on err, rather than gating it on err != nil.
Severity: this is real but narrow - all three independent verifiers converged on nit. It only manifests when a live, already-connection-limited sandbox happens to receive a preflight at that exact moment; the ordinary GET request on this same path is already fixed correctly (the 429 does carry Access-Control-Allow-Origin: * via cors.SetHeaders), and this path had no CORS handling at all before this PR, so it's an incomplete corner of a genuine improvement rather than a regression introduced by it.
Review catch. The connection limit is only reached when getDestination succeeded, so it sits past the `err != nil` preflight check at the top of `handler()` and never got one of its own. `HandleError` gave the response `Access-Control-Allow-Origin`, but the status is 429 — not ok — so a browser rejects it as a preflight result and never sends the request the 429 was meant for, leaving `sandbox_too_many_connections` exactly as unreadable from JS as before this branch. Answered inside the `!acquired` branch rather than by hoisting the check to the top of `handler()`: preflights for healthy sandboxes belong to envd, which owns a different allow-list, and the proxy should not start answering them. Answering here reaches the sandbox no more than the 429 would, so it is not counted as a blocked connection — the real request that follows still takes the 429 and the metric. Also fully qualifies the new `packages/shared/pkg/proxy/cors/` path in ARCHITECTURE.md: unqualified `pkg/...` in that section is orchestrator-relative, where the package does not exist. Verified the new test fails without the fix (429 instead of 204, and the blocked-connection metric double-counting). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
5aad415 to
d71980e
Compare
1 participant
Sibling of #3388 — same class of bug (a CORS gap that makes an SDK feature silently unusable in browsers), but in the sandbox proxy rather than the API. Tracked SDK-side as SDK-294; surfaced by the new browser test leg (SDK-292 / e2b#1609), which currently skips 13 tests because of it.
Problem
When a sandbox is running, the CORS headers on its host come from envd, inside the sandbox. When it is paused or killed there is no envd to answer, so the proxy synthesizes the error — and that response carried no
Access-Control-*headers at all. Measured against production:OPTIONS)GET)204·ACAO: *·ACAH: authorization,user-agent204·ACAO: *502· no CORS headers502· no CORS headersCORS support was therefore coupled to the sandbox being alive. For anything that probes liveness this is inverted: the one answer the probe exists to deliver is the one a browser cannot receive.
Sandbox.isRunning()maps the 502 tofalse, but in a browserfetchrejects before there is a status to read, so a bareTypeError: Failed to fetchreaches the caller, indistinguishable from being offline. Knock-on,checkSandboxHealthcan only returntrueorundefinedin a browser, so 24 SDK call sites downgrade an actionableTimeoutError("The sandbox was killed…")to a genericSandboxError("network error").Adding the headers to the 502 alone would not have fixed it. Every envd request carries
E2b-Sandbox-IdandE2b-Sandbox-Port(plusX-Access-Tokenfor secured sandboxes) — non-safelisted, so a browser always preflights. Per the Fetch standard a preflight response must have an ok status or the browser returns a network error regardless of which headers it carries. Hence two parts.Changes
New
packages/shared/pkg/proxy/cors/, used by bothclient-proxyand the edge:204on paths that would otherwise answer with an error status — inhandler()whengetDestinationfails, and in the pool'sErrorHandler, which ownsport_closedand the unreachable-sandbox 502. A bareOPTIONS(noAccess-Control-Request-Method) is not a preflight and still falls through to the error.Access-Control-Allow-Origin: *(matching what envd sends) on the synthesized responses.TemplatedError.HandleErroris the single choke point for all eight error templates, so it coverssandbox_not_found,port_closed,sandbox_still_transitioning,sandbox_resume_permission_denied,sandbox_too_many_connections,team_sandbox_limitand bothtraffic_access_token_errors at once. The rawhttp.Errorpaths — missing header, invalid host/sandbox-ID/port, and the unreachable-sandbox ones in the pool — go through the newcors.Error.The healthy path is untouched: envd keeps answering its own preflights, and the proxy must not add a second
Access-Control-Allow-Origin(a browser rejects two values as readily as none) — there is a test for that.Also fixes content negotiation, found while investigating.
template.gochose HTML over JSON by User-Agent sniffing, but afetch()from page scripts carries the browser's User-Agent and cannot override it (UA is a forbidden header name), so an SDK call from a browser got the HTML error page where it expects JSON, andAcceptwas ignored entirely:Intent now decides first: JSON when
Acceptprefers it, or whenSec-Fetch-Mode/X-Requested-Withmark the request as script-initiated. UA sniffing stays as the fallback, where it only catches genuine top-level navigations — which is what the HTML pages are for.Tests
packages/shared/pkg/proxy/cors/cors_test.go— the helpers in isolation.packages/shared/pkg/proxy/cors_test.go— handler-level, plus socket-level tests that reproduce the browser's preflight-then-request exchange against a missing and an unreachable sandbox, and assert the live-sandbox path does not get a duplicated header.packages/shared/pkg/proxy/template/template_test.go— a table over all eight constructors asserting the header in both HTML and JSON modes, so a newly added template cannot silently miss it, plus the content-negotiation matrix.tests/integration/.../sandbox_not_found_test.go— extended: a stopped sandbox's preflight is 2xx with the echoed headers and the follow-upGETis 502 withAccess-Control-Allow-Origin.Both new behaviors were checked to have teeth by reverting the fix and watching the tests fail. Verified with
go test -race ./packages/shared/pkg/proxy/...,make fmtandmake lint(the one remaining lint failure,cmd/create-build/proxyport_test.go, is pre-existing onmainand platform-related).Rollout
No SDK release is coupled to this; the flag removal is test-only. Deploy order: infra fix → staging → production → drop
canObserveStoppedSandboxfrom the JS SDK'stests/setup.ts, which should leave the 13 currently-skipped browser tests green with no SDK change. The browser CI leg only runs against production, so those stay red until the production rollout.🤖 Generated with Claude Code