feat(interp): route file-target output redirects through AllowedPaths#209
Conversation
Stop rejecting >, >>, 2>, 2>>, &>, &>> at validation. The runtime now opens the redirect target via r.open(), which is backed by the AllowedPaths sandbox added in PR #206. Targets inside AllowedPaths are created/truncated/appended; targets outside fail with "permission denied" and exit 1. The literal "/dev/null" continues to short-circuit to io.Discard so operators don't need to whitelist it. The opened file is returned as the redir Closer so the caller flushes and closes it before the next command runs. <> (read-write open) stays rejected; bash quirks (ambiguous-redirect, noclobber) remain out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db5b299187
ℹ️ 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".
A FIFO inside AllowedPaths would hang the script during redirect setup: opening a named pipe with O_WRONLY blocks until a reader connects, and the runner has no opportunity to observe context cancellation while it's stuck inside the open syscall. Pre-PR, validation rejected the entire class so this was unreachable. Stat the target through the sandbox (openat-based, never blocks) before opening for output. Reject non-regular files with a clear error; ENOENT and other Stat errors are ignored — the file will be created by O_CREATE or the open will surface the genuine failure. The same guard covers 2>file and &>file paths. Adds a unix-only test that bounds the redirect with a timeout so any future regression that reintroduces the hang fails fast instead of stalling the suite. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8541506d2b
ℹ️ 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".
Output redirects with an unsupported fd (anything other than 1 or 2) were rejected at runtime, but the rejection happened after r.literal had already expanded the redirect word. A command substitution in the target — e.g. 3>$(cmd) — would execute cmd for its side effects only to have its output discarded. Pre-PR validation rejected fd 3+ before any expansion ran, so this PR regressed that property. Move the fd-N check above r.literal. rd.N is a parser literal, not an expansion, so it can be inspected without driving any expansion side effects. Same guard applies to fd 0 on output ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ad84658b0
ℹ️ 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".
t.TempDir() returns backslash paths on Windows (C:\...), and concatenating them raw into a shell command leaves the backslashes for the parser to consume as escape characters — the redirect target ends up pointing at the wrong file. Single-quoting the path with the standard '\'' idiom defuses backslashes plus any other shell metacharacters that could appear in temp directory paths (spaces, $). Also replace a stray forward-slash concatenation in TestRedirFailureDoesNotRunCommand with filepath.Join, which was hiding the same Windows-correctness issue. docs/RULES.md requires cross-platform path handling in tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ae8c0efe2
ℹ️ 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".
| flags := os.O_WRONLY | os.O_CREATE | os.O_TRUNC | ||
| if rd.Op == syntax.AppOut { | ||
| flags = os.O_WRONLY | os.O_CREATE | os.O_APPEND |
There was a problem hiding this comment.
This new output-redirection path introduces os.O_WRONLY, os.O_CREATE, os.O_TRUNC, and os.O_APPEND in interp/, but analysis/symbols_interp.go does not allow those symbols. I checked go test ./...; TestInterpAllowedSymbols and TestVerificationInterpCleanPass fail on these lines, so CI will reject the commit until the interp allowed-symbol policy is updated/approved for these write flags.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — I missed running ./analysis/.... Confirmed TestInterpAllowedSymbols and TestVerificationInterpCleanPass were red on the previous head. Fixed in 23c16a5.
Added the four flag constants to analysis/symbols_interp.go with the same comment style as os.O_RDONLY, noting that the capability gate is allowedpaths.Sandbox.Open — the flags themselves are pure integers and only become a write capability when combined with the sandbox Open call (which is already authorized). Per project policy I won't add the verified/allowed_symbols label myself; that stays a human approval signal.
Full go test ./... is now green.
The new file-target output redirect path uses these four flag constants in interp/runner_redir.go, but they were not in the interp allowlist — TestInterpAllowedSymbols and TestVerificationInterpCleanPass fail without this addition. The flags are pure integer constants. The actual write capability gate is allowedpaths.Sandbox.Open, which is already authorized; passing O_WRONLY|O_CREATE|O_TRUNC into a non-sandbox path would still be prevented by the lack of any direct os.OpenFile call in interp/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
>,>>,2>,2>>,&>,&>>) are no longer rejected at validation; the runtime opens the target via theAllowedPathssandbox added in feat(allowedpaths): permit writes through sandbox open #206.AllowedPaths, the file is created/truncated/appended with mode0644. Outside, the open fails withpermission deniedand the command exits 1; the script continues with the next statement./dev/nullstill short-circuits toio.Discardso operators don't have to whitelist it.2>&1continues to dup whichever writer*orignow points at, so> file 2>&1correctly merges streams into the file.<>(read-write open) stays rejected; the demo doesn't need it.Stacking
This PR is stacked on top of #206 (allowed-paths writes) and is demo-only — base is
jules.macret/host-remediation/allowed-paths-writes, notmain.Out of scope (deferred, per design)
<>).>(cmd),<(cmd)).noclobbershopt.Tests
tests/scenarios/shell/redirections/file_target/— new scenarios covering truncate / overwrite / append /2>/&>/&>>/ heredoc +>combo /> file 2>&1/ sandbox-blocked /..-traversal-blocked. Happy-path scenarios are bash-asserted; the two sandbox-divergent scenarios setskip_assert_against_bash: true.interp/tests/redir_file_target_test.go— Go tests for behaviour scenarios can't directly express (file handle is closed before next command,0644mode bits, no file created when a later redirect aborts the command, no file leaks outside the sandbox).blocked_redirects/andredirections/devnull/*_blocked.yamlscenarios were updated from validation-time rejection (exit 2/> file redirection is not supported) to runtime-time rejection (exit 1/open <path>: permission denied)./dev/null) or fails at the sandbox open.Test plan
make fmtcleango test ./interp/... ./tests/... -timeout 180spassesgo run ./cmd/rshell --allow-all-commands --allowed-paths /tmp -c 'echo hi > /tmp/x && cat /tmp/x'printshi/etc/fooexits 1 withopen /etc/foo: permission denied>(cat <<EOF > /tmp/x ... EOF) writes the heredoc body to the fileCI will run the bash comparison suite (
RSHELL_BASH_TEST=1); locally that requires Docker Desktop file-sharing to include the macOS temp dir, which my workstation doesn't have configured, so I deferred that check to CI.🤖 Generated with Claude Code