Skip to content

protocol: keep root carveouts sandboxed#13452

Merged
viyatb-oai merged 8 commits intomainfrom
pr13452
Mar 8, 2026
Merged

protocol: keep root carveouts sandboxed#13452
viyatb-oai merged 8 commits intomainfrom
pr13452

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 4, 2026

Why

A restricted filesystem policy that grants :root read or write access but also carries explicit deny entries should still behave like scoped access with carveouts, not like unrestricted disk access.

Without that distinction, later platform backends cannot preserve blocked subpaths under root-level permissions because the protocol layer reports the policy as fully unrestricted.

What changed

  • taught FileSystemSandboxPolicy to treat root access plus explicit deny entries as scoped access rather than full-disk access
  • derived readable and writable roots from the filesystem root when root access is combined with carveouts, while preserving the denied paths as read-only subpaths
  • added protocol coverage for root-write policies with carveouts and a core sandboxing regression so those policies still require platform sandboxing

Verification

  • added protocol coverage in protocol/src/permissions.rs and protocol/src/protocol.rs for root access with explicit carveouts
  • added platform-sandbox regression coverage in core/src/sandboxing/mod.rs
  • verified the current PR state with just clippy

Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector
Copy link
Contributor

💡 Codex Review

let argv = build_bwrap_argv(inner, sandbox_policy, sandbox_policy_cwd, options);

P1 Badge Use split FS policy when building bubblewrap mounts

This call still builds the bubblewrap filesystem view from the legacy SandboxPolicy, even though the helper now receives split filesystem/network policies. For profiles that allow :root = "write" with explicit none carveouts, to_legacy_sandbox_policy projects to DangerFullAccess, so build_bwrap_argv gets an unrestricted policy and drops the carveouts entirely; on Linux that means denied subpaths remain writable/readable despite selecting a platform sandbox.


&& !matches!(
sandbox_policy,
SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. }
)

P2 Badge Run Windows sandbox when split policy is restricted

This predicate still rejects execution when the legacy policy is DangerFullAccess or ExternalSandbox, but root-carveout profiles can now have file_system_sandbox_policy.kind == Restricted while the legacy projection becomes DangerFullAccess. In that case select_initial can choose WindowsRestrictedToken, yet this check returns false and exec_windows_sandbox is skipped, so the intended carveout restrictions are not enforced on Windows.

ℹ️ 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 pr13451 branch 2 times, most recently from 1b9ccbd to a86cbeb Compare March 6, 2026 19:04
@bolinfest bolinfest force-pushed the pr13452 branch 2 times, most recently from d3e5080 to 7a24003 Compare March 6, 2026 19:35
@bolinfest bolinfest force-pushed the pr13452 branch 2 times, most recently from a8119da to b3a21ce Compare March 6, 2026 21:03
@bolinfest bolinfest force-pushed the pr13451 branch 2 times, most recently from 7922fbd to 66c5b0d Compare March 6, 2026 22:45
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 force-pushed the pr13451 branch 2 times, most recently from 80a644e to b449b83 Compare March 7, 2026 00:15
viyatb-oai added a commit that referenced this pull request Mar 7, 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`




---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13439).
* #13453
* #13452
* #13451
* #13449
* #13448
* #13445
* #13440
* __->__ #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
viyatb-oai added a commit that referenced this pull request Mar 7, 2026
## Why

`#13434` and `#13439` introduce split filesystem and network policies,
but the only code that could answer basic filesystem questions like "is
access effectively unrestricted?" or "which roots are readable and
writable for this cwd?" still lived on the legacy `SandboxPolicy` path.

That would force later backends to either keep projecting through
`SandboxPolicy` or duplicate path-resolution logic. This PR moves those
queries onto `FileSystemSandboxPolicy` itself so later runtime and
platform changes can consume the split policy directly.

## What changed

- added `FileSystemSandboxPolicy` helpers for full-read/full-write
checks, platform-default reads, readable roots, writable roots, and
explicit unreadable roots resolved against a cwd
- added a shared helper for the default read-only carveouts under
writable roots so the legacy and split-policy paths stay aligned
- added protocol coverage for full-access detection and derived
readable, writable, and unreadable roots

## Verification

- added protocol coverage in `protocol/src/protocol.rs` and
`protocol/src/permissions.rs` for full-root access and derived
filesystem roots
- verified the current PR state with `just clippy`




---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13440).
* #13453
* #13452
* #13451
* #13449
* #13448
* #13445
* __->__ #13440
* #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
viyatb-oai added a commit that referenced this pull request Mar 7, 2026
## Why

`apply_patch` safety approval was still checking writable paths through
the legacy `SandboxPolicy` projection.

That can hide explicit `none` carveouts when a split filesystem policy
projects back to compatibility `ExternalSandbox`, which leaves one more
approval path that can auto-approve writes inside paths that are
intentionally blocked.

## What changed

- passed `turn.file_system_sandbox_policy` into `assess_patch_safety`
- changed writable-path checks to derive effective access from
`FileSystemSandboxPolicy` instead of the legacy `SandboxPolicy`
- made those checks reject explicit unreadable roots before considering
broad write access or writable roots
- added regression coverage showing that an `ExternalSandbox`
compatibility projection still asks for approval when the split
filesystem policy blocks a subpath

## Verification

