feat: check redirect targets against edit rules and external_directory - #14
Conversation
👀 AI Code ReviewThe PR adds the ability to capture and check redirect targets against edit rules and external directories, improving command safety by including previously ignored redirects. It introduces a new interface for redirect information and various rules for handling file and well-known redirects. Powered by GPT-4o via GitHub Models |
Redirects (2>&1, >/dev/null, > file, etc.) were invisible to the plugin because unbash stores them in cmd.redirects, not cmd.suffix. This meant the reconstructed command text silently dropped redirects, bypassing all security checks. - chain.ts: include redirects in ChainSegment with wellKnown flag - config.ts: add permission.edit rule parsing - enforce.ts: check non-well-known redirects against edit rules; if target is outside cwd, also check external_directory
8f05d68 to
85a1939
Compare
👀 AI Code ReviewThe PR implements redirect checks against edit rules and adds support for file and file descriptor redirects. All tests pass, and the structure appears well-considered for security enforcement. Powered by GPT-4o via GitHub Models |
| function isWellKnownRedirect(redir: Redirect): boolean { | ||
| const target = redir.target?.text ?? redir.content ?? ""; | ||
| if (target === "/dev/null") return true; | ||
| if (/^\d+$/.test(target)) return true; |
There was a problem hiding this comment.
What is matching for this rule?
There was a problem hiding this comment.
Matches targets that are purely numeric — like the 1 in 2>&1. The idea is that FD redirect targets (e.g. 1, 2) are always single/all digits.
However, you are right to flag this — > 12345 (redirect to a file literally named 12345) would also match. I can tighten the check to only mark it well-known when fileDescriptor is explicitly set AND the target is all digits, which eliminates the ambiguity. Want me to make that change?
There was a problem hiding this comment.
Is it cover redirects like &>?
There was a problem hiding this comment.
Yes — the well-known check looks at the target, not the operator. \u0026>/dev/null has target /dev/null → well-known. \u0026> output.txt has target output.txt → checked against edit rules. So > redirects are fully covered.
1 participant
Redirects (
2>&1,>/dev/null,> file, etc.) were invisible to the plugin becauseunbashstores them incmd.redirects, notcmd.suffix. The reconstructed command text silently dropped redirects, bypassing all security checks.Changes
src/chain.ts: AddedRedirectInfointerface andredirectsfield toChainSegment. Redirect operators and targets are now included ingetCommandText. Statement-level redirects ({ echo hi; } > file) are captured too.src/config.ts: AddededitRulestoPluginConfig, parsed frompermission.edit.src/enforce.ts: NewresolveRedirectTargets()function:2>&1,/dev/null, heredocs) → always allowedcwd→ checked againsteditRulesonlycwd→ checked againsteditRulesandexternal_directoryTests
7 new redirect enforcement tests + 6 new chain parsing tests. All 66 tests pass.