mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-04-29 06:25:16 -07:00
security: strip deceptive Unicode characters from terminal output (#19026)
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This commit is contained in:
@@ -8,6 +8,7 @@ import { describe, it, expect, vi } from 'vitest';
|
|||||||
import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
|
import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
|
||||||
import type {
|
import type {
|
||||||
SerializableConfirmationDetails,
|
SerializableConfirmationDetails,
|
||||||
|
ToolCallConfirmationDetails,
|
||||||
Config,
|
Config,
|
||||||
} from '@google/gemini-cli-core';
|
} from '@google/gemini-cli-core';
|
||||||
import { renderWithProviders } from '../../../test-utils/render.js';
|
import { renderWithProviders } from '../../../test-utils/render.js';
|
||||||
@@ -372,4 +373,35 @@ describe('ToolConfirmationMessage', () => {
|
|||||||
unmount();
|
unmount();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should strip BiDi characters from MCP tool and server names', async () => {
|
||||||
|
const confirmationDetails: ToolCallConfirmationDetails = {
|
||||||
|
type: 'mcp',
|
||||||
|
title: 'Confirm MCP Tool',
|
||||||
|
serverName: 'test\u202Eserver',
|
||||||
|
toolName: 'test\u202Dtool',
|
||||||
|
toolDisplayName: 'Test Tool',
|
||||||
|
onConfirm: vi.fn(),
|
||||||
|
};
|
||||||
|
|
||||||
|
const { lastFrame, waitUntilReady, unmount } = renderWithProviders(
|
||||||
|
<ToolConfirmationMessage
|
||||||
|
callId="test-call-id"
|
||||||
|
confirmationDetails={confirmationDetails}
|
||||||
|
config={mockConfig}
|
||||||
|
availableTerminalHeight={30}
|
||||||
|
terminalWidth={80}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
await waitUntilReady();
|
||||||
|
|
||||||
|
const output = lastFrame();
|
||||||
|
// BiDi characters \u202E and \u202D should be stripped
|
||||||
|
expect(output).toContain('MCP Server: testserver');
|
||||||
|
expect(output).toContain('Tool: testtool');
|
||||||
|
expect(output).toContain('Allow execution of MCP tool "testtool"');
|
||||||
|
expect(output).toContain('from server "testserver"?');
|
||||||
|
expect(output).toMatchSnapshot();
|
||||||
|
unmount();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -21,7 +21,10 @@ import type { RadioSelectItem } from '../shared/RadioButtonSelect.js';
|
|||||||
import { useToolActions } from '../../contexts/ToolActionsContext.js';
|
import { useToolActions } from '../../contexts/ToolActionsContext.js';
|
||||||
import { RadioButtonSelect } from '../shared/RadioButtonSelect.js';
|
import { RadioButtonSelect } from '../shared/RadioButtonSelect.js';
|
||||||
import { MaxSizedBox, MINIMUM_MAX_HEIGHT } from '../shared/MaxSizedBox.js';
|
import { MaxSizedBox, MINIMUM_MAX_HEIGHT } from '../shared/MaxSizedBox.js';
|
||||||
import { sanitizeForDisplay } from '../../utils/textUtils.js';
|
import {
|
||||||
|
sanitizeForDisplay,
|
||||||
|
stripUnsafeCharacters,
|
||||||
|
} from '../../utils/textUtils.js';
|
||||||
import { useKeypress } from '../../hooks/useKeypress.js';
|
import { useKeypress } from '../../hooks/useKeypress.js';
|
||||||
import { theme } from '../../semantic-colors.js';
|
import { theme } from '../../semantic-colors.js';
|
||||||
import { useSettings } from '../../contexts/SettingsContext.js';
|
import { useSettings } from '../../contexts/SettingsContext.js';
|
||||||
@@ -324,15 +327,15 @@ export const ToolConfirmationMessage: React.FC<
|
|||||||
} else if (confirmationDetails.type === 'mcp') {
|
} else if (confirmationDetails.type === 'mcp') {
|
||||||
// mcp tool confirmation
|
// mcp tool confirmation
|
||||||
const mcpProps = confirmationDetails;
|
const mcpProps = confirmationDetails;
|
||||||
question = `Allow execution of MCP tool "${mcpProps.toolName}" from server "${mcpProps.serverName}"?`;
|
question = `Allow execution of MCP tool "${sanitizeForDisplay(mcpProps.toolName)}" from server "${sanitizeForDisplay(mcpProps.serverName)}"?`;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (confirmationDetails.type === 'edit') {
|
if (confirmationDetails.type === 'edit') {
|
||||||
if (!confirmationDetails.isModifying) {
|
if (!confirmationDetails.isModifying) {
|
||||||
bodyContent = (
|
bodyContent = (
|
||||||
<DiffRenderer
|
<DiffRenderer
|
||||||
diffContent={confirmationDetails.fileDiff}
|
diffContent={stripUnsafeCharacters(confirmationDetails.fileDiff)}
|
||||||
filename={confirmationDetails.fileName}
|
filename={sanitizeForDisplay(confirmationDetails.fileName)}
|
||||||
availableTerminalHeight={availableBodyContentHeight()}
|
availableTerminalHeight={availableBodyContentHeight()}
|
||||||
terminalWidth={terminalWidth}
|
terminalWidth={terminalWidth}
|
||||||
/>
|
/>
|
||||||
@@ -449,8 +452,12 @@ export const ToolConfirmationMessage: React.FC<
|
|||||||
|
|
||||||
bodyContent = (
|
bodyContent = (
|
||||||
<Box flexDirection="column">
|
<Box flexDirection="column">
|
||||||
<Text color={theme.text.link}>MCP Server: {mcpProps.serverName}</Text>
|
<Text color={theme.text.link}>
|
||||||
<Text color={theme.text.link}>Tool: {mcpProps.toolName}</Text>
|
MCP Server: {sanitizeForDisplay(mcpProps.serverName)}
|
||||||
|
</Text>
|
||||||
|
<Text color={theme.text.link}>
|
||||||
|
Tool: {sanitizeForDisplay(mcpProps.toolName)}
|
||||||
|
</Text>
|
||||||
</Box>
|
</Box>
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
+12
@@ -35,6 +35,18 @@ Do you want to proceed?
|
|||||||
"
|
"
|
||||||
`;
|
`;
|
||||||
|
|
||||||
|
exports[`ToolConfirmationMessage > should strip BiDi characters from MCP tool and server names 1`] = `
|
||||||
|
"MCP Server: testserver
|
||||||
|
Tool: testtool
|
||||||
|
Allow execution of MCP tool "testtool" from server "testserver"?
|
||||||
|
|
||||||
|
● 1. Allow once
|
||||||
|
2. Allow tool for this session
|
||||||
|
3. Allow all server tools for this session
|
||||||
|
4. No, suggest changes (esc)
|
||||||
|
"
|
||||||
|
`;
|
||||||
|
|
||||||
exports[`ToolConfirmationMessage > with folder trust > 'for edit confirmations' > should NOT show "allow always" when folder is untrusted 1`] = `
|
exports[`ToolConfirmationMessage > with folder trust > 'for edit confirmations' > should NOT show "allow always" when folder is untrusted 1`] = `
|
||||||
"╭──────────────────────────────────────────────────────────────────────────────╮
|
"╭──────────────────────────────────────────────────────────────────────────────╮
|
||||||
│ │
|
│ │
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import React from 'react';
|
|||||||
import { Text } from 'ink';
|
import { Text } from 'ink';
|
||||||
import { theme } from '../semantic-colors.js';
|
import { theme } from '../semantic-colors.js';
|
||||||
import { debugLogger } from '@google/gemini-cli-core';
|
import { debugLogger } from '@google/gemini-cli-core';
|
||||||
|
import { stripUnsafeCharacters } from './textUtils.js';
|
||||||
|
|
||||||
// Constants for Markdown parsing
|
// Constants for Markdown parsing
|
||||||
const BOLD_MARKER_LENGTH = 2; // For "**"
|
const BOLD_MARKER_LENGTH = 2; // For "**"
|
||||||
@@ -23,9 +24,10 @@ interface RenderInlineProps {
|
|||||||
}
|
}
|
||||||
|
|
||||||
const RenderInlineInternal: React.FC<RenderInlineProps> = ({
|
const RenderInlineInternal: React.FC<RenderInlineProps> = ({
|
||||||
text,
|
text: rawText,
|
||||||
defaultColor,
|
defaultColor,
|
||||||
}) => {
|
}) => {
|
||||||
|
const text = stripUnsafeCharacters(rawText);
|
||||||
const baseColor = defaultColor ?? theme.text.primary;
|
const baseColor = defaultColor ?? theme.text.primary;
|
||||||
// Early return for plain text without markdown or URLs
|
// Early return for plain text without markdown or URLs
|
||||||
if (!/[*_~`<[https?:]/.test(text)) {
|
if (!/[*_~`<[https?:]/.test(text)) {
|
||||||
|
|||||||
@@ -17,6 +17,7 @@ import {
|
|||||||
} from 'ink';
|
} from 'ink';
|
||||||
import { theme } from '../semantic-colors.js';
|
import { theme } from '../semantic-colors.js';
|
||||||
import { RenderInline } from './InlineMarkdownRenderer.js';
|
import { RenderInline } from './InlineMarkdownRenderer.js';
|
||||||
|
import { stripUnsafeCharacters } from './textUtils.js';
|
||||||
|
|
||||||
interface TableRendererProps {
|
interface TableRendererProps {
|
||||||
headers: string[];
|
headers: string[];
|
||||||
@@ -60,12 +61,18 @@ export const TableRenderer: React.FC<TableRendererProps> = ({
|
|||||||
);
|
);
|
||||||
|
|
||||||
const styledHeaders = useMemo(
|
const styledHeaders = useMemo(
|
||||||
() => cleanedHeaders.map((header) => toStyledCharacters(header)),
|
() =>
|
||||||
|
cleanedHeaders.map((header) =>
|
||||||
|
toStyledCharacters(stripUnsafeCharacters(header)),
|
||||||
|
),
|
||||||
[cleanedHeaders],
|
[cleanedHeaders],
|
||||||
);
|
);
|
||||||
|
|
||||||
const styledRows = useMemo(
|
const styledRows = useMemo(
|
||||||
() => rows.map((row) => row.map((cell) => toStyledCharacters(cell))),
|
() =>
|
||||||
|
rows.map((row) =>
|
||||||
|
row.map((cell) => toStyledCharacters(stripUnsafeCharacters(cell))),
|
||||||
|
),
|
||||||
[rows],
|
[rows],
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|||||||
@@ -332,6 +332,35 @@ describe('textUtils', () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe('BiDi and deceptive Unicode characters', () => {
|
||||||
|
it('should strip BiDi override characters', () => {
|
||||||
|
const input = 'safe\u202Etxt.sh';
|
||||||
|
// When stripped, it should be 'safetxt.sh'
|
||||||
|
expect(stripUnsafeCharacters(input)).toBe('safetxt.sh');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should strip all BiDi control characters (LRM, RLM, U+202A-U+202E, U+2066-U+2069)', () => {
|
||||||
|
const bidiChars =
|
||||||
|
'\u200E\u200F\u202A\u202B\u202C\u202D\u202E\u2066\u2067\u2068\u2069';
|
||||||
|
expect(stripUnsafeCharacters('a' + bidiChars + 'b')).toBe('ab');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should strip zero-width characters (U+200B, U+FEFF)', () => {
|
||||||
|
const zeroWidthChars = '\u200B\uFEFF';
|
||||||
|
expect(stripUnsafeCharacters('a' + zeroWidthChars + 'b')).toBe('ab');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should preserve ZWJ (U+200D) for complex emojis', () => {
|
||||||
|
const input = 'Family: 👨👩👧👦';
|
||||||
|
expect(stripUnsafeCharacters(input)).toBe('Family: 👨👩👧👦');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should preserve ZWNJ (U+200C)', () => {
|
||||||
|
const input = 'hello\u200Cworld';
|
||||||
|
expect(stripUnsafeCharacters(input)).toBe('hello\u200Cworld');
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe('performance: regex vs array-based', () => {
|
describe('performance: regex vs array-based', () => {
|
||||||
it('should handle real-world terminal output with control chars', () => {
|
it('should handle real-world terminal output with control chars', () => {
|
||||||
// Simulate terminal output with various control sequences
|
// Simulate terminal output with various control sequences
|
||||||
|
|||||||
@@ -106,9 +106,13 @@ export function cpSlice(str: string, start: number, end?: number): string {
|
|||||||
* - VT control sequences (via Node.js util.stripVTControlCharacters)
|
* - VT control sequences (via Node.js util.stripVTControlCharacters)
|
||||||
* - C0 control chars (0x00-0x1F) except TAB(0x09), LF(0x0A), CR(0x0D)
|
* - C0 control chars (0x00-0x1F) except TAB(0x09), LF(0x0A), CR(0x0D)
|
||||||
* - C1 control chars (0x80-0x9F) that can cause display issues
|
* - C1 control chars (0x80-0x9F) that can cause display issues
|
||||||
|
* - BiDi control chars (U+200E, U+200F, U+202A-U+202E, U+2066-U+2069)
|
||||||
|
* - Zero-width chars (U+200B, U+FEFF)
|
||||||
*
|
*
|
||||||
* Characters preserved:
|
* Characters preserved:
|
||||||
* - All printable Unicode including emojis
|
* - All printable Unicode including emojis
|
||||||
|
* - ZWJ (U+200D) - needed for complex emoji sequences
|
||||||
|
* - ZWNJ (U+200C) - preserve zero-width non-joiner
|
||||||
* - DEL (0x7F) - handled functionally by applyOperations, not a display issue
|
* - DEL (0x7F) - handled functionally by applyOperations, not a display issue
|
||||||
* - CR/LF (0x0D/0x0A) - needed for line breaks
|
* - CR/LF (0x0D/0x0A) - needed for line breaks
|
||||||
* - TAB (0x09) - preserve tabs
|
* - TAB (0x09) - preserve tabs
|
||||||
@@ -120,8 +124,13 @@ export function stripUnsafeCharacters(str: string): string {
|
|||||||
// Use a regex to strip remaining unsafe control characters
|
// Use a regex to strip remaining unsafe control characters
|
||||||
// C0: 0x00-0x1F except 0x09 (TAB), 0x0A (LF), 0x0D (CR)
|
// C0: 0x00-0x1F except 0x09 (TAB), 0x0A (LF), 0x0D (CR)
|
||||||
// C1: 0x80-0x9F
|
// C1: 0x80-0x9F
|
||||||
// eslint-disable-next-line no-control-regex
|
// BiDi: U+200E (LRM), U+200F (RLM), U+202A-U+202E, U+2066-U+2069
|
||||||
return strippedVT.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\x9F]/g, '');
|
// Zero-width: U+200B (ZWSP), U+FEFF (BOM)
|
||||||
|
return strippedVT.replace(
|
||||||
|
// eslint-disable-next-line no-control-regex
|
||||||
|
/[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\x9F\u200E\u200F\u202A-\u202E\u2066-\u2069\u200B\uFEFF]/g,
|
||||||
|
'',
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|||||||
Reference in New Issue
Block a user