From d7384c446f993bfa67a92850fada6777f7fc77b5 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Wed, 20 May 2026 15:55:57 -0400 Subject: [PATCH] fix(core): centralize path validation to prevent crashes from malformed prompts (#27211) --- packages/cli/src/acp/acpRpcDispatcher.test.ts | 1 + packages/cli/src/acp/acpSession.test.ts | 1 + packages/cli/src/acp/acpSession.ts | 203 ++++---- .../cli/src/acp/acpSessionManager.test.ts | 1 + .../src/ui/hooks/atCommandProcessor.test.ts | 132 ++++- .../cli/src/ui/hooks/atCommandProcessor.ts | 174 +++---- packages/core/src/config/config.test.ts | 47 ++ packages/core/src/config/config.ts | 6 + packages/core/src/index.ts | 2 + .../core/src/utils/atCommandUtils.test.ts | 452 ++++++++++++++++++ packages/core/src/utils/atCommandUtils.ts | 215 +++++++++ .../core/src/utils/path-validator.test.ts | 124 +++++ packages/core/src/utils/path-validator.ts | 93 ++++ 13 files changed, 1261 insertions(+), 190 deletions(-) create mode 100644 packages/core/src/utils/atCommandUtils.test.ts create mode 100644 packages/core/src/utils/atCommandUtils.ts create mode 100644 packages/core/src/utils/path-validator.test.ts create mode 100644 packages/core/src/utils/path-validator.ts diff --git a/packages/cli/src/acp/acpRpcDispatcher.test.ts b/packages/cli/src/acp/acpRpcDispatcher.test.ts index a677c5631b..6dff01c751 100644 --- a/packages/cli/src/acp/acpRpcDispatcher.test.ts +++ b/packages/cli/src/acp/acpRpcDispatcher.test.ts @@ -71,6 +71,7 @@ describe('GeminiAgent - RPC Dispatcher', () => { validatePathAccess: vi.fn().mockReturnValue(null), getWorkspaceContext: vi.fn().mockReturnValue({ addReadOnlyPath: vi.fn(), + getDirectories: vi.fn().mockReturnValue(['/tmp']), }), getPolicyEngine: vi.fn().mockReturnValue({ addRule: vi.fn(), diff --git a/packages/cli/src/acp/acpSession.test.ts b/packages/cli/src/acp/acpSession.test.ts index 785386086e..6d21971c79 100644 --- a/packages/cli/src/acp/acpSession.test.ts +++ b/packages/cli/src/acp/acpSession.test.ts @@ -149,6 +149,7 @@ describe('Session', () => { validatePathAccess: vi.fn().mockReturnValue(null), getWorkspaceContext: vi.fn().mockReturnValue({ addReadOnlyPath: vi.fn(), + getDirectories: vi.fn().mockReturnValue(['/tmp']), }), waitForMcpInit: vi.fn(), getDisableAlwaysAllow: vi.fn().mockReturnValue(false), diff --git a/packages/cli/src/acp/acpSession.ts b/packages/cli/src/acp/acpSession.ts index 75ec7af49e..430670d492 100644 --- a/packages/cli/src/acp/acpSession.ts +++ b/packages/cli/src/acp/acpSession.ts @@ -37,6 +37,8 @@ import { MessageBusType, PolicyDecision, type ToolConfirmationRequest, + resolveAtCommandPath, + type ResolvedAtCommandPath, } from '@google/gemini-cli-core'; import * as acp from '@agentclientprotocol/sdk'; import type { Part, FunctionCall } from '@google/genai'; @@ -1023,99 +1025,120 @@ export class Session { let currentPathSpec = pathName; let resolvedSuccessfully = false; let readDirectly = false; - try { - const absolutePath = path.resolve( + + const result = await resolveAtCommandPath( + pathName, + this.context.config, + (msg) => this.debug(msg), + ); + + let validationError: string | null = null; + let absolutePath: string; + let resolved: ResolvedAtCommandPath | undefined; + + if (result.status === 'resolved') { + resolved = result.resolved; + absolutePath = resolved.absolutePath; + } else if (result.status === 'unauthorized') { + absolutePath = result.absolutePath; + validationError = result.error; + } else if (result.status === 'invalid') { + // Already logged in resolveAtCommandPath + continue; + } else { + // Result is not_found. + // We still check if it's an unauthorized absolute path that we can ask permission for, + // specifically for paths that are completely outside the root and not even in any workspace directory. + // For relative paths not found anywhere, we resolve relative to targetDir for permission check. + absolutePath = path.resolve( this.context.config.getTargetDir(), pathName, ); + } - let validationError = this.context.config.validatePathAccess( - absolutePath, - 'read', - ); - - // We ask the user for explicit permission to read them if outside sandboxed workspace boundaries (and not already authorized). - if ( - validationError && - !isWithinRoot(absolutePath, this.context.config.getTargetDir()) - ) { - try { - const stats = await fs.stat(absolutePath); - if (stats.isFile()) { - const syntheticCallId = `resolve-prompt-${pathName}-${randomUUID()}`; - const params = { - sessionId: this.id, - options: [ - { - optionId: ToolConfirmationOutcome.ProceedOnce, - name: 'Allow once', - kind: 'allow_once', - }, - { - optionId: ToolConfirmationOutcome.Cancel, - name: 'Deny', - kind: 'reject_once', - }, - ] as acp.PermissionOption[], - toolCall: { - toolCallId: syntheticCallId, - status: 'pending', - title: `Allow access to absolute path: ${pathName}`, - content: [ - { - type: 'content', - content: { - type: 'text', - text: `The Agent needs access to read an attached file outside your workspace: ${pathName}`, - }, - }, - ], - locations: [], - kind: 'read', + if ( + !resolved && + validationError && + !isWithinRoot(absolutePath, this.context.config.getTargetDir()) + ) { + try { + const stats = await fs.stat(absolutePath); + if (stats.isFile()) { + const syntheticCallId = `resolve-prompt-${pathName}-${randomUUID()}`; + const params = { + sessionId: this.id, + options: [ + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Allow once', + kind: 'allow_once', }, - }; - - const output = RequestPermissionResponseSchema.parse( - await this.connection.requestPermission(params), - ); - - const outcome = - output.outcome.outcome === 'cancelled' - ? ToolConfirmationOutcome.Cancel - : z - .nativeEnum(ToolConfirmationOutcome) - .parse(output.outcome.optionId); - - if (outcome === ToolConfirmationOutcome.ProceedOnce) { - this.context.config - .getWorkspaceContext() - .addReadOnlyPath(absolutePath); - validationError = null; - } else { - this.debug( - `Direct read authorization denied for absolute path ${pathName}`, - ); - directContents.push({ - spec: pathName, - content: `[Warning: Access to absolute path \`${pathName}\` denied by user.]`, - }); - continue; - } - } - } catch (error) { - this.debug( - `Failed to request permission for absolute attachment ${pathName}: ${getErrorMessage(error)}`, - ); - await this.sendUpdate({ - sessionUpdate: 'agent_thought_chunk', - content: { - type: 'text', - text: `Warning: Failed to display permission dialog for \`${absolutePath}\`. Error: ${getErrorMessage(error)}`, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'Deny', + kind: 'reject_once', + }, + ] as acp.PermissionOption[], + toolCall: { + toolCallId: syntheticCallId, + status: 'pending', + title: `Allow access to absolute path: ${pathName}`, + content: [ + { + type: 'content', + content: { + type: 'text', + text: `The Agent needs access to read an attached file outside your workspace: ${pathName}`, + }, + }, + ], + locations: [], + kind: 'read', }, - }); - } - } + }; + const output = RequestPermissionResponseSchema.parse( + await this.connection.requestPermission(params), + ); + + const outcome = + output.outcome.outcome === 'cancelled' + ? ToolConfirmationOutcome.Cancel + : z + .nativeEnum(ToolConfirmationOutcome) + .parse(output.outcome.optionId); + + if (outcome === ToolConfirmationOutcome.ProceedOnce) { + this.context.config + .getWorkspaceContext() + .addReadOnlyPath(absolutePath); + validationError = null; + } else { + this.debug( + `Direct read authorization denied for absolute path ${pathName}`, + ); + directContents.push({ + spec: pathName, + content: `[Warning: Access to absolute path \`${pathName}\` denied by user.]`, + }); + continue; + } + } + } catch (error) { + this.debug( + `Failed to request permission for absolute attachment ${pathName}: ${getErrorMessage(error)}`, + ); + await this.sendUpdate({ + sessionUpdate: 'agent_thought_chunk', + content: { + type: 'text', + text: `Warning: Failed to display permission dialog for \`${absolutePath}\`. Error: ${getErrorMessage(error)}`, + }, + }); + } + } + + 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. @@ -1128,7 +1151,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, @@ -1187,7 +1212,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/acp/acpSessionManager.test.ts b/packages/cli/src/acp/acpSessionManager.test.ts index 7a896b1839..710f29f044 100644 --- a/packages/cli/src/acp/acpSessionManager.test.ts +++ b/packages/cli/src/acp/acpSessionManager.test.ts @@ -79,6 +79,7 @@ describe('AcpSessionManager', () => { validatePathAccess: vi.fn().mockReturnValue(null), getWorkspaceContext: vi.fn().mockReturnValue({ addReadOnlyPath: vi.fn(), + getDirectories: vi.fn().mockReturnValue(['/tmp']), }), getPolicyEngine: vi.fn().mockReturnValue({ addRule: vi.fn(), diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts index d80a8bfd80..bb301f7039 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.test.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.test.ts @@ -77,6 +77,12 @@ describe('handleAtCommand', () => { unsubscribe: vi.fn(), } as unknown as core.MessageBus; + const mockWorkspaceContext = { + isPathWithinWorkspace: (p: string) => + p.startsWith(testRootDir) || p.startsWith('/private' + testRootDir), + getDirectories: () => [testRootDir], + }; + mockConfig = { getToolRegistry, getTargetDir: () => testRootDir, @@ -91,11 +97,7 @@ describe('handleAtCommand', () => { }), getFileSystemService: () => new StandardFileSystemService(), getEnableRecursiveFileSearch: vi.fn(() => true), - getWorkspaceContext: () => ({ - isPathWithinWorkspace: (p: string) => - p.startsWith(testRootDir) || p.startsWith('/private' + testRootDir), - getDirectories: () => [testRootDir], - }), + getWorkspaceContext: () => mockWorkspaceContext, getMemoryContextManager: () => undefined, storage: { getProjectTempDir: () => path.join(os.tmpdir(), 'gemini-cli-temp'), @@ -106,7 +108,8 @@ describe('handleAtCommand', () => { } const workspaceContext = this.getWorkspaceContext(); - if (workspaceContext.isPathWithinWorkspace(absolutePath)) { + const directories = workspaceContext.getDirectories(); + if (directories.some((dir) => absolutePath.startsWith(dir))) { return true; } @@ -1462,31 +1465,126 @@ describe('handleAtCommand', () => { ); }); - it('should include agent nudge when agents are found', async () => { - const agentName = 'my-agent'; - const otherAgent = 'other-agent'; + it('should resolve files in multiple workspace directories', async () => { + const secondRootDir = await fsPromises.mkdtemp( + path.join(os.tmpdir(), 'second-root-'), + ); + try { + const fileContent = 'Second root content'; + const filePath = path.join(secondRootDir, 'second-file.txt'); + await fsPromises.writeFile(filePath, fileContent); - // Mock getAgentRegistry on the config - mockConfig.getAgentRegistry = vi.fn().mockReturnValue({ - getDefinition: (name: string) => - name === agentName || name === otherAgent ? { name } : undefined, + vi.spyOn( + mockConfig.getWorkspaceContext(), + 'getDirectories', + ).mockReturnValue([testRootDir, secondRootDir]); + + const query = '@second-file.txt'; + + const result = await handleAtCommand({ + query, + config: mockConfig, + addItem: mockAddItem, + onDebugMessage: mockOnDebugMessage, + messageId: 700, + signal: abortController.signal, + }); + + expect(result.processedQuery).toContainEqual( + expect.objectContaining({ text: fileContent }), + ); + expect(mockOnDebugMessage).toHaveBeenCalledWith( + expect.stringContaining(`resolved to file: ${filePath}`), + ); + } finally { + await fsPromises.rm(secondRootDir, { recursive: true, force: true }); + } + }); + + it('should attempt glob fallback if direct resolution is unauthorized', async () => { + const fileContent = 'Globbed content'; + const filePath = await createTestFile( + path.join(testRootDir, 'secret', 'file.txt'), + fileContent, + ); + + // Mock validatePathAccess to deny direct access but allow it via glob (just for test purposes) + vi.spyOn(mockConfig, 'validatePathAccess').mockImplementation((p) => { + if (p.includes('secret') && !p.includes('file.txt')) + return 'Unauthorized'; + // Let's say the direct path 'secret/file.txt' is unauthorized + if (p === filePath) return 'Access Denied'; + return null; }); - const query = `@${agentName} @${otherAgent}`; + const query = '@secret/file.txt'; + + await handleAtCommand({ + query, + config: mockConfig, + addItem: mockAddItem, + onDebugMessage: mockOnDebugMessage, + messageId: 701, + signal: abortController.signal, + }); + + // In this case, resolveAtCommandPath returns status: 'unauthorized'. + // resolveFilePaths should then try glob fallback. + expect(mockOnDebugMessage).toHaveBeenCalledWith( + expect.stringContaining('not found directly, attempting glob search.'), + ); + }); + + it('should skip malformed paths (the original crash scenario)', async () => { + // We use a quoted path so the parser treats the whole thing as one @path token + const malformedPath = + '"FAIL tests/int/my.test.ts ... AssertionError: expected true to be false"'; + const query = `@${malformedPath}`; const result = await handleAtCommand({ query, config: mockConfig, addItem: mockAddItem, onDebugMessage: mockOnDebugMessage, - messageId: 600, + messageId: 702, signal: abortController.signal, }); - const expectedNudge = `\n\nThe user has explicitly selected the following agent(s): ${agentName}, ${otherAgent}. Please use the following tool(s) to delegate the task: '${agentName}', '${otherAgent}'.\n\n`; + // Malformed path should be skipped and original query part preserved as text + expect(result.processedQuery).toEqual([{ text: query }]); + expect(mockOnDebugMessage).toHaveBeenCalledWith( + expect.stringContaining( + 'Identified invalid path fragment, attempting to extract path', + ), + ); + }); + it('should recover a buried path from a malformed fragment during handleAtCommand', async () => { + const buriedFile = 'src/recovered.ts'; + await createTestFile( + path.join(testRootDir, buriedFile), + 'Recovered content', + ); + const malformedFragment = `"FAIL ${buriedFile}:10:5 (AssertionError)"`; + const query = `@${malformedFragment}`; + + const result = await handleAtCommand({ + query, + config: mockConfig, + addItem: mockAddItem, + onDebugMessage: mockOnDebugMessage, + messageId: 703, + signal: abortController.signal, + }); + + // It should extract src/recovered.ts and attach its content expect(result.processedQuery).toContainEqual( - expect.objectContaining({ text: expectedNudge }), + expect.objectContaining({ text: 'Recovered content' }), + ); + expect(mockOnDebugMessage).toHaveBeenCalledWith( + expect.stringContaining( + 'Identified invalid path fragment, attempting to extract path', + ), ); }); }); diff --git a/packages/cli/src/ui/hooks/atCommandProcessor.ts b/packages/cli/src/ui/hooks/atCommandProcessor.ts index e23d70a60d..8fa33baa48 100644 --- a/packages/cli/src/ui/hooks/atCommandProcessor.ts +++ b/packages/cli/src/ui/hooks/atCommandProcessor.ts @@ -4,14 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as fs from 'node:fs/promises'; import * as path from 'node:path'; import type { PartListUnion, PartUnion } from '@google/genai'; import type { AnyToolInvocation, Config } from '@google/gemini-cli-core'; import { debugLogger, getErrorMessage, - isNodeError, unescapePath, resolveToRealPath, fileExists, @@ -19,6 +17,7 @@ import { REFERENCE_CONTENT_START, REFERENCE_CONTENT_END, CoreToolCallStatus, + resolveAtCommandPath, } from '@google/gemini-cli-core'; import { Buffer } from 'node:buffer'; import type { @@ -271,102 +270,100 @@ 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 result = await resolveAtCommandPath(pathName, config, onDebugMessage); - const relativePath = path.isAbsolute(pathName) - ? path.relative(dir, absolutePath) - : pathName; + if (result.status === 'resolved') { + const { absolutePath, relativePath, stats } = result.resolved; + 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 ( + result.status === 'not_found' || + result.status === 'unauthorized' + ) { + // If direct resolution fails, we attempt glob search if enabled. + // We also allow glob fallback for "unauthorized" results from resolveAtCommandPath, + // as they might represent a relative path that matched an unauthorized file in one directory + // but might have a valid match (via glob) in another. + if (config.getEnableRecursiveFileSearch() && globTool) { + onDebugMessage( + `Path ${pathName} not found directly, attempting glob search.`, + ); - 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.`, + for (const dir of config.getWorkspaceContext().getDirectories()) { + try { + const globResult = await globTool.buildAndExecute( + { + pattern: `**/*${pathName}*`, + path: dir, + }, + signal, ); - 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.`, - ); + 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 rawMatch = lines[1].trim(); + let firstMatchAbsolute: string; + try { + firstMatchAbsolute = resolveToRealPath(rawMatch); + } catch { + firstMatchAbsolute = rawMatch; } + const pathSpec = path.relative(dir, firstMatchAbsolute); + resolvedFiles.push({ + part, + pathSpec, + displayLabel: path.isAbsolute(pathName) ? pathSpec : pathName, + absolutePath: firstMatchAbsolute, + }); + onDebugMessage( + `Glob search for ${pathName} found ${firstMatchAbsolute}, using relative path: ${pathSpec}`, + ); + break; } else { onDebugMessage( - `Glob search for '**/*${pathName}*' found no files or an error. Path ${pathName} will be skipped.`, + `Glob search for '**/*${pathName}*' did not return a usable path. 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 search for '**/*${pathName}*' found no files or an error. Path ${pathName} will be skipped.`, ); } - } else { - onDebugMessage( - `Glob tool not found. Path ${pathName} will be skipped.`, + } catch (globError) { + debugLogger.warn( + `Error during glob search for ${pathName}: ${getErrorMessage(globError)}`, ); } - } else { - debugLogger.warn( - `Error stating path ${pathName}: ${getErrorMessage(error)}`, - ); + } + } else { + if (!config.getEnableRecursiveFileSearch() || !globTool) { onDebugMessage( - `Error stating path ${pathName}. Path ${pathName} will be skipped.`, + `Glob tool not found. Path ${pathName} will be skipped.`, ); } } @@ -524,7 +521,14 @@ async function readLocalFiles( config.getMessageBus(), ); - const pathSpecsToRead = resolvedFiles.map((rf) => rf.pathSpec); + const pathSpecsToRead = resolvedFiles.map((rf) => { + if (rf.absolutePath) { + return rf.pathSpec.endsWith('**') + ? path.join(rf.absolutePath, '**') + : rf.absolutePath; + } + return rf.pathSpec; + }); const fileLabelsForDisplay = resolvedFiles.map((rf) => rf.displayLabel); const respectFileIgnore = config.getFileFilteringOptions(); diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 15eeee82b0..49f350eef2 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3477,6 +3477,53 @@ describe('Config Quota & Preview Model Access', () => { expect(await config.getPlanModeRoutingEnabled()).toBe(false); }); }); + + describe('validatePathAccess (PathValidator integration)', () => { + it('should reject pathologically long paths', () => { + const config = new Config(baseParams); + const longPath = path.join(baseParams.targetDir, 'a'.repeat(5000)); + const result = config.validatePathAccess(longPath, 'read'); + expect(result).toContain('Invalid path: Path is too long'); + }); + + it('should reject paths with log markers', () => { + const config = new Config(baseParams); + const logPath = path.join( + baseParams.targetDir, + 'AssertionError: expected true to be false', + ); + const result = config.validatePathAccess(logPath, 'read'); + expect(result).toContain( + 'Invalid path: Path appears to be a misinterpreted log fragment', + ); + }); + + it('should reject paths with control characters', () => { + const config = new Config(baseParams); + const malformedPath = path.join( + baseParams.targetDir, + 'file\nwith\nnewline.txt', + ); + const result = config.validatePathAccess(malformedPath, 'read'); + expect(result).toContain( + 'Invalid path: Path contains invalid characters', + ); + }); + + it('should allow normal paths', () => { + const config = new Config(baseParams); + const normalPath = path.resolve(baseParams.targetDir, 'src/index.ts'); + const result = config.validatePathAccess(normalPath, 'read'); + + // It might return "Path not in workspace" or similar if not authorized, + // but it should NOT return the "Invalid path" prefix from PathValidator. + if (result) { + expect(result).not.toContain('Invalid path:'); + } else { + expect(result).toBeNull(); + } + }); + }); }); describe('Config JIT Initialization', () => { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 4429ce5de1..580c0a6a71 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'; @@ -3299,6 +3300,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 092249c027..541c16a8f3 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -93,6 +93,8 @@ 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/atCommandUtils.js'; export * from './utils/retry.js'; export * from './utils/shell-utils.js'; export { diff --git a/packages/core/src/utils/atCommandUtils.test.ts b/packages/core/src/utils/atCommandUtils.test.ts new file mode 100644 index 0000000000..7c7c6935e5 --- /dev/null +++ b/packages/core/src/utils/atCommandUtils.test.ts @@ -0,0 +1,452 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import * as path from 'node:path'; +import * as fsPromises from 'node:fs/promises'; +import type { Stats } from 'node:fs'; +import { resolveAtCommandPath } from './atCommandUtils.js'; +import { type Config } from '../config/config.js'; + +vi.mock('node:fs/promises'); + +describe('atCommandUtils', () => { + let mockConfig: Record; + let mockWorkspaceContext: Record; + + beforeEach(() => { + vi.resetAllMocks(); + + mockWorkspaceContext = { + getDirectories: vi.fn().mockReturnValue(['/mock/root']), + isPathReadable: vi.fn().mockReturnValue(true), + }; + + mockConfig = { + getTargetDir: vi.fn().mockReturnValue('/mock/root'), + getWorkspaceContext: vi.fn().mockReturnValue(mockWorkspaceContext), + validatePathAccess: vi.fn().mockReturnValue(null), + }; + }); + + it('should resolve a valid path', async () => { + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fsPromises.stat).mockResolvedValue(mockStats as unknown as Stats); + + const result = await resolveAtCommandPath( + 'file.ts', + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe( + path.resolve('/mock/root', 'file.ts'), + ); + expect(result.resolved.relativePath).toBe('file.ts'); + } + }); + + it('should resolve an absolute path', async () => { + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fsPromises.stat).mockResolvedValue(mockStats as unknown as Stats); + + const absolutePath = path.resolve('/mock/root', 'src/index.ts'); + const result = await resolveAtCommandPath( + absolutePath, + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absolutePath); + expect(result.resolved.relativePath).toBe(path.join('src', 'index.ts')); + } + }); + + it('should handle multiple directories in workspace context', async () => { + (mockWorkspaceContext['getDirectories'] as Mock).mockReturnValue([ + '/dir1', + '/dir2', + ]); + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + + vi.mocked(fsPromises.stat).mockImplementation(async (p) => { + if (p === path.resolve('/dir2', 'file.txt')) { + return mockStats as unknown as Stats; + } + throw new Error('ENOENT'); + }); + + const result = await resolveAtCommandPath( + 'file.txt', + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe( + path.resolve('/dir2', 'file.txt'), + ); + expect(result.resolved.relativePath).toBe('file.txt'); + } + }); + + it('should return invalid for invalid path (too long)', async () => { + const longPath = 'a'.repeat(5000); + const result = await resolveAtCommandPath( + longPath, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('invalid'); + }); + + it('should return invalid for path with log markers (and no valid subpath)', async () => { + const onDebug = vi.fn(); + const result = await resolveAtCommandPath( + 'FAIL AssertionError: expected true to be false', + mockConfig as unknown as Config, + onDebug, + ); + expect(result.status).toBe('invalid'); + expect(onDebug).toHaveBeenCalledWith( + expect.stringContaining('Skipping invalid path'), + ); + }); + + it('should return not_found if path does not exist in any workspace directory', async () => { + vi.mocked(fsPromises.stat).mockRejectedValue(new Error('ENOENT')); + + const result = await resolveAtCommandPath( + 'nonexistent.ts', + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('not_found'); + }); + + it('should resolve directory paths correctly', async () => { + const mockStats = { + isDirectory: () => true, + isFile: () => false, + }; + vi.mocked(fsPromises.stat).mockResolvedValue(mockStats as unknown as Stats); + + const result = await resolveAtCommandPath( + 'src', + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.stats.isDirectory()).toBe(true); + } + }); + + it('should respect validatePathAccess for paths within root', async () => { + (mockConfig['validatePathAccess'] as Mock).mockReturnValue( + 'Unauthorized access', + ); + // Mock getTargetDir to match the resolved path so it's considered "within root" + (mockConfig['getTargetDir'] as Mock).mockReturnValue('/mock/root'); + + const result = await resolveAtCommandPath( + 'secret.txt', + mockConfig as unknown as Config, + ); + expect(result.status).toBe('unauthorized'); + }); + + it('should return unauthorized for paths outside root', async () => { + (mockConfig['validatePathAccess'] as Mock).mockReturnValue( + 'Outside workspace', + ); + (mockConfig['getTargetDir'] as Mock).mockReturnValue('/mock/workspace'); + + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fsPromises.stat).mockResolvedValue(mockStats as unknown as Stats); + + // Path resolve will use /mock/root as base from mockWorkspaceContext + const result = await resolveAtCommandPath( + 'outside.txt', + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('unauthorized'); + if (result.status === 'unauthorized') { + expect(result.absolutePath).toBe( + path.resolve('/mock/root', 'outside.txt'), + ); + } + }); + + it('should not treat paths with shared prefixes as subpaths if not actually inside', async () => { + // /mock/root-backup/file.txt starts with /mock/root but is not inside it. + const dir = '/mock/root'; + const otherPath = '/mock/root-backup/file.txt'; + + (mockWorkspaceContext['getDirectories'] as Mock).mockReturnValue([dir]); + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fsPromises.stat).mockResolvedValue(mockStats as unknown as Stats); + + const result = await resolveAtCommandPath( + otherPath, + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(otherPath); + // It should NOT be relative to /mock/root because it's not actually inside it. + // path.relative('/mock/root', '/mock/root-backup/file.txt') -> '../root-backup/file.txt' + // Our fix should prevent this from being used as a relative path. + expect(result.resolved.relativePath).toBe(otherPath); + } + }); + + it('should resolve paths in deeply nested workspace directories', async () => { + const dir = path.join('/mock', 'root', 'nested', 'project'); + const relFile = path.join('src', 'index.ts'); + const absFile = path.join(dir, relFile); + + (mockWorkspaceContext['getDirectories'] as Mock).mockReturnValue([dir]); + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fsPromises.stat).mockResolvedValue(mockStats as unknown as Stats); + + const result = await resolveAtCommandPath( + absFile, + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absFile); + expect(result.resolved.relativePath).toBe(relFile); + } + }); + + it('should extract and resolve a buried path from a log fragment', async () => { + const buriedFile = 'src/utils/math.ts'; + const logFragment = `FAIL ${buriedFile}:42:1 (AssertionError)`; + + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fsPromises.stat).mockImplementation(async (p) => { + if (p === path.resolve('/mock/root', buriedFile)) { + return mockStats as unknown as Stats; + } + throw new Error('ENOENT'); + }); + + const result = await resolveAtCommandPath( + logFragment, + mockConfig as unknown as Config, + ); + + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe( + path.resolve('/mock/root', buriedFile), + ); + expect(result.resolved.relativePath).toBe(buriedFile); + } + }); + + describe('Best-Effort Path Extraction (tryExtractPath)', () => { + const mockFile = 'src/index.ts'; + const absMockFile = path.resolve('/mock/root', mockFile); + const mockStats = { isDirectory: () => false, isFile: () => true }; + + beforeEach(() => { + vi.mocked(fsPromises.stat).mockImplementation(async (p) => { + if (p === absMockFile) return mockStats as unknown as Stats; + throw new Error('ENOENT'); + }); + }); + + it('should extract path from "AssertionError: ..." format', async () => { + const result = await resolveAtCommandPath( + `AssertionError: expected something but got something else at ${mockFile}:10:5`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absMockFile); + } + }); + + it('should extract path wrapped in parentheses', async () => { + const result = await resolveAtCommandPath( + `FAIL (${mockFile})`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absMockFile); + } + }); + + it('should extract path wrapped in square brackets', async () => { + const result = await resolveAtCommandPath( + `FAIL [${mockFile}]`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absMockFile); + } + }); + + it('should extract path from "✓" pass marker', async () => { + const result = await resolveAtCommandPath( + `✓ ${mockFile}`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + }); + + it('should extract path from "×" fail marker', async () => { + const result = await resolveAtCommandPath( + `× ${mockFile}`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + }); + + it('should handle multiple trailing punctuation marks like file.txt...', async () => { + const result = await resolveAtCommandPath( + `FAIL ${mockFile}...`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absMockFile); + } + }); + + it('should handle nested wrappers like ("path/to/file.ts")', async () => { + const result = await resolveAtCommandPath( + `FAIL ("${mockFile}")`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.absolutePath).toBe(absMockFile); + } + }); + + it('should NOT strip traversal (..), but let central validation handle it', async () => { + const traversalPath = 'src/../../etc/passwd'; + const absPath = path.resolve('/mock/root', traversalPath); + + (mockConfig['validatePathAccess'] as Mock).mockImplementation((p) => { + if (p === absPath) return 'Outside workspace'; + return null; + }); + + const result = await resolveAtCommandPath( + `FAIL ${traversalPath}`, + mockConfig as unknown as Config, + ); + + // It should NOT be stripped. It should resolve to the absolute path and fail authorization. + expect(result.status).toBe('unauthorized'); + if (result.status === 'unauthorized') { + expect(result.absolutePath).toBe(absPath); + } + }); + + it('should reject paths with null bytes via validatePath', async () => { + const nullBytePath = 'src/index.ts\0.exe'; + const result = await resolveAtCommandPath( + `FAIL ${nullBytePath}`, + mockConfig as unknown as Config, + ); + // validatePath rejects strings with null bytes + expect(result.status).toBe('invalid'); + }); + + it('should handle paths with slashes and extensions correctly', async () => { + const complexPath = 'packages/core/src/utils/deep.test.ts'; + const absComplexPath = path.resolve('/mock/root', complexPath); + vi.mocked(fsPromises.stat).mockImplementation(async (p) => { + if (p === absComplexPath) return mockStats as unknown as Stats; + throw new Error('ENOENT'); + }); + + const result = await resolveAtCommandPath( + `FAIL ${complexPath}:123`, + mockConfig as unknown as Config, + ); + expect(result.status).toBe('resolved'); + if (result.status === 'resolved') { + expect(result.resolved.relativePath).toBe(complexPath); + } + }); + + it('should fail gracefully if no valid path can be extracted', async () => { + const result = await resolveAtCommandPath( + 'FAIL some random text with no slashes or dots', + mockConfig as unknown as Config, + ); + expect(result.status).toBe('invalid'); + }); + + it('should return unauthorized if the extracted path is not authorized', async () => { + const secretFile = '/etc/passwd'; + (mockConfig['validatePathAccess'] as Mock).mockImplementation((p) => + p === secretFile ? 'Unauthorized' : null, + ); + vi.mocked(fsPromises.stat).mockResolvedValue( + mockStats as unknown as Stats, + ); + + const result = await resolveAtCommandPath( + `FAIL ${secretFile}`, + mockConfig as unknown as Config, + ); + // It should try to resolve /etc/passwd, identify it as unauthorized, and return that status. + expect(result.status).toBe('unauthorized'); + }); + }); + + it('should include reason in debug message for unauthorized paths', async () => { + const onDebug = vi.fn(); + (mockConfig['validatePathAccess'] as Mock).mockReturnValue( + 'FORBIDDEN_ZONE', + ); + + await resolveAtCommandPath( + 'secret.txt', + mockConfig as unknown as Config, + onDebug, + ); + + expect(onDebug).toHaveBeenCalledWith( + expect.stringContaining('Reason: FORBIDDEN_ZONE'), + ); + }); +}); diff --git a/packages/core/src/utils/atCommandUtils.ts b/packages/core/src/utils/atCommandUtils.ts new file mode 100644 index 0000000000..f2a8d36362 --- /dev/null +++ b/packages/core/src/utils/atCommandUtils.ts @@ -0,0 +1,215 @@ +/** + * @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 } from './path-validator.js'; +import { type Config } from '../config/config.js'; +import { isNodeError, getErrorMessage } from './errors.js'; + +export interface ResolvedAtCommandPath { + absolutePath: string; + relativePath: string; + stats: { + isDirectory(): boolean; + isFile(): boolean; + }; +} + +/** + * Result of a path resolution attempt. + */ +export type ResolveAtCommandPathResult = + | { status: 'resolved'; resolved: ResolvedAtCommandPath } + | { status: 'unauthorized'; absolutePath: string; error: string } + | { status: 'invalid'; error: string } + | { status: 'not_found' }; + +/** + * Resolves a path from an @-command, ensuring it is valid and within workspace boundaries. + * Performs best-effort extraction if the input appears to be a misinterpreted log fragment. + */ +export async function resolveAtCommandPath( + pathName: string, + config: Config, + onDebugMessage: (msg: string) => void = () => {}, +): Promise { + const pathValidation = validatePath(pathName); + if (!pathValidation.isValid) { + // Attempt to extract a real path from the invalid fragment + const extractedPath = tryExtractPath(pathName); + if (extractedPath && extractedPath !== pathName) { + onDebugMessage( + `Identified invalid path fragment, attempting to extract path: "${extractedPath}" from "${pathName}"`, + ); + // Recurse once with the extracted path. + return resolveAtCommandPath(extractedPath, config, onDebugMessage); + } + + onDebugMessage( + `Skipping invalid path in @-command: ${pathName}. Reason: ${pathValidation.error}`, + ); + return { status: 'invalid', error: pathValidation.error! }; + } + + const workspaceDirs = config.getWorkspaceContext().getDirectories(); + + // If it's an absolute path, we only need to check it against authorization once. + if (path.isAbsolute(pathName)) { + const validationError = config.validatePathAccess(pathName, 'read'); + if (validationError) { + onDebugMessage( + `Skipping unauthorized absolute path: ${pathName}. Reason: ${validationError}`, + ); + return { + status: 'unauthorized', + absolutePath: pathName, + error: validationError, + }; + } + + try { + const stats = await fs.stat(pathName); + // Try to find if it's within one of the workspace directories to provide a nice relative path + let relativePath = pathName; + for (const dir of workspaceDirs) { + const rel = path.relative(dir, pathName); + if (!rel.startsWith('..') && !path.isAbsolute(rel)) { + relativePath = rel; + break; + } + } + + return { + status: 'resolved', + resolved: { + absolutePath: pathName, + relativePath, + stats, + }, + }; + } catch (error) { + if (isNodeError(error) && error.code === 'ENOENT') { + return { status: 'not_found' }; + } + onDebugMessage( + `Unexpected error stating path ${pathName}: ${getErrorMessage(error)}`, + ); + return { status: 'not_found' }; + } + } + + // For relative paths, try each workspace directory. + let lastUnauthorized: { absolutePath: string; error: string } | null = null; + + for (const dir of workspaceDirs) { + const absolutePath = path.resolve(dir, pathName); + + // Final workspace boundary check using centralized logic + const validationError = config.validatePathAccess(absolutePath, 'read'); + if (validationError) { + onDebugMessage( + `Skipping unauthorized path: ${absolutePath}. Reason: ${validationError}`, + ); + // We only care about unauthorized paths if we can't find a valid authorized one. + lastUnauthorized = { absolutePath, error: validationError }; + continue; + } + + try { + const stats = await fs.stat(absolutePath); + return { + status: 'resolved', + resolved: { + absolutePath, + relativePath: pathName, + stats, + }, + }; + } catch (error) { + if (isNodeError(error) && error.code === 'ENOENT') { + // Expected if path is not in this directory, continue to next + continue; + } + onDebugMessage( + `Unexpected error stating path ${absolutePath}: ${getErrorMessage(error)}`, + ); + } + } + + if (lastUnauthorized) { + return { status: 'unauthorized', ...lastUnauthorized }; + } + + return { status: 'not_found' }; +} + +/** + * Attempts to extract a valid-looking path from a noisy string (like a log fragment). + */ +function tryExtractPath(noisyString: string): string | null { + // Split by whitespace to find individual segments + const segments = noisyString.split(/\s+/); + + for (const segment of segments) { + // 1. Strip leading/trailing punctuation and quotes commonly found in logs + // We handle nested wrappers like ("path/to/file.txt") or (at src/index.ts) + let segmentToClean = segment; + const wrappers = [ + '(', + ')', + '[', + ']', + '{', + '}', + '"', + "'", + ',', + ';', + '!', + '.', + ]; + + let wasStripped = true; + while (wasStripped && segmentToClean.length > 0) { + wasStripped = false; + const firstChar = segmentToClean[0]; + const lastChar = segmentToClean[segmentToClean.length - 1]; + + // Strip known punctuation from the start or end + if (wrappers.includes(firstChar)) { + segmentToClean = segmentToClean.slice(1); + wasStripped = true; + } else if (wrappers.includes(lastChar)) { + segmentToClean = segmentToClean.slice(0, -1); + wasStripped = true; + } + } + + if (segmentToClean.length === 0) continue; + + // 2. Strip trailing line/column numbers (e.g. src/main.ts:10:5) + // We handle the case where it might be wrapped in more text, e.g. at (src/index.ts:123) + const lineMatch = segmentToClean.match(/^(.+?):(\d+)(?::\d+)?/); + const pathOnly = lineMatch ? lineMatch[1] : segmentToClean; + + // 3. Validate the extracted segment using centralized heuristics. + // We rely on validatePath and Config.validatePathAccess for robust checking + // rather than naive string stripping which can be bypassed or corrupt valid names. + if (validatePath(pathOnly).isValid) { + // Prioritize segments that actually look like paths (have slashes or dots) + if ( + pathOnly.includes('/') || + pathOnly.includes('\\') || + pathOnly.includes('.') + ) { + return pathOnly; + } + } + } + + return null; +} 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..da8ed570ee --- /dev/null +++ b/packages/core/src/utils/path-validator.test.ts @@ -0,0 +1,124 @@ +/** + * @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 allow paths with single quotes (apostrophes)', () => { + // This was previously a false positive + expect(validatePath("/Users/john's_files/project/index.ts").isValid).toBe( + true, + ); + }); + + it('should allow long paths with brackets or parentheses', () => { + // These were previously false positives (Next.js dynamic routes, Windows copies) + expect( + validatePath('packages/web/app/dashboard/[id]/settings/page.tsx').isValid, + ).toBe(true); + expect( + validatePath('/Users/name/Documents/Project (Copy)/index.ts').isValid, + ).toBe(true); + }); + + it('should only reject log markers at the start of a component', () => { + // Legitimate paths containing these strings should now be allowed + expect(validatePath('src/tests/FAIL_CASE.txt').isValid).toBe(true); + expect(validatePath('FAILURE_LOG.txt').isValid).toBe(true); + expect(validatePath('docs/AssertionError_details.md').isValid).toBe(true); + + // But they should be rejected if they start a component + expect(validatePath('FAIL tests/int/my.test.ts').isValid).toBe(false); + expect(validatePath('/project/root/FAIL tests/my.test.ts').isValid).toBe( + false, + ); + expect( + validatePath('AssertionError: expected true to be false').isValid, + ).toBe(false); + expect(validatePath('✓ test passed').isValid).toBe(false); + }); + + it('should reject misinterpreted log fragments with double 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 allow short paths with double 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); + }); + + it('should reject long paths with ellipses', () => { + expect( + validatePath('this/is/a/very/long/path/with/ellipses/.../and/more') + .isValid, + ).toBe(false); + }); + + it('should allow paths with Unicode characters', () => { + expect(validatePath('src/文件.ts').isValid).toBe(true); + expect(validatePath('docs/🚀_launch.md').isValid).toBe(true); + }); + + it('should allow paths with multiple consecutive slashes (normalizing is handled by OS layer)', () => { + expect(validatePath('src//index.ts').isValid).toBe(true); + }); + + it('should allow paths with trailing slashes', () => { + expect(validatePath('src/utils/').isValid).toBe(true); + }); + + it('should allow paths with dots as components', () => { + expect(validatePath('./src/../index.ts').isValid).toBe(true); + }); + + it('should reject paths that are only dots if they exceed suspicious length (none currently do)', () => { + expect(validatePath('...').isValid).toBe(true); // Short ellipses are allowed as filenames + }); + + it('should reject paths with mixed invalid characters', () => { + expect(validatePath('path\nwith\0invalid').isValid).toBe(false); + }); +}); diff --git a/packages/core/src/utils/path-validator.ts b/packages/core/src/utils/path-validator.ts new file mode 100644 index 0000000000..be234ad4e8 --- /dev/null +++ b/packages/core/src/utils/path-validator.ts @@ -0,0 +1,93 @@ +/** + * @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. + // We check for these at the start of the string OR at the start of any path component. + // This ensures we catch them in both raw model output and resolved absolute paths. + const logMarkerRegexes = [ + /(^|[/\\])AssertionError:/, + /(^|[/\\])FAIL /, + /(^|[/\\])✓ /, + /(^|[/\\])× /, + /(^|[/\\])TestingLibraryElementError:/, + ]; + for (const regex of logMarkerRegexes) { + if (regex.test(pathStr)) { + return { + isValid: false, + error: 'Path appears to be a misinterpreted log fragment.', + }; + } + } + + // Check for double quotes or ellipses in "paths" - almost always a misinterpretation if not a very short name. + // We removed single quotes from this list to support users with apostrophes in their home directories. + if (pathStr.includes('"') || pathStr.includes('...')) { + if (pathStr.length > 20) { + return { + isValid: false, + error: + 'Path contains suspicious characters (double 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 }; +}