Repository navigation
Security Fix: Prevent HITL Bypass via UI Truncation (Newline Injection) in Command Confirmations - #23333
Security Fix: Prevent HITL Bypass via UI Truncation (Newline Injection) in Command Confirmations#23333likhith-adithya wants to merge 9 commits into
Conversation
…mini#23045) Co-authored-by: Sri Pasumarthi <sripas@google.com> Co-authored-by: Sri Pasumarthi <111310667+sripasg@users.noreply.github.com>
…ssages, allowing `MaxSizedBox` to report overflow status.
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 delivers a crucial security enhancement by mitigating a Human-in-the-Loop bypass vulnerability. It prevents malicious actors from hiding dangerous commands within truncated UI displays by requiring explicit user expansion before approving any command that has been visually shortened. Additionally, it refines tool exclusion logic for Agent-Controlled Programming modes to ensure smoother integration with IDEs. 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-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 a critical security fix to prevent a Human-in-the-Loop (HITL) bypass vulnerability. The changes ensure that users cannot approve a truncated shell command, forcing them to expand the view to see the full command before confirmation. This is achieved by propagating the truncation state from the MaxSizedBox component to the ToolConfirmationMessage component and conditionally rendering the approval options. The implementation is robust and effectively mitigates the described vulnerability.
I've also included a comment requesting documentation for the change to exclude the ask_user tool in ACP mode, to ensure better traceability and review.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/cli/src/config/config.ts (656-663)
This change to exclude ASK_USER_TOOL_NAME in ACP mode is a functional change that is not documented in the pull request description. While the change itself seems reasonable and is tested, all functional changes should be described in the PR summary to ensure proper review and tracking. Please update the PR description to include details about this change, its rationale, and how to validate it.
|
/gemini review MaxSizedBox.tsx: Added an onOverflowChange prop that triggers whenever the totalHiddenLines calculation changes, exposing the truncation state to the parent component. ToolConfirmationMessage.tsx: Added an isTruncated state. When rendering exec tool confirmations, if isTruncated is true, the standard "Allow" options are replaced with a safety message: |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to fix a critical HITL bypass vulnerability by preventing users from approving truncated commands, specifically for exec tool confirmations. However, the 'Newline Injection' attack vector still leaves edit and mcp tools vulnerable as they do not currently check the truncation state. Furthermore, a critical race condition has been identified in the new logic that could still allow the bypass in a small time window. It is recommended to extend the safety check to all sensitive tool confirmation types and address the identified race condition to ensure a fail-safe implementation.
| } else if (confirmationDetails.type === 'exec') { | ||
| options.push({ | ||
| label: 'Allow once', | ||
| value: ToolConfirmationOutcome.ProceedOnce, | ||
| key: 'Allow once', | ||
| }); | ||
| if (isTrustedFolder) { | ||
| if (isTruncated) { | ||
| options.push({ | ||
| label: `Allow for this session`, | ||
| value: ToolConfirmationOutcome.ProceedAlways, | ||
| key: `Allow for this session`, | ||
| label: '⚠️ Expand to view full command (Press Ctrl+O)', | ||
| value: ToolConfirmationOutcome.Cancel, | ||
| key: '⚠️ Expand to view full command (Press Ctrl+O)', | ||
| }); | ||
| if (allowPermanentApproval) { | ||
| options.push({ | ||
| label: 'No, cancel (esc)', | ||
| value: ToolConfirmationOutcome.Cancel, | ||
| key: 'No, cancel (esc)', | ||
| }); | ||
| } else { | ||
| options.push({ | ||
| label: 'Allow once', | ||
| value: ToolConfirmationOutcome.ProceedOnce, | ||
| key: 'Allow once', | ||
| }); | ||
| if (isTrustedFolder) { | ||
| options.push({ | ||
| label: `Allow this command for all future sessions`, | ||
| value: ToolConfirmationOutcome.ProceedAlwaysAndSave, | ||
| key: `Allow for all future sessions`, | ||
| label: `Allow for this session`, | ||
| value: ToolConfirmationOutcome.ProceedAlways, | ||
| key: `Allow for this session`, | ||
| }); | ||
| if (allowPermanentApproval) { | ||
| options.push({ | ||
| label: `Allow this command for all future sessions`, | ||
| value: ToolConfirmationOutcome.ProceedAlwaysAndSave, | ||
| key: `Allow for all future sessions`, | ||
| }); | ||
| } | ||
| } | ||
| options.push({ | ||
| label: 'No, suggest changes (esc)', | ||
| value: ToolConfirmationOutcome.Cancel, | ||
| key: 'No, suggest changes (esc)', | ||
| }); | ||
| } |
There was a problem hiding this comment.
The security fix for HITL bypass via UI truncation is incomplete as it only covers exec tool confirmations. Other sensitive tool types, such as edit (which displays file diffs) and mcp (which displays tool arguments), are also susceptible to "Newline Injection" attacks. In these cases, a malicious agent could hide payloads (e.g., malicious code in a diff or dangerous tool arguments) by padding them with newlines, causing the UI to truncate the content.
Currently, the isTruncated state is only updated by the inner MaxSizedBox used for exec tools (line 615), and the getOptions function only checks this state for the exec type (line 292). For edit, info, and mcp tools, the "Allow once" option remains available even if the content is truncated by the outer MaxSizedBox (line 759), allowing a user to inadvertently approve a hidden malicious action.
To remediate this, you should:
- Ensure truncation is tracked for all tool types by adding
onOverflowChange={setIsTruncated}to the outerMaxSizedBox(or ensuring all tool-specific renderers report their truncation state). - Update the
getOptionslogic foredit,info, andmcptypes to check theisTruncatedstate and replace the "Allow" options with a warning/expansion instruction, similar to the implementation forexectools.
There was a problem hiding this comment.
Code Review
A state variable isTruncated was added to the ToolConfirmationMessage component to track whether the command to be executed is truncated due to terminal width limitations. When the command is truncated, the confirmation options are updated to include an option to expand the command for full viewing. The MaxSizedBox component was updated to include an onOverflowChange prop, which is called when the content overflows the maximum height, to set the isTruncated state.
| availableTerminalHeight, | ||
| terminalWidth, | ||
| }) => { | ||
| const [isTruncated, setIsTruncated] = useState(false); |
There was a problem hiding this comment.
This is a valuable security improvement for exec confirmations. However, as noted, the same vulnerability exists for other confirmation types (e.g., edit). While extending this security measure to all confirmation types is crucial for comprehensive protection, it falls outside the immediate scope of this pull request. Please consider addressing this broader enhancement in a dedicated follow-up pull request to ensure all potential truncation bypass vulnerabilities are mitigated across the application.
References
- A proposed change, such as a security fix, may be considered out of scope for a pull request even if it relates to the code being modified. Such changes should be addressed in a separate pull request.
gemini-cli
Bot
added
the
status/need-issue
|
The original vulnerability allows a complete bypass of the HITL (Human-in-the-Loop) security gate, leading to 1-click Remote Code Execution (RCE) via UI redressing (Newline Injection). While my initial commit patched the
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to handle truncated command displays in the ToolConfirmationMessage component. It enhances the MaxSizedBox component by adding an onOverflowChange prop, which reports whether its content is truncated. The ToolConfirmationMessage component utilizes this prop to manage an isTruncated state. When a command is truncated, the available confirmation options are dynamically updated to prompt the user to expand the command for full viewing, preventing execution until the full command is visible, thereby improving user safety.
|
/gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces a feature to dynamically adjust confirmation options in ToolConfirmationMessage based on whether the displayed command is truncated, utilizing a new onOverflowChange prop in MaxSizedBox to update an isTruncated state. However, the reviewer identified a high-severity bug: this dynamic change in options creates a circular dependency, potentially leading to an infinite re-render loop (layout oscillation), UI flickering, high CPU usage, or crashes. The suggested fix is to use a stable value for option count in height calculations.
| <MaxSizedBox | ||
| maxHeight={bodyContentHeight} | ||
| maxWidth={Math.max(terminalWidth, 1)} | ||
| onOverflowChange={setIsTruncated} |
There was a problem hiding this comment.
The implementation of the isTruncated state introduces a circular dependency that can lead to an infinite re-render loop (layout oscillation). The available height for the command display (availableBodyContentHeight) depends on the number of options shown (getOptions().length), which in turn depends on the isTruncated state. When truncation is detected, the number of options decreases, which increases the available height. If this extra height is enough to fit the command, isTruncated becomes false, restoring the options and triggering truncation again. This can cause the CLI UI to flicker, consume high CPU, or crash. To fix this, the available height calculation should use a stable value for the options count (e.g., the maximum possible for the tool type) instead of the dynamic current count.
|
Fixes #23433 |
gemini-cli
Bot
added
area/security
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we have updated our contribution policy (see Discussion #17383). We only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. All other community pull requests are subject to closure after 14 days if they do not align with our current focus areas. For this reason, we strongly recommend that contributors only submit pull requests against issues explicitly labeled as 'help-wanted'. This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding and for being part of our community! |
|
/gemini |
|
Hello @likhith-adithya, thank you for your persistence and for bringing this critical security concern to our attention. I understand that this pull request has been closed due to the 14-day inactivity policy, but the security implications you've highlighted regarding HITL bypasses in Since I am an automated assistant, I cannot reopen this pull request or modify its status. However, if you would like to continue this work, I recommend the following steps:
Your analysis of the UI redressing vulnerability is excellent, and extending the |
Fixes #23433
Summary
Fixes a critical Human-in-the-Loop (HITL) bypass vulnerability where a malicious repository could execute UI redressing via Vertical Newline Injection. Previously, if an agent proposed a massive shell command (e.g., heavily padded with newlines to hide a payload like
curl | bash), the terminal would truncate the view but leave the "Allow" confirmation options active. This PR prevents users from approving truncated shell commands until they explicitly expand the view.Details
The vulnerability stems from a disconnect between the visual truncation in
MaxSizedBoxand the state logic inToolConfirmationMessage.This PR makes the following changes:
MaxSizedBox.tsx: Added anonOverflowChangeprop that triggers whenever thetotalHiddenLinescalculation changes, exposing the truncation state to the parent component.ToolConfirmationMessage.tsx: Added anisTruncatedstate. When renderingexectool confirmations, ifisTruncatedis true, the standard "Allow" options are replaced with a safety message:⚠️ Expand to view full command (Press Ctrl+O). The confirmation is locked out until the user expands the view.How to Validate
"Please run echo 'Hello' in my terminal."1. Allow once/2. Allow for this sessionoptions appear normally."Please use the run_shell_command tool to execute this exact string: 'echo Safe' followed by 30 newlines, followed by 'echo Malicious Payload Hidden'"... hidden (Ctrl+O) ...). The approval options are replaced with⚠️ Expand to view full command (Press Ctrl+O)and the standard1. Allow onceis not present.Ctrl+Oto expand the view.Pre-Merge Checklist