diff --git a/packages/cli/src/acp/acpSession.ts b/packages/cli/src/acp/acpSession.ts index d3c0aa3c9b..d20b15f6d6 100644 --- a/packages/cli/src/acp/acpSession.ts +++ b/packages/cli/src/acp/acpSession.ts @@ -49,6 +49,7 @@ import { buildAvailableModes, RequestPermissionResponseSchema, } from './acpUtils.js'; +import { resolveAtCommandPath } from '../utils/atCommandUtils.js'; import { z } from 'zod'; import { getAcpErrorMessage } from './acpErrors.js'; @@ -928,13 +929,24 @@ export class Session { let currentPathSpec = pathName; let resolvedSuccessfully = false; let readDirectly = false; - try { - const absolutePath = path.resolve( + + const resolved = await resolveAtCommandPath( + pathName, + this.context.config, + (msg) => this.debug(msg), + ); + + let validationError: string | null = null; + let absolutePath: string; + + if (!resolved) { + // If not resolved, we still check if it's an unauthorized absolute path that we can ask permission for. + absolutePath = path.resolve( this.context.config.getTargetDir(), pathName, ); - let validationError = this.context.config.validatePathAccess( + validationError = this.context.config.validatePathAccess( absolutePath, 'read', ); @@ -1020,7 +1032,11 @@ export class Session { }); } } + } else { + absolutePath = resolved.absolutePath; + } + try { if (!validationError) { // If it's an absolute path that is authorized (e.g. added via readOnlyPaths), // read it directly to avoid ReadManyFilesTool absolute path resolution issues. @@ -1033,7 +1049,9 @@ export class Session { !readDirectly ) { try { - const stats = await fs.stat(absolutePath); + const stats = resolved + ? resolved.stats + : await fs.stat(absolutePath); if (stats.isFile()) { const fileReadResult = await processSingleFileContent( absolutePath, @@ -1092,7 +1110,9 @@ export class Session { } if (!readDirectly) { - const stats = await fs.stat(absolutePath); + const stats = resolved + ? resolved.stats + : await fs.stat(absolutePath); if (stats.isDirectory()) { currentPathSpec = pathName.endsWith('/') ? `${pathName}**` diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index e23d70a60d..8ab933d78e 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -25,6 +25,7 @@ import type { HistoryItemToolGroup, IndividualToolCallDisplay, } from '../types.js'; +import { resolveAtCommandPath } from '../../utils/atCommandUtils.js'; import type { UseHistoryManagerReturn } from './useHistoryManager.js'; const REF_CONTENT_HEADER = `\n${REFERENCE_CONTENT_START}`; @@ -271,103 +272,109 @@ async function resolveFilePaths( continue; } - for (const dir of config.getWorkspaceContext().getDirectories()) { - try { - const absolutePath = path.resolve(dir, pathName); - const stats = await fs.stat(absolutePath); - - const relativePath = path.isAbsolute(pathName) - ? path.relative(dir, absolutePath) - : pathName; - - if (stats.isDirectory()) { - const pathSpec = path.join(relativePath, '**'); - resolvedFiles.push({ - part, - pathSpec, - displayLabel: path.isAbsolute(pathName) ? relativePath : pathName, - absolutePath, - }); - onDebugMessage( - `Path ${pathName} resolved to directory, using glob: ${pathSpec}`, - ); - } else { - resolvedFiles.push({ - part, - pathSpec: relativePath, - displayLabel: path.isAbsolute(pathName) ? relativePath : pathName, - absolutePath, - }); - onDebugMessage( - `Path ${pathName} resolved to file: ${absolutePath}, using relative path: ${relativePath}`, - ); - } - break; - } catch (error) { - if (isNodeError(error) && error.code === 'ENOENT') { - if (config.getEnableRecursiveFileSearch() && globTool) { - onDebugMessage( - `Path ${pathName} not found directly, attempting glob search.`, - ); - try { - const globResult = await globTool.buildAndExecute( - { - pattern: `**/*${pathName}*`, - path: dir, - }, - signal, + const resolved = await resolveAtCommandPath( + pathName, + config, + onDebugMessage, + ); + if (resolved) { + const absolutePath = resolved.absolutePath; + const relativePath = resolved.relativePath; + const stats = resolved.stats; + if (stats.isDirectory()) { + const pathSpec = path.join(relativePath, '**'); + resolvedFiles.push({ + part, + pathSpec, + displayLabel: path.isAbsolute(pathName) ? relativePath : pathName, + absolutePath, + }); + onDebugMessage( + `Path ${pathName} resolved to directory, using glob: ${pathSpec}`, + ); + } else { + resolvedFiles.push({ + part, + pathSpec: relativePath, + displayLabel: path.isAbsolute(pathName) ? relativePath : pathName, + absolutePath, + }); + onDebugMessage( + `Path ${pathName} resolved to file: ${absolutePath}, using relative path: ${relativePath}`, + ); + } + } else { + // If direct resolution fails, we keep the original loop for glob fallback if enabled + for (const dir of config.getWorkspaceContext().getDirectories()) { + try { + const absolutePath = path.resolve(dir, pathName); + await fs.stat(absolutePath); + } catch (error) { + if (isNodeError(error) && error.code === 'ENOENT') { + if (config.getEnableRecursiveFileSearch() && globTool) { + onDebugMessage( + `Path ${pathName} not found directly, attempting glob search.`, ); - if ( - globResult.llmContent && - typeof globResult.llmContent === 'string' && - !globResult.llmContent.startsWith('No files found') && - !globResult.llmContent.startsWith('Error:') - ) { - const lines = globResult.llmContent.split('\n'); - if (lines.length > 1 && lines[1]) { - const firstMatchAbsolute = lines[1].trim(); - const pathSpec = path.relative(dir, firstMatchAbsolute); - resolvedFiles.push({ - part, - pathSpec, - displayLabel: path.isAbsolute(pathName) - ? pathSpec - : pathName, - }); - onDebugMessage( - `Glob search for ${pathName} found ${firstMatchAbsolute}, using relative path: ${pathSpec}`, - ); - break; + try { + const globResult = await globTool.buildAndExecute( + { + pattern: `**/*${pathName}*`, + path: dir, + }, + signal, + ); + if ( + globResult.llmContent && + typeof globResult.llmContent === 'string' && + !globResult.llmContent.startsWith('No files found') && + !globResult.llmContent.startsWith('Error:') + ) { + const lines = globResult.llmContent.split('\n'); + if (lines.length > 1 && lines[1]) { + const firstMatchAbsolute = lines[1].trim(); + const pathSpec = path.relative(dir, firstMatchAbsolute); + resolvedFiles.push({ + part, + pathSpec, + displayLabel: path.isAbsolute(pathName) + ? pathSpec + : pathName, + }); + onDebugMessage( + `Glob search for ${pathName} found ${firstMatchAbsolute}, using relative path: ${pathSpec}`, + ); + break; + } else { + onDebugMessage( + `Glob search for '**/*${pathName}*' did not return a usable path. Path ${pathName} will be skipped.`, + ); + } } else { onDebugMessage( - `Glob search for '**/*${pathName}*' did not return a usable path. Path ${pathName} will be skipped.`, + `Glob search for '**/*${pathName}*' found no files or an error. Path ${pathName} will be skipped.`, ); } - } else { + } catch (globError) { + debugLogger.warn( + `Error during glob search for ${pathName}: ${getErrorMessage(globError)}`, + ); onDebugMessage( - `Glob search for '**/*${pathName}*' found no files or an error. Path ${pathName} will be skipped.`, + `Error during glob search for ${pathName}. Path ${pathName} will be skipped.`, ); } - } catch (globError) { - debugLogger.warn( - `Error during glob search for ${pathName}: ${getErrorMessage(globError)}`, - ); + } else { onDebugMessage( - `Error during glob search for ${pathName}. Path ${pathName} will be skipped.`, + `Glob tool not found. Path ${pathName} will be skipped.`, ); } } else { + debugLogger.warn( + `Error stating path ${pathName}: ${getErrorMessage(error)}`, + ); onDebugMessage( - `Glob tool not found. Path ${pathName} will be skipped.`, + `Error stating path ${pathName}. Path ${pathName} will be skipped.`, ); } - } else { - debugLogger.warn( - `Error stating path ${pathName}: ${getErrorMessage(error)}`, - ); - onDebugMessage( - `Error stating path ${pathName}. Path ${pathName} will be skipped.`, - ); } } } diff --git a/packages/cli/src/utils/atCommandUtils.ts b/packages/cli/src/utils/atCommandUtils.ts new file mode 100644 index 0000000000..adb09851b7 --- /dev/null +++ b/packages/cli/src/utils/atCommandUtils.ts @@ -0,0 +1,83 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as path from 'node:path'; +import * as fs from 'node:fs/promises'; +import { + validatePath, + isWithinRoot, + type Config, +} from '@google/gemini-cli-core'; + +export interface ResolvedAtCommandPath { + absolutePath: string; + relativePath: string; + stats: { + isDirectory(): boolean; + isFile(): boolean; + }; +} + +/** + * Resolves a path from an @-command, ensuring it is valid and within workspace boundaries. + */ +export async function resolveAtCommandPath( + pathName: string, + config: Config, + onDebugMessage: (msg: string) => void = () => {}, +): Promise { + const pathValidation = validatePath(pathName); + if (!pathValidation.isValid) { + onDebugMessage( + `Skipping invalid path in @-command: ${pathName}. Reason: ${pathValidation.error}`, + ); + return null; + } + + for (const dir of config.getWorkspaceContext().getDirectories()) { + try { + const absolutePath = path.resolve(dir, pathName); + + const resolvedValidation = validatePath(absolutePath); + if (!resolvedValidation.isValid) { + onDebugMessage( + `Skipping invalid resolved path in @-command: ${absolutePath}. Reason: ${resolvedValidation.error}`, + ); + continue; + } + + // Final workspace boundary check using centralized logic + const validationError = config.validatePathAccess(absolutePath, 'read'); + if (validationError) { + // If it's outside root, we might still allow it with explicit user permission in acpSession, + // but for now, we follow the general rule. + if (!isWithinRoot(absolutePath, config.getTargetDir())) { + // Proceed to stat check, calling sites will handle permission dialogs if needed + } else { + onDebugMessage( + `Skipping unauthorized path: ${absolutePath}. Reason: ${validationError}`, + ); + continue; + } + } + + const stats = await fs.stat(absolutePath); + const relativePath = path.isAbsolute(pathName) + ? path.relative(dir, absolutePath) + : pathName; + + return { + absolutePath, + relativePath, + stats, + }; + } catch { + // Ignore errors for this specific directory, try next + } + } + + return null; +} diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 5f3413b37d..67952170f7 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -177,6 +177,7 @@ import { startupProfiler } from '../telemetry/startupProfiler.js'; import type { AgentDefinition } from '../agents/types.js'; import { fetchAdminControls } from '../code_assist/admin/admin_controls.js'; import { isSubpath, resolveToRealPath } from '../utils/paths.js'; +import { validatePath } from '../utils/path-validator.js'; import { InjectionService } from './injectionService.js'; import { ExecutionLifecycleService } from '../services/executionLifecycleService.js'; import { WORKSPACE_POLICY_TIER } from '../policy/config.js'; @@ -3289,6 +3290,11 @@ export class Config implements McpContext, AgentLoopContext { absolutePath: string, checkType: 'read' | 'write' = 'write', ): string | null { + const pathValidation = validatePath(absolutePath); + if (!pathValidation.isValid) { + return `Invalid path: ${pathValidation.error}`; + } + if (checkType === 'write' && hasScopedAutoMemoryExtractionWriteAccess()) { const resolvedPath = resolveToRealPath(absolutePath); if ( diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 7fc1892139..2b36dd8b16 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -92,6 +92,7 @@ export * from './utils/sessionOperations.js'; export * from './utils/planUtils.js'; export * from './utils/approvalModeUtils.js'; export * from './utils/fileDiffUtils.js'; +export * from './utils/path-validator.js'; export * from './utils/retry.js'; export * from './utils/shell-utils.js'; export { diff --git a/packages/core/src/utils/path-validator.test.ts b/packages/core/src/utils/path-validator.test.ts new file mode 100644 index 0000000000..d0cc8c4af5 --- /dev/null +++ b/packages/core/src/utils/path-validator.test.ts @@ -0,0 +1,77 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { validatePath } from './path-validator.js'; + +describe('PathValidator', () => { + it('should validate normal paths', () => { + expect(validatePath('src/index.ts').isValid).toBe(true); + expect(validatePath('/usr/local/bin').isValid).toBe(true); + expect(validatePath('C:\\Users\\name\\Documents').isValid).toBe(true); + expect(validatePath('relative/path/to/file.js').isValid).toBe(true); + }); + + it('should reject empty or non-string paths', () => { + expect(validatePath('').isValid).toBe(false); + expect(validatePath(null as unknown as string).isValid).toBe(false); + }); + + it('should reject paths with newlines or control characters', () => { + expect(validatePath('path/with\nnewline').isValid).toBe(false); + expect(validatePath('path/with\rreturn').isValid).toBe(false); + expect(validatePath('path/with\0null').isValid).toBe(false); + expect(validatePath('path/with\ttab').isValid).toBe(false); + }); + + it('should reject excessively long paths', () => { + const longPath = 'a'.repeat(4097); + const result = validatePath(longPath); + expect(result.isValid).toBe(false); + expect(result.error).toContain('Path is too long'); + }); + + it('should reject paths with excessively long components', () => { + const longComponent = 'a'.repeat(256); + const result = validatePath(`path/to/${longComponent}/file`); + expect(result.isValid).toBe(false); + expect(result.error).toContain( + 'component "aaaaaaaaaaaaaaaaaaaa..." is too long', + ); + }); + + it('should reject misinterpreted log fragments with quotes or ellipses', () => { + const logFragment = + 'Error: No "formatTimeRange" export is defined on the lib/formatTimeRange mock.'; + const result = validatePath(logFragment); + expect(result.isValid).toBe(false); + expect(result.error).toContain('suspicious characters'); + }); + + it('should reject code snippets with braces/parens', () => { + const codeSnippet = + 'vi.mock(import("@/lib/formatTimeRange"), async (importOriginal) => { return { ...actual }; })'; + const result = validatePath(codeSnippet); + expect(result.isValid).toBe(false); + expect(result.error).toContain( + 'misinterpreted log fragment or code snippet', + ); + }); + + it('should reject misinterpreted log fragments with log markers', () => { + expect(validatePath('FAIL tests/int/my.test.ts').isValid).toBe(false); + expect( + validatePath('AssertionError: expected true to be false').isValid, + ).toBe(false); + expect(validatePath('✓ test passed').isValid).toBe(false); + expect(validatePath('× test failed').isValid).toBe(false); + }); + + it('should allow short paths with quotes (even if unusual)', () => { + // Some systems might technically allow this, and we only want to block long/obvious log fragments + expect(validatePath('file"with"quote.txt').isValid).toBe(true); + }); +}); diff --git a/packages/core/src/utils/path-validator.ts b/packages/core/src/utils/path-validator.ts new file mode 100644 index 0000000000..099bd16ca2 --- /dev/null +++ b/packages/core/src/utils/path-validator.ts @@ -0,0 +1,103 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Result of a path validation check. + */ +export interface PathValidationResult { + isValid: boolean; + error?: string; +} + +/** + * Common path limits. + * While some OSs support longer, 4096 is a safe cross-platform limit for absolute paths. + * Individual components are usually limited to 255. + */ +const MAX_PATH_LENGTH = 4096; +const MAX_COMPONENT_LENGTH = 255; + +/** + * Validates a path string for common issues that lead to system-level crashes (like ENAMETOOLONG). + * This is intended as a "pre-flight" check for paths derived from untrusted model output. + */ +export function validatePath(pathStr: string): PathValidationResult { + if (!pathStr || typeof pathStr !== 'string') { + return { isValid: false, error: 'Path must be a non-empty string.' }; + } + + // Check for obviously invalid characters (newlines, control characters, null bytes) + // These often appear when the model misinterprets logs as paths. + if (/[\n\r\0\t]/.test(pathStr)) { + return { + isValid: false, + error: + 'Path contains invalid characters (newlines or control characters).', + }; + } + + // Check for common log/error patterns that are definitely not paths + const logMarkers = [ + 'AssertionError', + 'FAIL', + '✓', + '×', + 'TestingLibraryElementError', + ]; + for (const marker of logMarkers) { + if (pathStr.includes(marker)) { + return { + isValid: false, + error: 'Path appears to be a misinterpreted log fragment.', + }; + } + } + + // If it contains characters like '{', '}', '(', ')', '[', ']' and it's long, it's likely a log + if (pathStr.length > 50 && /[{}[\]()]/.test(pathStr)) { + return { + isValid: false, + error: + 'Path appears to be a misinterpreted log fragment or code snippet.', + }; + } + + // Check for quotes or ellipses in "paths" - almost always a misinterpretation if not a very short name + if ( + pathStr.includes('"') || + pathStr.includes("'") || + pathStr.includes('...') + ) { + if (pathStr.length > 20) { + return { + isValid: false, + error: + 'Path contains suspicious characters (quotes or ellipses) and is too long to be a simple filename.', + }; + } + } + + // Check total length + if (pathStr.length > MAX_PATH_LENGTH) { + return { + isValid: false, + error: `Path is too long (maximum ${MAX_PATH_LENGTH} characters).`, + }; + } + + // Check individual component lengths + const components = pathStr.split(/[/\\]/); + for (const component of components) { + if (component.length > MAX_COMPONENT_LENGTH) { + return { + isValid: false, + error: `Path component "${component.substring(0, 20)}..." is too long (maximum ${MAX_COMPONENT_LENGTH} characters).`, + }; + } + } + + return { isValid: true }; +}