Skip to content

fix(exec_policy) heredoc parsing file_redirect#20113

Open
dylan-hurd-oai wants to merge 6 commits intomainfrom
dh--heredoc-redirect-patch
Open

fix(exec_policy) heredoc parsing file_redirect#20113
dylan-hurd-oai wants to merge 6 commits intomainfrom
dh--heredoc-redirect-patch

Conversation

@dylan-hurd-oai
Copy link
Copy Markdown
Collaborator

Summary

Fixes a regression introduced in #10941 so that heredocs do not permit file redirects to be approved by rules, and adds scenario tests to cover this behavior.

Tests

  • Updated unit tests accordingly

@dylan-hurd-oai dylan-hurd-oai requested a review from a team as a code owner April 29, 2026 01:37
Bojun-Vvibe added a commit to Bojun-Vvibe/oss-contributions that referenced this pull request Apr 29, 2026
- anomalyco/opencode#24877: session directory routing fix (merge-after-nits)
- anomalyco/opencode#24852: JSON skill serialization for non-Anthropic models (merge-after-nits)
- openai/codex#20113: heredoc file_redirect sandbox bypass fix (merge-after-nits)
- openai/codex#20096: remote installed plugin cache for skills/MCP (merge-after-nits)
@evawong-oai
Copy link
Copy Markdown
Contributor

I found two security issues that look worth fixing before this lands.

  1. Heredoc prefix parsing still ignores shell variable assignments.

parse_heredoc_command_words currently skips variable_assignment while reducing a heredoc command to executable words. That means a script such as PATH=/tmp/evil:$PATH cat <<EOF can be represented to execpolicy as just cat. If there is an existing allow rule for cat, the policy path can treat the apparent command as explicitly allowed and bypass the sandbox, while shell execution resolves cat through the attacker controlled PATH.

Recommended fix: reject variable_assignment in the heredoc prefix parser instead of ignoring it. Keep comments allowed if needed, but return None for assignments so the command stays represented as the original shell invocation. Please add a parser regression for PATH=/tmp/evil:$PATH cat <<EOF, and ideally an exec policy regression showing that an existing cat allow rule does not make that command bypass the sandbox.

  1. Requested prefix rules are still allowed for complex heredoc parsing.

This PR correctly marks heredoc and file redirect fallback parsing as complex so auto derived amendments are suppressed. But model supplied prefix_rule values still pass through derive_requested_execpolicy_amendment_from_prefix_rule even when used_complex_parsing is true. For example, a heredoc that runs an interpreter from stdin can still surface a durable allow rule for the reduced interpreter argv. If the user approves that amendment once, future different heredoc bodies with the same argv prefix can match the persisted rule.

Recommended fix: apply the same complex parsing gate to requested amendments. In practice, compute requested_amendment only when auto_amendment_allowed is true, otherwise set it to None. Please add a regression where a heredoc command with a model supplied interpreter prefix rule still returns proposed_execpolicy_amendment: None.

I think the first item is directly in the parser lines touched here. The second item is a related policy boundary issue that this PR is now depending on, so it would be best to close both while the heredoc policy behavior is being tightened.

dylan-hurd-oai and others added 2 commits April 28, 2026 20:16
Windows read-only sandbox policy prompts for the unreduced fallback command, so keep this assertion on platforms where the sandboxed fallback result is stable.

Co-authored-by: Codex <noreply@openai.com>
@evawong-oai
Copy link
Copy Markdown
Contributor

evawong-oai commented Apr 29, 2026

Thanks for the Slack discussion. I agree this should not block the PR.

The risky case is a broad saved rule like cat or python. This PR prevents that.

The remaining case saves the exact shell command. That is much less reusable.

It also keeps the normal user flow where an approved command can offer a saved rule.

So I am good with this PR. If we want stricter behavior for complex shell scripts, we can handle that as follow up work.

Copy link
Copy Markdown
Contributor

@evawong-oai evawong-oai left a comment

Choose a reason for hiding this comment

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

Approving based on the thread. The remaining exact command rule concern does not need to block this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants