Skip to content
This repository was archived by the owner on May 15, 2026. It is now read-only.

fix(security): require explicit approval for shell redirection/background in command auto-approve - #11367

Closed
0xMink wants to merge 2 commits into
RooCodeInc:mainfrom
0xMink:fix/command-auto-approve-redirection
Closed

fix(security): require explicit approval for shell redirection/background in command auto-approve#11367
0xMink wants to merge 2 commits into
RooCodeInc:mainfrom
0xMink:fix/command-auto-approve-redirection

Conversation

@0xMink

@0xMink0xMink commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

Refs #11095

Summary

  • Command auto-approval could approve an allowlisted prefix while shell operators (redirection/background) alter execution semantics under shell: true. For example, auto-approving git show would also auto-approve git show > out.txt, allowing writes to an arbitrary file.
  • Commands containing file redirection (<, >, >>, <<, etc.) now require explicit approval (ask_user).
    • Safe fd-to-fd duplication is excluded (for example 2>&1, >&2, <&3, 0<&4).
  • Commands containing a standalone background operator (&) now require explicit approval.
  • fd-to-fd stripping now uses a token-boundary lookahead (?=$|\s|[;&|()<>]) to avoid false negatives such as >&2file and <&3in, where the redirection target is a word and therefore represents file redirection, not fd-to-fd duplication.

Test plan

  • containsShellFileRedirection() — 21 cases: detects output/input/append/here-doc/stderr/mixed redirections (9), excludes safe fd-to-fd like 2>&1, >&2, <&3, 0<&4 (5), token-boundary edge cases — >&2file, <&3in, 2>&1&&, 2>&1>out.txt, 0<&4| (5), general sanity checks (2)
  • containsBackgroundOperator() — 8 cases: detects standalone &, excludes &&, &>, >&, <&
  • getCommandDecision() — 15 integration cases: redirection forces ask_user, fd-to-fd preserves auto_approve, background forces ask_user, token-boundary edge cases, denylist regression
  • Existing regression coverage for containsDangerousSubstitution, findLongestPrefixMatch, getSingleCommandDecision — 11 cases
  • Total: 55 tests, all passing

@dosubotdosubotBot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Feb 10, 2026
@ghost

ghost commented Feb 10, 2026

Copy link
Copy Markdown

RooviewerClockSee task

All issues from previous reviews have been addressed. Quote-aware operator detection via stripQuotedSegments is correctly implemented. No new issues found.

  • containsShellFileRedirection strips output fd-to-fd (>&N) but not input fd-to-fd (<&N), producing false positives for commands like cmd <&3. Very low severity.
Previous reviews

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

Comment threadsrc/core/auto-approval/commands.ts Outdated
@0xMink
0xMinkforce-pushed the fix/command-auto-approve-redirection branch from 4c99f4f to cf5084fCompareFebruary 10, 2026 09:35
@0xMink
0xMinkforce-pushed the fix/command-auto-approve-redirection branch from cf5084f to 2ab6380CompareFebruary 10, 2026 10:05
@0xMink

Copy link
Copy Markdown
ContributorAuthor

Addressed the review feedback:

  • Fixed false negative: Added token-boundary lookahead to fd-to-fd stripping so >&2file and <&3in are correctly treated as file redirection (not fd-to-fd). The boundary (?=$|\s|[;&|()<>]) ensures stripping only when the fd target is purely numeric and token-delimited.
  • Added input fd-to-fd support:<&N and 0<&N patterns are now stripped alongside >&N, and excluded from background operator detection.
  • Kept correct behavior:2>&1&& still strips; 0<&4| still strips; >&2 at end-of-string still strips.
  • Consistent stripping: Both containsShellFileRedirection() and the sub-command normalization in getCommandDecision() use the same boundary-aware regex.
  • Tests: 55/55 passing (up from 44). Added regression tests for >&2file, <&3in, operator-adjacent fd-to-fd, and input fd duplication at both unit and integration levels.

@0xMink

Copy link
Copy Markdown
ContributorAuthor

@roomote Could you please re-review? I addressed your <&N note, and also tightened fd-to-fd stripping with a token-boundary lookahead to avoid false negatives like >&2file / <&3in. Tests expanded from 44 to 55 and I force-pushed the update. Thanks.

@ghost

Copy link
Copy Markdown

FixarooClockSee task

No outstanding issues to fix. The reviewer has confirmed all previous issues are addressed, and all 55 tests pass locally.

…ound in command auto-approve
Commands containing file redirection (<, >, >>, <<) or standalone
background operators (&) now force explicit user approval instead
of being auto-approved via prefix matching. Safe fd-to-fd redirections
(2>&1, >&2) remain auto-approvable.
Also tightens the pre-existing fd-to-fd stripping regex with a
token-boundary lookahead to prevent false negatives like >&2file
and <&3in being silently stripped as fd-to-fd.
Adds the first dedicated test suite for the command auto-approval
pipeline (55 tests).
@0xMink
0xMinkforce-pushed the fix/command-auto-approve-redirection branch from 2ab6380 to 0d2a949CompareFebruary 10, 2026 23:31
@0xMink

Copy link
Copy Markdown
ContributorAuthor

Rebased onto main, resolved add/add in commands.spec.ts with #11365 / #11382, and made redirection/background detection quote-aware to avoid false positives in quoted payloads (e.g. node -e "const f=(a)=>a"). All CI checks are green.

@roomote please re-review.

@ghost

ghost commented Feb 11, 2026

Copy link
Copy Markdown

FixarooClockSee task

Verified the latest state of the PR. The reviewer confirmed all previous issues are resolved and no new issues were found. All 79 tests pass locally. No code changes needed.

@hannesrudolph

Copy link
Copy Markdown
Contributor

Closing: PR lacks clear evidence of manual testing.

@robertjmcintyre

Copy link
Copy Markdown
Contributor

@0xMink - I think that this is a fairly important issue, especially as people get more and more comfortable with giving Roo increasing autonomy. Is it possible to get this reactivated and committed?

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugSomething isn't workingsize:LThis PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@0xMink@hannesrudolph@robertjmcintyre