Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { describe, it, expect, vi } from 'vitest';
import { ToolConfirmationMessage } from './ToolConfirmationMessage.js';
import type {
SerializableConfirmationDetails,
ToolCallConfirmationDetails,
Config,
} from '@google/gemini-cli-core';
import { renderWithProviders } from '../../../test-utils/render.js';
Expand Down Expand Up @@ -372,4 +373,35 @@ describe('ToolConfirmationMessage', () => {
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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import type { RadioSelectItem } from '../shared/RadioButtonSelect.js';
import { useToolActions } from '../../contexts/ToolActionsContext.js';
import { RadioButtonSelect } from '../shared/RadioButtonSelect.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 { theme } from '../../semantic-colors.js';
import { useSettings } from '../../contexts/SettingsContext.js';
Expand Down Expand Up @@ -324,15 +327,15 @@ export const ToolConfirmationMessage: React.FC<
} else if (confirmationDetails.type === 'mcp') {
// mcp tool confirmation
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.isModifying) {
bodyContent = (
<DiffRenderer
diffContent={confirmationDetails.fileDiff}
filename={confirmationDetails.fileName}
diffContent={stripUnsafeCharacters(confirmationDetails.fileDiff)}
filename={sanitizeForDisplay(confirmationDetails.fileName)}
availableTerminalHeight={availableBodyContentHeight()}
terminalWidth={terminalWidth}
/>
Expand Down Expand Up @@ -449,8 +452,12 @@ export const ToolConfirmationMessage: React.FC<

bodyContent = (
<Box flexDirection="column">
<Text color={theme.text.link}>MCP Server: {mcpProps.serverName}</Text>
<Text color={theme.text.link}>Tool: {mcpProps.toolName}</Text>
<Text color={theme.text.link}>
MCP Server: {sanitizeForDisplay(mcpProps.serverName)}
</Text>
<Text color={theme.text.link}>
Tool: {sanitizeForDisplay(mcpProps.toolName)}
</Text>
Comment thread
This conversation was marked as resolved.
</Box>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`] = `
"╭──────────────────────────────────────────────────────────────────────────────╮
│ │
Expand Down
4 changes: 3 additions & 1 deletion packages/cli/src/ui/utils/InlineMarkdownRenderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import React from 'react';
import { Text } from 'ink';
import { theme } from '../semantic-colors.js';
import { debugLogger } from '@google/gemini-cli-core';
import { stripUnsafeCharacters } from './textUtils.js';

// Constants for Markdown parsing
const BOLD_MARKER_LENGTH = 2; // For "**"
Expand All @@ -23,9 +24,10 @@ interface RenderInlineProps {
}

const RenderInlineInternal: React.FC<RenderInlineProps> = ({
text,
text: rawText,
defaultColor,
}) => {
const text = stripUnsafeCharacters(rawText);
const baseColor = defaultColor ?? theme.text.primary;
// Early return for plain text without markdown or URLs
if (!/[*_~`<[https?:]/.test(text)) {
Expand Down
11 changes: 9 additions & 2 deletions packages/cli/src/ui/utils/TableRenderer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
} from 'ink';
import { theme } from '../semantic-colors.js';
import { RenderInline } from './InlineMarkdownRenderer.js';
import { stripUnsafeCharacters } from './textUtils.js';

interface TableRendererProps {
headers: string[];
Expand Down Expand Up @@ -60,12 +61,18 @@ export const TableRenderer: React.FC<TableRendererProps> = ({
);

const styledHeaders = useMemo(
() => cleanedHeaders.map((header) => toStyledCharacters(header)),
() =>
cleanedHeaders.map((header) =>
toStyledCharacters(stripUnsafeCharacters(header)),
),
[cleanedHeaders],
);

const styledRows = useMemo(
() => rows.map((row) => row.map((cell) => toStyledCharacters(cell))),
() =>
rows.map((row) =>
row.map((cell) => toStyledCharacters(stripUnsafeCharacters(cell))),
),
[rows],
);

Expand Down
29 changes: 29 additions & 0 deletions packages/cli/src/ui/utils/textUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,35 @@ describe('textUtils', () => {
});
});

describe('BiDi and deceptive Unicode characters', () => {
Comment thread
This conversation was marked as resolved.
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', () => {
it('should handle real-world terminal output with control chars', () => {
// Simulate terminal output with various control sequences
Expand Down
13 changes: 11 additions & 2 deletions packages/cli/src/ui/utils/textUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,13 @@ export function cpSlice(str: string, start: number, end?: number): string {
* - VT control sequences (via Node.js util.stripVTControlCharacters)
* - C0 control chars (0x00-0x1F) except TAB(0x09), LF(0x0A), CR(0x0D)
* - 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:
* - All printable Unicode including emojis
* - ZWJ (U+200D) - needed for complex emoji sequences
Comment thread
This conversation was marked as resolved.
* - ZWNJ (U+200C) - preserve zero-width non-joiner
* - DEL (0x7F) - handled functionally by applyOperations, not a display issue
* - CR/LF (0x0D/0x0A) - needed for line breaks
* - TAB (0x09) - preserve tabs
Expand All @@ -120,8 +124,13 @@ export function stripUnsafeCharacters(str: string): string {
// Use a regex to strip remaining unsafe control characters
// C0: 0x00-0x1F except 0x09 (TAB), 0x0A (LF), 0x0D (CR)
// C1: 0x80-0x9F
// eslint-disable-next-line no-control-regex
return strippedVT.replace(/[\x00-\x08\x0B\x0C\x0E-\x1F\x80-\x9F]/g, '');
// BiDi: U+200E (LRM), U+200F (RLM), U+202A-U+202E, U+2066-U+2069
// 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,
'',
);
}

/**
Expand Down
Loading