security: strip deceptive Unicode characters from terminal output#19026
security: strip deceptive Unicode characters from terminal output#19026ehedlund merged 7 commits intogoogle-gemini:mainfrom
Conversation
Summary of ChangesHello @ehedlund, 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 significantly enhances the security posture of the CLI by implementing robust sanitization of terminal output. It prevents potential obfuscation attacks that leverage deceptive Unicode characters, such as BiDirectional overrides and zero-width characters, to mislead users about tool parameters or file names. By systematically stripping these unsafe characters, the changes ensure that information displayed to the user is accurate and untampered, thereby mitigating a class of security vulnerabilities. Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security enhancement by stripping deceptive Unicode characters from terminal output, utilizing the stripUnsafeCharacters utility in InlineMarkdownRenderer and ToolConfirmationMessage. However, a significant vulnerability remains as the sanitization logic was overlooked for the file diff display within the ToolConfirmationMessage, potentially exposing users to deceptive file modifications. This oversight is addressed by recommending the application of stripUnsafeCharacters to confirmationDetails.fileDiff, aligning with the principle of defense-in-depth. Further improvements are suggested to make the BiDi character stripping more comprehensive by including Left-to-Right Mark (LRM) and Right-to-Left Mark (RLM), with corresponding updates recommended for the stripping logic, documentation, and tests.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx (334)
The fileDiff content passed to DiffRenderer is not sanitized for deceptive Unicode characters (such as BiDi overrides or zero-width characters). While this PR correctly adds sanitization for the fileName on line 334, the actual diff content on line 333 remains vulnerable. An attacker (e.g., via a malicious LLM response) could use BiDi characters to obfuscate the changes being shown to the user, potentially tricking them into approving a harmful file modification.
Since this PR's primary goal is to strip deceptive Unicode characters from terminal output, this rendering path should also be protected. I recommend applying stripUnsafeCharacters to confirmationDetails.fileDiff before passing it to DiffRenderer.
References
- Always treat user-provided data as untrusted and apply proper validation and sanitization at the point of use, even if it is believed to have been filtered or sanitized upstream. This follows the principle of defense-in-depth.
packages/cli/src/ui/utils/textUtils.ts (109)
For more comprehensive protection against deceptive BiDi characters, please also strip Left-to-Right Mark (LRM, U+200E) and Right-to-Left Mark (RLM, U+200F). This JSDoc should be updated to reflect their inclusion.
* - BiDi control chars (U+200E, U+200F, U+202A-U+202E, U+2066-U+2069)
packages/cli/src/ui/utils/textUtils.ts (130)
To implement the stripping of LRM and RLM characters, please add \u200E and \u200F to this regular expression. These are invisible characters that can alter the visual order of text and should be removed as part of this security enhancement.
/["\x00-\x08\x0B\x0C\x0E-\x1F\x80-\x9F\u200E\u200F\u202A-\u202E\u2066-\u2069\u200B\u200C\uFEFF]/g,
packages/cli/src/ui/utils/textUtils.test.ts (342-346)
Please update this test case to also verify the stripping of LRM (\u200E) and RLM (\u200F) characters, ensuring test coverage for the more comprehensive BiDi character stripping.
it('should strip all BiDi control characters (LRM, RLM, U+202A-U+202B\u202C\u202D\u202E, U+2066-U+2069)', () => {
const bidiChars =
'\u200E\u200F\u202A\u202B\u202C\u202D\u202E\u2066\u2067\u2068\u2069';
expect(stripUnsafeCharacters('a' + bidiChars + 'b')).toBe('ab');
});
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively hardens the CLI against deceptive Unicode characters by introducing and applying stripUnsafeCharacters in key areas, including the general markdown renderer and diff displays. This is a great security improvement.
However, I've identified a critical issue where sanitizeForDisplay is used for displaying filenames and tool/server names. This function collapses whitespace, which can misrepresent the string being displayed. This is especially dangerous when displaying shell commands for confirmation, as it can alter the command's meaning. My review includes suggestions to use stripUnsafeCharacters instead to ensure accurate representation while still providing the intended security benefits.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx
Outdated
Show resolved
Hide resolved
…updated TableRenderer with stripUnsafeCharacters)
Summary
This PR addresses security concerns regarding deceptive Unicode characters (e.g., BiDi overrides, zero-width characters) being used to obfuscate terminal output and tool call parameters.
Details
stripUnsafeCharactersinpackages/cli/src/ui/utils/textUtils.tsto strip:stripUnsafeCharactersintoRenderInlineinpackages/cli/src/ui/utils/InlineMarkdownRenderer.tsx.ToolConfirmationMessage.tsx(MCP tool/server names, diff filenames, and file diff content).Related Issues
https://github.com/google-gemini/maintainers-gemini-cli/issues/1206
How to Validate
npm test -w @google/gemini-cli -- src/ui/utils/textUtils.test.tsPre-Merge Checklist