Conversation
…config trade-offs
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Independence Check.
|
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Independence Check.
|
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Independence Check.
|
The sandbox router read the sandbox URL from the environment alone, so an install that leaves E2B_SANDBOX_URL unset — the runtime-configured case, where each browser is told the host it reached the dashboard on — sent every server-side envd call to the build-time domain instead. Killing a terminal's pty on leaving the page failed every time. resolveServerSandboxUrl applies the browser's rule to the request the procedure is serving, and the browser config is now expressed through it so the two cannot drift.
8631ec1 to
de4b2e0
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any new bugs. The commit pushed since my last review (8631ec1) substantively addresses the concerns raised in my earlier passes: the header-derived sandbox-origin fallback that trusted X-Forwarded-Host/Proto (the SSRF-adjacent path) was removed entirely in favor of operator-configured values only, resolveSandboxUrl() is now validated eagerly at startup via validateRuntimeConfig() alongside the other URL resolvers, and isSecureCookie() now throws on an unrecognized DASHBOARD_COOKIE_SECURE value instead of silently falling back to secure=true. Because this PR still touches auth-cookie handling, request-origin/URL resolution, and container startup validation, a human look is worthwhile as an additional step before merging.
Extended reasoning...
Overview
Since my previous reviews on this PR, one further commit (8631ec1, "fix(config): validate startup settings and use configured sandbox URLs") landed. I re-ran the full diff against the base and read the current state of src/core/server/runtime-config.ts, src/configs/cookies.ts, src/instrumentation.ts, and src/core/server/api/routers/sandbox.ts directly (not just relying on the resolution metadata) to confirm what changed.
Security risks
All four items I previously flagged across two reviews were substantively fixed in this commit, verified by direct code reading:
- The entire
requestOrigin()/forwardedProtocol()/hostnameOf()/resolveServerSandboxUrl()machinery that trustedX-Forwarded-Host/X-Forwarded-Protowith no allow-list (the SSRF concern) was deleted.resolveBrowserRuntimeConfig()and the sandbox router now call the header-freeresolveSandboxUrl(), which only reads operator-set env vars — this also moots the related multi-valueX-Forwarded-Hostparsing bug, since that code path no longer exists. resolveSandboxUrl()is now called eagerly from a newvalidateRuntimeConfig(), invoked fromsrc/instrumentation.ts'sregister()at server startup (withprocess.exit(1)on failure), closing the gap where a malformed sandbox URL previously surfaced only when a user opened a terminal.isSecureCookie()now explicitly matches'true'/'false'and throws on anything else, rather than treating any non-'false'string (e.g.'0','off') assecure=true; this throw is also wired intovalidateRuntimeConfig()so a badDASHBOARD_COOKIE_SECUREfails startup instead of silently reproducing the plain-HTTP login loop the variable exists to fix.- New tests in
tests/integration/runtime-config-startup.test.tscover startup rejection for each malformed URL var and for an invalid cookie flag, plus successful startup with a valid configuration — matching the code changes.
I did not find new security issues in this delta; the change is a genuine narrowing of trust (removing a header-trust path) plus fail-fast validation, not new attack surface.
Level of scrutiny
This PR remains large and touches security-relevant code (auth cookie flags, server-side URL/origin resolution used for SDK routing, container startup). That, combined with the significant design change in this latest commit (removing the request-header-based sandbox routing entirely), is exactly the kind of change where a maintainer's sign-off adds value beyond an automated pass, even though the automated review this round found nothing new and confirms the previously raised issues were fixed correctly.
Other factors
The fix commit was authored by drankou, who is a CODEOWNER for this repository (per CODEOWNERS), and the same person resolved the four inline threads I had opened — this is independent-of-author engagement, and my own reading of the code corroborates that the resolutions reflect real fixes rather than administrative closures. No CHANGES_REQUESTED or unresolved third-party objections remain visible in the timeline. Test coverage for the new startup-validation and cookie paths is present and appears to match the implementation.
This review covers commit 8631ec1, which is no longer the latest commit on this pull request; later commits are not covered by it.
There was a problem hiding this comment.
i think you should do validation inside env.ts for consistency
| E2B_INFRA_API_URL: z.url().optional(), | ||
| E2B_DASHBOARD_API_URL: z.url().optional(), | ||
| E2B_SANDBOX_URL: z.url().optional(), | ||
|
|
||
| // Read on the server and explicitly passed to the browser by the layout. | ||
| PUBLIC_E2B_DOMAIN: z.string().optional(), | ||
| PUBLIC_SANDBOX_URL: z.url().optional(), |
There was a problem hiding this comment.
let's make them required and just deprecate the old ones instead of fallback
Summary
Run the Dashboard from one prebuilt container image, configured for each installation when the container starts. The authenticated Next.js layout sends the public domain and sandbox URL to
ClientConfigProvider; the terminal and filesystem inspector read them throughuseClientConfig()on their first render.PUBLIC_E2B_DOMAINand optionalPUBLIC_SANDBOX_URLoverride the legacy names at runtime. ExistingNEXT_PUBLIC_*build configuration and theE2B_SANDBOX_URLalias remain supported.{ domain, sandboxUrl }is serialized for browser consumers. Configuration arrives with the page, so no/api/configendpoint or client fetch/cache is needed.PUBLIC_SANDBOX_URL.DASHBOARD_COOKIE_SECURE=falsesupports plain-HTTP installations. Secure cookies remain the production default. Invalid cookie flags and resolved API/sandbox URLs stop server initialization with a named configuration error, even when telemetry is disabled.Validation
PUBLIC_SANDBOX_URL, legacyE2B_SANDBOX_URL, andDASHBOARD_COOKIE_SECURE. Earlier isolated checks verified that one image serves different runtime configurations and does not expose server endpoints or credentials.Deployment notes
PUBLIC_has no special meaning to Next.js; the server explicitly reads and passes these two settings. Restart the container and reload open pages after configuration changes.PUBLIC_SANDBOX_URLexplicitly for a local proxy, such ashttp://127.0.0.1:3002when both run on the same machine. There is no request-host fallback.Containercheck is path-filtered and should not be made required.