Conversation
… filter `exec` outside a subshell composes its own std::process::Command and calls CommandExt::exec, replacing the shell process image without ever reaching commands::execute_external_command. It is therefore not covered by the filtering applied there: a CmdExecFilter that denies `rm` is still replaceable by `exec /bin/rm`. Run the same pre_external_cmd filter in the builtin, before cmd.exec(), and honor its decision. The program is resolved first (new commands::resolve_external_program, mirroring external dispatch) so the filter is shown the executable that will actually run; otherwise the OS would redo its own PATH search against the child environment, which need not match the shell's. PreFilterResult is #[non_exhaustive]; this site is about to replace the process image, so an unrecognized decision fails closed rather than being guessed at as "continue". Tests run in a dedicated child process: a successful exec replaces the process image and a broken implementation would take the test runner with it. Against the unfiltered exec.rs they fail with "the denied program ran and replaced the shell: /tmp/.../marker exists". Two limits found while porting, noted for follow-up rather than worked around: ExternalCmdParams exposes neither the command name as written nor argv0, so a policy cannot match on the spelling used or see an `exec -a` override; and a successful exec never returns, so post_external_cmd can never fire for it. Assisted-by: Claude Code:claude-opus-5
Public API changes for crate: brush-coreRemoved itemsAdded itemsChanged itemsPerformance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
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
2 participants
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.
Draft, and mostly not my work. The first commit is @reubeno's #972 (
static-filter-attempt-minimized), rebased onto currentmain. The second is one fix on top. Opened as a draft so the rebase is reviewable rather than described — take it, adapt it, or close it; it's your design and I'd rather you drive it.Context in #1183: you asked me in June whether the statically registered filters would serve our use case, I didn't answer and pushed a competing seam in #1184 instead. #1184 is now closed. This is me answering properly.
Commit 1 — #972 rebased onto
30a3bceOne cherry-pick. Only two files conflicted,
brush-core/src/commands.rsandbrush-core/src/shell/execution.rs. Two things inexecute_via_externalneeded carrying forward that the branch predates, and are easy to lose in a mechanical rebase:main'sargv0override —execute_external_commandgrew anargv0_overrideparameter, which the branch's call site doesn't pass;main's$_update after external commands (update_last_arg_variable), which the branch'sfinallyblock drops.Both are restored in the rebase.
Commit 2 — route the
execbuiltin through the filterbrush-builtins/src/exec.rscomposes its ownstd::process::Commandand callsCommandExt::exec, replacing the shell process image without ever reachingexecute_external_command. So aCmdExecFilterthat deniesrmis still replaceable byexec /bin/rm. #972 touches nobrush-builtins/files, so the gap is inherited.This runs the same
pre_external_cmdfilter in the builtin, beforecmd.exec(), and resolves the program first (newcommands::resolve_external_program) so the filter is shown the executable that will actually run — otherwise the OS redoes its ownPATHsearch against the child environment, which need not match the shell's.PreFilterResultis#[non_exhaustive], and this site is about to replace the process image, so an unrecognized decision fails closed rather than being guessed at as "continue".Test. Cases run in a dedicated child process — this test binary, re-invoked — because a successful
execreplaces the process image and a broken implementation takes the test runner with it. Each asserts the marker the denied program would have created does not exist, and that the filter was consulted exactly once. Against the unfilteredexec.rsthey fail withthe denied program ran and replaced the shell: /tmp/.../marker exists.What I found porting a real policy onto this
Offered as feedback, not as requests. Our downstream is an object-capability layer for LLM coding agents; the shell is the tool surface, and a per-invocation grant has to be enforced in-process because a name containing a path separator bypasses both
PATHand the builtin table.ExternalCmdParamsexposes neither the command name as written norargv0— only theExternalCommand(program, args, envs, cwd). A policy can't match on the spelling used, and can't seeexec -a impostor /usr/bin/touch. I dropped two tests in the port because the information isn't there.cmd_exec_filteris#[serde(skip, default = …)]— a serialized shell returns with a freshly-defaulted filter rather than the installed one. If the default is the no-op, a round-trip silently unconfines; if it isn't, a stateful policy still returns without its state.execnever returns, sopost_external_cmdcan never fire for it. An audit log assuming pre/post pairing will mis-attribute it — probably worth a line infilter-architecture.md.SourceFilterhandles./source, but>,>>and<>reachShell::open_fileunfiltered. All fouropen_filecall sites already know their access intent explicitly, so it needn't be recovered after the fact.while … done, which for a runaway-bounding policy means the leash doesn't hold. We needed a notion of a terminating error downstream.Validation
At the head commit against base
30a3bce:cargo xtask ci pre-commit— fmt, build, clippy--workspace --all-features --all-targets, schemas clean; unit 390/390; integration (nextest--workspace) 2709/2709;cargo test --test brush-compat-testsunchanged from cleanmain.cargo denyreportsadvisories FAILED, identically on cleanmain:chacha20 0.10.1was yanked after the lockfile was pinned.cargo update -p chacha20resolves to 0.10.2.