Fix command injection via newline and quoting/wildcard blocklist bypass#429
Conversation
Addresses two critical security vulnerabilities in command validation: 1. Newline bypass (wonderwhy-er#422): The extractCommands() separator array did not include newline characters (\n, \r\n, \r). An attacker could chain a benign command with a blocked command using a newline, and the shell would execute both while validation only checked the first. 2. Quoting and wildcard bypass (wonderwhy-er#421): extractBaseCommand() did not strip surrounding quotes from command tokens, so "rm" would not match the blocked command rm. Additionally, glob characters (*, ?, [, ]) in command paths (e.g. /usr/bin/su*o) would evade literal blocklist matching but expand to the real binary at shell execution time. Changes: - Add \r\n, \n, \r to the command separators array - Strip single and double quotes from command tokens before comparison - Reject any command containing glob/wildcard characters in both extractBaseCommand() and validateCommand()
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughWalkthroughextractCommands now treats CR/LF/newline as command separators; extractBaseCommand strips surrounding quotes, rejects tokens containing glob/brace characters by returning null (propagated as 'INVALID_COMMAND'); validateCommand enforces fail-closed behavior when invalid or glob-containing tokens are present. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
codeant-ai
Bot
added
the
size:S
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/command-manager.ts`:
- Around line 221-225: The firstToken quote-stripping only removes a perfectly
wrapped pair and misses concatenated quoted/unquoted fragments (e.g. `"r"m`), so
before you perform the basename/blocklist check normalize quoting across the
whole token (or replace internal single/double quote characters) so `"r"m`,
`r"m"`, and `'r''m'` become rm, or alternatively parse the command with a
shell-aware tokenizer; apply this change where firstToken is prepared for the
basename/blocklist comparison so the blocklist check sees the actual command
name.
- Around line 248-252: The glob-wildcard rejection currently tests the entire
input string; change it to only test the resolved executable token by calling
getBaseCommand(command) (the same helper used at line 229) and run the
/[*?\[\]]/ test against that returned base command; if the base command contains
glob chars return false, otherwise allow the rest of the command to contain
quoted/escaped metacharacters. Ensure you preserve the existing regex and return
behavior but switch the input from the full command string to
getBaseCommand(command).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…utable token Address CodeRabbit review feedback: - Quote stripping now removes all quote chars instead of only outer pairs, so concatenated fragments like "r"m, r"m", 'r''m' resolve to the actual command name and get caught by the blocklist. - Glob/wildcard rejection moved from validateCommand (entire input) into extractBaseCommand (executable token only), so legitimate args with wildcards like `ls *.txt` are no longer blocked.
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:S
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command-manager.ts (1)
250-253:⚠️ Potential issue | 🟠 MajorGlob bypass via fallback path to unprotected
getBaseCommand.When
extractBaseCommandreturnsnulldue to glob characters, the command isn't added toallCommands, making it empty. This triggers the fallback togetBaseCommand, which lacks both quote stripping and glob rejection.Bypass example:
- Input:
/usr/bin/su*oorsu*oextractBaseCommandreturnsnull(glob detected)allCommandsis empty → fallback triggeredgetBaseCommandreturns/usr/bin/su*o(no protection)- Doesn't match
sudoin blocklist → command allowedConsider rejecting commands when
extractBaseCommandreturnsnulldue to glob patterns, or apply the same protections in the fallback path.🔒 Proposed fix: fail-closed when glob causes extraction failure
// If there are no commands extracted, fall back to base command if (allCommands.length === 0) { const baseCommand = this.getBaseCommand(command); + // Reject if the base command contains glob/wildcard characters + if (/[*?\[\]]/.test(baseCommand)) { + return false; + } return !blockedCommands.includes(baseCommand); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command-manager.ts` around lines 250 - 253, The current fallback path uses getBaseCommand when allCommands is empty, allowing commands with globs to bypass checks; update the logic so that if extractBaseCommand returns null for any input (indicating a glob/invalid extraction) you fail-closed — i.e., treat the command as blocked instead of falling back — or alternatively make the fallback call reuse the same protections as extractBaseCommand (strip quotes and detect/reject glob patterns) before consulting blockedCommands; locate references to allCommands, extractBaseCommand, and getBaseCommand in the command filtering code and enforce the fail-closed behavior or identical sanitization on the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/command-manager.ts`:
- Around line 250-253: The current fallback path uses getBaseCommand when
allCommands is empty, allowing commands with globs to bypass checks; update the
logic so that if extractBaseCommand returns null for any input (indicating a
glob/invalid extraction) you fail-closed — i.e., treat the command as blocked
instead of falling back — or alternatively make the fallback call reuse the same
protections as extractBaseCommand (strip quotes and detect/reject glob patterns)
before consulting blockedCommands; locate references to allCommands,
extractBaseCommand, and getBaseCommand in the command filtering code and enforce
the fail-closed behavior or identical sanitization on the fallback.
|
Friendly bump -- let me know if anything needs changing. |
When extractBaseCommand returns null (e.g. due to glob in the command name), validateCommand falls back to getBaseCommand which had no glob protection. This allowed e.g. /usr/bin/su*o to bypass the blocklist. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:S
|
CodeAnt AI Incremental review completed. |
|
Added a glob/wildcard check in the |
|
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/command-manager.ts (1)
250-258:⚠️ Potential issue | 🔴 CriticalFallback-only glob check leaves the common path unprotected.
This guard only fires when
extractCommandsreturned an empty array, i.e. the input is a single unparseable token. As soon asextractCommandsproduces at least one benign base command, control skips this branch entirely and any glob-containing sibling segment (already silently dropped byextractBaseCommandreturningnull) escapes validation. See the detailed repro and fix in the comment on lines 225–229 — resolving that comment fixes this gap as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command-manager.ts` around lines 250 - 258, The current glob-wildcard guard in the fallback branch only runs when extractCommands yields no parses; fix by validating every extracted command instead of only the empty case: in the code paths using extractCommands (and inside getBaseCommand/extractBaseCommand flows), iterate allCommands (and treat any null/undefined base returned by extractBaseCommand as a rejection) and run the glob regex /[*?\[\]]/ against each baseCommand, returning false if any base contains a glob or any base is missing; ensure blockedCommands check is applied per-base as well so no glob-containing sibling segments slip through.
🤖 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/command-manager.ts`:
- Line 227: Update the command rejection regex to include brace-expansion
characters so brace patterns like /usr/bin/s{udo,x} are caught; find the checks
that test firstToken with the pattern /[*?\[\]]/ (around where firstToken is
examined) and replace both occurrences (the one at the firstToken check and the
later similar check near line 254) with a character class that also includes {
and } (i.e., add curly braces to the class) so brace expansion is rejected
alongside *, ?, and bracket patterns.
---
Duplicate comments:
In `@src/command-manager.ts`:
- Around line 250-258: The current glob-wildcard guard in the fallback branch
only runs when extractCommands yields no parses; fix by validating every
extracted command instead of only the empty case: in the code paths using
extractCommands (and inside getBaseCommand/extractBaseCommand flows), iterate
allCommands (and treat any null/undefined base returned by extractBaseCommand as
a rejection) and run the glob regex /[*?\[\]]/ against each baseCommand,
returning false if any base contains a glob or any base is missing; ensure
blockedCommands check is applied per-base as well so no glob-containing sibling
segments slip through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…to blocklist
- When extractBaseCommand returns null (glob/invalid), push sentinel
'__INVALID_COMMAND__' instead of silently dropping the segment. validateCommand
now rejects the entire input if any segment is invalid, preventing e.g.
'ls && su*o' from bypassing the blocklist.
- Extend the glob regex to include brace-expansion chars ({}) so
'/usr/bin/s{udo,x}' is also rejected.
|
Addressed the latest coderabbitai feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/command-manager.ts (1)
213-221:$()-token branch inextractBaseCommandskips quote-stripping and glob rejection.This branch is mostly unreachable in practice because
extractCommandsintercepts$()at the character level (lines 60–81), but it is reachable via the error fallback at line 172 (extractBaseCommand(commandString)whenextractCommandsthrows). IffirstTokenis something like$(rm*)or$("r"m), this path returnsrm*/"r"mwithout applying the quote-strip or glob check added below, so the sentinel/rejection paths never fire.For defense-in-depth consistency, route the inner token through the same normalization used at lines 225–231 (strip quotes, reject glob/brace) before returning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command-manager.ts` around lines 213 - 221, The $()-token branch in extractBaseCommand currently returns the raw inner token and bypasses the quote-stripping and glob/brace rejection used later; update the branch to normalize the inner token the same way as the main path: after computing inner and innerTokens, set the candidate to path.basename(innerTokens[0]).toLowerCase(), then strip surrounding single/double quotes and run the same glob/brace rejection logic used later (the same checks applied to firstToken) before returning, so extractBaseCommand and the fallback from extractCommands behave consistently.
🤖 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/command-manager.ts`:
- Around line 139-143: When extractBaseCommand(currentCmd.trim()) returns null,
only push the fail-closed sentinel '__INVALID_COMMAND__' if the original segment
actually contains glob/brace characters; otherwise keep the previous silent-drop
behavior for benign nulls (env-var only segments, bare $VARIABLE/paren tokens,
empty $() or thrown exceptions). Concretely: compute a quick check for
glob/brace characters in the segment (e.g. presence of '*', '?', '[', ']', '{',
'}') and change the block that currently does commands.push(baseCommand !== null
? baseCommand : '__INVALID_COMMAND__') to push the sentinel only when
baseCommand === null && segmentContainsGlob; do the same modification in the
other place that handles the final segment where baseCommand is treated
similarly.
---
Nitpick comments:
In `@src/command-manager.ts`:
- Around line 213-221: The $()-token branch in extractBaseCommand currently
returns the raw inner token and bypasses the quote-stripping and glob/brace
rejection used later; update the branch to normalize the inner token the same
way as the main path: after computing inner and innerTokens, set the candidate
to path.basename(innerTokens[0]).toLowerCase(), then strip surrounding
single/double quotes and run the same glob/brace rejection logic used later (the
same checks applied to firstToken) before returning, so extractBaseCommand and
the fallback from extractCommands behave consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (currentCmd.trim()) { | ||
| const baseCommand = this.extractBaseCommand(currentCmd.trim()); | ||
| if (baseCommand) commands.push(baseCommand); | ||
| // null means glob/invalid — push sentinel so validateCommand fails closed | ||
| commands.push(baseCommand !== null ? baseCommand : '__INVALID_COMMAND__'); | ||
| } |
There was a problem hiding this comment.
Fail-closed sentinel is pushed for all extractBaseCommand nulls, not only glob rejection — regresses legitimate multi-segment inputs.
extractBaseCommand returns null in several non-malicious cases beyond glob detection:
- Segment is only env-var assignments, e.g.
FOO=bar→ line 181 strips to empty → line 184 returnsnull. - Segment is only dollar-variable /
(tokens, e.g.$MYVAR→ loop at lines 191–206 never setsfirstToken→ line 210 returnsnull. $()with empty inner (line 220).- Any thrown exception (line 238).
Previously these were silently dropped by the if (baseCommand) guard. With the new sentinel push, benign inputs such as FOO=bar; ls or $MYVAR && ls are now blocked outright via the check at line 264, which was not an intended goal of #421/#422 and will surface as Error: Command not allowed: ... in the caller (src/tools/improved-process-tools.ts lines 119–125).
Disambiguate: only emit the sentinel when the segment actually contains glob/brace characters, and keep the silent-drop behavior for the other null causes.
🛡️ Proposed fix — gate sentinel on glob presence in the segment
if (commandString.startsWith(separator, i)) {
// We found a separator - extract the command before it
if (currentCmd.trim()) {
- const baseCommand = this.extractBaseCommand(currentCmd.trim());
- // null means glob/invalid — push sentinel so validateCommand fails closed
- commands.push(baseCommand !== null ? baseCommand : '__INVALID_COMMAND__');
+ const trimmed = currentCmd.trim();
+ const baseCommand = this.extractBaseCommand(trimmed);
+ if (baseCommand !== null) {
+ commands.push(baseCommand);
+ } else if (/[*?\[\]{}]/.test(trimmed)) {
+ // Segment contained a glob/brace token we could not
+ // safely resolve — fail closed.
+ commands.push('__INVALID_COMMAND__');
+ }
+ // else: env-vars-only / variable-only / empty — drop silently.
}Apply the same treatment to the final-segment block at lines 159–163.
Also applies to: 159-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command-manager.ts` around lines 139 - 143, When
extractBaseCommand(currentCmd.trim()) returns null, only push the fail-closed
sentinel '__INVALID_COMMAND__' if the original segment actually contains
glob/brace characters; otherwise keep the previous silent-drop behavior for
benign nulls (env-var only segments, bare $VARIABLE/paren tokens, empty $() or
thrown exceptions). Concretely: compute a quick check for glob/brace characters
in the segment (e.g. presence of '*', '?', '[', ']', '{', '}') and change the
block that currently does commands.push(baseCommand !== null ? baseCommand :
'__INVALID_COMMAND__') to push the sentinel only when baseCommand === null &&
segmentContainsGlob; do the same modification in the other place that handles
the final segment where baseCommand is treated similarly.
|
Friendly bump -- let me know if anything needs changing. |
1 participant
User description
Summary
Fixes two critical command injection vulnerabilities in
src/command-manager.tsthat allow complete bypass of theblockedCommandssecurity feature:Newline bypass ([Security] Command Injection via Blocklist Bypass using Newline Character #422):
extractCommands()did not treat newline characters as command separators. An attacker could injectecho benign\nrm -rf /and validation would only checkecho, while the shell would execute both commands. Fixed by adding\r\n,\n,\rto the separators array.Quoting and wildcard bypass ([Security] Command Injection via Blocklist Bypass using Quoting and Wildcards #421):
extractBaseCommand()did not strip quotes from tokens, so"rm"would not match the blocked commandrm. Wildcard paths like/usr/bin/su*owould also evade literal blocklist matching but expand at shell execution time. Fixed by stripping surrounding quotes before comparison and rejecting any command containing glob characters (*,?,[,]).Changes
All changes are in
src/command-manager.ts:\r\n,\n,\rto theseparatorsarray inextractCommands()extractBaseCommand()extractBaseCommand()andvalidateCommand()Test plan
echo benign\nrm fileis correctly split into two commands andrmis blocked"rm" fileis correctly identified asrmand blocked/usr/bin/su*ois rejected outright due to glob charactersnpm test)Closes #421
Closes #422
Summary by CodeRabbit
CodeAnt-AI Description
Block command-blocklist bypasses through newlines, quotes, and wildcard command names
What Changed
"rm","r"m, or'r''m'are now recognized as the blocked command they represent.*,?,[or]are now rejected to stop path-based bypasses like/usr/bin/su*o.Impact
✅ Fewer command-injection bypasses✅ Safer blocked-command enforcement✅ Lower risk of hidden shell commands🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.