Commit Graph

11 Commits

  • chore: clean up argument-comment lint and roll out all-target CI on macOS (#16054)
    ## Why
    
    `argument-comment-lint` was green in CI even though the repo still had
    many uncommented literal arguments. The main gap was target coverage:
    the repo wrapper did not force Cargo to inspect test-only call sites, so
    examples like the `latest_session_lookup_params(true, ...)` tests in
    `codex-rs/tui_app_server/src/lib.rs` never entered the blocking CI path.
    
    This change cleans up the existing backlog, makes the default repo lint
    path cover all Cargo targets, and starts rolling that stricter CI
    enforcement out on the platform where it is currently validated.
    
    ## What changed
    
    - mechanically fixed existing `argument-comment-lint` violations across
    the `codex-rs` workspace, including tests, examples, and benches
    - updated `tools/argument-comment-lint/run-prebuilt-linter.sh` and
    `tools/argument-comment-lint/run.sh` so non-`--fix` runs default to
    `--all-targets` unless the caller explicitly narrows the target set
    - fixed both wrappers so forwarded cargo arguments after `--` are
    preserved with a single separator
    - documented the new default behavior in
    `tools/argument-comment-lint/README.md`
    - updated `rust-ci` so the macOS lint lane keeps the plain wrapper
    invocation and therefore enforces `--all-targets`, while Linux and
    Windows temporarily pass `-- --lib --bins`
    
    That temporary CI split keeps the stricter all-targets check where it is
    already cleaned up, while leaving room to finish the remaining Linux-
    and Windows-specific target-gated cleanup before enabling
    `--all-targets` on those runners. The Linux and Windows failures on the
    intermediate revision were caused by the wrapper forwarding bug, not by
    additional lint findings in those lanes.
    
    ## Validation
    
    - `bash -n tools/argument-comment-lint/run.sh`
    - `bash -n tools/argument-comment-lint/run-prebuilt-linter.sh`
    - shell-level wrapper forwarding check for `-- --lib --bins`
    - shell-level wrapper forwarding check for `-- --tests`
    - `just argument-comment-lint`
    - `cargo test` in `tools/argument-comment-lint`
    - `cargo test -p codex-terminal-detection`
    
    ## Follow-up
    
    - Clean up remaining Linux-only target-gated callsites, then switch the
    Linux lint lane back to the plain wrapper invocation.
    - Clean up remaining Windows-only target-gated callsites, then switch
    the Windows lint lane back to the plain wrapper invocation.
  • Protect first-time project .codex creation across Linux and macOS sandboxes (#15067)
    ## Problem
    
    Codex already treated an existing top-level project `./.codex` directory
    as protected, but there was a gap on first creation.
    
    If `./.codex` did not exist yet, a turn could create files under it,
    such as `./.codex/config.toml`, without going through the same approval
    path as later modifications. That meant the initial write could bypass
    the intended protection for project-local Codex state.
    
    ## What this changes
    
    This PR closes that first-creation gap in the Unix enforcement layers:
    
    - `codex-protocol`
    - treat the top-level project `./.codex` path as a protected carveout
    even when it does not exist yet
    - avoid injecting the default carveout when the user already has an
    explicit rule for that exact path
    - macOS Seatbelt
    - deny writes to both the exact protected path and anything beneath it,
    so creating `./.codex` itself is blocked in addition to writes inside it
    - Linux bubblewrap
    - preserve the same protected-path behavior for first-time creation
    under `./.codex`
    - tests
    - add protocol regressions for missing `./.codex` and explicit-rule
    collisions
    - add Unix sandbox coverage for blocking first-time `./.codex` creation
      - tighten Seatbelt policy assertions around excluded subpaths
    
    ## Scope
    
    This change is intentionally scoped to protecting the top-level project
    `.codex` subtree from agent writes.
    
    It does not make `.codex` unreadable, and it does not change the product
    behavior around loading project skills from `.codex` when project config
    is untrusted.
    
    ## Why this shape
    
    The fix is pointed rather than broad:
    - it preserves the current model of “project `.codex` is protected from
    writes”
    - it closes the security-relevant first-write hole
    - it avoids folding a larger permissions-model redesign into this PR
    
    ## Validation
    
    - `cargo test -p codex-protocol`
    - `cargo test -p codex-sandboxing seatbelt`
    - `cargo test -p codex-exec --test all
    sandbox_blocks_first_time_dot_codex_creation -- --nocapture`
    
    ---------
    
    Co-authored-by: Michael Bolin <mbolin@openai.com>
  • fix: allow restricted filesystem profiles to read helper executables (#15114)
    ## Summary
    
    This PR fixes restricted filesystem permission profiles so Codex's
    runtime-managed helper executables remain readable without requiring
    explicit user configuration.
    
    - add implicit readable roots for the configured `zsh` helper path and
    the main execve wrapper
    - allowlist the shared `$CODEX_HOME/tmp/arg0` root when the execve
    wrapper lives there, so session-specific helper paths keep working
    - dedupe injected paths and avoid adding duplicate read entries to the
    sandbox policy
    - add regression coverage for restricted read mode with helper
    executable overrides
    
    ## Testing 
    before this change: got this error when executing a shell command via
    zsh fork:
    ```
    "sandbox error: sandbox denied exec error, exit code: 127, stdout: , stderr: /etc/zprofile:11: operation not permitted: /usr/libexec/path_helper\nzsh:1: operation not permitted: .codex/skills/proxy-a/scripts/fetch_example.sh\n"
    ```
    
    saw this change went away after this change, meaning the readable roots
    and injected correctly.
  • fix: canonicalize symlinked Linux sandbox cwd (#14849)
    ## Problem
    On Linux, Codex can be launched from a workspace path that is a symlink
    (for example, a symlinked checkout or a symlinked parent directory).
    
    Our sandbox policy intentionally canonicalizes writable/readable roots
    to the real filesystem path before building the bubblewrap mounts. That
    part is correct and needed for safety.
    
    The remaining bug was that bubblewrap could still inherit the helper
    process's logical cwd, which might be the symlinked alias instead of the
    mounted canonical path. In that case, the sandbox starts in a cwd that
    does not exist inside the sandbox namespace even though the real
    workspace is mounted. This can cause sandboxed commands to fail in
    symlinked workspaces.
    
    ## Fix
    This PR keeps the sandbox policy behavior the same, but separates two
    concepts that were previously conflated:
    
    - the canonical cwd used to define sandbox mounts and permissions
    - the caller's logical cwd used when launching the command
    
    On the Linux bubblewrap path, we now thread the logical command cwd
    through the helper explicitly and only add `--chdir <canonical path>`
    when the logical cwd differs from the mounted canonical path.
    
    That means:
    - permissions are still computed from canonical paths
    - bubblewrap starts the command from a cwd that definitely exists inside
    the sandbox
    - we do not widen filesystem access or undo the earlier symlink
    hardening
    
    ## Why This Is Safe
    This is a narrow Linux-only launch fix, not a policy change.
    
    - Writable/readable root canonicalization stays intact.
    - Protected metadata carveouts still operate on canonical roots.
    - We only override bubblewrap's inherited cwd when the logical path
    would otherwise point at a symlink alias that is not mounted in the
    sandbox.
    
    ## Tests
    - kept the existing protocol/core regression coverage for symlink
    canonicalization
    - added regression coverage for symlinked cwd handling in the Linux
    bubblewrap builder/helper path
    
    Local validation:
    - `just fmt`
    - `cargo test -p codex-protocol`
    - `cargo test -p codex-core
    normalize_additional_permissions_canonicalizes_symlinked_write_paths`
    - `cargo clippy -p codex-linux-sandbox -p codex-protocol -p codex-core
    --tests -- -D warnings`
    - `cargo build --bin codex`
    
    ## Context
    This is related to #14694. The earlier writable-root symlink fix
    addressed the mount/permission side; this PR fixes the remaining
    symlinked-cwd launch mismatch in the Linux sandbox path.
  • fix: fix symlinked writable roots in sandbox policies (#14674)
    ## Summary
    - normalize effective readable, writable, and unreadable sandbox roots
    after resolving special paths so symlinked roots use canonical runtime
    paths
    - add a protocol regression test for a symlinked writable root with a
    denied child and update protocol expectations to canonicalized effective
    paths
    - update macOS seatbelt tests to assert against effective normalized
    roots produced by the shared policy helpers
    
    ## Testing
    - just fmt
    - cargo test -p codex-protocol
    - cargo test -p codex-core explicit_unreadable_paths_are_excluded_
    - cargo clippy -p codex-protocol -p codex-core --tests -- -D warnings
    
    ## Notes
    - This is intended to fix the symlinked TMPDIR bind failure in
    bubblewrap described in #14672.
    Fixes #14672
  • refactor: centralize filesystem permissions precedence (#14174)
    ## Stack
    
       fix: fail closed for unsupported split windows sandboxing #14172
       fix: preserve split filesystem semantics in linux sandbox #14173
       fix: align core approvals with split sandbox policies #14171
    -> refactor: centralize filesystem permissions precedence #14174
    
    ## Summary
    - add a shared per-path split filesystem precedence helper in
    `FileSystemSandboxPolicy`
    - derive readable, writable, and unreadable roots from the same
    most-specific resolution rules
    - add regression coverage for nested `write` / `read` / `none` carveouts
    and legacy bridge enforcement detection
    
    ## Testing
    - cargo test -p codex-protocol
    - cargo clippy -p codex-protocol --tests -- -D warnings
  • fix: keep permissions profiles forward compatible (#14107)
    ## Summary
    - preserve unknown `:special_path` tokens, including nested entries, so
    older Codex builds warn and ignore instead of failing config load
    - fail closed with a startup warning when a permissions profile has
    missing or empty filesystem entries instead of aborting profile
    compilation
    - normalize Windows verbatim paths like `\?\C:\...` before absolute-path
    validation while keeping explicit errors for truly invalid paths
    
    ## Testing
    - just fmt
    - cargo test -p codex-core permissions_profiles_allow
    - cargo test -p codex-core
    normalize_absolute_path_for_platform_simplifies_windows_verbatim_paths
    - cargo test -p codex-protocol
    unknown_special_paths_are_ignored_by_legacy_bridge
    - cargo clippy -p codex-core -p codex-protocol --all-targets -- -D
    warnings
    - cargo clean
  • fix(protocol): preserve legacy workspace-write semantics (#13957)
    ## Summary
    This is a fast follow to the initial `[permissions]` structure.
    
    - keep the new split-policy carveout behavior for narrower non-write
    entries under broader writable roots
    - preserve legacy `WorkspaceWrite` semantics by using a cwd-aware bridge
    that drops only redundant nested readable roots when projecting from
    `SandboxPolicy`
    - route the legacy macOS seatbelt adapter through that same legacy
    bridge so redundant nested readable roots do not become read-only
    carveouts on macOS
    - derive the legacy bridge for `command_exec` using the sandbox root cwd
    rather than the request cwd so policy derivation matches later sandbox
    enforcement
    - add regression coverage for the legacy macOS nested-readable-root case
    
    ## Examples
    ### Legacy `workspace-write` on macOS
    A legacy `workspace-write` policy can redundantly list a nested readable
    root under an already-writable workspace root.
    
    For example, legacy config can effectively mean:
    - workspace root (`.` / `cwd`) is writable
    - `docs/` is also listed in `readable_roots`
    
    The new shared split-policy helper intentionally treats a narrower
    non-write entry under a broader writable root as a carveout for real
    `[permissions]` configs. Without this fast follow, the unchanged macOS
    seatbelt legacy adapter could project that legacy shape into a
    `FileSystemSandboxPolicy` that treated `docs/` like a read-only carveout
    under the writable workspace root. In practice, legacy callers on macOS
    could unexpectedly lose write access inside `docs/`, even though that
    path was writable before the `[permissions]` migration work.
    
    This change fixes that by routing the legacy seatbelt path through the
    cwd-aware legacy bridge, so:
    - legacy `workspace-write` keeps `docs/` writable when `docs/` was only
    a redundant readable root
    - explicit `[permissions]` entries like `'.' = 'write'` and `'docs' =
    'read'` still make `docs/` read-only, which is the new intended
    split-policy behavior
    
    ### Legacy `command_exec` with a subdirectory cwd
    `command_exec` can run a command from a request cwd that is narrower
    than the sandbox root cwd.
    
    For example:
    - sandbox root cwd is `/repo`
    - request cwd is `/repo/subdir`
    - legacy policy is still `workspace-write` rooted at `/repo`
    
    Before this fast follow, `command_exec` derived the legacy bridge using
    the request cwd, but the sandbox was later built using the sandbox root
    cwd. That mismatch could miss redundant legacy readable roots during
    projection and accidentally reintroduce read-only carveouts for paths
    that should still be writable under the legacy model.
    
    This change fixes that by deriving the legacy bridge with the same
    sandbox root cwd that sandbox enforcement later uses.
    
    ## Verification
    - `just fmt`
    - `cargo test -p codex-core
    seatbelt_legacy_workspace_write_nested_readable_root_stays_writable`
    - `cargo test -p codex-core test_sandbox_config_parsing`
    - `cargo clippy -p codex-core -p codex-app-server --all-targets -- -D
    warnings`
    - `cargo clean`
  • protocol: keep root carveouts sandboxed (#13452)
    ## 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`
    
    
    
    
    ---
    [//]: # (BEGIN SAPLING FOOTER)
    Stack created with [Sapling](https://sapling-scm.com). Best reviewed
    with [ReviewStack](https://reviewstack.dev/openai/codex/pull/13452).
    * #13453
    * __->__ #13452
    * #13451
    * #13449
    * #13448
    * #13445
    * #13440
    * #13439
    
    ---------
    
    Co-authored-by: viyatb-oai <viyatb@openai.com>
  • protocol: derive effective file access from filesystem policies (#13440)
    ## 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>
  • config: add initial support for the new permission profile config language 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