diff --git a/packages/cli/src/test-utils/mockConfig.ts b/packages/cli/src/test-utils/mockConfig.ts index e970fdb726..30031a0599 100644 --- a/packages/cli/src/test-utils/mockConfig.ts +++ b/packages/cli/src/test-utils/mockConfig.ts @@ -152,6 +152,7 @@ export const createMockConfig = (overrides: Partial = {}): Config => getBlockedMcpServers: vi.fn().mockReturnValue([]), getExperiments: vi.fn().mockReturnValue(undefined), getHasAccessToPreviewModel: vi.fn().mockReturnValue(false), + validatePathAccess: vi.fn().mockReturnValue(null), ...overrides, }) as unknown as Config; diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 87888265aa..1cddd7c094 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -145,6 +145,7 @@ vi.mock('./contexts/SessionContext.js'); vi.mock('./components/shared/text-buffer.js'); vi.mock('./hooks/useLogger.js'); vi.mock('./hooks/useInputHistoryStore.js'); +vi.mock('./hooks/atCommandProcessor.js'); vi.mock('./hooks/useHookDisplayState.js'); vi.mock('./hooks/useTerminalTheme.js', () => ({ useTerminalTheme: vi.fn(), @@ -2734,4 +2735,67 @@ describe('AppContainer State Management', () => { compUnmount(); }); }); + + describe('Permission Handling', () => { + it('shows permission dialog when checkPermissions returns paths', async () => { + const { checkPermissions } = await import( + './hooks/atCommandProcessor.js' + ); + vi.mocked(checkPermissions).mockResolvedValue(['/test/file.txt']); + + let unmount: () => void; + await act(async () => (unmount = renderAppContainer().unmount)); + + await waitFor(() => expect(capturedUIActions).toBeTruthy()); + + await act(async () => + capturedUIActions.handleFinalSubmit('read @file.txt'), + ); + + expect(capturedUIState.permissionConfirmationRequest).not.toBeNull(); + expect(capturedUIState.permissionConfirmationRequest?.files).toEqual([ + '/test/file.txt', + ]); + await act(async () => unmount!()); + }); + + it.each([true, false])( + 'handles permissions when allowed is %s', + async (allowed) => { + const { checkPermissions } = await import( + './hooks/atCommandProcessor.js' + ); + vi.mocked(checkPermissions).mockResolvedValue(['/test/file.txt']); + const addReadOnlyPathSpy = vi.spyOn( + mockConfig.getWorkspaceContext(), + 'addReadOnlyPath', + ); + const { submitQuery } = mockedUseGeminiStream(); + + let unmount: () => void; + await act(async () => (unmount = renderAppContainer().unmount)); + + await waitFor(() => expect(capturedUIActions).toBeTruthy()); + + await act(async () => + capturedUIActions.handleFinalSubmit('read @file.txt'), + ); + + await act(async () => + capturedUIState.permissionConfirmationRequest?.onComplete({ + allowed, + }), + ); + + if (allowed) { + expect(addReadOnlyPathSpy).toHaveBeenCalledWith('/test/file.txt'); + } else { + expect(addReadOnlyPathSpy).not.toHaveBeenCalled(); + } + expect(submitQuery).toHaveBeenCalledWith('read @file.txt'); + expect(capturedUIState.permissionConfirmationRequest).toBeNull(); + await act(async () => unmount!()); + }, + ); + }); }); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 84b51e5f2d..c228bd43ea 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -28,7 +28,9 @@ import { type HistoryItemToolGroup, AuthState, type ConfirmationRequest, + type PermissionConfirmationRequest, } from './types.js'; +import { checkPermissions } from './hooks/atCommandProcessor.js'; import { MessageType, StreamingState } from './types.js'; import { ToolActionsProvider } from './contexts/ToolActionsContext.js'; import { @@ -844,6 +846,8 @@ Logging in with Google... Restarting Gemini CLI to continue. const [authConsentRequest, setAuthConsentRequest] = useState(null); + const [permissionConfirmationRequest, setPermissionConfirmationRequest] = + useState(null); useEffect(() => { const handleConsentRequest = (payload: ConsentRequestPayload) => { @@ -1078,11 +1082,30 @@ Logging in with Google... Restarting Gemini CLI to continue. ); const handleFinalSubmit = useCallback( - (submittedValue: string) => { + async (submittedValue: string) => { const isSlash = isSlashCommand(submittedValue.trim()); const isIdle = streamingState === StreamingState.Idle; if (isSlash || (isIdle && isMcpReady)) { + if (!isSlash) { + const permissions = await checkPermissions(submittedValue, config); + if (permissions.length > 0) { + setPermissionConfirmationRequest({ + files: permissions, + onComplete: (result) => { + setPermissionConfirmationRequest(null); + if (result.allowed) { + permissions.forEach((p) => + config.getWorkspaceContext().addReadOnlyPath(p), + ); + } + void submitQuery(submittedValue); + }, + }); + addInput(submittedValue); + return; + } + } void submitQuery(submittedValue); } else { // Check messageQueue.length === 0 to only notify on the first queued item @@ -1103,6 +1126,7 @@ Logging in with Google... Restarting Gemini CLI to continue. isMcpReady, streamingState, messageQueue.length, + config, ], ); @@ -1221,7 +1245,7 @@ Logging in with Google... Restarting Gemini CLI to continue. !showPrivacyNotice && geminiClient?.isInitialized?.() ) { - handleFinalSubmit(initialPrompt); + void handleFinalSubmit(initialPrompt); initialPromptSubmitted.current = true; } }, [ @@ -1714,6 +1738,7 @@ Logging in with Google... Restarting Gemini CLI to continue. adminSettingsChanged || !!commandConfirmationRequest || !!authConsentRequest || + !!permissionConfirmationRequest || !!customDialog || confirmUpdateExtensionRequests.length > 0 || !!loopDetectionConfirmationRequest || @@ -1819,6 +1844,7 @@ Logging in with Google... Restarting Gemini CLI to continue. authConsentRequest, confirmUpdateExtensionRequests, loopDetectionConfirmationRequest, + permissionConfirmationRequest, geminiMdFileCount, streamingState, initError, @@ -1925,6 +1951,7 @@ Logging in with Google... Restarting Gemini CLI to continue. authConsentRequest, confirmUpdateExtensionRequests, loopDetectionConfirmationRequest, + permissionConfirmationRequest, geminiMdFileCount, streamingState, initError, diff --git a/packages/cli/src/ui/components/DialogManager.tsx b/packages/cli/src/ui/components/DialogManager.tsx index 6d4db7ca3b..a502a39030 100644 --- a/packages/cli/src/ui/components/DialogManager.tsx +++ b/packages/cli/src/ui/components/DialogManager.tsx @@ -117,6 +117,20 @@ export const DialogManager = ({ ); } + if (uiState.permissionConfirmationRequest) { + const files = uiState.permissionConfirmationRequest.files; + const filesList = files.map((f) => `- ${f}`).join('\n'); + return ( + { + uiState.permissionConfirmationRequest?.onComplete({ allowed }); + }} + terminalWidth={terminalWidth} + /> + ); + } + // commandConfirmationRequest and authConsentRequest are kept separate // to avoid focus deadlocks and state race conditions between the // synchronous command loop and the asynchronous auth flow. diff --git a/packages/cli/src/ui/contexts/UIActionsContext.tsx b/packages/cli/src/ui/contexts/UIActionsContext.tsx index a0dd1b3152..4c42998d16 100644 --- a/packages/cli/src/ui/contexts/UIActionsContext.tsx +++ b/packages/cli/src/ui/contexts/UIActionsContext.tsx @@ -52,7 +52,7 @@ export interface UIActions { setConstrainHeight: (value: boolean) => void; onEscapePromptChange: (show: boolean) => void; refreshStatic: () => void; - handleFinalSubmit: (value: string) => void; + handleFinalSubmit: (value: string) => Promise; handleClearScreen: () => void; handleProQuotaChoice: ( choice: 'retry_later' | 'retry_once' | 'retry_always' | 'upgrade', diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index 45111a29cc..1459424835 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -14,6 +14,7 @@ import type { HistoryItemWithoutId, StreamingState, ActiveHook, + PermissionConfirmationRequest, } from '../types.js'; import type { CommandContext, SlashCommand } from '../commands/types.js'; import type { TextBuffer } from '../components/shared/text-buffer.js'; @@ -85,6 +86,7 @@ export interface UIState { authConsentRequest: ConfirmationRequest | null; confirmUpdateExtensionRequests: ConfirmationRequest[]; loopDetectionConfirmationRequest: LoopDetectionConfirmationRequest | null; + permissionConfirmationRequest: PermissionConfirmationRequest | null; geminiMdFileCount: number; streamingState: StreamingState; initError: string | null; diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index b3a53c9b7e..999182e8c8 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -1188,40 +1188,6 @@ describe('handleAtCommand', () => { expect.stringContaining(`using glob: ${path.join(subDirPath, '**')}`), ); }); - - it('should skip absolute paths outside workspace', async () => { - const outsidePath = '/tmp/outside-workspace.txt'; - const query = `Check @${outsidePath} please.`; - - const mockWorkspaceContext = { - isPathWithinWorkspace: vi.fn((path: string) => - path.startsWith(testRootDir), - ), - getDirectories: () => [testRootDir], - addDirectory: vi.fn(), - getInitialDirectories: () => [testRootDir], - setDirectories: vi.fn(), - onDirectoriesChanged: vi.fn(() => () => {}), - } as unknown as ReturnType; - mockConfig.getWorkspaceContext = () => mockWorkspaceContext; - - const result = await handleAtCommand({ - query, - config: mockConfig, - addItem: mockAddItem, - onDebugMessage: mockOnDebugMessage, - messageId: 502, - signal: abortController.signal, - }); - - expect(result).toEqual({ - processedQuery: [{ text: `Check @${outsidePath} please.` }], - }); - - expect(mockOnDebugMessage).toHaveBeenCalledWith( - `Path ${outsidePath} is not in the workspace and will be skipped.`, - ); - }); }); it("should not add the user's turn to history, as that is the caller's responsibility", async () => { diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index a316e5df36..28bbef074c 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -13,6 +13,8 @@ import { getErrorMessage, isNodeError, unescapePath, + resolveToRealPath, + fileExists, ReadManyFilesTool, REFERENCE_CONTENT_START, REFERENCE_CONTENT_END, @@ -152,6 +154,35 @@ function categorizeAtCommands( return { agentParts, resourceParts, fileParts }; } +/** + * Checks if the query contains any file paths that require read permission. + * Returns an array of such paths. + */ +export async function checkPermissions( + query: string, + config: Config, +): Promise { + const commandParts = parseAllAtCommands(query); + const { fileParts } = categorizeAtCommands(commandParts, config); + const permissionsRequired: string[] = []; + + for (const part of fileParts) { + const pathName = part.content.substring(1); + if (!pathName) continue; + + const resolvedPathName = resolveToRealPath( + path.resolve(config.getTargetDir(), pathName), + ); + + if (config.validatePathAccess(resolvedPathName, 'read')) { + if (await fileExists(resolvedPathName)) { + permissionsRequired.push(resolvedPathName); + } + } + } + return permissionsRequired; +} + interface ResolvedFile { part: AtCommandPart; pathSpec: string; @@ -189,17 +220,6 @@ async function resolveFilePaths( continue; } - const resolvedPathName = path.isAbsolute(pathName) - ? pathName - : path.resolve(config.getTargetDir(), pathName); - - if (!config.isPathAllowed(resolvedPathName)) { - onDebugMessage( - `Path ${pathName} is not in the workspace and will be skipped.`, - ); - continue; - } - const gitIgnored = respectFileIgnore.respectGitIgnore && fileDiscovery.shouldIgnoreFile(pathName, { @@ -229,9 +249,7 @@ async function resolveFilePaths( for (const dir of config.getWorkspaceContext().getDirectories()) { try { - const absolutePath = path.isAbsolute(pathName) - ? pathName - : path.resolve(dir, pathName); + const absolutePath = path.resolve(dir, pathName); const stats = await fs.stat(absolutePath); const relativePath = path.isAbsolute(pathName) diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index aa00b800a5..08452c98f5 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -451,6 +451,11 @@ export interface LoopDetectionConfirmationRequest { onComplete: (result: { userSelection: 'disable' | 'keep' }) => void; } +export interface PermissionConfirmationRequest { + files: string[]; + onComplete: (result: { allowed: boolean }) => void; +} + export interface ActiveHook { name: string; eventName: string; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 92e20f9163..8ee7c1c1a5 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -1880,9 +1880,22 @@ export class Config { * Validates if a path is allowed and returns a detailed error message if not. * * @param absolutePath The absolute path to validate. + * @param checkType The type of access to check ('read' or 'write'). Defaults to 'write' for safety. * @returns An error message string if the path is disallowed, null otherwise. */ - validatePathAccess(absolutePath: string): string | null { + validatePathAccess( + absolutePath: string, + checkType: 'read' | 'write' = 'write', + ): string | null { + // For read operations, check read-only paths first + if (checkType === 'read') { + if (this.getWorkspaceContext().isPathReadable(absolutePath)) { + return null; + } + } + + // Then check standard allowed paths (Workspace + Temp) + // This covers 'write' checks and acts as a fallback/temp-dir check for 'read' if (this.isPathAllowed(absolutePath)) { return null; } diff --git a/packages/core/src/tools/glob.ts b/packages/core/src/tools/glob.ts index 23c38871f7..a734d76794 100644 --- a/packages/core/src/tools/glob.ts +++ b/packages/core/src/tools/glob.ts @@ -123,8 +123,10 @@ class GlobToolInvocation extends BaseToolInvocation< this.config.getTargetDir(), this.params.dir_path, ); - const validationError = - this.config.validatePathAccess(searchDirAbsolute); + const validationError = this.config.validatePathAccess( + searchDirAbsolute, + 'read', + ); if (validationError) { return { llmContent: validationError, @@ -318,7 +320,10 @@ export class GlobTool extends BaseDeclarativeTool { params.dir_path || '.', ); - const validationError = this.config.validatePathAccess(searchDirAbsolute); + const validationError = this.config.validatePathAccess( + searchDirAbsolute, + 'read', + ); if (validationError) { return validationError; } diff --git a/packages/core/src/tools/grep.ts b/packages/core/src/tools/grep.ts index 06278910bb..c47d65c37b 100644 --- a/packages/core/src/tools/grep.ts +++ b/packages/core/src/tools/grep.ts @@ -123,7 +123,10 @@ class GrepToolInvocation extends BaseToolInvocation< let searchDirAbs: string | null = null; if (pathParam) { searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam); - const validationError = this.config.validatePathAccess(searchDirAbs); + const validationError = this.config.validatePathAccess( + searchDirAbs, + 'read', + ); if (validationError) { return { llmContent: validationError, @@ -623,7 +626,10 @@ export class GrepTool extends BaseDeclarativeTool { this.config.getTargetDir(), params.dir_path, ); - const validationError = this.config.validatePathAccess(resolvedPath); + const validationError = this.config.validatePathAccess( + resolvedPath, + 'read', + ); if (validationError) { return validationError; } diff --git a/packages/core/src/tools/ls.ts b/packages/core/src/tools/ls.ts index 6241d28793..a264f5cf54 100644 --- a/packages/core/src/tools/ls.ts +++ b/packages/core/src/tools/ls.ts @@ -143,7 +143,10 @@ class LSToolInvocation extends BaseToolInvocation { this.params.dir_path, ); - const validationError = this.config.validatePathAccess(resolvedDirPath); + const validationError = this.config.validatePathAccess( + resolvedDirPath, + 'read', + ); if (validationError) { return { llmContent: validationError, @@ -331,7 +334,7 @@ export class LSTool extends BaseDeclarativeTool { this.config.getTargetDir(), params.dir_path, ); - return this.config.validatePathAccess(resolvedPath); + return this.config.validatePathAccess(resolvedPath, 'read'); } protected createInvocation( diff --git a/packages/core/src/tools/read-file.ts b/packages/core/src/tools/read-file.ts index 2fa5772187..b71f5c8e29 100644 --- a/packages/core/src/tools/read-file.ts +++ b/packages/core/src/tools/read-file.ts @@ -76,7 +76,10 @@ class ReadFileToolInvocation extends BaseToolInvocation< } async execute(): Promise { - const validationError = this.config.validatePathAccess(this.resolvedPath); + const validationError = this.config.validatePathAccess( + this.resolvedPath, + 'read', + ); if (validationError) { return { llmContent: validationError, @@ -213,7 +216,10 @@ export class ReadFileTool extends BaseDeclarativeTool< params.file_path, ); - const validationError = this.config.validatePathAccess(resolvedPath); + const validationError = this.config.validatePathAccess( + resolvedPath, + 'read', + ); if (validationError) { return validationError; } diff --git a/packages/core/src/tools/read-many-files.ts b/packages/core/src/tools/read-many-files.ts index ab90e86a90..89919dc2cb 100644 --- a/packages/core/src/tools/read-many-files.ts +++ b/packages/core/src/tools/read-many-files.ts @@ -221,7 +221,10 @@ ${finalExclusionPatternsForDescription const fullPath = path.resolve(this.config.getTargetDir(), relativePath); - const validationError = this.config.validatePathAccess(fullPath); + const validationError = this.config.validatePathAccess( + fullPath, + 'read', + ); if (validationError) { skippedFiles.push({ path: fullPath, diff --git a/packages/core/src/tools/ripGrep.ts b/packages/core/src/tools/ripGrep.ts index 892960fa94..68fa8cfb20 100644 --- a/packages/core/src/tools/ripGrep.ts +++ b/packages/core/src/tools/ripGrep.ts @@ -164,7 +164,10 @@ class GrepToolInvocation extends BaseToolInvocation< const pathParam = this.params.dir_path || '.'; const searchDirAbs = path.resolve(this.config.getTargetDir(), pathParam); - const validationError = this.config.validatePathAccess(searchDirAbs); + const validationError = this.config.validatePathAccess( + searchDirAbs, + 'read', + ); if (validationError) { return { llmContent: validationError, @@ -582,7 +585,10 @@ export class RipGrepTool extends BaseDeclarativeTool< this.config.getTargetDir(), params.dir_path, ); - const validationError = this.config.validatePathAccess(resolvedPath); + const validationError = this.config.validatePathAccess( + resolvedPath, + 'read', + ); if (validationError) { return validationError; } diff --git a/packages/core/src/utils/workspaceContext.ts b/packages/core/src/utils/workspaceContext.ts index ff912083fb..dfb47ce3be 100755 --- a/packages/core/src/utils/workspaceContext.ts +++ b/packages/core/src/utils/workspaceContext.ts @@ -24,6 +24,7 @@ export interface AddDirectoriesResult { export class WorkspaceContext { private directories = new Set(); private initialDirectories: Set; + private readOnlyPaths = new Set(); private onDirectoriesChangedListeners = new Set<() => void>(); /** @@ -113,6 +114,24 @@ export class WorkspaceContext { return result; } + /** + * Adds a path to the read-only list. + * These paths are allowed for reading but not for writing (unless they are also in the workspace). + */ + addReadOnlyPath(pathToAdd: string): void { + try { + // Check if it exists + if (!fs.existsSync(pathToAdd)) { + return; + } + // Resolve symlinks + const resolved = fs.realpathSync(path.resolve(this.targetDir, pathToAdd)); + this.readOnlyPaths.add(resolved); + } catch (e) { + debugLogger.warn(`Failed to add read-only path ${pathToAdd}:`, e); + } + } + private resolveAndValidateDir(directory: string): string { const absolutePath = path.resolve(this.targetDir, directory); @@ -174,6 +193,34 @@ export class WorkspaceContext { } } + /** + * Checks if a path is allowed to be read. + * This includes workspace paths and explicitly added read-only paths. + * @param pathToCheck The path to validate + * @returns True if the path is readable, false otherwise + */ + isPathReadable(pathToCheck: string): boolean { + if (this.isPathWithinWorkspace(pathToCheck)) { + return true; + } + try { + const fullyResolvedPath = this.fullyResolvedPath(pathToCheck); + + for (const allowedPath of this.readOnlyPaths) { + // Allow exact matches or subpaths (if allowedPath is a directory) + if ( + fullyResolvedPath === allowedPath || + this.isPathWithinRoot(fullyResolvedPath, allowedPath) + ) { + return true; + } + } + return false; + } catch (_error) { + return false; + } + } + /** * Fully resolves a path, including symbolic links. * If the path does not exist, it returns the fully resolved path as it would be