Skip to content

seatbelt: honor split filesystem sandbox policies#13448

Merged
viyatb-oai merged 10 commits intomainfrom
pr13448
Mar 8, 2026
Merged

seatbelt: honor split filesystem sandbox policies#13448
viyatb-oai merged 10 commits intomainfrom
pr13448

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Mar 4, 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

Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector
Copy link
Contributor

💡 Codex Review

FileSystemSandboxKind::Restricted => {
FileSystemSandboxPolicy::from(&effective_policy)
}

P1 Badge Preserve denied subpaths when merging additional permissions

When additional permissions include any extra read/write roots, this branch rebuilds the filesystem policy from effective_policy (legacy SandboxPolicy), which cannot encode FileSystemAccessMode::None carveouts. As a result, explicit unreadable paths from the original split policy are dropped, so a command that requests unrelated extra access can regain access to paths that were explicitly denied (for example, a blocked subdirectory under the workspace) and run with a wider sandbox than configured.


let sandbox_type = match file_system_sandbox_policy.kind {
FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => {

P2 Badge Skip platform sandbox for full-access restricted filesystem policy

This selection logic keys only on file_system_sandbox_policy.kind, so profiles expressed as restricted entries with :root = "write" are treated as sandbox-required even though they already grant full disk write access (legacy projection becomes DangerFullAccess/ExternalSandbox). On Linux setups where codex_linux_sandbox_exe is not present, that forces the Linux sandbox path and can fail with MissingLinuxSandboxExecutable instead of running normally, which makes valid full-access profiles unexpectedly break.

ℹ️ 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 pr13448 branch 2 times, most recently from 3e354ad to 6f31748 Compare March 6, 2026 19:35
@bolinfest bolinfest force-pushed the pr13448 branch 2 times, most recently from f0f11de to 8451b47 Compare March 6, 2026 21:03
@bolinfest bolinfest force-pushed the pr13445 branch 2 times, most recently from b1aa31b to 3cec98b Compare March 6, 2026 21:32
@bolinfest bolinfest force-pushed the pr13448 branch 2 times, most recently from 42b9605 to a796513 Compare March 6, 2026 22:45
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>
Base automatically changed from pr13445 to main March 7, 2026 08:01
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>
@dylan-hurd-oai
Copy link
Collaborator

@codex review

Copy link
Collaborator

@dylan-hurd-oai dylan-hurd-oai left a comment

Choose a reason for hiding this comment

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

1 non-blocking comment from me, but also let's see what codex says about the current version 😄

Ok(path) => path,
Err(err) => panic!("root path must be absolute: {err}"),
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be a method on AbsolutePathBuf?

Copy link
Collaborator

Choose a reason for hiding this comment

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

probably but we only use it here in seatbelt. trying to reduce number of edits outside this file :(

@viyatb-oai viyatb-oai enabled auto-merge (squash) March 8, 2026 00:21
@viyatb-oai viyatb-oai merged commit bf5c2f4 into main Mar 8, 2026
70 of 75 checks passed
@viyatb-oai viyatb-oai deleted the pr13448 branch March 8, 2026 00:35
@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.

3 participants