feat(shell): sensitive-path pattern guard for run_command args (#259)#911
feat(shell): sensitive-path pattern guard for run_command args (#259)#911KuaaMU wants to merge 2 commits into
Conversation
…ine#259) Path-argument scanner on top of the existing allowlist gate. When a run_command or run_background call matches the allowlist but one of its argv tokens resolves into a sensitive prefix or matches a sensitive filename pattern, the call is demoted to the confirm gate — same code path as RISKY_ARGS. Default blocklist: - Path prefixes: ~/.ssh, ~/.aws, ~/.gnupg, ~/.kube, /etc/shadow, /etc/sudoers - Filename patterns (case-insensitive): *.env, *.env.*, *.key, *.pem, id_rsa*, id_ed25519*, *credentials*, *secret* User-configurable via ReasonixConfig.sensitivePaths (prefixes + patterns). Refs: OWASP AI Agent Cheat Sheet §1, §8
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07f3f716db
ℹ️ 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".
| if (chain === null) return isAllowed(cmd, extra); | ||
| return chainAllowed(chain, (seg) => isAllowed(seg, extra)); | ||
| if (chain === null) return isAllowed(cmd, extra, projectRoot, sensitivePathConfig); | ||
| return chainAllowed(chain, (seg) => isAllowed(seg, extra, projectRoot, sensitivePathConfig)); |
There was a problem hiding this comment.
Include redirect targets in sensitive-path checks
Sensitive-path demotion can be bypassed with input redirection because isCommandAllowed parses chains/redirects and then re-checks each segment using only seg.argv.join(" "), which drops redirect targets. As a result, commands like cat < ~/.ssh/id_rsa (or head < /etc/shadow) are treated as plain cat/head and skip the confirm gate despite reading sensitive files.
Useful? React with 👍 / 👎.
|
|
||
| /** Ensure prefix matches only at directory boundaries (not mid-segment). */ | ||
| function pathStartsWithPrefix(normalized: string, prefix: string): boolean { | ||
| return normalized === prefix || normalized.startsWith(`${prefix}/`); |
There was a problem hiding this comment.
Normalize prefix boundary matching for Windows paths
The prefix boundary check hardcodes / as the separator, so on Windows normalized paths like C:\Users\me\.ssh\id_rsa will not satisfy startsWith(${prefix}/) when prefix is C:\Users\me\.ssh. This means child paths under sensitive prefixes are not demoted on Windows (only an exact directory match is), allowing the new guard to be bypassed on that platform.
Useful? React with 👍 / 👎.
|
Solid piece of work — OWASP § 1 / § 8 is the right framing, and demoting through the existing Two real fixes before merge:
After those two, happy to merge — the defaults, chain handling, and boundary-correct prefix match are otherwise exactly right. |
Recent PRs hit Windows-specific bugs that Ubuntu CI couldn't catch — literal forward-slash separators in path comparisons (#911) and `partial.startsWith("/")` as an "is absolute path" heuristic (#922) both sailed through `ubuntu-latest` while shipping broken on the platform that most of the user base actually runs. Add `windows-latest` to the existing matrix so every PR exercises the Windows code paths (taskkill tree-kill, MS Store node-shim detection, path separator handling, etc.) before review. Job name now includes the OS so the two runs are distinguishable in branch protection. Also swap the hardcoded `/tmp/tau-dry.json` in the τ-bench dry-run for `${{ runner.temp }}` — RUNNER_TEMP is set on every GitHub Actions runner including Windows, where `/tmp` isn't a path. Co-authored-by: reasonix <reasonix@deepseek.com>
1. Read config.sensitivePaths in registerRooted and forward to registerShellTools so the YAML override actually takes effect. 2. Use pathMod.sep in pathStartsWithPrefix instead of hardcoded '/' so Windows backslash paths match correctly.
|
Fixed both issues in the latest push:
|
What
Path-argument scanner on top of the existing allowlist gate. When a
run_commandorrun_backgroundcall matches the allowlist but one of its argv tokens resolves into a sensitive prefix or matches a sensitive filename pattern, the call is demoted to the confirm gate — same code path asRISKY_ARGS.Why
Read-only commands like
cat,grep,find,head,tailare on the fast path with any path argument. The agent cancat ~/.ssh/id_rsaorgrep -r AWS_SECRET /etcand pipe the contents straight into the LLM context — a real exfiltration vector even on a local CLI.OWASP AI Agent Cheat Sheet § 8 (Data Protection / Path traversal) and § 1 frame this as the complement to per-tool least-privilege: not just which command, but against which resources.
Default blocklist
Path prefixes (tilde-relative, resolved at runtime):
~/.ssh,~/.aws,~/.gnupg,~/.kube/etc/shadow,/etc/sudoersFilename patterns (case-insensitive basename match):
*.env,*.env.*,*.key,*.pemid_rsa*,id_ed25519**credentials*,*secret*User configuration
Changes
src/tools/shell/parse.tshasSensitivePathArgs(),isAllowed()/isCommandAllowed()accept optionalprojectRoot+sensitivePathConfigsrc/tools/shell.tsShellToolsOptions.sensitivePaths, passrootDir+ config through to dispatchsrc/config.tsReasonixConfig.sensitivePathstests/shell-tools.test.tsDesign notes
isAllowed(cmd)(2-arg form) still works — sensitive-path check only fires whenprojectRootis provided.-d), URLs (http://...), env vars ($HOME) are not resolved as paths.~/.sshmatches~/.ssh/id_rsabut NOT~/.ssh_extra/config.Acceptance criteria (from #259)
SENSITIVE_PATHSconfig layer (default list + user override)src/tools/shell.ts/parse.tsthat flags any argv token resolving into the blocklistRISKY_ARGS(chains + confirm gate work uniformly)~/-relative paths, cwd-relative paths, glob patterns, case-insensitive on win32./src/.env.exampleshould still trigger