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".
…_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".
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".
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".
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 👍 / 👎.
|
Thanks for sharing the idea. The allowlist for unix local client is a valid feature addition, so #4640 will add the change and its commit includes you as a co-author. |
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).