Skip to content

fix: allow restricted filesystem profiles to read helper executables#15114

Merged
celia-oai merged 2 commits intomainfrom
dev/cc/fix-3
Mar 20, 2026
Merged

fix: allow restricted filesystem profiles to read helper executables#15114
celia-oai merged 2 commits intomainfrom
dev/cc/fix-3

Conversation

@celia-oai
Copy link
Collaborator

@celia-oai celia-oai commented Mar 18, 2026

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.

@celia-oai celia-oai requested a review from bolinfest March 18, 2026 23:48
@celia-oai celia-oai marked this pull request as ready for review March 18, 2026 23:48
Copy link
Collaborator

@bolinfest bolinfest left a comment

Choose a reason for hiding this comment

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

I flagged a number of big-O issues in this PR. Let's take a step back and think about this design some more.

@celia-oai celia-oai requested a review from bolinfest March 19, 2026 03:28
changes

Update codex-rs/core/src/config/mod.rs

Co-authored-by: Michael Bolin <mbolin@openai.com>

changes
@celia-oai celia-oai force-pushed the dev/cc/fix-3 branch 4 times, most recently from 667ebb4 to 5125da0 Compare March 20, 2026 01:08
@celia-oai celia-oai requested a review from bolinfest March 20, 2026 01:09
) -> Vec<AbsolutePathBuf> {
let arg0_root = AbsolutePathBuf::from_absolute_path(codex_home.join("tmp").join("arg0")).ok();
let zsh_path = zsh_path.and_then(|path| AbsolutePathBuf::from_absolute_path(path).ok());
let execve_wrapper_root = main_execve_wrapper_exe.and_then(|path| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see, so we are relying on getting the parent folder of codex-execve-wrapper to find the right folder of ~/.codex/tmp/arg0. This certainly works for now, but in a follow-up, I would prefer we do something more robust!

Let's add a TODO and then in a subsequent PR, we should:

  • update Arg0DispatchPaths to include the path entry added by prepend_path_entry_for_codex_aliases()
  • thread all that through to here so that we can add that entry to the return value instead of relying on the parent of main_execve_wrapper_exe

Also, I see these comments and I think they are out of date:

/// Path to the `codex-linux-sandbox` executable. This must be set if
/// [`crate::exec::SandboxType::LinuxSeccomp`] is used. Note that this
/// cannot be set in the config file: it must be set in code via
/// [`ConfigOverrides`].
///
/// When this program is invoked, arg0 will be set to `codex-linux-sandbox`.
pub codex_linux_sandbox_exe: Option<PathBuf>,
/// Path to the `codex-execve-wrapper` executable used for shell
/// escalation. This cannot be set in the config file: it must be set in
/// code via [`ConfigOverrides`].
pub main_execve_wrapper_exe: Option<PathBuf>,

That is, I do not think we should be relying on ConfigOverrides to thread the values of Arg0DispatchPaths through. Ideally we would try to clean that up as part of the TODO, as well.

}
});

let mut readable_roots = Vec::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

protip (not worth waiting for CI for this one small change, tho)

Suggested change
let mut readable_roots = Vec::new();
let mut readable_roots = Vec::with_capacity(2);

@celia-oai celia-oai merged commit 9eef2e9 into main Mar 20, 2026
33 checks passed
@celia-oai celia-oai deleted the dev/cc/fix-3 branch March 20, 2026 22:51
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 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