fix(sdk): carry API key on execd requests per use_server_proxy declaration - #1687
fix(sdk): carry API key on execd requests per use_server_proxy declaration#1687jiawen7777 wants to merge 1 commit into
Conversation
…ation Multi-tenant servers authenticate the proxy route too, but execd adapters sent no API key — Sandbox.connect() surfaced the 401 as a 30s timeout. Attach the key only when the client declared proxy mode; in direct mode it must never travel into the untrusted sandbox. Fixes opensandbox-group#1686
github-actions
Bot
added
sdk/python
size/L
![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: 824254411d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if connection_config.use_server_proxy: | ||
| api_key = connection_config.get_api_key() | ||
| if api_key: | ||
| headers["OPEN-SANDBOX-API-KEY"] = api_key |
There was a problem hiding this comment.
When a JavaScript client supplies an API key while using the default direct mode, ConnectionConfig inserts that key into its shared headers (sdks/sandbox/javascript/src/config/connection.ts:342-346), and DefaultAdapterFactory.createExecdStack() forwards those headers to execd (sdks/sandbox/javascript/src/factory/defaultAdapterFactory.ts:53-62). Thus the new direct-mode security invariant is Python-only: JavaScript still sends the lifecycle credential into the untrusted sandbox. Apply the same useServerProxy gate to JavaScript execd/egress header construction while retaining authentication on lifecycle requests.
AGENTS.md reference: sdks/AGENTS.md:L122-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Make sense and it's the mirror image of the bug fixed here: instead of missing the key in proxy mode, the JS/C# SDKs attach it unconditionally, so in direct mode the credential travels straight into the untrusted sandbox with no server in the path to strip it.
There was a problem hiding this comment.
I've kept this PR scoped to Python (single concern) and opened #1689 to track the JS/C# side — the fix direction is the same declaration-gated injection the Go SDK already implements.
1 participant
Summary
HealthAdaptermissing authentication headers whenuse_server_proxy=True, causing health check to always fail with 401 #248)Sandbox.connect()surfaced the 401 as a 30s health-check timeout.SandboxEndpoint.build_request_headers()is now the shared header assembly used by all 14 execd adapters (async + sync). The key is attached only when the client declared server-proxy mode (ConnectionConfig.use_server_proxy); in direct mode the key is never sent — execd performs no auth and the key must not travel into the untrusted sandbox.Testing
6 new tests in
tests/test_execd_proxy_auth_headers.pylock: declared proxy → key attached regardless of endpoint URL shape; direct mode → key never attached; no key configured → header omitted (async + sync). Full SDK suite,ruff check, andpyrightall pass.e2e: verified against a multi-tenant deployment — without the fix
connect()fails after 30s withSandboxReadyTimeoutExceptionwhile the server logs401 MISSING_API_KEYon the proxy health route; with the fixconnect()succeeds in <1s and commands/files work with no workaround.Breaking Changes
Checklist
use_server_proxy=True; single-tenant deployments unaffected (server already exempts proxy paths per fix(server): skip auth for proxy-to-sandbox paths; strict path matching #250)