diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index 49dd08ac53..c9a7cd7f89 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -220,6 +220,7 @@ describe('InputPrompt', () => { col = newText.length; } mockBuffer.cursor = [0, col]; + mockBuffer.allVisualLines = [newText]; mockBuffer.viewportVisualLines = [newText]; mockBuffer.allVisualLines = [newText]; mockBuffer.visualToLogicalMap = [[0, 0]]; @@ -2273,6 +2274,7 @@ describe('InputPrompt', () => { async ({ text, visualCursor }) => { mockBuffer.text = text; mockBuffer.lines = [text]; + mockBuffer.allVisualLines = [text]; mockBuffer.viewportVisualLines = [text]; mockBuffer.visualCursor = visualCursor as [number, number]; props.config.getUseBackgroundColor = () => false; @@ -2322,6 +2324,7 @@ describe('InputPrompt', () => { async ({ text, visualCursor, visualToLogicalMap }) => { mockBuffer.text = text; mockBuffer.lines = text.split('\n'); + mockBuffer.allVisualLines = text.split('\n'); mockBuffer.viewportVisualLines = text.split('\n'); mockBuffer.visualCursor = visualCursor as [number, number]; mockBuffer.visualToLogicalMap = visualToLogicalMap as Array< @@ -2342,6 +2345,7 @@ describe('InputPrompt', () => { const text = 'first line\n\nthird line'; mockBuffer.text = text; mockBuffer.lines = text.split('\n'); + mockBuffer.allVisualLines = text.split('\n'); mockBuffer.viewportVisualLines = text.split('\n'); mockBuffer.visualCursor = [1, 0]; // cursor on the blank line mockBuffer.visualToLogicalMap = [ @@ -2361,11 +2365,98 @@ describe('InputPrompt', () => { }); }); + describe('scrolling large inputs', () => { + it('should correctly render scrolling down and up for large inputs', async () => { + const lines = Array.from({ length: 50 }).map((_, i) => `testline ${i}`); + + // Since we need to test how the React component tree responds to TextBuffer state changes, + // we must provide a fake TextBuffer implementation that triggers re-renders like the real one. + + const TestWrapper = () => { + const [bufferState, setBufferState] = useState({ + text: lines.join('\n'), + lines, + allVisualLines: lines, + viewportVisualLines: lines.slice(0, 10), + visualToLogicalMap: lines.map((_, i) => [i, 0]), + visualCursor: [0, 0] as [number, number], + visualScrollRow: 0, + viewportHeight: 10, + }); + + const fakeBuffer = { + ...mockBuffer, + ...bufferState, + handleInput: vi.fn().mockImplementation((key) => { + let newRow = bufferState.visualCursor[0]; + let newScroll = bufferState.visualScrollRow; + if (key.name === 'down') { + newRow = Math.min(49, newRow + 1); + if (newRow >= newScroll + 10) newScroll++; + } else if (key.name === 'up') { + newRow = Math.max(0, newRow - 1); + if (newRow < newScroll) newScroll--; + } + setBufferState({ + ...bufferState, + visualCursor: [newRow, 0], + visualScrollRow: newScroll, + viewportVisualLines: lines.slice(newScroll, newScroll + 10), + }); + return true; + }), + } as unknown as TextBuffer; + + return ; + }; + + const { stdout, unmount, stdin } = await renderWithProviders( + , + { + uiActions, + }, + ); + + // Verify initial render + await waitFor(() => { + expect(stdout.lastFrame()).toContain('testline 0'); + expect(stdout.lastFrame()).not.toContain('testline 49'); + }); + + // Move cursor to bottom + for (let i = 0; i < 49; i++) { + act(() => { + stdin.write('\x1b[B'); // Arrow Down + }); + } + + await waitFor(() => { + expect(stdout.lastFrame()).toContain('testline 49'); + expect(stdout.lastFrame()).not.toContain('testline 0'); + }); + + // Move cursor back to top + for (let i = 0; i < 49; i++) { + act(() => { + stdin.write('\x1b[A'); // Arrow Up + }); + } + + await waitFor(() => { + expect(stdout.lastFrame()).toContain('testline 0'); + expect(stdout.lastFrame()).not.toContain('testline 49'); + }); + + unmount(); + }); + }); + describe('multiline rendering', () => { it('should correctly render multiline input including blank lines', async () => { const text = 'hello\n\nworld'; mockBuffer.text = text; mockBuffer.lines = text.split('\n'); + mockBuffer.allVisualLines = text.split('\n'); mockBuffer.viewportVisualLines = text.split('\n'); mockBuffer.allVisualLines = text.split('\n'); mockBuffer.visualCursor = [2, 5]; // cursor at the end of "world" @@ -3592,7 +3683,9 @@ describe('InputPrompt', () => { async ({ relX, relY, mouseCol, mouseRow }) => { props.buffer.text = 'hello world\nsecond line'; props.buffer.lines = ['hello world', 'second line']; + props.buffer.allVisualLines = ['hello world', 'second line']; props.buffer.viewportVisualLines = ['hello world', 'second line']; + props.buffer.viewportHeight = 10; props.buffer.visualToLogicalMap = [ [0, 0], [1, 0], @@ -3630,6 +3723,7 @@ describe('InputPrompt', () => { it('should unfocus embedded shell on click', async () => { props.buffer.text = 'hello'; props.buffer.lines = ['hello']; + props.buffer.allVisualLines = ['hello']; props.buffer.viewportVisualLines = ['hello']; props.buffer.visualToLogicalMap = [[0, 0]]; props.isEmbeddedShellFocused = true; @@ -3671,6 +3765,7 @@ describe('InputPrompt', () => { lines: currentLines, viewportVisualLines: currentLines, allVisualLines: currentLines, + viewportHeight: 10, pastedContent: { [id]: largeText }, transformationsByLine: isExpanded ? currentLines.map(() => []) @@ -3759,6 +3854,7 @@ describe('InputPrompt', () => { lines: currentLines, viewportVisualLines: currentLines, allVisualLines: currentLines, + viewportHeight: 10, pastedContent: { [id]: largeText }, transformationsByLine: isExpanded ? currentLines.map(() => []) @@ -3830,7 +3926,9 @@ describe('InputPrompt', () => { props.config.getUseBackgroundColor = () => false; props.buffer.text = 'hello world'; props.buffer.lines = ['hello world']; + props.buffer.allVisualLines = ['hello world']; props.buffer.viewportVisualLines = ['hello world']; + props.buffer.viewportHeight = 10; props.buffer.visualToLogicalMap = [[0, 0]]; props.buffer.visualCursor = [0, 11]; props.buffer.visualScrollRow = 0; @@ -4137,6 +4235,7 @@ describe('InputPrompt', () => { const text = 'hello'; mockBuffer.text = text; mockBuffer.lines = [text]; + mockBuffer.allVisualLines = [text]; mockBuffer.viewportVisualLines = [text]; mockBuffer.visualToLogicalMap = [[0, 0]]; mockBuffer.visualCursor = [0, 3]; // Cursor after 'hel' @@ -4167,6 +4266,7 @@ describe('InputPrompt', () => { const text = 'πŸ‘hello'; mockBuffer.text = text; mockBuffer.lines = [text]; + mockBuffer.allVisualLines = [text]; mockBuffer.viewportVisualLines = [text]; mockBuffer.visualToLogicalMap = [[0, 0]]; mockBuffer.visualCursor = [0, 2]; // Cursor after 'πŸ‘h' (Note: 'πŸ‘' is one code point but width 2) @@ -4196,6 +4296,7 @@ describe('InputPrompt', () => { const text = 'πŸ˜€πŸ˜€πŸ˜€'; mockBuffer.text = text; mockBuffer.lines = [text]; + mockBuffer.allVisualLines = [text]; mockBuffer.viewportVisualLines = [text]; mockBuffer.visualToLogicalMap = [[0, 0]]; mockBuffer.visualCursor = [0, 2]; // Cursor after 2 emojis (each 1 code point, width 2) @@ -4225,7 +4326,9 @@ describe('InputPrompt', () => { const lines = ['πŸ˜€πŸ˜€', 'hello πŸ˜€', 'world']; mockBuffer.text = lines.join('\n'); mockBuffer.lines = lines; + mockBuffer.allVisualLines = lines; mockBuffer.viewportVisualLines = lines; + mockBuffer.viewportHeight = 10; mockBuffer.visualToLogicalMap = [ [0, 0], [1, 0], @@ -4262,7 +4365,9 @@ describe('InputPrompt', () => { const lines = ['first line', 'second line', 'third line']; mockBuffer.text = lines.join('\n'); mockBuffer.lines = lines; + mockBuffer.allVisualLines = lines; mockBuffer.viewportVisualLines = lines; + mockBuffer.viewportHeight = 10; mockBuffer.visualToLogicalMap = [ [0, 0], [1, 0], @@ -4303,6 +4408,7 @@ describe('InputPrompt', () => { it('should report cursor position 0 when input is empty and placeholder is shown', async () => { mockBuffer.text = ''; mockBuffer.lines = ['']; + mockBuffer.allVisualLines = ['']; mockBuffer.viewportVisualLines = ['']; mockBuffer.visualToLogicalMap = [[0, 0]]; mockBuffer.visualCursor = [0, 0]; @@ -4335,6 +4441,7 @@ describe('InputPrompt', () => { const applyVisualState = (visualLine: string, cursorCol: number): void => { mockBuffer.text = logicalLine; mockBuffer.lines = [logicalLine]; + mockBuffer.allVisualLines = [visualLine]; mockBuffer.viewportVisualLines = [visualLine]; mockBuffer.allVisualLines = [visualLine]; mockBuffer.visualToLogicalMap = [[0, 0]]; diff --git a/packages/cli/src/ui/components/InputPrompt.tsx b/packages/cli/src/ui/components/InputPrompt.tsx index 4547c19d8a..a8248bdd85 100644 --- a/packages/cli/src/ui/components/InputPrompt.tsx +++ b/packages/cli/src/ui/components/InputPrompt.tsx @@ -12,6 +12,10 @@ import { SuggestionsDisplay, MAX_WIDTH } from './SuggestionsDisplay.js'; import { theme } from '../semantic-colors.js'; import { useInputHistory } from '../hooks/useInputHistory.js'; import { escapeAtSymbols } from '../hooks/atCommandProcessor.js'; +import { + ScrollableList, + type ScrollableListRef, +} from './shared/ScrollableList.js'; import { HalfLinePaddedBox } from './shared/HalfLinePaddedBox.js'; import { type TextBuffer, @@ -95,6 +99,10 @@ export function isTerminalPasteTrusted( return kittyProtocolSupported; } +export type ScrollableItem = + | { type: 'visualLine'; lineText: string; absoluteVisualIdx: number } + | { type: 'ghostLine'; ghostLine: string; index: number }; + export interface InputPromptProps { buffer: TextBuffer; onSubmit: (value: string) => void; @@ -268,6 +276,7 @@ export const InputPrompt: React.FC = ({ const pasteTimeoutRef = useRef(null); const innerBoxRef = useRef(null); const hasUserNavigatedSuggestions = useRef(false); + const listRef = useRef>(null); const [reverseSearchActive, setReverseSearchActive] = useState(false); const [commandSearchActive, setCommandSearchActive] = useState(false); @@ -556,7 +565,10 @@ export const InputPrompt: React.FC = ({ if (isEmbeddedShellFocused) { setEmbeddedShellFocused(false); } - const visualRow = buffer.visualScrollRow + relY; + const currentScrollTop = Math.round( + listRef.current?.getScrollState().scrollTop ?? buffer.visualScrollRow, + ); + const visualRow = currentScrollTop + relY; buffer.moveToVisualPosition(visualRow, relX); }, { isActive: focus }, @@ -570,7 +582,10 @@ export const InputPrompt: React.FC = ({ (_event, relX, relY) => { if (!isAlternateBuffer) return; - const visualLine = buffer.viewportVisualLines[relY]; + const currentScrollTop = Math.round( + listRef.current?.getScrollState().scrollTop ?? buffer.visualScrollRow, + ); + const visualLine = buffer.allVisualLines[currentScrollTop + relY]; if (!visualLine) return; // Even if we click past the end of the line, we might want to collapse an expanded paste @@ -578,10 +593,7 @@ export const InputPrompt: React.FC = ({ const logicalPos = isPastEndOfLine ? null - : buffer.getLogicalPositionFromVisual( - buffer.visualScrollRow + relY, - relX, - ); + : buffer.getLogicalPositionFromVisual(currentScrollTop + relY, relX); // Check for paste placeholder (collapsed state) if (logicalPos) { @@ -603,7 +615,9 @@ export const InputPrompt: React.FC = ({ // If we didn't click a placeholder to expand, check if we are inside or after // an expanded paste region and collapse it. - const row = buffer.visualScrollRow + relY; + const visualRow = currentScrollTop + relY; + const mapEntry = buffer.visualToLogicalMap[visualRow]; + const row = mapEntry ? mapEntry[0] : visualRow; const expandedId = buffer.getExpandedPasteAtLine(row); if (expandedId) { buffer.togglePasteExpansion( @@ -1350,10 +1364,8 @@ export const InputPrompt: React.FC = ({ priority: true, }); - const linesToRender = buffer.viewportVisualLines; const [cursorVisualRowAbsolute, cursorVisualColAbsolute] = buffer.visualCursor; - const scrollVisualRow = buffer.visualScrollRow; const getGhostTextLines = useCallback(() => { if ( @@ -1468,6 +1480,155 @@ export const InputPrompt: React.FC = ({ const { inlineGhost, additionalLines } = getGhostTextLines(); + const scrollableData = useMemo(() => { + const items: ScrollableItem[] = buffer.allVisualLines.map( + (lineText, index) => ({ + type: 'visualLine', + lineText, + absoluteVisualIdx: index, + }), + ); + + additionalLines.forEach((ghostLine, index) => { + items.push({ + type: 'ghostLine', + ghostLine, + index, + }); + }); + + return items; + }, [buffer.allVisualLines, additionalLines]); + + const renderItem = useCallback( + ({ item }: { item: ScrollableItem; index: number }) => { + if (item.type === 'ghostLine') { + const padding = Math.max(0, inputWidth - stringWidth(item.ghostLine)); + return ( + + + {item.ghostLine} + {' '.repeat(padding)} + + + ); + } + + const { lineText, absoluteVisualIdx } = item; + // console.log('renderItem called with:', lineText); + const mapEntry = buffer.visualToLogicalMap[absoluteVisualIdx]; + if (!mapEntry) return ; + + const isOnCursorLine = + focus && absoluteVisualIdx === cursorVisualRowAbsolute; + const renderedLine: React.ReactNode[] = []; + const [logicalLineIdx] = mapEntry; + const logicalLine = buffer.lines[logicalLineIdx] || ''; + const transformations = + buffer.transformationsByLine[logicalLineIdx] ?? []; + const tokens = parseInputForHighlighting( + logicalLine, + logicalLineIdx, + transformations, + ...(focus && buffer.cursor[0] === logicalLineIdx + ? [buffer.cursor[1]] + : []), + ); + const visualStartCol = + buffer.visualToTransformedMap[absoluteVisualIdx] ?? 0; + const visualEndCol = visualStartCol + cpLen(lineText); + const segments = parseSegmentsFromTokens( + tokens, + visualStartCol, + visualEndCol, + ); + let charCount = 0; + segments.forEach((seg, segIdx) => { + const segLen = cpLen(seg.text); + let display = seg.text; + if (isOnCursorLine) { + const relCol = cursorVisualColAbsolute; + const segStart = charCount; + const segEnd = segStart + segLen; + if (relCol >= segStart && relCol < segEnd) { + const charToHighlight = cpSlice( + display, + relCol - segStart, + relCol - segStart + 1, + ); + const highlighted = showCursor + ? chalk.inverse(charToHighlight) + : charToHighlight; + display = + cpSlice(display, 0, relCol - segStart) + + highlighted + + cpSlice(display, relCol - segStart + 1); + } + charCount = segEnd; + } else { + charCount += segLen; + } + const color = + seg.type === 'command' || seg.type === 'file' || seg.type === 'paste' + ? theme.text.accent + : theme.text.primary; + renderedLine.push( + + {display} + , + ); + }); + + const currentLineGhost = isOnCursorLine ? inlineGhost : ''; + if ( + isOnCursorLine && + cursorVisualColAbsolute === cpLen(lineText) && + !currentLineGhost + ) { + renderedLine.push( + + {showCursor ? chalk.inverse(' ') : ' '} + , + ); + } + const showCursorBeforeGhost = + focus && + isOnCursorLine && + cursorVisualColAbsolute === cpLen(lineText) && + currentLineGhost; + return ( + + + {renderedLine} + {showCursorBeforeGhost && (showCursor ? chalk.inverse(' ') : ' ')} + {currentLineGhost && ( + {currentLineGhost} + )} + + + ); + }, + [ + buffer.visualToLogicalMap, + buffer.lines, + buffer.transformationsByLine, + buffer.cursor, + buffer.visualToTransformedMap, + focus, + cursorVisualRowAbsolute, + cursorVisualColAbsolute, + showCursor, + inlineGhost, + inputWidth, + ], + ); + const useBackgroundColor = config.getUseBackgroundColor(); const isLowColor = isLowColorDepth(); const terminalBg = theme.background.primary || 'black'; @@ -1485,6 +1646,46 @@ export const InputPrompt: React.FC = ({ return false; }, [useBackgroundColor, isLowColor, terminalBg]); + const prevCursorRef = useRef(buffer.visualCursor); + const prevTextRef = useRef(buffer.text); + + // Effect to ensure cursor remains visible after interactions + useEffect(() => { + const cursorChanged = prevCursorRef.current !== buffer.visualCursor; + const textChanged = prevTextRef.current !== buffer.text; + + prevCursorRef.current = buffer.visualCursor; + prevTextRef.current = buffer.text; + + if (!cursorChanged && !textChanged) return; + + if (!listRef.current || !focus) return; + const { scrollTop, innerHeight } = listRef.current.getScrollState(); + if (innerHeight === 0) return; + + const cursorVisualRow = buffer.visualCursor[0]; + const actualScrollTop = Math.round(scrollTop); + + // If cursor is out of the currently visible viewport... + if ( + cursorVisualRow < actualScrollTop || + cursorVisualRow >= actualScrollTop + innerHeight + ) { + // Calculate minimal scroll to make it visible + let newScrollTop = actualScrollTop; + if (cursorVisualRow < actualScrollTop) { + newScrollTop = cursorVisualRow; + } else if (cursorVisualRow >= actualScrollTop + innerHeight) { + newScrollTop = cursorVisualRow - innerHeight + 1; + } + + listRef.current.scrollToIndex({ index: newScrollTop }); + } + }, [buffer.visualCursor, buffer.text, focus]); + + const listBackgroundColor = + useLineFallback || !useBackgroundColor ? undefined : theme.background.input; + useEffect(() => { if (onSuggestionsVisibilityChange) { onSuggestionsVisibilityChange(shouldShowSuggestions); @@ -1615,153 +1816,30 @@ export const InputPrompt: React.FC = ({ {placeholder} ) ) : ( - linesToRender - .map((lineText: string, visualIdxInRenderedSet: number) => { - const absoluteVisualIdx = - scrollVisualRow + visualIdxInRenderedSet; - const mapEntry = buffer.visualToLogicalMap[absoluteVisualIdx]; - if (!mapEntry) return null; - - const cursorVisualRow = - cursorVisualRowAbsolute - scrollVisualRow; - const isOnCursorLine = - focus && visualIdxInRenderedSet === cursorVisualRow; - - const renderedLine: React.ReactNode[] = []; - - const [logicalLineIdx] = mapEntry; - const logicalLine = buffer.lines[logicalLineIdx] || ''; - const transformations = - buffer.transformationsByLine[logicalLineIdx] ?? []; - const tokens = parseInputForHighlighting( - logicalLine, - logicalLineIdx, - transformations, - ...(focus && buffer.cursor[0] === logicalLineIdx - ? [buffer.cursor[1]] - : []), - ); - const startColInTransformed = - buffer.visualToTransformedMap[absoluteVisualIdx] ?? 0; - const visualStartCol = startColInTransformed; - const visualEndCol = visualStartCol + cpLen(lineText); - const segments = parseSegmentsFromTokens( - tokens, - visualStartCol, - visualEndCol, - ); - let charCount = 0; - segments.forEach((seg, segIdx) => { - const segLen = cpLen(seg.text); - let display = seg.text; - - if (isOnCursorLine) { - const relativeVisualColForHighlight = - cursorVisualColAbsolute; - const segStart = charCount; - const segEnd = segStart + segLen; - if ( - relativeVisualColForHighlight >= segStart && - relativeVisualColForHighlight < segEnd - ) { - const charToHighlight = cpSlice( - display, - relativeVisualColForHighlight - segStart, - relativeVisualColForHighlight - segStart + 1, - ); - const highlighted = showCursor - ? chalk.inverse(charToHighlight) - : charToHighlight; - display = - cpSlice( - display, - 0, - relativeVisualColForHighlight - segStart, - ) + - highlighted + - cpSlice( - display, - relativeVisualColForHighlight - segStart + 1, - ); - } - charCount = segEnd; - } else { - // Advance the running counter even when not on cursor line - charCount += segLen; - } - - const color = - seg.type === 'command' || - seg.type === 'file' || - seg.type === 'paste' - ? theme.text.accent - : theme.text.primary; - - renderedLine.push( - - {display} - , - ); - }); - - const currentLineGhost = isOnCursorLine ? inlineGhost : ''; - if ( - isOnCursorLine && - cursorVisualColAbsolute === cpLen(lineText) - ) { - if (!currentLineGhost) { - renderedLine.push( - - {showCursor ? chalk.inverse(' ') : ' '} - , - ); - } + + 1} + keyExtractor={(item) => + item.type === 'visualLine' + ? `line-${item.absoluteVisualIdx}` + : `ghost-${item.index}` } - - const showCursorBeforeGhost = - focus && - isOnCursorLine && - cursorVisualColAbsolute === cpLen(lineText) && - currentLineGhost; - - return ( - - - {renderedLine} - {showCursorBeforeGhost && - (showCursor ? chalk.inverse(' ') : ' ')} - {currentLineGhost && ( - - {currentLineGhost} - - )} - - - ); - }) - .concat( - additionalLines.map((ghostLine, index) => { - const padding = Math.max( - 0, - inputWidth - stringWidth(ghostLine), - ); - return ( - - {ghostLine} - {' '.repeat(padding)} - - ); - }), - ) + width="100%" + backgroundColor={listBackgroundColor} + containerHeight={Math.min( + buffer.viewportHeight, + scrollableData.length, + )} + /> + )} diff --git a/packages/cli/src/ui/components/__snapshots__/InputPrompt.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/InputPrompt.test.tsx.snap index ab6fe9b928..caa270d8c4 100644 --- a/packages/cli/src/ui/components/__snapshots__/InputPrompt.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/InputPrompt.test.tsx.snap @@ -93,7 +93,7 @@ exports[`InputPrompt > Highlighting and Cursor Display > single-line scenarios > exports[`InputPrompt > History Navigation and Completion Suppression > should not render suggestions during history navigation 1`] = ` "β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€ > second message -β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„ + " `; @@ -120,30 +120,30 @@ exports[`InputPrompt > command search (Ctrl+R when not in shell) > expands and c exports[`InputPrompt > command search (Ctrl+R when not in shell) > renders match window and expanded view (snapshots) > command-search-render-collapsed-match 1`] = ` "β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€ (r:) commit -β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„ - git commit -m "feat: add search" in src/app + + " `; exports[`InputPrompt > command search (Ctrl+R when not in shell) > renders match window and expanded view (snapshots) > command-search-render-expanded-match 1`] = ` "β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€ (r:) commit -β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„ - git commit -m "feat: add search" in src/app + + " `; exports[`InputPrompt > image path transformation snapshots > should snapshot collapsed image path 1`] = ` "β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€ > [Image ...reenshot2x.png] -β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„ + " `; exports[`InputPrompt > image path transformation snapshots > should snapshot expanded image path when cursor is on it 1`] = ` "β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€β–€ > @/path/to/screenshots/screenshot2x.png -β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„β–„ + " `; diff --git a/packages/cli/src/ui/components/shared/ScrollableList.tsx b/packages/cli/src/ui/components/shared/ScrollableList.tsx index 326005726f..c857e97b70 100644 --- a/packages/cli/src/ui/components/shared/ScrollableList.tsx +++ b/packages/cli/src/ui/components/shared/ScrollableList.tsx @@ -36,6 +36,9 @@ interface ScrollableListProps extends VirtualizedListProps { copyModeEnabled?: boolean; isStatic?: boolean; fixedItemHeight?: boolean; + targetScrollIndex?: number; + containerHeight?: number; + scrollbarThumbColor?: string; } export type ScrollableListRef = VirtualizedListRef; diff --git a/packages/cli/src/ui/components/shared/VirtualizedList.tsx b/packages/cli/src/ui/components/shared/VirtualizedList.tsx index e7b756b649..b527724492 100644 --- a/packages/cli/src/ui/components/shared/VirtualizedList.tsx +++ b/packages/cli/src/ui/components/shared/VirtualizedList.tsx @@ -29,6 +29,8 @@ export type VirtualizedListProps = { keyExtractor: (item: T, index: number) => string; initialScrollIndex?: number; initialScrollOffsetInIndex?: number; + targetScrollIndex?: number; + backgroundColor?: string; scrollbarThumbColor?: string; renderStatic?: boolean; isStatic?: boolean; @@ -39,6 +41,7 @@ export type VirtualizedListProps = { stableScrollback?: boolean; copyModeEnabled?: boolean; fixedItemHeight?: boolean; + containerHeight?: number; }; export type VirtualizedListRef = { @@ -159,6 +162,17 @@ function VirtualizedList( }; } + if (typeof props.targetScrollIndex === 'number') { + // NOTE: When targetScrollIndex is specified, we rely on the component + // correctly tracking targetScrollIndex instead of initialScrollIndex. + // We set isInitialScrollSet.current = true inside the second layout effect + // to avoid it overwriting the targetScrollIndex. + return { + index: props.targetScrollIndex, + offset: 0, + }; + } + return { index: 0, offset: 0 }; }); @@ -242,7 +256,7 @@ function VirtualizedList( return { totalHeight, offsets }; }, [heights, data, estimatedItemHeight, keyExtractor]); - const scrollableContainerHeight = containerHeight; + const scrollableContainerHeight = props.containerHeight ?? containerHeight; const getAnchorForScrollTop = useCallback( ( @@ -259,6 +273,32 @@ function VirtualizedList( [], ); + const [prevTargetScrollIndex, setPrevTargetScrollIndex] = useState( + props.targetScrollIndex, + ); + const prevOffsetsLength = useRef(offsets.length); + + // NOTE: If targetScrollIndex is provided, and we haven't rendered items yet (offsets.length <= 1), + // we do NOT set scrollAnchor yet, because actualScrollTop wouldn't know the real offset! + // We wait until offsets populate. + if ( + (props.targetScrollIndex !== undefined && + props.targetScrollIndex !== prevTargetScrollIndex && + offsets.length > 1) || + (props.targetScrollIndex !== undefined && + prevOffsetsLength.current <= 1 && + offsets.length > 1) + ) { + if (props.targetScrollIndex !== prevTargetScrollIndex) { + setPrevTargetScrollIndex(props.targetScrollIndex); + } + prevOffsetsLength.current = offsets.length; + setIsStickingToBottom(false); + setScrollAnchor({ index: props.targetScrollIndex, offset: 0 }); + } else { + prevOffsetsLength.current = offsets.length; + } + const actualScrollTop = useMemo(() => { const offset = offsets[scrollAnchor.index]; if (typeof offset !== 'number') { @@ -309,9 +349,14 @@ function VirtualizedList( const containerChanged = prevContainerHeight.current !== scrollableContainerHeight; + // If targetScrollIndex is provided, we NEVER auto-snap to the bottom + // because the parent is explicitly managing the scroll position. + const shouldAutoScroll = props.targetScrollIndex === undefined; + if ( - (listGrew && (isStickingToBottom || wasAtBottom)) || - (isStickingToBottom && containerChanged) + shouldAutoScroll && + ((listGrew && (isStickingToBottom || wasAtBottom)) || + (isStickingToBottom && containerChanged)) ) { const newIndex = data.length > 0 ? data.length - 1 : 0; if ( @@ -331,6 +376,7 @@ function VirtualizedList( actualScrollTop > totalHeight - scrollableContainerHeight) && data.length > 0 ) { + // We still clamp the scroll top if it's completely out of bounds const newScrollTop = Math.max(0, totalHeight - scrollableContainerHeight); const newAnchor = getAnchorForScrollTop(newScrollTop, offsets); if ( @@ -359,6 +405,7 @@ function VirtualizedList( getAnchorForScrollTop, offsets, isStickingToBottom, + props.targetScrollIndex, ]); useLayoutEffect(() => { @@ -366,11 +413,17 @@ function VirtualizedList( isInitialScrollSet.current || offsets.length <= 1 || totalHeight <= 0 || - containerHeight <= 0 + scrollableContainerHeight <= 0 ) { return; } + if (props.targetScrollIndex !== undefined) { + // If we are strictly driving from targetScrollIndex, do not apply initialScrollIndex + isInitialScrollSet.current = true; + return; + } + if (typeof initialScrollIndex === 'number') { const scrollToEnd = initialScrollIndex === SCROLL_TO_ITEM_END || @@ -404,19 +457,21 @@ function VirtualizedList( initialScrollOffsetInIndex, offsets, totalHeight, - containerHeight, + scrollableContainerHeight, getAnchorForScrollTop, data.length, heights, - scrollableContainerHeight, + props.targetScrollIndex, ]); const startIndex = Math.max( 0, findLastIndex(offsets, (offset) => offset <= actualScrollTop) - 1, ); + const viewHeightForEndIndex = + scrollableContainerHeight > 0 ? scrollableContainerHeight : 50; const endIndexOffset = offsets.findIndex( - (offset) => offset > actualScrollTop + scrollableContainerHeight, + (offset) => offset > actualScrollTop + viewHeightForEndIndex, ); const endIndex = endIndexOffset === -1 @@ -618,11 +673,11 @@ function VirtualizedList( }, getScrollIndex: () => scrollAnchor.index, getScrollState: () => { - const maxScroll = Math.max(0, totalHeight - containerHeight); + const maxScroll = Math.max(0, totalHeight - scrollableContainerHeight); return { scrollTop: Math.min(getScrollTop(), maxScroll), scrollHeight: totalHeight, - innerHeight: containerHeight, + innerHeight: scrollableContainerHeight, }; }, }), @@ -635,7 +690,6 @@ function VirtualizedList( scrollableContainerHeight, getScrollTop, setPendingScrollTop, - containerHeight, ], ); @@ -646,6 +700,7 @@ function VirtualizedList( overflowX="hidden" scrollTop={copyModeEnabled ? 0 : scrollTop} scrollbarThumbColor={props.scrollbarThumbColor ?? theme.text.secondary} + backgroundColor={props.backgroundColor} width="100%" height="100%" flexDirection="column" diff --git a/packages/cli/src/ui/components/shared/text-buffer.ts b/packages/cli/src/ui/components/shared/text-buffer.ts index 72d842ec98..d6b95d6016 100644 --- a/packages/cli/src/ui/components/shared/text-buffer.ts +++ b/packages/cli/src/ui/components/shared/text-buffer.ts @@ -2907,6 +2907,25 @@ export function useTextBuffer({ const [scrollRowState, setScrollRowState] = useState(0); + const { height } = viewport; + const totalVisualLines = visualLines.length; + const maxScrollStart = Math.max(0, totalVisualLines - height); + let newVisualScrollRow = scrollRowState; + + if (visualCursor[0] < scrollRowState) { + newVisualScrollRow = visualCursor[0]; + } else if (visualCursor[0] >= scrollRowState + height) { + newVisualScrollRow = visualCursor[0] - height + 1; + } + + newVisualScrollRow = clamp(newVisualScrollRow, 0, maxScrollStart); + + if (newVisualScrollRow !== scrollRowState) { + setScrollRowState(newVisualScrollRow); + } + + const actualScrollRowState = newVisualScrollRow; + useEffect(() => { if (onChange) { onChange(text); @@ -2920,28 +2939,6 @@ export function useTextBuffer({ }); }, [viewport.width, viewport.height]); - // Update visual scroll (vertical) - useEffect(() => { - const { height } = viewport; - const totalVisualLines = visualLines.length; - const maxScrollStart = Math.max(0, totalVisualLines - height); - let newVisualScrollRow = scrollRowState; - - if (visualCursor[0] < scrollRowState) { - newVisualScrollRow = visualCursor[0]; - } else if (visualCursor[0] >= scrollRowState + height) { - newVisualScrollRow = visualCursor[0] - height + 1; - } - - // When the number of visual lines shrinks (e.g., after widening the viewport), - // ensure scroll never starts beyond the last valid start so we can render a full window. - newVisualScrollRow = clamp(newVisualScrollRow, 0, maxScrollStart); - - if (newVisualScrollRow !== scrollRowState) { - setScrollRowState(newVisualScrollRow); - } - }, [visualCursor, scrollRowState, viewport, visualLines.length]); - const insert = useCallback( (ch: string, { paste = false }: { paste?: boolean } = {}): void => { if (typeof ch !== 'string') { @@ -3495,10 +3492,10 @@ export function useTextBuffer({ const visualScrollRow = useMemo(() => { const totalVisualLines = visualLines.length; return Math.min( - scrollRowState, + actualScrollRowState, Math.max(0, totalVisualLines - viewport.height), ); - }, [visualLines.length, scrollRowState, viewport.height]); + }, [visualLines.length, actualScrollRowState, viewport.height]); const renderedVisualLines = useMemo( () => visualLines.slice(visualScrollRow, visualScrollRow + viewport.height), @@ -3694,6 +3691,7 @@ export function useTextBuffer({ viewportVisualLines: renderedVisualLines, visualCursor, visualScrollRow, + viewportHeight: viewport.height, visualToLogicalMap, transformedToLogicalMaps, visualToTransformedMap, @@ -3799,6 +3797,7 @@ export function useTextBuffer({ renderedVisualLines, visualCursor, visualScrollRow, + viewport.height, visualToLogicalMap, transformedToLogicalMaps, visualToTransformedMap, @@ -3914,6 +3913,7 @@ export interface TextBuffer { viewportVisualLines: string[]; // The subset of visual lines to be rendered based on visualScrollRow and viewport.height visualCursor: [number, number]; // Visual cursor [row, col] relative to the start of all visualLines visualScrollRow: number; // Scroll position for visual lines (index of the first visible visual line) + viewportHeight: number; // The maximum height of the viewport /** * For each visual line (by absolute index in allVisualLines) provides a tuple * [logicalLineIndex, startColInLogical] that maps where that visual line diff --git a/packages/core/src/policy/sandboxPolicyManager.test.ts b/packages/core/src/policy/sandboxPolicyManager.test.ts new file mode 100644 index 0000000000..034ab68735 --- /dev/null +++ b/packages/core/src/policy/sandboxPolicyManager.test.ts @@ -0,0 +1,72 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SandboxPolicyManager } from './sandboxPolicyManager.js'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +describe('SandboxPolicyManager', () => { + const tempDir = path.join(os.tmpdir(), 'gemini-test-sandbox-policy'); + const configPath = path.join(tempDir, 'sandbox.toml'); + + beforeEach(() => { + if (!fs.existsSync(tempDir)) { + fs.mkdirSync(tempDir, { recursive: true }); + } + }); + + afterEach(() => { + if (fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + it('should add and retrieve session approvals', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addSessionApproval('ls', { + fileSystem: { read: ['/tmp'], write: [] }, + network: false, + }); + + const perms = manager.getCommandPermissions('ls'); + expect(perms.fileSystem?.read).toContain('/tmp'); + }); + + it('should protect against prototype pollution (session)', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addSessionApproval('__proto__', { + fileSystem: { read: ['/POLLUTED'], write: [] }, + network: true, + }); + + const perms = manager.getCommandPermissions('any-command'); + expect(perms.fileSystem?.read).not.toContain('/POLLUTED'); + }); + + it('should protect against prototype pollution (persistent)', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addPersistentApproval('constructor', { + fileSystem: { read: ['/POLLUTED_PERSISTENT'], write: [] }, + network: true, + }); + + const perms = manager.getCommandPermissions('constructor'); + expect(perms.fileSystem?.read).not.toContain('/POLLUTED_PERSISTENT'); + }); + + it('should lowercase command names for normalization', () => { + const manager = new SandboxPolicyManager(configPath); + manager.addSessionApproval('NPM', { + fileSystem: { read: ['/node_modules'], write: [] }, + network: true, + }); + + const perms = manager.getCommandPermissions('npm'); + expect(perms.fileSystem?.read).toContain('/node_modules'); + }); +}); diff --git a/packages/core/src/policy/sandboxPolicyManager.ts b/packages/core/src/policy/sandboxPolicyManager.ts index c8a4d2f8df..8b3d9a5744 100644 --- a/packages/core/src/policy/sandboxPolicyManager.ts +++ b/packages/core/src/policy/sandboxPolicyManager.ts @@ -13,6 +13,7 @@ import { fileURLToPath } from 'node:url'; import { debugLogger } from '../utils/debugLogger.js'; import { type SandboxPermissions } from '../services/sandboxManager.js'; import { sanitizePaths } from '../services/sandboxManager.js'; +import { normalizeCommand } from '../utils/shell-utils.js'; export const SandboxModeConfigSchema = z.object({ network: z.boolean(), @@ -104,6 +105,10 @@ export class SandboxPolicyManager { this.config = this.loadConfig(); } + private isProtectedKey(key: string): boolean { + return key === '__proto__' || key === 'constructor' || key === 'prototype'; + } + private loadConfig(): SandboxTomlSchemaType { if (!fs.existsSync(this.configPath)) { return SandboxPolicyManager.DEFAULT_CONFIG; @@ -154,8 +159,15 @@ export class SandboxPolicyManager { } getCommandPermissions(commandName: string): SandboxPermissions { - const persistent = this.config.commands[commandName]; - const session = this.sessionApprovals[commandName]; + const normalized = normalizeCommand(commandName); + if (this.isProtectedKey(normalized)) { + return { + fileSystem: { read: [], write: [] }, + network: false, + }; + } + const persistent = this.config.commands[normalized]; + const session = this.sessionApprovals[normalized]; return { fileSystem: { @@ -176,25 +188,25 @@ export class SandboxPolicyManager { commandName: string, permissions: SandboxPermissions, ): void { - const existing = this.sessionApprovals[commandName] || { + const normalized = normalizeCommand(commandName); + if (this.isProtectedKey(normalized)) { + return; + } + const existing = this.sessionApprovals[normalized] || { fileSystem: { read: [], write: [] }, network: false, }; - this.sessionApprovals[commandName] = { + this.sessionApprovals[normalized] = { fileSystem: { - read: Array.from( - new Set([ - ...(existing.fileSystem?.read ?? []), - ...(permissions.fileSystem?.read ?? []), - ]), - ), - write: Array.from( - new Set([ - ...(existing.fileSystem?.write ?? []), - ...(permissions.fileSystem?.write ?? []), - ]), - ), + read: sanitizePaths([ + ...(existing.fileSystem?.read ?? []), + ...(permissions.fileSystem?.read ?? []), + ]), + write: sanitizePaths([ + ...(existing.fileSystem?.write ?? []), + ...(permissions.fileSystem?.write ?? []), + ]), }, network: existing.network || permissions.network || false, }; @@ -204,7 +216,11 @@ export class SandboxPolicyManager { commandName: string, permissions: SandboxPermissions, ): void { - const existing = this.config.commands[commandName] || { + const normalized = normalizeCommand(commandName); + if (this.isProtectedKey(normalized)) { + return; + } + const existing = this.config.commands[normalized] || { allowed_paths: [], allow_network: false, }; @@ -216,7 +232,7 @@ export class SandboxPolicyManager { ]; const newPaths = new Set(sanitizePaths(newPathsArray)); - this.config.commands[commandName] = { + this.config.commands[normalized] = { allowed_paths: Array.from(newPaths), allow_network: existing.allow_network || permissions.network || false, }; diff --git a/packages/core/src/sandbox/utils/proactivePermissions.ts b/packages/core/src/sandbox/utils/proactivePermissions.ts index a5e11e2c3c..c4ec0c1520 100644 --- a/packages/core/src/sandbox/utils/proactivePermissions.ts +++ b/packages/core/src/sandbox/utils/proactivePermissions.ts @@ -8,6 +8,7 @@ import os from 'node:os'; import path from 'node:path'; import fs from 'node:fs'; import { type SandboxPermissions } from '../../services/sandboxManager.js'; +import { normalizeCommand } from '../../utils/shell-utils.js'; const NETWORK_RELIANT_TOOLS = new Set([ 'npm', @@ -45,7 +46,7 @@ export function isNetworkReliantCommand( commandName: string, subCommand?: string, ): boolean { - const normalizedCommand = commandName.toLowerCase().replace(/\.exe$/, ''); + const normalizedCommand = normalizeCommand(commandName); if (!NETWORK_RELIANT_TOOLS.has(normalizedCommand)) { return false; } @@ -82,7 +83,7 @@ export function isNetworkReliantCommand( export async function getProactiveToolSuggestions( commandName: string, ): Promise { - const normalizedCommand = commandName.toLowerCase().replace(/\.exe$/, ''); + const normalizedCommand = normalizeCommand(commandName); if (!NETWORK_RELIANT_TOOLS.has(normalizedCommand)) { return undefined; } diff --git a/packages/core/src/sandbox/windows/GeminiSandbox.cs b/packages/core/src/sandbox/windows/GeminiSandbox.cs index 6275b701c4..eef08b250b 100644 --- a/packages/core/src/sandbox/windows/GeminiSandbox.cs +++ b/packages/core/src/sandbox/windows/GeminiSandbox.cs @@ -21,6 +21,8 @@ using System.Text; */ public class GeminiSandbox { // P/Invoke constants and structures + private const int JobObjectExtendedLimitInformation = 9; + private const int JobObjectNetRateControlInformation = 32; private const uint JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE = 0x00002000; private const uint JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION = 0x00000400; private const uint JOB_OBJECT_LIMIT_ACTIVE_PROCESS = 0x00000008; @@ -74,6 +76,9 @@ public class GeminiSandbox { [DllImport("kernel32.dll", SetLastError = true)] static extern bool AssignProcessToJobObject(IntPtr hJob, IntPtr hProcess); + [DllImport("kernel32.dll", SetLastError = true)] + static extern uint ResumeThread(IntPtr hThread); + [DllImport("advapi32.dll", SetLastError = true)] static extern bool OpenProcessToken(IntPtr ProcessHandle, uint DesiredAccess, out IntPtr TokenHandle); @@ -191,7 +196,8 @@ public class GeminiSandbox { IntPtr hToken = IntPtr.Zero; IntPtr hRestrictedToken = IntPtr.Zero; - IntPtr lowIntegritySid = IntPtr.Zero; + IntPtr hJob = IntPtr.Zero; + PROCESS_INFORMATION pi = new PROCESS_INFORMATION(); try { // 1. Duplicate Primary Token @@ -208,6 +214,7 @@ public class GeminiSandbox { // 2. Lower Integrity Level to Low // S-1-16-4096 is the SID for "Low Mandatory Level" + IntPtr lowIntegritySid = IntPtr.Zero; if (ConvertStringSidToSid("S-1-16-4096", out lowIntegritySid)) { TOKEN_MANDATORY_LABEL tml = new TOKEN_MANDATORY_LABEL(); tml.Label.Sid = lowIntegritySid; @@ -226,25 +233,42 @@ public class GeminiSandbox { } // 3. Setup Job Object for cleanup - IntPtr hJob = CreateJobObject(IntPtr.Zero, null); + hJob = CreateJobObject(IntPtr.Zero, null); + if (hJob == IntPtr.Zero) { + Console.Error.WriteLine("Error: CreateJobObject failed (" + Marshal.GetLastWin32Error() + ")"); + return 1; + } + JOBOBJECT_EXTENDED_LIMIT_INFORMATION jobLimits = new JOBOBJECT_EXTENDED_LIMIT_INFORMATION(); jobLimits.BasicLimitInformation.LimitFlags = JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE | JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION; - + IntPtr lpJobLimits = Marshal.AllocHGlobal(Marshal.SizeOf(jobLimits)); - Marshal.StructureToPtr(jobLimits, lpJobLimits, false); - SetInformationJobObject(hJob, 9 /* JobObjectExtendedLimitInformation */, lpJobLimits, (uint)Marshal.SizeOf(jobLimits)); - Marshal.FreeHGlobal(lpJobLimits); + try { + Marshal.StructureToPtr(jobLimits, lpJobLimits, false); + if (!SetInformationJobObject(hJob, JobObjectExtendedLimitInformation, lpJobLimits, (uint)Marshal.SizeOf(jobLimits))) { + Console.Error.WriteLine("Error: SetInformationJobObject(Limits) failed (" + Marshal.GetLastWin32Error() + ")"); + return 1; + } + } finally { + Marshal.FreeHGlobal(lpJobLimits); + } if (!networkAccess) { JOBOBJECT_NET_RATE_CONTROL_INFORMATION netLimits = new JOBOBJECT_NET_RATE_CONTROL_INFORMATION(); netLimits.MaxBandwidth = 1; netLimits.ControlFlags = 0x1 | 0x2; // ENABLE | MAX_BANDWIDTH netLimits.DscpTag = 0; - + IntPtr lpNetLimits = Marshal.AllocHGlobal(Marshal.SizeOf(netLimits)); - Marshal.StructureToPtr(netLimits, lpNetLimits, false); - SetInformationJobObject(hJob, 32 /* JobObjectNetRateControlInformation */, lpNetLimits, (uint)Marshal.SizeOf(netLimits)); - Marshal.FreeHGlobal(lpNetLimits); + try { + Marshal.StructureToPtr(netLimits, lpNetLimits, false); + if (!SetInformationJobObject(hJob, JobObjectNetRateControlInformation, lpNetLimits, (uint)Marshal.SizeOf(netLimits))) { + // Some versions of Windows might not support network rate control, but we should know if it fails. + Console.Error.WriteLine("Warning: SetInformationJobObject(NetRate) failed (" + Marshal.GetLastWin32Error() + "). Network might not be throttled."); + } + } finally { + Marshal.FreeHGlobal(lpNetLimits); + } } // 4. Handle Internal Commands or External Process @@ -310,32 +334,49 @@ public class GeminiSandbox { commandLine += QuoteArgument(args[i]); } - PROCESS_INFORMATION pi = new PROCESS_INFORMATION(); - // Creation Flags: 0x04000000 (CREATE_BREAKAWAY_FROM_JOB) to allow job assignment if parent is in job - uint creationFlags = 0; + // Creation Flags: 0x01000000 (CREATE_BREAKAWAY_FROM_JOB) to allow job assignment if parent is in job + // 0x00000004 (CREATE_SUSPENDED) to prevent the process from executing before being placed in the job + uint creationFlags = 0x01000000 | 0x00000004; if (!CreateProcessAsUser(hRestrictedToken, null, commandLine, IntPtr.Zero, IntPtr.Zero, true, creationFlags, IntPtr.Zero, cwd, ref si, out pi)) { - Console.WriteLine("Error: CreateProcessAsUser failed (" + Marshal.GetLastWin32Error() + ") Command: " + commandLine); + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: CreateProcessAsUser failed (" + err + ") Command: " + commandLine); return 1; } - AssignProcessToJobObject(hJob, pi.hProcess); - - // Wait for exit - uint waitResult = WaitForSingleObject(pi.hProcess, 0xFFFFFFFF); - uint exitCode = 0; - GetExitCodeProcess(pi.hProcess, out exitCode); + if (!AssignProcessToJobObject(hJob, pi.hProcess)) { + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: AssignProcessToJobObject failed (" + err + ") Command: " + commandLine); + TerminateProcess(pi.hProcess, 1); + return 1; + } - CloseHandle(pi.hProcess); - CloseHandle(pi.hThread); - CloseHandle(hJob); + ResumeThread(pi.hThread); + + if (WaitForSingleObject(pi.hProcess, 0xFFFFFFFF) == 0xFFFFFFFF) { + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: WaitForSingleObject failed (" + err + ")"); + } + + uint exitCode = 0; + if (!GetExitCodeProcess(pi.hProcess, out exitCode)) { + int err = Marshal.GetLastWin32Error(); + Console.Error.WriteLine("Error: GetExitCodeProcess failed (" + err + ")"); + return 1; + } return (int)exitCode; } finally { if (hToken != IntPtr.Zero) CloseHandle(hToken); if (hRestrictedToken != IntPtr.Zero) CloseHandle(hRestrictedToken); + if (hJob != IntPtr.Zero) CloseHandle(hJob); + if (pi.hProcess != IntPtr.Zero) CloseHandle(pi.hProcess); + if (pi.hThread != IntPtr.Zero) CloseHandle(pi.hThread); } } + [DllImport("kernel32.dll", SetLastError = true)] + static extern bool TerminateProcess(IntPtr hProcess, uint uExitCode); + [DllImport("kernel32.dll", SetLastError = true)] static extern uint WaitForSingleObject(IntPtr hHandle, uint dwMilliseconds); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts index 8b38179177..c814f740f7 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.test.ts @@ -25,17 +25,40 @@ vi.mock('../../utils/shell-utils.js', async (importOriginal) => { }; }); -// TODO: reenable once test is fixed -describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { +describe('WindowsSandboxManager', () => { let manager: WindowsSandboxManager; let testCwd: string; + /** + * Creates a temporary directory and returns its canonical real path. + */ + function createTempDir(name: string, parent = os.tmpdir()): string { + const rawPath = fs.mkdtempSync(path.join(parent, `gemini-test-${name}-`)); + return fs.realpathSync(rawPath); + } + + const helperExePath = path.resolve( + __dirname, + WindowsSandboxManager.HELPER_EXE, + ); + beforeEach(() => { vi.spyOn(os, 'platform').mockReturnValue('win32'); vi.spyOn(sandboxManager, 'tryRealpath').mockImplementation(async (p) => p.toString(), ); - testCwd = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-cli-test-')); + + // Mock existsSync to skip the csc.exe auto-compilation of helper during unit tests. + const originalExistsSync = fs.existsSync; + vi.spyOn(fs, 'existsSync').mockImplementation((p) => { + if (typeof p === 'string' && path.resolve(p) === helperExePath) { + return true; + } + return originalExistsSync(p); + }); + + testCwd = createTempDir('cwd'); + manager = new WindowsSandboxManager({ workspace: testCwd, modeConfig: { readonly: false, allowOverrides: true }, @@ -45,7 +68,9 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { afterEach(() => { vi.restoreAllMocks(); - fs.rmSync(testCwd, { recursive: true, force: true }); + if (testCwd && fs.existsSync(testCwd)) { + fs.rmSync(testCwd, { recursive: true, force: true }); + } }); it('should prepare a GeminiSandbox.exe command', async () => { @@ -155,8 +180,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should handle persistent permissions from policyManager', async () => { - const persistentPath = path.join(testCwd, 'persistent_path'); - fs.mkdirSync(persistentPath, { recursive: true }); + const persistentPath = createTempDir('persistent', testCwd); const mockPolicyManager = { getCommandPermissions: vi.fn().mockReturnValue({ @@ -189,6 +213,8 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { expect(icaclsArgs).toContainEqual([ persistentPath, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -234,10 +260,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should grant Low Integrity access to the workspace and allowed paths', async () => { - const allowedPath = path.join(os.tmpdir(), 'gemini-cli-test-allowed'); - if (!fs.existsSync(allowedPath)) { - fs.mkdirSync(allowedPath); - } + const allowedPath = createTempDir('allowed'); try { const req: SandboxRequest = { command: 'test', @@ -257,13 +280,17 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { .map((c) => c[1]); expect(icaclsArgs).toContainEqual([ - path.resolve(testCwd), + testCwd, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); expect(icaclsArgs).toContainEqual([ - path.resolve(allowedPath), + allowedPath, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -273,13 +300,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should grant Low Integrity access to additional write paths', async () => { - const extraWritePath = path.join( - os.tmpdir(), - 'gemini-cli-test-extra-write', - ); - if (!fs.existsSync(extraWritePath)) { - fs.mkdirSync(extraWritePath); - } + const extraWritePath = createTempDir('extra-write'); try { const req: SandboxRequest = { command: 'test', @@ -303,7 +324,9 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { .map((c) => c[1]); expect(icaclsArgs).toContainEqual([ - path.resolve(extraWritePath), + extraWritePath, + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -330,26 +353,26 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }, }; - await manager.prepareCommand(req); + // Rejected because it's an unreachable/invalid UNC path or it doesn't exist + await expect(manager.prepareCommand(req)).rejects.toThrow(); const icaclsArgs = vi .mocked(spawnAsync) .mock.calls.filter((c) => c[0] === 'icacls') .map((c) => c[1]); - expect(icaclsArgs).not.toContainEqual([ - uncPath, - '/setintegritylevel', - '(OI)(CI)Low', - ]); + expect(icaclsArgs).not.toContainEqual(expect.arrayContaining([uncPath])); }, ); it.runIf(process.platform === 'win32')( 'should allow extended-length and local device paths', async () => { - const longPath = '\\\\?\\C:\\very\\long\\path'; - const devicePath = '\\\\.\\PhysicalDrive0'; + // Create actual files for inheritance/existence checks + const longPath = path.join(testCwd, 'very_long_path.txt'); + const devicePath = path.join(testCwd, 'device_path.txt'); + fs.writeFileSync(longPath, ''); + fs.writeFileSync(devicePath, ''); const req: SandboxRequest = { command: 'test', @@ -373,12 +396,16 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { .map((c) => c[1]); expect(icaclsArgs).toContainEqual([ - longPath, + path.resolve(longPath), + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); expect(icaclsArgs).toContainEqual([ - devicePath, + path.resolve(devicePath), + '/grant', + '*S-1-16-4096:(OI)(CI)(M)', '/setintegritylevel', '(OI)(CI)Low', ]); @@ -420,10 +447,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should deny Low Integrity access to forbidden paths', async () => { - const forbiddenPath = path.join(os.tmpdir(), 'gemini-cli-test-forbidden'); - if (!fs.existsSync(forbiddenPath)) { - fs.mkdirSync(forbiddenPath); - } + const forbiddenPath = createTempDir('forbidden'); try { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, @@ -440,7 +464,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { await managerWithForbidden.prepareCommand(req); expect(spawnAsync).toHaveBeenCalledWith('icacls', [ - path.resolve(forbiddenPath), + forbiddenPath, '/deny', '*S-1-16-4096:(OI)(CI)(F)', ]); @@ -450,10 +474,7 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { }); it('should override allowed paths if a path is also in forbidden paths', async () => { - const conflictPath = path.join(os.tmpdir(), 'gemini-cli-test-conflict'); - if (!fs.existsSync(conflictPath)) { - fs.mkdirSync(conflictPath); - } + const conflictPath = createTempDir('conflict'); try { const managerWithForbidden = new WindowsSandboxManager({ workspace: testCwd, @@ -478,14 +499,14 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { call[1] && call[1].includes('/setintegritylevel') && call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), + call[1][0] === conflictPath, ); const denyCallIndex = spawnMock.mock.calls.findIndex( (call) => call[1] && call[1].includes('/deny') && call[0] === 'icacls' && - call[1][0] === path.resolve(conflictPath), + call[1][0] === conflictPath, ); // Conflict should have been filtered out of allow calls @@ -513,8 +534,8 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { expect(result.args[5]).toBe(filePath); }); - it('should safely handle special characters in __write path', async () => { - const maliciousPath = path.join(testCwd, 'foo"; echo bar; ".txt'); + it('should safely handle special characters in __write path using environment variables', async () => { + const maliciousPath = path.join(testCwd, 'foo & echo bar; ! .txt'); fs.writeFileSync(maliciousPath, ''); const req: SandboxRequest = { command: '__write', @@ -545,4 +566,23 @@ describe.skipIf(os.platform() === 'win32')('WindowsSandboxManager', () => { expect(result.args[4]).toBe('__read'); expect(result.args[5]).toBe(filePath); }); + + it('should return a cleanup function that deletes the temporary manifest', async () => { + const req: SandboxRequest = { + command: 'test', + args: [], + cwd: testCwd, + env: {}, + }; + + const result = await manager.prepareCommand(req); + const manifestPath = result.args[3]; + + expect(fs.existsSync(manifestPath)).toBe(true); + expect(result.cleanup).toBeDefined(); + + result.cleanup?.(); + expect(fs.existsSync(manifestPath)).toBe(false); + expect(fs.existsSync(path.dirname(manifestPath))).toBe(false); + }); }); diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index 3328c2b918..3cfb85b36a 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -16,7 +16,6 @@ import { findSecretFiles, type GlobalSandboxOptions, sanitizePaths, - tryRealpath, type SandboxPermissions, type ParsedSandboxDenial, resolveSandboxPaths, @@ -36,23 +35,28 @@ import { } from './commandSafety.js'; import { verifySandboxOverrides } from '../utils/commandUtils.js'; import { parseWindowsSandboxDenials } from './windowsSandboxDenialUtils.js'; +import { isSubpath, resolveToRealPath } from '../../utils/paths.js'; const __filename = fileURLToPath(import.meta.url); const __dirname = path.dirname(__filename); +// S-1-16-4096 is the SID for "Low Mandatory Level" (Low Integrity) +const LOW_INTEGRITY_SID = '*S-1-16-4096'; + /** * A SandboxManager implementation for Windows that uses Restricted Tokens, * Job Objects, and Low Integrity levels for process isolation. * Uses a native C# helper to bypass PowerShell restrictions. */ export class WindowsSandboxManager implements SandboxManager { + static readonly HELPER_EXE = 'GeminiSandbox.exe'; private readonly helperPath: string; private initialized = false; private readonly allowedCache = new Set(); private readonly deniedCache = new Set(); constructor(private readonly options: GlobalSandboxOptions) { - this.helperPath = path.resolve(__dirname, 'GeminiSandbox.exe'); + this.helperPath = path.resolve(__dirname, WindowsSandboxManager.HELPER_EXE); } isKnownSafeCommand(args: string[]): boolean { @@ -259,9 +263,14 @@ export class WindowsSandboxManager implements SandboxManager { this.options.modeConfig?.network ?? req.policy?.networkAccess ?? false; const networkAccess = defaultNetwork || mergedAdditional.network; - // 1. Handle filesystem permissions for Low Integrity - // Grant "Low Mandatory Level" write access to the workspace. - // If not in readonly mode OR it's a strictly approved pipeline, allow workspace writes + const { allowed: allowedPaths, forbidden: forbiddenPaths } = + await resolveSandboxPaths(this.options, req); + + // Track all roots where Low Integrity write access has been granted. + // New files created within these roots will inherit the Low label. + const writableRoots: string[] = []; + + // 1. Workspace access const isApproved = allowOverrides ? await isStrictlyApproved( command, @@ -272,20 +281,19 @@ export class WindowsSandboxManager implements SandboxManager { if (!isReadonlyMode || isApproved) { await this.grantLowIntegrityAccess(this.options.workspace); + writableRoots.push(this.options.workspace); } - const { allowed: allowedPaths, forbidden: forbiddenPaths } = - await resolveSandboxPaths(this.options, req); - - // Grant "Low Mandatory Level" access to includeDirectories. + // 2. Globally included directories const includeDirs = sanitizePaths(this.options.includeDirectories); for (const includeDir of includeDirs) { await this.grantLowIntegrityAccess(includeDir); + writableRoots.push(includeDir); } - // Grant "Low Mandatory Level" read/write access to allowedPaths. + // 3. Explicitly allowed paths from the request policy for (const allowedPath of allowedPaths) { - const resolved = await tryRealpath(allowedPath); + const resolved = resolveToRealPath(allowedPath); try { await fs.promises.access(resolved, fs.constants.F_OK); } catch { @@ -295,23 +303,32 @@ export class WindowsSandboxManager implements SandboxManager { ); } await this.grantLowIntegrityAccess(resolved); + writableRoots.push(resolved); } - // Grant "Low Mandatory Level" write access to additional permissions write paths. + // 4. Additional write paths (e.g. from internal __write command) const additionalWritePaths = sanitizePaths( mergedAdditional.fileSystem?.write, ); for (const writePath of additionalWritePaths) { - const resolved = await tryRealpath(writePath); + const resolved = resolveToRealPath(writePath); try { await fs.promises.access(resolved, fs.constants.F_OK); + await this.grantLowIntegrityAccess(resolved); + continue; } catch { - throw new Error( - `Sandbox request rejected: Additional write path does not exist: ${resolved}. ` + - 'On Windows, granular sandbox access can only be granted to existing paths to avoid broad parent directory permissions.', + // If the file doesn't exist, it's only allowed if it resides within a granted root. + const isInherited = writableRoots.some((root) => + isSubpath(root, resolved), ); + + if (!isInherited) { + throw new Error( + `Sandbox request rejected: Additional write path does not exist and its parent directory is not allowed: ${resolved}. ` + + 'On Windows, granular sandbox access can only be granted to existing paths to avoid broad parent directory permissions.', + ); + } } - await this.grantLowIntegrityAccess(resolved); } // 2. Collect secret files and apply protective ACLs @@ -382,15 +399,6 @@ export class WindowsSandboxManager implements SandboxManager { const manifestPath = path.join(tempDir, 'manifest.txt'); fs.writeFileSync(manifestPath, allForbidden.join('\n')); - // Cleanup on exit - process.on('exit', () => { - try { - fs.rmSync(tempDir, { recursive: true, force: true }); - } catch { - // Ignore errors - } - }); - // 5. Construct the helper command // GeminiSandbox.exe --forbidden-manifest [args...] const program = this.helperPath; @@ -411,6 +419,13 @@ export class WindowsSandboxManager implements SandboxManager { args: finalArgs, env: finalEnv, cwd: req.cwd, + cleanup: () => { + try { + fs.rmSync(tempDir, { recursive: true, force: true }); + } catch { + // Ignore errors + } + }, }; } @@ -422,7 +437,7 @@ export class WindowsSandboxManager implements SandboxManager { return; } - const resolvedPath = await tryRealpath(targetPath); + const resolvedPath = resolveToRealPath(targetPath); if (this.allowedCache.has(resolvedPath)) { return; } @@ -446,8 +461,12 @@ export class WindowsSandboxManager implements SandboxManager { } try { + // 1. Grant explicit Modify access to the Low Integrity SID + // 2. Set the Mandatory Label to Low to allow "Write Up" from Low processes await spawnAsync('icacls', [ resolvedPath, + '/grant', + `${LOW_INTEGRITY_SID}:(OI)(CI)(M)`, '/setintegritylevel', '(OI)(CI)Low', ]); @@ -469,7 +488,7 @@ export class WindowsSandboxManager implements SandboxManager { return; } - const resolvedPath = await tryRealpath(targetPath); + const resolvedPath = resolveToRealPath(targetPath); if (this.deniedCache.has(resolvedPath)) { return; } @@ -479,9 +498,6 @@ export class WindowsSandboxManager implements SandboxManager { return; } - // S-1-16-4096 is the SID for "Low Mandatory Level" (Low Integrity) - const LOW_INTEGRITY_SID = '*S-1-16-4096'; - // icacls flags: (OI) Object Inherit, (CI) Container Inherit, (F) Full Access Deny. // Omit /T (recursive) for performance; (OI)(CI) ensures inheritance for new items. // Windows dynamically evaluates existing items, though deep explicit Allow ACEs diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index 524726cdd4..1cd1e77269 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -28,7 +28,14 @@ const Platform = { /** Returns a command to create an empty file. */ touch(filePath: string) { return this.isWindows - ? { command: 'cmd.exe', args: ['/c', `type nul > "${filePath}"`] } + ? { + command: 'powershell.exe', + args: [ + '-NoProfile', + '-Command', + `New-Item -Path "${filePath}" -ItemType File -Force`, + ], + } : { command: 'touch', args: [filePath] }; }, @@ -48,18 +55,13 @@ const Platform = { /** Returns a command to perform a network request. */ curl(url: string) { - return this.isWindows - ? { - command: 'powershell.exe', - args: ['-Command', `Invoke-WebRequest -Uri ${url} -TimeoutSec 1`], - } - : { command: 'curl', args: ['-s', '--connect-timeout', '1', url] }; + return { command: 'curl', args: ['-s', '--connect-timeout', '1', url] }; }, /** Returns a command that checks if the current terminal is interactive. */ isPty() { return this.isWindows - ? 'cmd.exe /c echo True' + ? 'powershell.exe -NoProfile -Command "echo True"' : 'bash -c "if [ -t 1 ]; then echo True; else echo False; fi"'; }, @@ -103,8 +105,7 @@ function ensureSandboxAvailable(): boolean { if (platform === 'win32') { // Windows sandboxing relies on icacls, which is a core system utility and // always available. - // TODO: reenable once test is fixed - return false; + return true; } if (platform === 'darwin') { @@ -167,23 +168,28 @@ describe('SandboxManager Integration', () => { expect(result.stdout.trim()).toBe('sandbox test'); }); - it('supports interactive pseudo-terminals (node-pty)', async () => { - const handle = await ShellExecutionService.execute( - Platform.isPty(), - workspace, - () => {}, - new AbortController().signal, - true, - { - sanitizationConfig: getSecureSanitizationConfig(), - sandboxManager: manager, - }, - ); + // The Windows sandbox wrapper (GeminiSandbox.exe) uses standard pipes + // for I/O interception, which breaks ConPTY pseudo-terminal inheritance. + it.skipIf(Platform.isWindows)( + 'supports interactive pseudo-terminals (node-pty)', + async () => { + const handle = await ShellExecutionService.execute( + Platform.isPty(), + workspace, + () => {}, + new AbortController().signal, + true, + { + sanitizationConfig: getSecureSanitizationConfig(), + sandboxManager: manager, + }, + ); - const result = await handle.result; - expect(result.exitCode).toBe(0); - expect(result.output).toContain('True'); - }); + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(result.output).toContain('True'); + }, + ); }); describe('File System Access', () => { @@ -511,18 +517,23 @@ describe('SandboxManager Integration', () => { if (server) await new Promise((res) => server.close(() => res())); }); - it('blocks network access by default', async () => { - const { command, args } = Platform.curl(url); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); + // Windows Job Object rate limits exempt loopback (127.0.0.1) traffic, + // so this test cannot verify loopback blocking on Windows. + it.skipIf(Platform.isWindows)( + 'blocks network access by default', + async () => { + const { command, args } = Platform.curl(url); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); - const result = await runCommand(sandboxed); - expect(result.status).not.toBe(0); - }); + const result = await runCommand(sandboxed); + expect(result.status).not.toBe(0); + }, + ); it('grants network access when explicitly allowed', async () => { const { command, args } = Platform.curl(url); diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 4a3ac48f00..245b7f0eee 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -154,7 +154,11 @@ describe('ShellTool', () => { return mockSandboxManager; }, sandboxPolicyManager: { - getCommandPermissions: vi.fn().mockReturnValue(undefined), + getCommandPermissions: vi.fn().mockReturnValue({ + fileSystem: { read: [], write: [] }, + network: false, + }), + getModeConfig: vi.fn().mockReturnValue({ readonly: false }), addPersistentApproval: vi.fn(), addSessionApproval: vi.fn(), @@ -708,6 +712,39 @@ describe('ShellTool', () => { it('should throw an error if validation fails', () => { expect(() => shellTool.build({ command: '' })).toThrow(); }); + + it('should NOT return a sandbox expansion prompt for npm install when sandboxing is disabled', async () => { + const bus = (shellTool as unknown as { messageBus: MessageBus }) + .messageBus; + const mockBus = getMockMessageBusInstance( + bus, + ) as unknown as TestableMockMessageBus; + mockBus.defaultToolDecision = 'allow'; + + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(false); + const params = { command: 'npm install' }; + const invocation = shellTool.build(params); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + // Should be false because standard confirm mode is 'allow' + expect(confirmation).toBe(false); + }); + + it('should return a sandbox expansion prompt for npm install when sandboxing is enabled', async () => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + const params = { command: 'npm install' }; + const invocation = shellTool.build(params); + + const confirmation = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + expect(confirmation).not.toBe(false); + expect(confirmation && confirmation.type).toBe('sandbox_expansion'); + }); }); describe('getDescription', () => { @@ -950,6 +987,10 @@ describe('ShellTool', () => { describe('sandbox heuristics', () => { const mockAbortSignal = new AbortController().signal; + beforeEach(() => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + }); + it('should suggest proactive permissions for npm commands', async () => { const homeDir = path.join(tempRootDir, 'home'); fs.mkdirSync(homeDir); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 511d15279d..399f5f7ab6 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -10,7 +10,10 @@ import path from 'node:path'; import os from 'node:os'; import crypto from 'node:crypto'; import { debugLogger } from '../index.js'; -import type { SandboxPermissions } from '../services/sandboxManager.js'; +import { + type SandboxPermissions, + getPathIdentity, +} from '../services/sandboxManager.js'; import { ToolErrorType } from './tool-error.js'; import { BaseDeclarativeTool, @@ -44,6 +47,7 @@ import { parseCommandDetails, hasRedirection, isNakedSensitiveCommand, + normalizeCommand, } from '../utils/shell-utils.js'; import { buildParamArgsPattern } from '../policy/utils.js'; import { SHELL_TOOL_NAME } from './tool-names.js'; @@ -52,7 +56,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { getShellDefinition } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; import type { AgentLoopContext } from '../config/agent-loop-context.js'; -import { isSubpath } from '../utils/paths.js'; +import { isSubpath, resolveToRealPath } from '../utils/paths.js'; import { getProactiveToolSuggestions, isNetworkReliantCommand, @@ -260,77 +264,103 @@ export class ShellToolInvocation extends BaseToolInvocation< return this.getConfirmationDetails(abortSignal); } - // Proactively suggest expansion for known network-heavy Node.js ecosystem tools - // (npm install, etc.) to avoid hangs when network is restricted by default. - // We do this even if the command is "allowed" by policy because the DEFAULT - // permissions are usually insufficient for these commands. - const command = stripShellWrapper(this.params.command); - const rootCommands = getCommandRoots(command); - const rootCommand = rootCommands[0]; + if (this.context.config.getSandboxEnabled()) { + const command = stripShellWrapper(this.params.command); + const rootCommands = getCommandRoots(command); + const rawRootCommand = rootCommands[0]; - if (rootCommand) { - const proactive = await getProactiveToolSuggestions(rootCommand); - if (proactive) { - const approved = - this.context.config.sandboxPolicyManager.getCommandPermissions( - rootCommand, - ); - const missingNetwork = !!proactive.network && !approved?.network; - - // Detect commands or sub-commands that definitely need network - const parsed = parseCommandDetails(command); - const subCommand = parsed?.details[0]?.args?.[0]; - const needsNetwork = isNetworkReliantCommand(rootCommand, subCommand); - - if (needsNetwork) { - // Add write permission to the current directory if we are in readonly mode + if (rawRootCommand) { + const rootCommand = normalizeCommand(rawRootCommand); + const proactive = await getProactiveToolSuggestions(rootCommand); + if (proactive) { const mode = this.context.config.getApprovalMode(); - const isReadonlyMode = - this.context.config.sandboxPolicyManager.getModeConfig(mode) - ?.readonly ?? false; + const modeConfig = + this.context.config.sandboxPolicyManager.getModeConfig(mode); + const approved = + this.context.config.sandboxPolicyManager.getCommandPermissions( + rootCommand, + ); - if (isReadonlyMode) { - const cwd = - this.params.dir_path || this.context.config.getTargetDir(); - proactive.fileSystem = proactive.fileSystem || { - read: [], - write: [], - }; - proactive.fileSystem.write = proactive.fileSystem.write || []; - if (!proactive.fileSystem.write.includes(cwd)) { - proactive.fileSystem.write.push(cwd); - proactive.fileSystem.read = proactive.fileSystem.read || []; - if (!proactive.fileSystem.read.includes(cwd)) { - proactive.fileSystem.read.push(cwd); + const hasNetwork = modeConfig.network || approved.network; + const missingNetwork = !!proactive.network && !hasNetwork; + + // Detect commands or sub-commands that definitely need network + const parsed = parseCommandDetails(command); + const subCommand = parsed?.details[0]?.args?.[0]; + const needsNetwork = isNetworkReliantCommand(rootCommand, subCommand); + + if (needsNetwork) { + // Add write permission to the current directory if we are in readonly mode + const isReadonlyMode = modeConfig.readonly ?? false; + + if (isReadonlyMode) { + const cwd = + this.params.dir_path || this.context.config.getTargetDir(); + proactive.fileSystem = proactive.fileSystem || { + read: [], + write: [], + }; + proactive.fileSystem.write = proactive.fileSystem.write || []; + if (!proactive.fileSystem.write.includes(cwd)) { + proactive.fileSystem.write.push(cwd); + proactive.fileSystem.read = proactive.fileSystem.read || []; + if (!proactive.fileSystem.read.includes(cwd)) { + proactive.fileSystem.read.push(cwd); + } } } - } - const missingRead = (proactive.fileSystem?.read || []).filter( - (p) => !approved?.fileSystem?.read?.includes(p), - ); - const missingWrite = (proactive.fileSystem?.write || []).filter( - (p) => !approved?.fileSystem?.write?.includes(p), - ); + const isApproved = ( + requestedPath: string, + approvedPaths?: string[], + ): boolean => { + if (!approvedPaths || approvedPaths.length === 0) return false; + const requestedRealIdentity = getPathIdentity( + resolveToRealPath(requestedPath), + ); - const needsExpansion = - missingRead.length > 0 || missingWrite.length > 0 || missingNetwork; + // Identity check is fast, subpath check is slower + return approvedPaths.some((p) => { + const approvedRealIdentity = getPathIdentity( + resolveToRealPath(p), + ); + return ( + requestedRealIdentity === approvedRealIdentity || + isSubpath(approvedRealIdentity, requestedRealIdentity) + ); + }); + }; - if (needsExpansion) { - const details = await this.getConfirmationDetails( - abortSignal, - proactive, + const missingRead = (proactive.fileSystem?.read || []).filter( + (p) => !isApproved(p, approved.fileSystem?.read), ); - if (details && details.type === 'sandbox_expansion') { - const originalOnConfirm = details.onConfirm; - details.onConfirm = async (outcome: ToolConfirmationOutcome) => { - await originalOnConfirm(outcome); - if (outcome !== ToolConfirmationOutcome.Cancel) { - this.proactivePermissionsConfirmed = proactive; - } - }; + const missingWrite = (proactive.fileSystem?.write || []).filter( + (p) => !isApproved(p, approved.fileSystem?.write), + ); + + const needsExpansion = + missingRead.length > 0 || + missingWrite.length > 0 || + missingNetwork; + + if (needsExpansion) { + const details = await this.getConfirmationDetails( + abortSignal, + proactive, + ); + if (details && details.type === 'sandbox_expansion') { + const originalOnConfirm = details.onConfirm; + details.onConfirm = async ( + outcome: ToolConfirmationOutcome, + ) => { + await originalOnConfirm(outcome); + if (outcome !== ToolConfirmationOutcome.Cancel) { + this.proactivePermissionsConfirmed = proactive; + } + }; + } + return details; } - return details; } } } @@ -755,20 +785,22 @@ export class ShellToolInvocation extends BaseToolInvocation< ); // Proactive permission suggestions for Node ecosystem tools - const proactive = - await getProactiveToolSuggestions(rootCommandDisplay); - if (proactive) { - if (proactive.network) { - sandboxDenial.network = true; - } - if (proactive.fileSystem?.read) { - for (const p of proactive.fileSystem.read) { - readPaths.add(p); + if (this.context.config.getSandboxEnabled()) { + const proactive = + await getProactiveToolSuggestions(rootCommandDisplay); + if (proactive) { + if (proactive.network) { + sandboxDenial.network = true; } - } - if (proactive.fileSystem?.write) { - for (const p of proactive.fileSystem.write) { - writePaths.add(p); + if (proactive.fileSystem?.read) { + for (const p of proactive.fileSystem.read) { + readPaths.add(p); + } + } + if (proactive.fileSystem?.write) { + for (const p of proactive.fileSystem.write) { + writePaths.add(p); + } } } } diff --git a/packages/core/src/tools/shell_proactive.test.ts b/packages/core/src/tools/shell_proactive.test.ts new file mode 100644 index 0000000000..c2327789de --- /dev/null +++ b/packages/core/src/tools/shell_proactive.test.ts @@ -0,0 +1,180 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + vi, + describe, + it, + expect, + beforeEach, + beforeAll, + afterEach, +} from 'vitest'; +import os from 'node:os'; +import type _fs from 'node:fs'; +import { ShellTool } from './shell.js'; +import { type Config } from '../config/config.js'; +import { createMockMessageBus } from '../test-utils/mock-message-bus.js'; +import * as proactivePermissions from '../sandbox/utils/proactivePermissions.js'; + +import { initializeShellParsers } from '../utils/shell-utils.js'; + +vi.mock('node:fs', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + default: { + ...original, + realpathSync: vi.fn((p) => p), + }, + realpathSync: vi.fn((p) => p), + }; +}); + +vi.mock('../sandbox/utils/proactivePermissions.js', () => ({ + getProactiveToolSuggestions: vi.fn(), + isNetworkReliantCommand: vi.fn(), +})); + +const mockPlatform = (platform: string) => { + vi.stubGlobal( + 'process', + Object.create(process, { + platform: { + get: () => platform, + }, + }), + ); + vi.spyOn(os, 'platform').mockReturnValue(platform as NodeJS.Platform); +}; + +describe('ShellTool Proactive Expansion', () => { + let mockConfig: Config; + let shellTool: ShellTool; + + beforeAll(async () => { + await initializeShellParsers(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); + }); + + beforeEach(() => { + vi.clearAllMocks(); + mockPlatform('darwin'); + + mockConfig = { + get config() { + return this; + }, + getSandboxEnabled: vi.fn().mockReturnValue(false), + getTargetDir: vi.fn().mockReturnValue('/tmp'), + getApprovalMode: vi.fn().mockReturnValue('strict'), + sandboxPolicyManager: { + getCommandPermissions: vi.fn().mockReturnValue({ + fileSystem: { read: [], write: [] }, + network: false, + }), + getModeConfig: vi.fn().mockReturnValue({ readonly: false }), + }, + getEnableInteractiveShell: vi.fn().mockReturnValue(false), + getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true), + getShellToolInactivityTimeout: vi.fn().mockReturnValue(1000), + } as unknown as Config; + + const bus = createMockMessageBus(); + shellTool = new ShellTool(mockConfig, bus); + }); + + it('should NOT call getProactiveToolSuggestions when sandboxing is disabled', async () => { + const invocation = shellTool.build({ command: 'npm install' }); + const abortSignal = new AbortController().signal; + + await invocation.shouldConfirmExecute(abortSignal); + + expect( + proactivePermissions.getProactiveToolSuggestions, + ).not.toHaveBeenCalled(); + }); + + it('should call getProactiveToolSuggestions when sandboxing is enabled', async () => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + vi.mocked( + proactivePermissions.getProactiveToolSuggestions, + ).mockResolvedValue({ + network: true, + }); + vi.mocked(proactivePermissions.isNetworkReliantCommand).mockReturnValue( + true, + ); + + const invocation = shellTool.build({ command: 'npm install' }); + const abortSignal = new AbortController().signal; + + await invocation.shouldConfirmExecute(abortSignal); + + expect( + proactivePermissions.getProactiveToolSuggestions, + ).toHaveBeenCalledWith('npm'); + }); + + it('should normalize command names (lowercase and strip .exe) when sandboxing is enabled', async () => { + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + vi.mocked( + proactivePermissions.getProactiveToolSuggestions, + ).mockResolvedValue({ + network: true, + }); + vi.mocked(proactivePermissions.isNetworkReliantCommand).mockReturnValue( + true, + ); + + const invocation = shellTool.build({ command: 'NPM.EXE install' }); + const abortSignal = new AbortController().signal; + + await invocation.shouldConfirmExecute(abortSignal); + + expect( + proactivePermissions.getProactiveToolSuggestions, + ).toHaveBeenCalledWith('npm'); + }); + + it('should NOT request expansion if paths are already approved (case-insensitive subpath)', async () => { + // This test assumes Darwin or Windows for case-insensitivity + vi.mocked(mockConfig.getSandboxEnabled).mockReturnValue(true); + vi.mocked( + proactivePermissions.getProactiveToolSuggestions, + ).mockResolvedValue({ + fileSystem: { read: ['/project/src'], write: [] }, + }); + vi.mocked(proactivePermissions.isNetworkReliantCommand).mockReturnValue( + true, + ); + + // Current approval is for the parent dir, with different casing + vi.mocked( + mockConfig.sandboxPolicyManager.getCommandPermissions, + ).mockReturnValue({ + fileSystem: { read: ['/PROJECT'], write: [] }, + network: false, + }); + + const invocation = shellTool.build({ command: 'npm install' }); + const result = await invocation.shouldConfirmExecute( + new AbortController().signal, + ); + + // If it's correctly approved, result should be false (no expansion needed) + // or a normal 'exec' confirmation, but NOT 'sandbox_expansion'. + if (result) { + expect(result.type).not.toBe('sandbox_expansion'); + } else { + expect(result).toBe(false); + } + }); +}); diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index 590f3aab58..1a4266834d 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -15,6 +15,7 @@ import { shortenPath, normalizePath, resolveToRealPath, + makeRelative, } from './paths.js'; vi.mock('node:fs', async (importOriginal) => { @@ -215,7 +216,7 @@ describe('isSubpath', () => { }); }); -describe('isSubpath on Windows', () => { +describe.skipIf(process.platform !== 'win32')('isSubpath on Windows', () => { afterEach(() => vi.unstubAllGlobals()); beforeEach(() => mockPlatform('win32')); @@ -268,6 +269,20 @@ describe('isSubpath on Windows', () => { }); }); +describe.skipIf(process.platform !== 'darwin')('isSubpath on Darwin', () => { + afterEach(() => vi.unstubAllGlobals()); + + beforeEach(() => mockPlatform('darwin')); + + it('should be case-insensitive for path components on Darwin', () => { + expect(isSubpath('/PROJECT', '/project/src')).toBe(true); + }); + + it('should return true for a direct subpath on Darwin', () => { + expect(isSubpath('/Users/Test', '/Users/Test/file.txt')).toBe(true); + }); +}); + describe('shortenPath', () => { describe.skipIf(process.platform === 'win32')('on POSIX', () => { it('should not shorten a path that is shorter than maxLen', () => { @@ -586,6 +601,54 @@ describe('resolveToRealPath', () => { }); }); +describe('makeRelative', () => { + describe.skipIf(process.platform === 'win32')('on POSIX', () => { + it('should return relative path if targetPath is already relative', () => { + expect(makeRelative('foo/bar', '/root')).toBe('foo/bar'); + }); + + it('should return relative path from root to target', () => { + const root = '/Users/test/project'; + const target = '/Users/test/project/src/file.ts'; + expect(makeRelative(target, root)).toBe('src/file.ts'); + }); + + it('should return "." if target and root are the same', () => { + const root = '/Users/test/project'; + expect(makeRelative(root, root)).toBe('.'); + }); + + it('should handle parent directories with ..', () => { + const root = '/Users/test/project/src'; + const target = '/Users/test/project/docs/readme.md'; + expect(makeRelative(target, root)).toBe('../docs/readme.md'); + }); + }); + + describe.skipIf(process.platform !== 'win32')('on Windows', () => { + it('should return relative path if targetPath is already relative', () => { + expect(makeRelative('foo/bar', 'C:\\root')).toBe('foo/bar'); + }); + + it('should return relative path from root to target', () => { + const root = 'C:\\Users\\test\\project'; + const target = 'C:\\Users\\test\\project\\src\\file.ts'; + expect(makeRelative(target, root)).toBe('src\\file.ts'); + }); + + it('should return "." if target and root are the same', () => { + const root = 'C:\\Users\\test\\project'; + expect(makeRelative(root, root)).toBe('.'); + }); + + it('should handle parent directories with ..', () => { + const root = 'C:\\Users\\test\\project\\src'; + const target = 'C:\\Users\\test\\project\\docs\\readme.md'; + expect(makeRelative(target, root)).toBe('..\\docs\\readme.md'); + }); + }); +}); + describe('normalizePath', () => { it('should resolve a relative path to an absolute path', () => { const result = normalizePath('some/relative/path'); @@ -615,7 +678,19 @@ describe('normalizePath', () => { }); }); - describe.skipIf(process.platform === 'win32')('on POSIX', () => { + describe.skipIf(process.platform !== 'darwin')('on Darwin', () => { + beforeEach(() => mockPlatform('darwin')); + afterEach(() => vi.unstubAllGlobals()); + + it('should lowercase the entire path', () => { + const result = normalizePath('/Users/TEST'); + expect(result).toBe('/users/test'); + }); + }); + + describe.skipIf( + process.platform === 'win32' || process.platform === 'darwin', + )('on Linux', () => { it('should preserve case', () => { const result = normalizePath('/usr/Local/Bin'); expect(result).toContain('Local'); diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index 312bacd7ea..135e047530 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -325,9 +325,14 @@ export function getProjectHash(projectRoot: string): string { * - On Windows, converts to lowercase for case-insensitivity. */ export function normalizePath(p: string): string { - const resolved = path.resolve(p); + const platform = process.platform; + const isWindows = platform === 'win32'; + const pathModule = isWindows ? path.win32 : path; + + const resolved = pathModule.resolve(p); const normalized = resolved.replace(/\\/g, '/'); - return process.platform === 'win32' ? normalized.toLowerCase() : normalized; + const isCaseInsensitive = isWindows || platform === 'darwin'; + return isCaseInsensitive ? normalized.toLowerCase() : normalized; } /** @@ -337,11 +342,25 @@ export function normalizePath(p: string): string { * @returns True if childPath is a subpath of parentPath, false otherwise. */ export function isSubpath(parentPath: string, childPath: string): boolean { - const isWindows = process.platform === 'win32'; + const platform = process.platform; + const isWindows = platform === 'win32'; + const isDarwin = platform === 'darwin'; const pathModule = isWindows ? path.win32 : path; - // On Windows, path.relative is case-insensitive. On POSIX, it's case-sensitive. - const relative = pathModule.relative(parentPath, childPath); + // Resolve both paths to absolute to ensure consistent comparison, + // especially when mixing relative and absolute paths or when casing differs. + let p = pathModule.resolve(parentPath); + let c = pathModule.resolve(childPath); + + // On Windows, path.relative is case-insensitive. + // On POSIX (including Darwin), path.relative is case-sensitive. + // We want it to be case-insensitive on Darwin to match user expectation and sandbox policy. + if (isDarwin) { + p = p.toLowerCase(); + c = c.toLowerCase(); + } + + const relative = pathModule.relative(p, c); return ( !relative.startsWith(`..${pathModule.sep}`) && diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index 2370aa25c4..0dda7c4881 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -21,6 +21,7 @@ import { parseCommandDetails, splitCommands, stripShellWrapper, + normalizeCommand, hasRedirection, resolveExecutable, } from './shell-utils.js'; @@ -115,6 +116,23 @@ const mockPowerShellResult = ( }); }; +describe('normalizeCommand', () => { + it('should lowercase the command', () => { + expect(normalizeCommand('NPM')).toBe('npm'); + }); + + it('should remove .exe extension', () => { + expect(normalizeCommand('node.exe')).toBe('node'); + }); + + it('should handle absolute paths', () => { + expect(normalizeCommand('/usr/bin/npm')).toBe('npm'); + expect(normalizeCommand('C:\\Program Files\\nodejs\\node.exe')).toBe( + 'node', + ); + }); +}); + describe('getCommandRoots', () => { it('should return a single command', () => { expect(getCommandRoots('ls -l')).toEqual(['ls']); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index bcc6dd795c..b3985db0e7 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -310,6 +310,20 @@ function normalizeCommandName(raw: string): string { return raw.trim(); } +/** + * Normalizes a command name for sandbox policy lookups. + * Converts to lowercase and removes the .exe extension for cross-platform consistency. + * + * @param commandName - The command name to normalize. + * @returns The normalized command name. + */ +export function normalizeCommand(commandName: string): string { + // Split by both separators and get the last non-empty part + const parts = commandName.split(/[\\/]/).filter(Boolean); + const base = parts.length > 0 ? parts[parts.length - 1] : ''; + return base.toLowerCase().replace(/\.exe$/, ''); +} + function extractNameFromNode(node: Node): string | null { switch (node.type) { case 'command': {