feat(py-sdk): Add follow_redirects for sandbox#1123
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a follow_redirects option to the Python sandbox SDK connection configuration and propagates it through both async and sync HTTPX clients (including SSE clients and generated-client wrappers), plus documents and tests the behavior.
Changes:
- Add
follow_redirects: bool = FalsetoConnectionConfigandConnectionConfigSync. - Pass
follow_redirectsinto all handwritten async/sync httpx clients and generated-client wrappers used by adapters (including SSE clients). - Add unit tests and update the Python SDK README configuration table.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sdks/sandbox/python/src/opensandbox/config/connection.py | Adds follow_redirects to async connection config. |
| sdks/sandbox/python/src/opensandbox/config/connection_sync.py | Adds follow_redirects to sync connection config. |
| sdks/sandbox/python/src/opensandbox/adapters/sandboxes_adapter.py | Propagates follow_redirects into async lifecycle clients. |
| sdks/sandbox/python/src/opensandbox/adapters/metrics_adapter.py | Propagates follow_redirects into async metrics clients. |
| sdks/sandbox/python/src/opensandbox/adapters/isolated_filesystem_adapter.py | Propagates follow_redirects into async isolated filesystem clients. |
| sdks/sandbox/python/src/opensandbox/adapters/isolated_adapter.py | Propagates follow_redirects into async isolated + SSE clients. |
| sdks/sandbox/python/src/opensandbox/adapters/health_adapter.py | Propagates follow_redirects into async health clients. |
| sdks/sandbox/python/src/opensandbox/adapters/filesystem_adapter.py | Propagates follow_redirects into async filesystem clients. |
| sdks/sandbox/python/src/opensandbox/adapters/egress_adapter.py | Propagates follow_redirects into async egress clients. |
| sdks/sandbox/python/src/opensandbox/adapters/diagnostics_adapter.py | Propagates follow_redirects into async diagnostics clients. |
| sdks/sandbox/python/src/opensandbox/adapters/command_adapter.py | Propagates follow_redirects into async command + SSE clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/sandboxes_adapter.py | Propagates follow_redirects into sync lifecycle clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/metrics_adapter.py | Propagates follow_redirects into sync metrics clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/isolated_filesystem_adapter.py | Propagates follow_redirects into sync isolated filesystem clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/isolated_adapter.py | Propagates follow_redirects into sync isolated + SSE clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/health_adapter.py | Propagates follow_redirects into sync health clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/filesystem_adapter.py | Propagates follow_redirects into sync filesystem clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/egress_adapter.py | Propagates follow_redirects into sync egress clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/diagnostics_adapter.py | Propagates follow_redirects into sync diagnostics clients. |
| sdks/sandbox/python/src/opensandbox/sync/adapters/command_adapter.py | Propagates follow_redirects into sync command + SSE clients. |
| sdks/sandbox/python/tests/test_follow_redirects_config.py | Adds unit tests validating redirect behavior wiring for async/sync + SSE. |
| sdks/sandbox/python/README.md | Documents follow_redirects in config table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| follow_redirects: bool = Field( | ||
| default=False, description="Whether HTTP clients should follow redirects" | ||
| ) |
| | `request_timeout` | Timeout for API requests | 30 seconds | - | | ||
| | `debug` | Enable debug logging for HTTP requests | `False` | - | | ||
| | `headers` | Custom HTTP headers | Empty | - | | ||
| | `follow_redirects` | Follow HTTP redirects for SDK requests | `False` | - | |
![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: 8ec44773be
ℹ️ 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".
| headers=headers, | ||
| timeout=timeout, | ||
| transport=self.connection_config.transport, | ||
| follow_redirects=self.connection_config.follow_redirects, |
There was a problem hiding this comment.
When users set follow_redirects=True, this client follows 30x responses while carrying the headers dict that includes OPEN-SANDBOX-API-KEY above. httpx only strips the standard Authorization header on cross-origin redirects, so an open redirect or misconfigured API/proxy returning Location: https://... can make the SDK send the API key to that other host; please constrain authenticated redirects to same-origin or remove the custom auth header before following.
Useful? React with 👍 / 👎.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbd6900e35
ℹ️ 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".
| headers=headers, | ||
| timeout=timeout, | ||
| transport=self.connection_config.transport, | ||
| follow_redirects=self.connection_config.follow_redirects, |
There was a problem hiding this comment.
When secure access or egress auth is enabled, the endpoint resolver returns custom auth headers for sandbox calls (server/opensandbox_server/services/endpoint_auth.py:37-44; specs/sandbox-lifecycle.yml:1299-1302), and this client installs those headers as defaults before enabling redirects. For a 30x from execd/egress to another host, httpx strips standard Authorization but not these OPENSANDBOX-* headers, so the per-sandbox access tokens are resent to the redirect target. Please only follow same-origin redirects here, or rebuild redirected requests without endpoint headers.
Useful? React with 👍 / 👎.
Pangjiping
left a comment
Pangjiping
left a comment
There was a problem hiding this comment.
Review Summary
Core feature well-implemented. Cross-origin API key stripping addresses prior security feedback correctly — SDK hook runs last, origin comparison covers scheme+host+port.
| Priority | Issue |
|---|---|
| P1 | Only strips OPEN-SANDBOX-API-KEY — endpoint auth headers (OPENSANDBOX-ACCESS-KEY etc.) still leak on cross-origin redirect |
| P2 | Missing tests: same-origin preserves key, default follow_redirects=False rejects redirect |
| P3 | 20 adapters repeat identical kwargs block — extract to factory method |
See inline comments for details.
|
|
||
| hooks = _copy_event_hooks(event_hooks) | ||
| hooks.setdefault("request", []).append(strip_api_key) | ||
| return hooks |
There was a problem hiding this comment.
P1 — Only strips OPEN-SANDBOX-API-KEY, endpoint auth headers still leak.
Adapters like command_adapter and isolated_adapter inject self.execd_endpoint.headers which may include OPENSANDBOX-ACCESS-KEY or other per-sandbox auth headers. These are NOT stripped on cross-origin redirect.
Suggestion: strip all headers matching OPEN-SANDBOX-* / OPENSANDBOX-* prefix instead of a single header name:
def _strip_api_key_for_cross_origin_request(
request: httpx.Request,
*,
base_origin: tuple[str, str | None, int | None],
) -> None:
if _origin(request.url) == base_origin:
return
keys_to_remove = [
k for k in request.headers.keys()
if k.upper().startswith(("OPEN-SANDBOX-", "OPENSANDBOX-"))
]
for k in keys_to_remove:
del request.headers[k]| headers=headers, | ||
| timeout=timeout, | ||
| transport=self.connection_config.transport, | ||
| follow_redirects=self.connection_config.follow_redirects, |
There was a problem hiding this comment.
P3 — Identical kwargs block repeated across 20 adapters.
Every adapter has the same 3-4 line pattern:
follow_redirects=self.connection_config.follow_redirects,
event_hooks=build_async_api_key_redirect_event_hooks(
base_url, self.connection_config.event_hooks
),Fragile — new adapters will easily miss this. Consider a factory method on ConnectionConfig:
def build_httpx_kwargs(self, base_url: str) -> dict:
return {
"follow_redirects": self.follow_redirects,
"event_hooks": build_async_api_key_redirect_event_hooks(
base_url, self.event_hooks
),
}Then adapters just do **self.connection_config.build_httpx_kwargs(base_url).
| ), | ||
| ) | ||
| event_hooks: dict[str, list[Callable[..., Any]]] = Field( | ||
| default_factory=dict, |
There was a problem hiding this comment.
P2 — event_hooks type does not distinguish sync vs async callables.
ConnectionConfig (async) and ConnectionConfigSync both declare dict[str, list[Callable[..., Any]]]. A user could pass sync hooks to async config — runtime fails with await on a sync function, no early validation.
Not a blocker, but worth a docstring note or type alias like AsyncEventHook = Callable[..., Awaitable[Any]].
| self._client = Client( | ||
| base_url=base_url, | ||
| timeout=timeout, | ||
| follow_redirects=self.connection_config.follow_redirects, |
There was a problem hiding this comment.
P2 — Generated Client gets follow_redirects but no event_hooks.
If the generated Client ever makes requests directly (not through the injected httpx client), OPEN-SANDBOX-API-KEY will leak on cross-origin redirect. Please verify the generated Client always delegates to the injected httpx client for actual HTTP calls.
| headers={"Location": "http://redirect.local/final"}, | ||
| request=request, | ||
| ) | ||
| return httpx.Response(204, request=request) |
There was a problem hiding this comment.
P2 — Missing test cases:
-
Same-origin redirect preserves API key — tests verify cross-origin strips key, but no test confirms same-origin redirect keeps it intact. Important invariant.
-
follow_redirects=False(default) surfaces 307 to caller — should verify default config does NOT follow redirects.
Example:
def test_same_origin_redirect_preserves_api_key() -> None:
# 307 to same host should keep OPEN-SANDBOX-API-KEY
...
def test_default_config_does_not_follow_redirects() -> None:
cfg = ConnectionConfigSync(protocol="http", ...)
assert cfg.follow_redirects is False
# verify 307 response returned as-is
...
Summary
follow_redirectsonConnectionConfigandConnectionConfigSyncso Python SDK users can control httpx redirect behavior.Testing
PYTHONPATH=src UV_CACHE_DIR=/tmp/uv-cache uv run pytest tests/test_follow_redirects_config.py tests/test_connection_config.py tests/ test_command_service_sse_client_config.py -q --no-covPYTHONPATH=src UV_CACHE_DIR=/tmp/uv-cache uv run ruff check src/opensandbox/config/connection.py src/opensandbox/config/ connection_sync.py src/opensandbox/adapters src/opensandbox/sync/adapters tests/test_follow_redirects_config.pyBreaking Changes
Checklist