- `cargo test -p codex-core safety::tests::`
- `cargo test -p codex-core test_sandbox_config_parsing`
- `cargo clippy -p codex-core --all-targets -- -D warnings`

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13445).
* #13453
* #13452
* #13451
* #13449
* #13448
* __->__ #13445
* #13440
* #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
viyatb-oai added a commit that referenced this pull request Mar 8, 2026
## Why

After `#13440` and `#13445`, macOS Seatbelt policy generation was still
deriving filesystem and network behavior from the legacy `SandboxPolicy`
projection.

That projection loses explicit unreadable carveouts and conflates split
network decisions, so the generated Seatbelt policy could still be wider
than the split policy that Codex had already computed.

## What changed

- added Seatbelt entrypoints that accept `FileSystemSandboxPolicy` and
`NetworkSandboxPolicy` directly
- built read and write policy stanzas from access roots plus excluded
subpaths so explicit unreadable carveouts survive into the generated
Seatbelt policy
- switched network policy generation to consult `NetworkSandboxPolicy`
directly
- failed closed when managed-network or proxy-constrained sessions do
not yield usable loopback proxy endpoints
- updated the macOS callers and test helpers that now need to carry the
split policies explicitly

## Verification

- added regression coverage in `core/src/seatbelt.rs` for unreadable
carveouts under both full-disk and scoped-readable policies
- verified the current PR state with `just clippy`




---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13448).
* #13453
* #13452
* #13451
* #13449
* __->__ #13448
* #13445
* #13440
* #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
viyatb-oai added a commit that referenced this pull request Mar 8, 2026
## Why

The Linux sandbox helper still only accepted the legacy `SandboxPolicy`
payload.

That meant the runtime could compute split filesystem and network
policies, but the helper would immediately collapse them back to the
compatibility projection before applying seccomp or staging the
bubblewrap inner command.

## What changed

- added hidden `--file-system-sandbox-policy` and
`--network-sandbox-policy` flags alongside the legacy `--sandbox-policy`
flag so the helper can migrate incrementally
- updated the core-side Landlock wrapper to pass the split policies
explicitly when launching `codex-linux-sandbox`
- added helper-side resolution logic that accepts either the legacy
policy alone or a complete split-policy pair and normalizes that into
one effective configuration
- switched Linux helper network decisions to use `NetworkSandboxPolicy`
directly
- added `FromStr` support for the split policy types so the helper can
parse them from CLI JSON

## Verification

- added helper coverage in `linux-sandbox/src/linux_run_main_tests.rs`
for split-policy flags and policy resolution
- added CLI argument coverage in `core/src/landlock.rs`
- verified the current PR state with `just clippy`




---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13449).
* #13453
* #13452
* #13451
* __->__ #13449
* #13448
* #13445
* #13440
* #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
Base automatically changed from pr13451 to main March 8, 2026 04:29
viyatb-oai added a commit that referenced this pull request Mar 8, 2026
## Why

After the split-policy plumbing landed, additional-permissions widening
still rebuilt filesystem access through the legacy projection in a few
places.

That can erase explicit deny entries and make the runtime treat a policy
as fully writable even when it still has blocked subpaths, which in turn
can skip the platform sandbox when it is still needed.

## What changed

- preserved explicit deny entries when merging additional read and write
permissions into `FileSystemSandboxPolicy`
- switched platform-sandbox selection to rely on
`FileSystemSandboxPolicy::has_full_disk_write_access()` instead of ad
hoc root-write checks
- kept the widened policy path in `core/src/exec.rs` and
`core/src/sandboxing/mod.rs` aligned so denied subpaths survive both
policy merging and sandbox selection
- added regression coverage for root-write policies that still carry
carveouts

## Verification

- added regression coverage in `core/src/sandboxing/mod.rs` showing that
root write plus carveouts still requires the platform sandbox
- verified the current PR state with `just clippy`




---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13451).
* #13453
* #13452
* __->__ #13451
* #13449
* #13448
* #13445
* #13440
* #13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
rebroad pushed a commit to rebroad/codex that referenced this pull request Mar 8, 2026
## Why

After `openai#13440` and `openai#13445`, macOS Seatbelt policy generation was still
deriving filesystem and network behavior from the legacy `SandboxPolicy`
projection.

That projection loses explicit unreadable carveouts and conflates split
network decisions, so the generated Seatbelt policy could still be wider
than the split policy that Codex had already computed.

## What changed

- added Seatbelt entrypoints that accept `FileSystemSandboxPolicy` and
`NetworkSandboxPolicy` directly
- built read and write policy stanzas from access roots plus excluded
subpaths so explicit unreadable carveouts survive into the generated
Seatbelt policy
- switched network policy generation to consult `NetworkSandboxPolicy`
directly
- failed closed when managed-network or proxy-constrained sessions do
not yield usable loopback proxy endpoints
- updated the macOS callers and test helpers that now need to carry the
split policies explicitly

## Verification

- added regression coverage in `core/src/seatbelt.rs` for unreadable
carveouts under both full-disk and scoped-readable policies
- verified the current PR state with `just clippy`




---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13448).
* openai#13453
* openai#13452
* openai#13451
* openai#13449
* __->__ openai#13448
* openai#13445
* openai#13440
* openai#13439

---------

Co-authored-by: viyatb-oai <viyatb@openai.com>
@viyatb-oai viyatb-oai merged commit 3b5fe5c into main Mar 8, 2026
31 checks passed
@viyatb-oai viyatb-oai deleted the pr13452 branch March 8, 2026 05:15
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 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