Skip to content

fix: preserve deny-read sandboxing for safe commands#23943

Merged
bolinfest merged 1 commit into
mainfrom
pr23943
May 29, 2026
Merged

fix: preserve deny-read sandboxing for safe commands#23943
bolinfest merged 1 commit into
mainfrom
pr23943

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 21, 2026

Why

Permission profiles can mark filesystem entries as unreadable with deny rules, including glob patterns. Several shell execution paths treated known-safe commands or execpolicy allow rules as sufficient to run outside the filesystem sandbox. That is not valid for read-capable commands: for example, cat or ls may be reasonable to allow generally, but dropping the sandbox would also drop deny-read constraints such as **/*.env.

What changed

  • Added a shared check that treats active deny-read restrictions as incompatible with unsandboxed execution.
  • Kept first-attempt execution sandboxed for explicit escalation and execpolicy allow bypasses when deny-read entries are present.
  • Prevented no-sandbox retry after a sandbox denial when the active filesystem policy contains deny-read entries.
  • Updated the zsh-fork execve path so prefix-rule allow decisions continue inside the current sandbox when deny-read restrictions are active.

Verification

  • cargo test -p codex-core tools::sandboxing::tests
  • cargo test -p codex-core tools::runtimes::shell::unix_escalation::tests
  • cargo test -p codex-core shell_command_enforces_glob_deny_read_policy

@bolinfest bolinfest requested a review from a team as a code owner May 21, 2026 22:11
Copy link
Copy Markdown
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: 101e4b3c61

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

Comment thread codex-rs/core/src/tools/orchestrator.rs Outdated
Comment thread codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs Outdated
Comment thread codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
@bolinfest bolinfest force-pushed the pr23943 branch 3 times, most recently from 971a875 to 4663a5d Compare May 28, 2026 21:39
@bolinfest bolinfest requested a review from viyatb-oai May 28, 2026 21:42
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai left a comment

Choose a reason for hiding this comment

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

Found one P2 regression in the deny-read plus zsh-fork approval flow; an inline suggested fix is attached.

Comment thread codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
@bolinfest
Copy link
Copy Markdown
Collaborator Author

@codex review

@bolinfest bolinfest force-pushed the pr23943 branch 2 times, most recently from 2e323c1 to 4798594 Compare May 28, 2026 22:52
@bolinfest bolinfest requested a review from viyatb-oai May 28, 2026 23:02
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai left a comment

Choose a reason for hiding this comment

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

There is one remaining sibling regression in unified-exec zsh-fork after the shell-command fix; an inline suggested fix is attached.

Comment thread codex-rs/core/src/tools/runtimes/unified_exec.rs Outdated
@bolinfest bolinfest merged commit 6e10142 into main May 29, 2026
47 of 62 checks passed
@bolinfest bolinfest deleted the pr23943 branch May 29, 2026 05:49
@github-actions github-actions Bot locked and limited conversation to collaborators May 29, 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