Conversation
…en reader test snapshots
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses a critical security vulnerability regarding command injection in the shell tool, while also introducing accessibility enhancements for the CLI interface. The security fix ensures that shell substitution syntax is properly parsed and blocked when unsafe, while the UI updates improve the experience for users relying on screen readers. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
gemini-cli
bot
added
priority/p1
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces accessibility enhancements for the Rewind UI and implements security measures to prevent command injection in shell tools. The UI components now offer optimized layouts for screen readers, while the core logic includes a new utility to detect and block shell command substitution. Review feedback identifies a security bypass regarding process substitution and recommends additional test cases to handle escaped characters and quoted strings within the detection logic.
| return true; | ||
| } | ||
|
|
||
| if (char === '<' && command[i + 1] === '(') { |
There was a problem hiding this comment.
The detectCommandSubstitution function has a critical vulnerability. It fails to detect process substitution using the >(...) syntax, which can be exploited for command injection (e.g., echo hello > >(whoami)). This allows an attacker to bypass the security check. The provided code suggestion updates the condition to also detect and block the >(...) syntax, alongside the existing <(...) check.
Additionally, the current implementation has issues that can lead to false positives, such as incorrect escape handling for \$(...) outside double quotes and incorrectly flagging <(...) inside double quotes. While the immediate fix addresses the >(...) bypass, a more comprehensive review of the detectCommandSubstitution logic is recommended to universally handle shell escapes and ensure process substitution is only checked when not inside quotes, to improve accuracy and prevent further bypasses or usability issues. Please ensure that any changes to escape handling do not result in the sanitization of ANSI escape sequences, as preserving them is the desired behavior.
| if (char === '<' && command[i + 1] === '(') { | |
| if ((char === '<' || char === '>') && command[i + 1] === '(') { |
References
- Do not sanitize command names for ANSI escape sequences or other C0 control characters, as the current behavior is desired.
| describe('command injection detection', () => { | ||
| it('should block $() command substitution', async () => { | ||
| const tool = new ShellTool(mockConfig, createMockMessageBus()); | ||
| const invocation = tool.build({ command: 'echo $(whoami)' }); | ||
| const result = await invocation.execute(new AbortController().signal); | ||
| expect(result.returnDisplay).toContain('Blocked'); | ||
| }); | ||
|
|
||
| it('should block backtick command substitution', async () => { | ||
| const tool = new ShellTool(mockConfig, createMockMessageBus()); | ||
| const invocation = tool.build({ command: 'echo `whoami`' }); | ||
| const result = await invocation.execute(new AbortController().signal); | ||
| expect(result.returnDisplay).toContain('Blocked'); | ||
| }); | ||
|
|
||
| it('should allow normal commands without substitution', async () => { | ||
| mockShellExecutionService.mockImplementation((_cmd, _cwd, _callback) => ({ | ||
| pid: 12345, | ||
| result: Promise.resolve({ | ||
| output: 'hello', | ||
| rawOutput: Buffer.from('hello'), | ||
| exitCode: 0, | ||
| signal: null, | ||
| error: null, | ||
| aborted: false, | ||
| pid: 12345, | ||
| executionMethod: 'child_process', | ||
| backgrounded: false, | ||
| }), | ||
| })); | ||
| const tool = new ShellTool(mockConfig, createMockMessageBus()); | ||
| const invocation = tool.build({ command: 'echo hello' }); | ||
| const result = await invocation.execute(new AbortController().signal); | ||
| expect(result.returnDisplay).not.toContain('Blocked'); | ||
| }); | ||
|
|
||
| it('should allow single quoted strings with special chars', async () => { | ||
| mockShellExecutionService.mockImplementation((_cmd, _cwd, _callback) => ({ | ||
| pid: 12345, | ||
| result: Promise.resolve({ | ||
| output: '$(not substituted)', | ||
| rawOutput: Buffer.from('$(not substituted)'), | ||
| exitCode: 0, | ||
| signal: null, | ||
| error: null, | ||
| aborted: false, | ||
| pid: 12345, | ||
| executionMethod: 'child_process', | ||
| backgrounded: false, | ||
| }), | ||
| })); | ||
| const tool = new ShellTool(mockConfig, createMockMessageBus()); | ||
| const invocation = tool.build({ | ||
| command: "echo '$(not substituted)'", | ||
| }); | ||
| const result = await invocation.execute(new AbortController().signal); | ||
| expect(result.returnDisplay).not.toContain('Blocked'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The new tests for command injection are a great start, but they could be more comprehensive to ensure the detection logic is robust. Please consider adding tests for these edge cases:
- Escaped substitution outside quotes: A command like
echo \$(whoami)should be allowed, as the backslash escapes the substitution. - Process substitution inside double quotes: A command like
echo "<(whoami)"should be allowed, as process substitution is not active inside double quotes. - Process substitution without quotes: A command like
echo <(whoami)should be blocked. - Escaped substitution inside double quotes: A command like
echo "\$(whoami)"should be allowed.
Adding these tests will help verify the correctness of the detectCommandSubstitution function and prevent both false positives and potential bypasses.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a command injection detection mechanism in the ShellTool to prevent the execution of shell commands containing command or process substitution syntax. It includes a new utility function detectCommandSubstitution and comprehensive unit tests. Feedback highlights that the backslash escape logic is overly broad, potentially allowing bypasses within double quotes, and notes that process substitution should not be flagged when contained within double-quoted strings.
| if (char === '\\' && !inSingleQuote) { | ||
| i += 2; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The logic for handling backslash escapes is overly broad. Inside double quotes, a backslash is only an escape character for a limited set of characters (
if (char === '\\') {
if (inDoubleQuote) {
if (i + 1 < command.length && ["$", "\\", "\"", "\u0060"].includes(command[i + 1])) {
i += 2;
continue;
}
} else if (!inSingleQuote) {
i += 2;
continue;
}
}| if ((char === '<' || char === '>') && command[i + 1] === '(') { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Process substitution (<() or >()) is not active inside double-quoted strings. The current implementation incorrectly flags these as command injection when they appear inside double quotes.
| if ((char === '<' || char === '>') && command[i + 1] === '(') { | |
| return true; | |
| } | |
| if (!inDoubleQuote && (char === '<' || char === '>') && command[i + 1] === '(') { | |
| return true; | |
| } |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a command injection detection mechanism within the ShellTool to prevent the execution of command substitution syntax, such as $(), backticks, and process substitution. The changes include the addition of a detectCommandSubstitution utility and a suite of unit tests. Review feedback identifies several security concerns related to PowerShell compatibility, specifically the need to handle PowerShell's unique escaping rules and additional subexpression syntaxes like @(...) and (...). Additionally, a suggestion was made to clarify the error message regarding process substitution for better clarity.
| export function detectCommandSubstitution(command: string): boolean { | ||
| let inSingleQuote = false; | ||
| let inDoubleQuote = false; | ||
| let i = 0; | ||
|
|
||
| while (i < command.length) { | ||
| const char = command[i]; | ||
|
|
||
| if (char === "'" && !inDoubleQuote) { | ||
| inSingleQuote = !inSingleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '"' && !inSingleQuote) { | ||
| inDoubleQuote = !inDoubleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '\\') { | ||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
| if (inDoubleQuote) { | ||
| const next = command[i + 1]; | ||
| if (['$', '`', '"', '\\', '\n'].includes(next)) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } else if (!inDoubleQuote) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (char === '$' && command[i + 1] === '(') { | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| !inDoubleQuote && | ||
| (char === '<' || char === '>') && | ||
| command[i + 1] === '(' | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (char === '`') { | ||
| return true; | ||
| } | ||
|
|
||
| i++; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The detectCommandSubstitution function incorrectly applies Bash-style backslash escaping rules when the target shell is PowerShell on Windows. In PowerShell, the backslash (\) is a literal character, and the backtick (`) is used for escaping. This allows an attacker to bypass the command substitution check by using a backslash to "escape" a command substitution pattern (e.g., \$(whoami)). The detectCommandSubstitution function will incorrectly interpret \ as an escape for the $ and allow the command to proceed, but PowerShell will treat \ as a literal and execute the $(whoami) command substitution.
Remediation: Modify detectCommandSubstitution to correctly apply PowerShell's escaping rules when the operating system is Windows. Specifically, it should not treat \ as an escape character for $ or other special characters in PowerShell contexts.
| export function detectCommandSubstitution(command: string): boolean { | ||
| let inSingleQuote = false; | ||
| let inDoubleQuote = false; | ||
| let i = 0; | ||
|
|
||
| while (i < command.length) { | ||
| const char = command[i]; | ||
|
|
||
| if (char === "'" && !inDoubleQuote) { | ||
| inSingleQuote = !inSingleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '"' && !inSingleQuote) { | ||
| inDoubleQuote = !inDoubleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '\\') { | ||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
| if (inDoubleQuote) { | ||
| const next = command[i + 1]; | ||
| if (['$', '`', '"', '\\', '\n'].includes(next)) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } else if (!inDoubleQuote) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (char === '$' && command[i + 1] === '(') { | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| !inDoubleQuote && | ||
| (char === '<' || char === '>') && | ||
| command[i + 1] === '(' | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (char === '`') { | ||
| return true; | ||
| } | ||
|
|
||
| i++; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The detectCommandSubstitution function fails to detect PowerShell's array subexpression syntax (@(...)). In PowerShell, @(...) can be used for command execution (e.g., echo @(whoami) will execute whoami). Since detectCommandSubstitution does not explicitly check for this pattern, an attacker can bypass the intended command injection prevention by using @(...) instead of $(...) or backticks.
Remediation: Extend detectCommandSubstitution to include checks for PowerShell's array subexpression syntax (@(...)) when the target shell is PowerShell.
| export function detectCommandSubstitution(command: string): boolean { | ||
| let inSingleQuote = false; | ||
| let inDoubleQuote = false; | ||
| let i = 0; | ||
|
|
||
| while (i < command.length) { | ||
| const char = command[i]; | ||
|
|
||
| if (char === "'" && !inDoubleQuote) { | ||
| inSingleQuote = !inSingleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '"' && !inSingleQuote) { | ||
| inDoubleQuote = !inDoubleQuote; | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
|
|
||
| if (char === '\\') { | ||
| if (inSingleQuote) { | ||
| i++; | ||
| continue; | ||
| } | ||
| if (inDoubleQuote) { | ||
| const next = command[i + 1]; | ||
| if (['$', '`', '"', '\\', '\n'].includes(next)) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } else if (!inDoubleQuote) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (char === '$' && command[i + 1] === '(') { | ||
| return true; | ||
| } | ||
|
|
||
| if ( | ||
| !inDoubleQuote && | ||
| (char === '<' || char === '>') && | ||
| command[i + 1] === '(' | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| if (char === '`') { | ||
| return true; | ||
| } | ||
|
|
||
| i++; | ||
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
The detectCommandSubstitution function does not detect simple PowerShell subexpressions enclosed in parentheses ((...)). In PowerShell, a command enclosed in parentheses (e.g., (whoami)) will be executed. This allows an attacker to bypass the command injection prevention by directly executing commands within parentheses, which is not caught by the current detectCommandSubstitution logic.
Remediation: Extend detectCommandSubstitution to include checks for PowerShell subexpressions ((...)) when the target shell is PowerShell.
| llmContent: | ||
| 'Command injection detected: command substitution syntax ' + | ||
| '($(), backticks, <>()) found in command arguments. ' + | ||
| 'This is a security risk and the command was blocked.', |
There was a problem hiding this comment.
The error message uses the notation <>() to refer to process substitution, which is non-standard and potentially confusing for both the user and the LLM. Since the implementation correctly blocks both input process substitution <() and output process substitution >(()), the message should be updated to be more explicit or use a clearer representation like <() or >(). This ensures the feedback provided to the LLM is accurate and actionable.
| llmContent: | |
| 'Command injection detected: command substitution syntax ' + | |
| '($(), backticks, <>()) found in command arguments. ' + | |
| 'This is a security risk and the command was blocked.', | |
| llmContent: | |
| 'Command injection detected: command substitution syntax ' + | |
| '($(), backticks, \u003c() or \u003e()) found in command arguments. ' + | |
| 'This is a security risk and the command was blocked.', |
References
- To prevent prompt injection, avoid including user-provided input in content passed to the LLM (llmContent).
- To prevent prompt injection, sanitize any additional context from hooks by escaping HTML-like tag characters such as < and >.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces command injection detection for the shell tool by identifying command substitution syntax such as $(), backticks, and process substitution. It implements specific detection logic for Bash and PowerShell and includes a suite of tests to verify the security boundaries. However, the PowerShell detection logic is currently too restrictive, as it blocks any opening parenthesis outside of double quotes, which will cause functional regressions by blocking standard language constructs like 'if' and 'foreach' statements. The feedback suggests refining this logic or using a more robust parsing approach.
| if (char === '(' && !inDoubleQuote) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The current implementation of detectPowerShellSubstitution is overly restrictive because it blocks any opening parenthesis ( found outside of double quotes. In PowerShell, parentheses are not only used for command subexpressions (e.g., (whoami)) but are also required for standard control flow structures such as if (condition), foreach ($item in $items), and catch (Exception).
This will cause a significant functional regression, as legitimate scripts using these common constructs will be blocked with a "Command injection detected" error.
Additionally, line 976 blocks @( even inside double quotes (e.g., echo "@()"), which is a false positive since array subexpressions are treated as literal strings when double-quoted in PowerShell.
Consider refining the detection logic to distinguish between dangerous subexpressions and valid language syntax, or leverage the existing PowerShell parser (POWERSHELL_PARSER_SCRIPT) to identify risky AST nodes (like SubExpressionAst or ParenthesizedExpressionAst) more accurately.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces command injection detection for the ShellTool by identifying shell substitution syntax such as $(), backticks, and process substitution. It adds a detectCommandSubstitution utility with specialized parsing logic for Bash and PowerShell environments, along with comprehensive unit tests. Feedback was provided regarding the manual string parsing logic, specifically noting that the escape character handling in both Bash and PowerShell detection functions lacks explicit bounds checks when processing the end of a command string, which could lead to fragile behavior.
| if (char === '\\') { | ||
| if (inDoubleQuote) { | ||
| const next = command[i + 1]; | ||
| if (['$', '`', '"', '\\', '\n'].includes(next)) { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } else { | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
The manual parsing logic for backslash escapes in detectBashSubstitution is potentially fragile. While it correctly skips characters, it doesn't account for the end of the string if a backslash is the final character. Although command[i + 1] returns undefined in JavaScript and won't crash, it's better to explicitly check bounds or use a more robust parsing approach to avoid skipping logic that might be bypassed by clever encoding or multi-byte characters.
| if (char === '`' && !inSingleQuote) { | ||
| i += 2; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
In detectPowerShellSubstitution, the backtick escape logic skips two characters without checking if the backtick is at the end of the string. While this won't cause a runtime crash in Node.js, it's a best practice to ensure the index remains within bounds during manual string parsing to prevent logic errors in edge cases.
|
The latest commit addresses all bot review feedback including bounds checking |
…on and normalizeCommand
…leRenderer and integrity
|
Hi @Adib234 , I've implemented the required changes to resolve this issue. Could you please review this PR and let me know if anything needs to be updated or improved. Thank you |
1 participant
Summary
Fixes a command injection vulnerability in
run_shell_commandwhere shellsubstitution syntax (
$(), backticks,<()) in command arguments was beingexecuted by the shell instead of treated as literal strings.
Details
Added
detectCommandSubstitution()inshell-utils.tsthat parses the commandstring respecting bash quoting rules:
$()and backticks unless escaped$(), backticks, and<()process substitutionIf substitution is detected, the command is blocked and a safe error message
is returned instead of executing the injected command.
Related Issues
Fixes #14926
How to Validate
Run the shell tool with a command containing
$():echo $(whoami)
Expected:Command is blocked with injection warning.
Run a normal command without substitution:
echo hello
Expected: Command executes normally.
Run tests:
npx vitest run packages/core/src/tools/shell.test.ts
Expected: 44/44 tests pass.
Pre-Merge Checklist