Fix redirect tokens (2>/dev/null) parsed as rm positional arguments#30
Fix redirect tokens (2>/dev/null) parsed as rm positional arguments#30pyxl-dev wants to merge 4 commits intokenryu42:mainfrom
Conversation
shell-quote parses "2>/dev/null" into ["2", {op:">"}, "/dev/null"],
leaving the fd number and redirect target as string tokens in the
same segment as the command. This causes false positives when
rm -rf is used with redirects, because the redirect target (e.g.
/dev/null) is treated as an rm target path outside cwd.
Add redirect operator handling in splitShellCommands() that strips
the fd number from the current segment and skips the redirect target.
Fix kenryu42#29
📝 WalkthroughWalkthroughAdded redirect-operator handling to shell command tokenization: introduced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a false positive in the Key changes:
One logic concern: the Confidence Score: 4/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Next token from shell-quote] --> B{token undefined?}
B -- yes --> INC[i++, continue]
B -- no --> C{isOperator?\ne.g. &&, pipe, semicolon}
C -- yes --> D[Push current segment\nReset current\ni++]
C -- no --> E{isRedirectOp?\ne.g. >, >>, <, >&}
E -- yes --> F{current not empty AND\ncurrent.last matches /^\\d+$/ ?}
F -- yes --> G[Pop fd number\ne.g. pop '2' from 2>]
F -- no --> H[Skip redirect op + target\ni += 2]
G --> H
E -- no --> I{typeof token !== string?}
I -- yes --> INC2[i++, continue]
I -- no --> J[Process string token\nAdd to current\ni++]
D --> A
H --> A
INC --> A
INC2 --> A
J --> A
Last reviewed commit: 6896e62 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (isRedirectOp(token)) { | ||
| if (current.length > 0 && /^\d+$/.test(current[current.length - 1] ?? '')) { | ||
| current.pop(); | ||
| } | ||
| i += 2; // skip operator + redirect target | ||
| continue; |
There was a problem hiding this comment.
Pure-digit filenames incorrectly stripped as fd numbers
The heuristic /^\d+$/ at line 44 pops the last current token when it's all digits, assuming it's an fd number (e.g. 2 in 2>/dev/null). However, it also incorrectly matches legitimate pure-digit filenames that happen to appear immediately before a redirect operator.
For example, consider rm -rf 123 > /dev/null where 123 is a real directory name outside the cwd. The parsed token sequence is ['rm', '-rf', '123', { op: '>' }, '/dev/null']. When the redirect op is encountered, '123' matches /^\d+$/, gets popped, and the final segment becomes ['rm', '-rf'] — no path argument, so the command is no longer blocked even though it should be.
This is a false negative in a security check. A safer alternative would be to only pop when the last token consists of exactly one or two digits (covering fd numbers 0–99):
if (current.length > 0 && /^\d{1,2}$/.test(current[current.length - 1] ?? '')) {
This still covers every realistic fd number while being far less likely to match a directory named with a pure-digit string of three or more characters.
![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: 6896e622e3
ℹ️ 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".
| i += 2; // skip operator + redirect target | ||
| continue; |
There was a problem hiding this comment.
splitShellCommands() now advances by two tokens for every redirect, which also skips the $ token in targets like >$(git reset --hard). That prevents the existing $/( command-substitution branch from extracting the inner segment, so the dangerous command is no longer analyzed as its own command; in flows where fallback scanning is intentionally skipped (for example display-command heads in analyzeSegment), this becomes an allow-path for destructive nested commands.
Useful? React with 👍 / 👎.
src/core/shell.ts
Outdated
| if (current.length > 0 && /^\d+$/.test(current[current.length - 1] ?? '')) { | ||
| current.pop(); |
There was a problem hiding this comment.
The fd cleanup pops any trailing all-digit token before a redirect, but that also removes legitimate positional arguments in commands like rm -rf 123 > /tmp/log (where 123 is a path, not an fd). After this pop, analyzeRm() can see no deletion target and return allow, which weakens protections (notably paranoid/unknown-cwd behavior) for numeric-only paths.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/shell.ts (1)
459-465: Rename the helper to match the internal-function convention.This helper is module-private, so the repo guideline expects a leading underscore.
♻️ Suggested rename
- if (isRedirectOp(token)) { + if (_isRedirectOp(token)) {-function isRedirectOp(token: ParseEntry): boolean { +function _isRedirectOp(token: ParseEntry): boolean {As per coding guidelines, "Use leading underscore for private/internal module functions in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/shell.ts` around lines 459 - 465, Rename the module-private helper function isRedirectOp to follow the internal-function convention by changing its name to _isRedirectOp, and update every call site and export (if any) to use _isRedirectOp; the function signature and logic using ParseEntry and REDIRECT_OPS should remain the same, so search for references to isRedirectOp and replace them with _isRedirectOp to keep consistency with the repository guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/shell.ts`:
- Around line 43-48: The top-level scanner strips redirection tokens but nested
command substitutions still include them; update extractCommandSubstitution (and
any helper that builds nested segments) to detect redirect operators using
isRedirectOp and skip the operator and its target just like the top-level loop:
if the token before a redirect is a bare number remove it from the current
segment (current.pop()), then advance the index to skip operator + redirect
target (i += 2) and continue, ensuring nested segments do not retain fd or
redirect targets.
---
Nitpick comments:
In `@src/core/shell.ts`:
- Around line 459-465: Rename the module-private helper function isRedirectOp to
follow the internal-function convention by changing its name to _isRedirectOp,
and update every call site and export (if any) to use _isRedirectOp; the
function signature and logic using ParseEntry and REDIRECT_OPS should remain the
same, so search for references to isRedirectOp and replace them with
_isRedirectOp to keep consistency with the repository guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e4f5d2a-e2a4-47c3-83c2-3ac3b58bb08c
⛔ Files ignored due to path filters (2)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
src/core/shell.tstests/core/rules-rm.test.ts
- Limit fd number stripping to 1-2 digits (/^\d{1,2}$/) to avoid
false negatives with numeric directory names like "123"
- Add redirect handling in extractCommandSubstitution() so redirects
in $() subshells are also properly stripped
- Add tests for numeric dir before redirect and subshell redirect
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@pyxl-dev please address the codex's p1, p2 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/shell.ts (1)
38-49: Redirect stripping logic is sound, but consider edge case for malformed redirects.The logic correctly handles the
shell-quotetokenization of redirects. However,i += 2assumes the redirect target token always exists. For a malformed command likerm -rf foo >(missing target),tokens[i+1]may be undefined, and skipping it is benign but leaves intent unclear.Consider adding a bounds check for clarity and defensive coding:
🛡️ Optional: Add bounds check before skipping target
if (isRedirectOp(token)) { if (current.length > 0 && /^\d{1,2}$/.test(current[current.length - 1] ?? '')) { current.pop(); } - i += 2; // skip operator + redirect target + // skip operator; also skip redirect target if present + i += (i + 1 < tokens.length) ? 2 : 1; continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/shell.ts` around lines 38 - 49, The redirect-handling block should defensively check for the existence of the redirect target token before skipping it: in the isRedirectOp(token) branch (function/variable names: isRedirectOp, tokens, current, i) verify that tokens[i+1] exists (and is not another operator) before doing the i += 2 skip; if tokens[i+1] is missing, only advance i by 1 (or handle the malformed redirect explicitly) and leave a comment explaining the defensive behavior so malformed inputs like "rm >" are handled clearly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/shell.ts`:
- Around line 38-49: The redirect-handling block should defensively check for
the existence of the redirect target token before skipping it: in the
isRedirectOp(token) branch (function/variable names: isRedirectOp, tokens,
current, i) verify that tokens[i+1] exists (and is not another operator) before
doing the i += 2 skip; if tokens[i+1] is missing, only advance i by 1 (or handle
the malformed redirect explicitly) and leave a comment explaining the defensive
behavior so malformed inputs like "rm >" are handled clearly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25b37c83-d224-4255-abe8-92a6a16c447d
⛔ Files ignored due to path filters (2)
dist/bin/cc-safety-net.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (2)
src/core/shell.tstests/core/rules-rm.test.ts
2 participants
Summary
rm -rf <valid-path> 2>/dev/nullis blocked becauseshell-quoteparses2>/dev/nullinto separate string tokens ("2","/dev/null") that get treated asrmtarget pathsisRedirectOp()helper and redirect handling insplitShellCommands()that strips fd numbers and skips redirect targetsRoot Cause
shell-quoteparses2>/dev/nullinto:"2"(string) → treated as positional arg torm{ op: ">" }(object) → skipped by non-string check"/dev/null"(string) → treated as positional arg tormSince
/dev/nullis outside cwd, the command is incorrectly blocked.Fix
In
splitShellCommands(), before the generic non-string token skip, detect redirect operators (>,>>,<,>&,<&,>|), pop any trailing bare digit from the current segment (the fd number), and skip the next token (the redirect target).Test plan
bun run checkpasses (lint, types, dead code, 1057 tests)rm -rfwith2>/dev/null,2>&1,> /tmp/logrm -rf /outside/cwd 2>/dev/nullis still blockedFix #29
Summary by CodeRabbit
Bug Fixes
Tests