fix(sandbox): allowlist exec environment; require explicit opt-in for unconfined Linux - #4617
fix(sandbox): allowlist exec environment; require explicit opt-in for unconfined Linux#4617simpleqt wants to merge 5 commits into
Conversation
… unconfined Linux Two gaps between the documented environment isolation and the Unix-local implementation: - Commands inherited the full host environment (os.environ.copy()), so a sandboxed command could read host credentials (OPENAI_API_KEY, AWS_*, GITHUB_TOKEN) with printenv. The exec context now inherits only an allowlist (PATH, locale, TZ, TERM, TMPDIR, CA locations) plus manifest environment entries; inherit_environment=True restores the old behavior for workflows that depend on it. - On Linux, _confined_exec_command returned commands unmodified (the sandbox-exec wrapper is macOS-only), so 'sandboxed' commands ran with no OS-level confinement while the docs recommended UnixLocalSandboxClient as the default local client on macOS or Linux. Creating a session on Linux now raises with guidance toward DockerSandboxClient unless allow_unconfined_linux=True is passed explicitly (with a runtime warning). macOS behavior is unchanged. Docs updated accordingly (clients.md environment-isolation notes, guide.md client selection wording).
![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: 14b4307609
ℹ️ 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".
| if self.state.inherit_environment: | ||
| env = dict(os.environ) |
There was a problem hiding this comment.
When inherit_environment is read from UnixLocalSandboxSessionState, any serialized RunState/session_state that deserializes with "inherit_environment": true makes _resolved_exec_context() copy the current host os.environ. On a resumed sandbox, this bypasses the new default without the current caller passing UnixLocalSandboxClientOptions(inherit_environment=True), so a supplied resume payload can expose host credentials to sandbox commands via printenv. Keep the full-env opt-in in trusted run/client options, or strip it during deserialization, instead of honoring persisted state.
AGENTS.md reference: AGENTS.md:L151-L151
Useful? React with 👍 / 👎.
| ) -> SandboxSession: | ||
| resolved_options = options if options is not None else UnixLocalSandboxClientOptions() | ||
| manifest = manifest if manifest is not None else Manifest() | ||
| if sys.platform != "darwin" and not resolved_options.allow_unconfined_linux: |
There was a problem hiding this comment.
This opt-in check only runs during create(), but the normal resume path (RunState/SandboxRunConfig.session_state) calls UnixLocalSandboxClient.resume() and returns UnixLocalSandboxSession.from_state(state) without any equivalent check. On Linux, a resumed Unix-local session still reaches _confined_exec_command() where non-darwin returns the raw command, so resumed workflows execute unconfined even though no allow_unconfined_linux=True was supplied. Apply the same trusted opt-in before resuming.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
| "SSL_CERT_DIR", | ||
| "REQUESTS_CA_BUNDLE", | ||
| "NODE_EXTRA_CA_CERTS", | ||
| "PIP_INDEX_URL", |
There was a problem hiding this comment.
When hosts use private package indexes, PIP_INDEX_URL often embeds a username/password or token. Because this name is in the default allowlist, a sandboxed command can still read that token with printenv PIP_INDEX_URL even with inherit_environment=False, undermining the credential-isolation fix. Drop it from the inherited set and require callers to put it in the manifest environment when it is intentionally needed.
Useful? React with 👍 / 👎.
…_INDEX_URL Review follow-ups: - The inherit_environment flag lived on the serializable session state, so a RunState payload deserialized with inherit_environment=true would silently re-enable full host-environment inheritance on resume. Both flags are now set from trusted client-constructor values on every create()/resume(); the serialized value is overwritten, never trusted. - The Linux unconfined check only ran in create(); resume() now enforces the same opt-in (state.error op=create/resume, same guidance). - PIP_INDEX_URL removed from the default env allowlist: private package indexes commonly embed credentials in the URL (user:pass@host).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1841aefb65
ℹ️ 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".
| ) -> SandboxSession: | ||
| if not isinstance(state, UnixLocalSandboxSessionState): | ||
| raise TypeError("UnixLocalSandboxClient.resume expects a UnixLocalSandboxSessionState") | ||
| if sys.platform != "darwin" and not self._allow_unconfined_linux: |
There was a problem hiding this comment.
When resuming via RunState or SandboxRunConfig.session_state on Linux, the current trusted SandboxRunConfig.options is still available to the runtime but client.resume() has no way to see it; this branch checks only the constructor flag while fresh create() accepts resolved_options.allow_unconfined_linux. A workflow that opts in with UnixLocalSandboxClientOptions(allow_unconfined_linux=True) can create and serialize a session, then fail to resume with the same run config. Please either make this opt-in client-only or apply the trusted run option before this check. The fresh evidence is the current diff's create-vs-resume split, not the earlier missing-check issue.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
Review follow-up: create() accepted allow_unconfined_linux via per-call options while resume() only honored the constructor flag, so a workflow opting in through options could create and serialize a session but fail to resume it under the same run config. Both flags (allow_unconfined_linux, inherit_environment) are now constructor arguments on UnixLocalSandboxClient only — the single trusted channel that applies to create() and resume() alike; the options class documents why they are deliberately not per-call options. Regression test resumes with the same opted-in client.
|
Good catch on the create/resume asymmetry — fixed by making both flags constructor-only on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf95e16db7
ℹ️ 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".
| class UnixLocalSandboxSessionState(SandboxSessionState): | ||
| type: Literal["unix_local"] = "unix_local" | ||
| workspace_root_owned: bool = False | ||
| inherit_environment: bool = False |
There was a problem hiding this comment.
When a Runner-owned Unix-local session is serialized, client.serialize_session_state(resources.state) now includes the new inherit_environment sandbox-state field under the existing RunState schema version 1.17. Because src/agents/run_state.py is not bumped, an SDK at the previous implementation that already accepts 1.17 can read this newer snapshot, ignore the unknown field, and resume with the old full-host-environment behavior instead of the intended allowlisted default. Please either bump/add the RunState schema summary and compatibility regression, or keep this runtime opt-in out of the persisted RunState shape if it is not meant to be durable.
AGENTS.md reference: AGENTS.md:L204-L204
Useful? React with 👍 / 👎.
Review follow-up: inherit_environment lived on the serializable UnixLocalSandboxSessionState, so RunState snapshots carried it under the unbumped 1.17 schema — an older SDK reading the snapshot would ignore the unknown field and resume with the old full-host-environment behavior. The flag is now a runtime-only attribute on the session object, wired from the trusted client constructor in create() and resume(); it never enters the persisted state. Regression test asserts the serialized payload contains no inherit_environment key.
|
Took the second option — the flag is now runtime-only: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa17d8c7b
ℹ️ 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".
| "to explicitly accept unconfined host execution." | ||
| ), | ||
| error_code=ErrorCode.UNCONFINED_LINUX_NOT_ALLOWED, | ||
| op="create", |
There was a problem hiding this comment.
When the Linux guard fires while the call also carries protected mount authority, such as a Manifest with inline S3 credentials, @redact_mount_error_data only preserves structured SandboxErrors whose op is listed in OpName; create and resume are not listed there. This new ConfigurationError is therefore replaced with the generic mount-redaction RuntimeError, so callers lose the UNCONFINED_LINUX_NOT_ALLOWED code and the actionable Docker/opt-in guidance in exactly the credential-bearing manifest cases this decorator protects. Add the new operations to OpName or use an existing operation before raising here. .agents/references/sandbox-runtime-boundary.mdL45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 4667a57. Added "create"/"resume" to OpName (errors.py), so the guard's ConfigurationError now goes through _replace_structured_sandbox_error and survives the redaction boundary with its type, UNCONFINED_LINUX_NOT_ALLOWED code and retryable=False intact instead of collapsing into the generic RuntimeError. The human-readable message is still replaced by the boundary's standard redaction text for credential-bearing manifests (by design there), but programmatic handling now works. Added test_linux_guard_error_survives_mount_redaction which builds a manifest with an inline-credential S3Mount and asserts the structured error (and that the secret never appears in str(error)); it fails with a plain RuntimeError before the fix.
| | Client | Install | Choose it when | Example | | ||
| | --- | --- | --- | --- | | ||
| | `UnixLocalSandboxClient` | none | Fastest local iteration on macOS or Linux. Good default for local development. | [Unix-local starter](https://github.com/openai/openai-agents-python/blob/main/examples/sandbox/unix_local_runner.py) | | ||
| | `UnixLocalSandboxClient` | none | Fastest local iteration on macOS (Seatbelt confinement). On Linux it requires `allow_unconfined_linux=True` because there is no OS-level confinement — prefer `DockerSandboxClient` there. | [Unix-local starter](https://github.com/openai/openai-agents-python/blob/main/examples/sandbox/unix_local_runner.py) | |
There was a problem hiding this comment.
This row now warns that Linux requires allow_unconfined_linux=True, but it still links the Unix-local starter as the example; a repo search of docs/sandbox and examples/sandbox still finds many snippets, including examples/sandbox/unix_local_runner.py, constructing UnixLocalSandboxClient() without that opt-in. On Linux the new create guard raises before workspace setup, so the linked starter and copied snippets no longer run unless users infer and add the unsafe opt-in themselves. Please update the linked example/snippets to select Docker on Linux or make the explicit opt-in visible.
AGENTS.md reference: AGENTS.md:L157-L157
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4667a57 — all Unix-local example snippets now go through a shared unix_local_client() helper in examples/sandbox/misc/example_support.py: on Linux it exits early with actionable guidance unless AGENTS_ALLOW_UNCONFINED_LINUX=1 is set (pointing at the Docker examples), and on macOS behavior is unchanged. Converted all construction sites (unix_local_runner.py incl. its docstring, unix_local_pty.py, memory*.py, handoffs.py, sandbox_agent*_*.py, docs/coding_task.py, healthcare_support/workflow.py, tutorials/misc.py, both temporal examples, shared_session_workdirs.py), so a repo search no longer finds bare UnixLocalSandboxClient() constructions outside tests. Also annotated the README and docs/sandbox_agents.md snippets, and docs/sandbox/clients.md now mentions the env gate and links the helper.
Add "create"/"resume" to OpName so the unconfined-Linux ConfigurationError survives the mount-redaction boundary as a structured error with its error_code and retryable classification intact; previously it collapsed into the generic redacted RuntimeError for credential-bearing manifests. Route every Unix-local example through a shared unix_local_client() helper that exits with actionable guidance on Linux unless AGENTS_ALLOW_UNCONFINED_LINUX=1 is set (Docker examples recommended there), and note the opt-in in the README/docs snippets.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4667a571b1
ℹ️ 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".
|
|
||
| sandbox_clients: list[SandboxClientProvider] = [ | ||
| SandboxClientProvider("local", UnixLocalSandboxClient()), | ||
| SandboxClientProvider("local", unix_local_client()), |
There was a problem hiding this comment.
On Linux without AGENTS_ALLOW_UNCONFINED_LINUX=1, this unconditional unix_local_client() call raises SystemExit before the worker can register Daytona, E2B, or Docker providers. That blocks the Temporal example even for its default daytona backend or a user selecting Docker, because startup never reaches the later provider-registration and warning logic. Treat the local provider like the other optional backends: only append it when the helper can build it, otherwise let the worker continue without local.
AGENTS.md reference: AGENTS.md:L157-L157
Useful? React with 👍 / 👎.
Two gaps between the documented environment isolation and the Unix-local
implementation:
sandboxed command could read host credentials (OPENAI_API_KEY, AWS_*,
GITHUB_TOKEN) with printenv. The exec context now inherits only an
allowlist (PATH, locale, TZ, TERM, TMPDIR, CA locations) plus manifest
environment entries; inherit_environment=True restores the old
behavior for workflows that depend on it.
sandbox-exec wrapper is macOS-only), so 'sandboxed' commands ran with
no OS-level confinement while the docs recommended UnixLocalSandboxClient
as the default local client on macOS or Linux. Creating a session on
Linux now raises with guidance toward DockerSandboxClient unless
allow_unconfined_linux=True is passed explicitly (with a runtime
warning). macOS behavior is unchanged.
Docs updated accordingly (clients.md environment-isolation notes,
guide.md client selection wording).