Skip to content

sandboxing: plumb split sandbox policies through runtime#13439

Merged
viyatb-oai merged 4 commits intomainfrom
pr13439
Mar 7, 2026
Merged

sandboxing: plumb split sandbox policies through runtime#13439
viyatb-oai merged 4 commits intomainfrom
pr13439

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 4, 2026

Why

#13434 introduces split FileSystemSandboxPolicy and NetworkSandboxPolicy, but the runtime still made most execution-time sandbox decisions from the legacy SandboxPolicy projection.

That projection loses information about combinations like unrestricted filesystem access with restricted network access. In practice, that means the runtime can choose the wrong platform sandbox behavior or set the wrong network-restriction environment for a command even when config has already separated those concerns.

This PR carries the split policies through the runtime so sandbox selection, process spawning, and exec handling can consult the policy that actually matters.

What changed

  • threaded FileSystemSandboxPolicy and NetworkSandboxPolicy through TurnContext, ExecRequest, sandbox attempts, shell escalation state, unified exec, and app-server exec overrides
  • updated sandbox selection in core/src/sandboxing/mod.rs and core/src/exec.rs to key off FileSystemSandboxPolicy.kind plus NetworkSandboxPolicy, rather than inferring behavior only from the legacy SandboxPolicy
  • updated process spawning in core/src/spawn.rs and the platform wrappers to use NetworkSandboxPolicy when deciding whether to set CODEX_SANDBOX_NETWORK_DISABLED
  • kept additional-permissions handling and legacy ExternalSandbox compatibility projections aligned with the split policies, including explicit user-shell execution and Windows restricted-token routing
  • updated callers across core, app-server, and linux-sandbox to pass the split policies explicitly

Verification

  • added regression coverage in core/tests/suite/user_shell_cmd.rs to verify RunUserShellCommand does not inherit CODEX_SANDBOX_NETWORK_DISABLED from the active turn
  • added coverage in core/src/exec.rs for Windows restricted-token sandbox selection when the legacy projection is ExternalSandbox
  • updated Linux sandbox coverage in linux-sandbox/tests/suite/landlock.rs to exercise the split-policy exec path
  • verified the current PR state with just clippy

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link
Contributor

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3d8f475bce

ℹ️ 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".

@bolinfest bolinfest force-pushed the pr13434 branch 2 times, most recently from 15b518e to 7abd701 Compare March 4, 2026 09:10
@bolinfest bolinfest force-pushed the pr13434 branch 2 times, most recently from 5cd0fd9 to 974327b Compare March 6, 2026 20:02
@bolinfest bolinfest force-pushed the pr13439 branch 3 times, most recently from 0e7b5f0 to b4e9baa Compare March 6, 2026 21:03
@bolinfest bolinfest force-pushed the pr13434 branch 2 times, most recently from c19cf38 to fac9b40 Compare March 6, 2026 22:12
@bolinfest bolinfest force-pushed the pr13439 branch 2 times, most recently from 45efe16 to eb5869b Compare March 6, 2026 23:02
Base automatically changed from pr13434 to main March 6, 2026 23:39
bolinfest added a commit that referenced this pull request Mar 6, 2026
…guage in config.toml (#13434)

## Why

`SandboxPolicy` currently mixes together three separate concerns:

- parsing layered config from `config.toml`
- representing filesystem sandbox state
- carrying basic network policy alongside filesystem choices

That makes the existing config awkward to extend and blocks the new TOML
proposal where `[permissions]` becomes a table of named permission
profiles selected by `default_permissions`. (The idea is that if
`default_permissions` is not specified, we assume the user is opting
into the "traditional" way to configure the sandbox.)

This PR adds the config-side plumbing for those profiles while still
projecting back to the legacy `SandboxPolicy` shape that the current
macOS and Linux sandbox backends consume.

It also tightens the filesystem profile model so scoped entries only
exist for `:project_roots`, and so nested keys must stay within a
project root instead of using `.` or `..` traversal.

This drops support for the short-lived `[permissions.network]` in
`config.toml` because now that would be interpreted as a profile named
`network` within `[permissions]`.

## What Changed

- added `PermissionsToml`, `PermissionProfileToml`,
`FilesystemPermissionsToml`, and `FilesystemPermissionToml` so config
can parse named profiles under `[permissions.<profile>.filesystem]`
- added top-level `default_permissions` selection, validation for
missing or unknown profiles, and compilation from a named profile into
split `FileSystemSandboxPolicy` and `NetworkSandboxPolicy` values
- taught config loading to choose between the legacy `sandbox_mode` path
and the profile-based path without breaking legacy users
- introduced `codex-protocol::permissions` for the split filesystem and
network sandbox types, and stored those alongside the legacy projected
`sandbox_policy` in runtime `Permissions`
- modeled `FileSystemSpecialPath` so only `ProjectRoots` can carry a
nested `subpath`, matching the intended config syntax instead of
allowing invalid states for other special paths
- restricted scoped filesystem maps to `:project_roots`, with validation
that nested entries are non-empty descendant paths and cannot use `.` or
`..` to escape the project root
- kept existing runtime consumers working by projecting
`FileSystemSandboxPolicy` back into `SandboxPolicy`, with an explicit
error for profiles that request writes outside the workspace root
- loaded proxy settings from top-level `[network]`
- regenerated `core/config.schema.json`

## Verification

- added config coverage for profile deserialization,
`default_permissions` selection, top-level `[network]` loading, network
enablement, rejection of writes outside the workspace root, rejection of
nested entries for non-`:project_roots` special paths, and rejection of
parent-directory traversal in `:project_roots` maps
- added protocol coverage for the legacy bridge rejecting non-workspace
writes

## Docs

- update the Codex config docs on developers.openai.com/codex to
document named `[permissions.<profile>]` entries, `default_permissions`,
scoped `:project_roots` syntax, the descendant-path restriction for
nested `:project_roots` entries, and top-level `[network]` proxy
configuration






---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13434).
* #13453
* #13452
* #13451
* #13449
* #13448
* #13445
* #13440
* #13439
* __->__ #13434
@bolinfest bolinfest requested a review from viyatb-oai March 6, 2026 23:47
@bolinfest bolinfest enabled auto-merge (squash) March 7, 2026 00:44
@viyatb-oai viyatb-oai disabled auto-merge March 7, 2026 01:02
@viyatb-oai viyatb-oai enabled auto-merge (squash) March 7, 2026 01:40
…merge

# Conflicts:
#	codex-rs/app-server/src/codex_message_processor.rs
#	codex-rs/core/src/exec.rs
@viyatb-oai viyatb-oai merged commit 22ac6b9 into main Mar 7, 2026
56 of 61 checks passed
@viyatb-oai viyatb-oai deleted the pr13439 branch March 7, 2026 02:30
@github-actions github-actions bot locked and limited conversation to collaborators Mar 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants