feat(interp): host-binary exec via host:<name>=<path> [DEMO ONLY]#228
Draft
julesmcrt wants to merge 6 commits intojules.macret/host-remediation/truncatefrom
Draft
Conversation
Collaborator
Author
|
@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: 3a9876fc1b
ℹ️ 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".
julesmcrt
added a commit
that referenced
this pull request
May 5, 2026
…builtins Address Codex review (PR #228): an entry like "host:cat=/bin/false" flipped isAllowed=true and the isKnown branch then ran the cat builtin with stdin/AllowedPaths access, never executing the host binary. Compute isKnown before isAllowed and gate the host entry on !isKnown so a collision with a builtin name no longer launders builtin access. host_exec span tag is now keyed on hostDispatch (the actually-exec'd case) rather than mere entry presence. Includes a cross-platform regression test that exercises the gate without invoking the host-exec stub. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4205dd775
ℹ️ 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".
julesmcrt
added a commit
that referenced
this pull request
May 6, 2026
…solute paths on Windows Address Codex review (PR #228, two comments). P1 — host_exec_test.go: the tests bound host:echo, host:cat, host:false to /bin/echo, /bin/cat, /usr/bin/false respectively, but echo/cat/false are all rshell builtins. After the !isKnown gate fix in c4205dd, those entries no longer authorize the name (builtins win), so the tests rejected with exit 127 / "command not allowed" before ever invoking the host binary. Switched to non-builtin command names (hostecho, hostcat, hostfalse) bound to the same binaries, so dispatch actually reaches runHostCommand. Verified by running the test binary on debian:bookworm-slim — all nine TestHostExec* now pass. Also rewrote TestHostExecBuiltinTakesPrecedence to point host:echo at /usr/bin/false: if the host path were erroneously preferred, the script would exit 1; because the builtin wins it exits 0. The previous form pointed both at /bin/echo, which produced identical output and could not distinguish the two paths. P2 — allowed_commands_test.go: the cross-platform host-entry tests used /bin/false and /usr/bin/true as host paths. AllowedCommands validates with filepath.IsAbs, which is false on Windows for paths without a drive letter or UNC prefix, so those tests fail at New() during interp.New on Windows. Switched to filepath.Join(t.TempDir(), "fake-binary"), which is absolute on every OS and is never actually exec'd (the tests assert the dispatch gate, not the exec). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93376921ce
ℹ️ 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".
julesmcrt
added a commit
that referenced
this pull request
May 6, 2026
…gate Address Codex review (PR #228, host_exec_linux.go:64). When MaxExecutionTime or the parent context expired while a host binary was running, exec.CommandContext killed the child and cmd.Run returned *exec.ExitError with ExitCode() == -1. The previous code mapped that to exit 130 and returned, leaving r.exit.err nil — so Run() returned ExitStatus(130) instead of context.DeadlineExceeded / context.Canceled. Net effect: cmd/rshell's timeout path never printed "execution timed out" or returned 124, and run-span telemetry classed the run as success. Now check ctx.Err() before mapping the signal to a numeric code: when the context expired, propagate the context error via r.exit.fatal so Run() returns it directly. The numeric return becomes symbolic (only observed when r.exit.err is nil). Test: new TestHostExecDeadlineSurfacesAsDeadlineExceeded asserts that a 100ms ctx.WithTimeout against `sleep 30` returns context.DeadlineExceeded within 2s; updated TestHostExecContextCancellationKillsProcess to assert context.Canceled is the propagated error. Both pass on debian:bookworm-slim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e34d2474
ℹ️ 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".
julesmcrt
added a commit
that referenced
this pull request
May 6, 2026
…ess env Address Codex review (PR #228, host_exec_linux.go:95). Previously filterHostEnv() called os.LookupEnv(name) for each allowlisted name — the ambient Go process env. That broke three guarantees: 1. The runner's documented "no host environment inherited by default" property: a runner constructed with the empty default Env still leaked PATH/HOME/LANG to host binaries from the surrounding Go process. 2. interp.Env(...) caller-provided values: the runner env was ignored entirely, so the host binary saw the wrong PATH/HOME/LANG. 3. Inline command assignments (PATH=/safe hostcmd): again ignored. filterHostEnv is now a method on Runner that reads from r.writeEnv — the same overlay that builtins read from. call() applies inline assigns to r.writeEnv before dispatching, so the host binary sees the post-assignment view. New tests on debian:bookworm-slim: - TestHostExecDefaultEnvIsEmpty: with no interp.Env set, ambient PATH/HOME/LANG must NOT appear in the host binary's env. - TestHostExecInlineAssignmentTakesEffect: PATH=/inline-override env must show PATH=/inline-override and not the runner's initial PATH. Existing TestHostExecEnvFilteredToAllowlist / TestHostExecForwardsHomeAndLang updated to plant env via interp.Env rather than t.Setenv. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77bc3d56cc
ℹ️ 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".
julesmcrt
added a commit
that referenced
this pull request
May 6, 2026
Address Codex review (PR #228, host_exec_linux.go:101). Previously filterHostEnv accepted any Declared() variable in hostEnvAllowlist. That diverged from bash: a script-level assignment without a prior export, e.g. LANG=fresh hostcmd is shell-local in bash and is NOT propagated to child processes. Verified with `bash -c 'CUSTOM_VAR=/tmp; env | grep CUSTOM_VAR'` which prints nothing. filterHostEnv now requires both vr.Declared() and vr.Exported. The two valid sources of host-forwarded env are unaffected: - interp.Env(...) values are stored as exported (that is their purpose as the initial environment). - Inline command assignments (`PATH=/safe hostcmd`) propagate because call() at runner_exec.go:110 forces vr.Exported = true before dispatch. New test TestHostExecUnexportedAssignmentNotForwarded verifies the divergent case ('LANG=should-not-leak; env' must not show LANG in the host binary's environment). Passes on debian:bookworm-slim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
julesmcrt
force-pushed
the
jules.macret/host-remediation/truncate
branch
from
2acb2e8 to
8cbe726
Compare
Add a parallel "host:" namespace to AllowedCommands so a single allowlisted host binary (e.g. host:logrotate=/usr/sbin/logrotate) can be exec'd directly. Plumbs stdin/stdout/stderr, propagates exit code, kills via SIGKILL on context cancel, forwards only PATH/HOME/LANG to the child. Linux-only; non-linux platforms return 127. This fundamentally changes rshell's threat model — the entire reason rshell exists is to *not* execute host binaries. This entry-point exists for the host-remediation demo (step 6: logrotate -f) and is not intended to merge to main without a real design pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…builtins Address Codex review (PR #228): an entry like "host:cat=/bin/false" flipped isAllowed=true and the isKnown branch then ran the cat builtin with stdin/AllowedPaths access, never executing the host binary. Compute isKnown before isAllowed and gate the host entry on !isKnown so a collision with a builtin name no longer launders builtin access. host_exec span tag is now keyed on hostDispatch (the actually-exec'd case) rather than mere entry presence. Includes a cross-platform regression test that exercises the gate without invoking the host-exec stub. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…solute paths on Windows Address Codex review (PR #228, two comments). P1 — host_exec_test.go: the tests bound host:echo, host:cat, host:false to /bin/echo, /bin/cat, /usr/bin/false respectively, but echo/cat/false are all rshell builtins. After the !isKnown gate fix in c4205dd, those entries no longer authorize the name (builtins win), so the tests rejected with exit 127 / "command not allowed" before ever invoking the host binary. Switched to non-builtin command names (hostecho, hostcat, hostfalse) bound to the same binaries, so dispatch actually reaches runHostCommand. Verified by running the test binary on debian:bookworm-slim — all nine TestHostExec* now pass. Also rewrote TestHostExecBuiltinTakesPrecedence to point host:echo at /usr/bin/false: if the host path were erroneously preferred, the script would exit 1; because the builtin wins it exits 0. The previous form pointed both at /bin/echo, which produced identical output and could not distinguish the two paths. P2 — allowed_commands_test.go: the cross-platform host-entry tests used /bin/false and /usr/bin/true as host paths. AllowedCommands validates with filepath.IsAbs, which is false on Windows for paths without a drive letter or UNC prefix, so those tests fail at New() during interp.New on Windows. Switched to filepath.Join(t.TempDir(), "fake-binary"), which is absolute on every OS and is never actually exec'd (the tests assert the dispatch gate, not the exec). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gate Address Codex review (PR #228, host_exec_linux.go:64). When MaxExecutionTime or the parent context expired while a host binary was running, exec.CommandContext killed the child and cmd.Run returned *exec.ExitError with ExitCode() == -1. The previous code mapped that to exit 130 and returned, leaving r.exit.err nil — so Run() returned ExitStatus(130) instead of context.DeadlineExceeded / context.Canceled. Net effect: cmd/rshell's timeout path never printed "execution timed out" or returned 124, and run-span telemetry classed the run as success. Now check ctx.Err() before mapping the signal to a numeric code: when the context expired, propagate the context error via r.exit.fatal so Run() returns it directly. The numeric return becomes symbolic (only observed when r.exit.err is nil). Test: new TestHostExecDeadlineSurfacesAsDeadlineExceeded asserts that a 100ms ctx.WithTimeout against `sleep 30` returns context.DeadlineExceeded within 2s; updated TestHostExecContextCancellationKillsProcess to assert context.Canceled is the propagated error. Both pass on debian:bookworm-slim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ess env Address Codex review (PR #228, host_exec_linux.go:95). Previously filterHostEnv() called os.LookupEnv(name) for each allowlisted name — the ambient Go process env. That broke three guarantees: 1. The runner's documented "no host environment inherited by default" property: a runner constructed with the empty default Env still leaked PATH/HOME/LANG to host binaries from the surrounding Go process. 2. interp.Env(...) caller-provided values: the runner env was ignored entirely, so the host binary saw the wrong PATH/HOME/LANG. 3. Inline command assignments (PATH=/safe hostcmd): again ignored. filterHostEnv is now a method on Runner that reads from r.writeEnv — the same overlay that builtins read from. call() applies inline assigns to r.writeEnv before dispatching, so the host binary sees the post-assignment view. New tests on debian:bookworm-slim: - TestHostExecDefaultEnvIsEmpty: with no interp.Env set, ambient PATH/HOME/LANG must NOT appear in the host binary's env. - TestHostExecInlineAssignmentTakesEffect: PATH=/inline-override env must show PATH=/inline-override and not the runner's initial PATH. Existing TestHostExecEnvFilteredToAllowlist / TestHostExecForwardsHomeAndLang updated to plant env via interp.Env rather than t.Setenv. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Codex review (PR #228, host_exec_linux.go:101). Previously filterHostEnv accepted any Declared() variable in hostEnvAllowlist. That diverged from bash: a script-level assignment without a prior export, e.g. LANG=fresh hostcmd is shell-local in bash and is NOT propagated to child processes. Verified with `bash -c 'CUSTOM_VAR=/tmp; env | grep CUSTOM_VAR'` which prints nothing. filterHostEnv now requires both vr.Declared() and vr.Exported. The two valid sources of host-forwarded env are unaffected: - interp.Env(...) values are stored as exported (that is their purpose as the initial environment). - Inline command assignments (`PATH=/safe hostcmd`) propagate because call() at runner_exec.go:110 forces vr.Exported = true before dispatch. New test TestHostExecUnexportedAssignmentNotForwarded verifies the divergent case ('LANG=should-not-leak; env' must not show LANG in the host binary's environment). Passes on debian:bookworm-slim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
julesmcrt
force-pushed
the
jules.macret/host-remediation/logrotate-host-exec
branch
from
a78f561 to
d4bd28b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fundamentally changes rshell's threat model and is not intended to ship as-is. It exists so that step 6 of the host-remediation demo (running
logrotate -f /etc/logrotate.d/appfrom inside a restricted-shell-driven remediation flow) has something concrete to point at and provoke a design conversation around.A real product version of this would need: a proper RFC, an audit trail, argv constraints / lint rules per binary, a configuration story that survives review by Security, a multi-binary scaffolding decision, cross-platform support, and (likely) a separate option/permission model rather than overloading
AllowedCommands.What this adds
A new
host:<name>=<absolute-path>syntax forAllowedCommands. When dispatch sees a non-builtin command name matching<name>, the binary at<absolute-path>is exec'd directly:exec.CommandContext(ctx, path, args[1:]...)— context cancel kills the child via SIGKILL (Go's defaultCancel).cmd.Stdin = r.stdin,cmd.Stdout = r.stdout,cmd.Stderr = r.stderr— full passthrough, so existing pipe/redirect plumbing on the runner inherits naturally.cmd.Dir = r.Dir— runs in the runner's cwd.cmd.Env— minimal: onlyPATH,HOME,LANGare forwarded; everything else from the parent process env is stripped.r.exit.codeso$?works. Process killed by signal (e.g. cancel) maps to 130; any non-ExitErrorstart failure surfaces to stderr and returns 127.host:is consulted only when the name is not a known builtin.rshell.command.host_exec=trueso the demo narrative ("look, the agent reached for the host") is visible in trace data.Linux-only. On darwin/windows the dispatch returns 127 with
host execution not supported on this platform.Shape choice
I chose Shape A (extend
AllowedCommandswith ahost:namespace) over Shape B (a separateHostCommands(map[string]string)option). Reasoning:AllowedCommands. Mixing host binaries into the same surface keeps that mental model intact for the demo and means CLI users only have to learn one flag (--allowed-commands) for both rshell builtins and host binaries.=<path>suffix makes host entries visually distinct fromrshell:<name>entries, and the namespace prefix keeps the parsing unambiguous.Out of scope (deferred until a real design pass)
logrotate -f <fixed-path>, and the flag-parser sandbox already prevents shell metacharacters from reachingargs.CI: known failures
TestInterpAllowedSymbols(inanalysis/) will fail becauseos/execis inpermanentlyBanned, andos.LookupEnvis not oninterpAllowedSymbols. This is by design — those allowlists exist precisely to gate decisions like this one. PerAGENTS.md, theverified/allowed_symbolslabel is for human manual approval and is not added in this PR.Verification
make fmt— clean.go test ./interp/... ./cmd/rshell/...— pass on darwin (Linux-onlyhost_exec_test.gocross-compiles cleanly).Manual smoke (darwin, builtin echo path):
Manual smoke (darwin, host stub path):
Tests added
interp/host_exec_test.go(Linux-only):/bin/echo)/usr/bin/false→ 1)/bin/catreading from a piped stdin)/bin/sleep 30within 2sPATH,HOME,LANGforwarded; sentinel var strippedScenario tests intentionally skipped — the docker harness adds variability and bash-comparison is meaningless when the whole point is to invoke a host binary.
Test plan
logrotate -f /etc/logrotate.d/app(deferred to demo runtime)🤖 Generated with Claude Code