From ba5abc74883a3e53b21963bae18cb91d01a40e98 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Mon, 18 May 2026 12:49:39 -0400 Subject: [PATCH] test: add unit tests for resolveAtCommandPath and path validator edge cases --- packages/cli/src/utils/atCommandUtils.test.ts | 178 ++++++++++++++++++ packages/cli/src/utils/atCommandUtils.ts | 20 +- .../path-validation.consolidated.test.ts | 61 ++++++ .../core/src/utils/path-validator.test.ts | 15 ++ 4 files changed, 259 insertions(+), 15 deletions(-) create mode 100644 packages/cli/src/utils/atCommandUtils.test.ts create mode 100644 packages/core/src/config/path-validation.consolidated.test.ts diff --git a/packages/cli/src/utils/atCommandUtils.test.ts b/packages/cli/src/utils/atCommandUtils.test.ts new file mode 100644 index 0000000000..862e494eff --- /dev/null +++ b/packages/cli/src/utils/atCommandUtils.test.ts @@ -0,0 +1,178 @@ +/** + * @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 fs from 'node:fs/promises'; +import { resolveAtCommandPath } from './atCommandUtils.js'; +import { type Config } from '@google/gemini-cli-core'; + +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(fs.stat).mockResolvedValue(mockStats as unknown as fs.Stats); + + const result = await resolveAtCommandPath( + 'file.ts', + mockConfig as unknown as Config, + ); + + expect(result).not.toBeNull(); + expect(result?.absolutePath).toBe(path.resolve('/mock/root', 'file.ts')); + expect(result?.relativePath).toBe('file.ts'); + }); + + it('should resolve an absolute path', async () => { + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fs.stat).mockResolvedValue(mockStats as unknown as fs.Stats); + + const absolutePath = path.resolve('/mock/root', 'src/index.ts'); + const result = await resolveAtCommandPath( + absolutePath, + mockConfig as unknown as Config, + ); + + expect(result).not.toBeNull(); + expect(result?.absolutePath).toBe(absolutePath); + expect(result?.relativePath).toBe('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(fs.stat).mockImplementation(async (p) => { + if (p === path.resolve('/dir2', 'file.txt')) { + return mockStats as unknown as fs.Stats; + } + throw new Error('ENOENT'); + }); + + const result = await resolveAtCommandPath( + 'file.txt', + mockConfig as unknown as Config, + ); + + expect(result?.absolutePath).toBe(path.resolve('/dir2', 'file.txt')); + expect(result?.relativePath).toBe('file.txt'); + }); + + it('should return null for invalid path (too long)', async () => { + const longPath = 'a'.repeat(5000); + const result = await resolveAtCommandPath( + longPath, + mockConfig as unknown as Config, + ); + expect(result).toBeNull(); + }); + + it('should return null for path with log markers', async () => { + const onDebug = vi.fn(); + const result = await resolveAtCommandPath( + 'FAIL tests/my.test.ts', + mockConfig as unknown as Config, + onDebug, + ); + expect(result).toBeNull(); + expect(onDebug).toHaveBeenCalledWith( + expect.stringContaining('Skipping invalid path'), + ); + }); + + it('should return null if path does not exist in any workspace directory', async () => { + vi.mocked(fs.stat).mockRejectedValue(new Error('ENOENT')); + + const result = await resolveAtCommandPath( + 'nonexistent.ts', + mockConfig as unknown as Config, + ); + + expect(result).toBeNull(); + }); + + it('should resolve directory paths correctly', async () => { + const mockStats = { + isDirectory: () => true, + isFile: () => false, + }; + vi.mocked(fs.stat).mockResolvedValue(mockStats as unknown as fs.Stats); + + const result = await resolveAtCommandPath( + 'src', + mockConfig as unknown as Config, + ); + + expect(result?.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).toBeNull(); + }); + + it('should return null for unauthorized paths (letting calling site handle it, e.g. acpSession permission dialog)', async () => { + (mockConfig.validatePathAccess as Mock).mockReturnValue( + 'Outside workspace', + ); + (mockConfig.getTargetDir as Mock).mockReturnValue('/mock/workspace'); + + const mockStats = { + isDirectory: () => false, + isFile: () => true, + }; + vi.mocked(fs.stat).mockResolvedValue(mockStats as unknown as fs.Stats); + + // Path resolve will use /mock/root as base from mockWorkspaceContext + const result = await resolveAtCommandPath( + 'outside.txt', + mockConfig as unknown as Config, + ); + + // Should now return null so acpSession can trigger its own permission flow + expect(result).toBeNull(); + }); +}); diff --git a/packages/cli/src/utils/atCommandUtils.ts b/packages/cli/src/utils/atCommandUtils.ts index adb09851b7..a62bc3d496 100644 --- a/packages/cli/src/utils/atCommandUtils.ts +++ b/packages/cli/src/utils/atCommandUtils.ts @@ -6,11 +6,7 @@ import * as path from 'node:path'; import * as fs from 'node:fs/promises'; -import { - validatePath, - isWithinRoot, - type Config, -} from '@google/gemini-cli-core'; +import { validatePath, type Config } from '@google/gemini-cli-core'; export interface ResolvedAtCommandPath { absolutePath: string; @@ -52,16 +48,10 @@ export async function resolveAtCommandPath( // 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; - } + onDebugMessage( + `Skipping unauthorized path: ${absolutePath}. Reason: ${validationError}`, + ); + continue; } const stats = await fs.stat(absolutePath); diff --git a/packages/core/src/config/path-validation.consolidated.test.ts b/packages/core/src/config/path-validation.consolidated.test.ts new file mode 100644 index 0000000000..051291d24b --- /dev/null +++ b/packages/core/src/config/path-validation.consolidated.test.ts @@ -0,0 +1,61 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import path from 'node:path'; +import { Config, type ConfigParameters } from './config.js'; + +describe('Config Path Validation Integration', () => { + let config: Config; + const projectRoot = process.cwd(); + + beforeEach(() => { + const params: ConfigParameters = { + sessionId: 'test-session', + targetDir: projectRoot, + debugMode: false, + model: 'test-model', + cwd: projectRoot, + }; + config = new Config(params); + }); + + it('should reject pathologically long paths in validatePathAccess', () => { + const longPath = path.join(projectRoot, '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 in validatePathAccess', () => { + const logPath = path.join( + projectRoot, + '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 in validatePathAccess', () => { + const malformedPath = path.join(projectRoot, 'file\nwith\nnewline.txt'); + const result = config.validatePathAccess(malformedPath, 'read'); + expect(result).toContain('Invalid path: Path contains invalid characters'); + }); + + it('should allow normal paths in validatePathAccess', () => { + const normalPath = path.resolve(projectRoot, 'src/index.ts'); + const result = config.validatePathAccess(normalPath, 'read'); + + // It might return "outside workspace" if not fully initialized, + // but it should NOT return the "Invalid path" prefix from PathValidator. + if (result) { + expect(result).not.toContain('Invalid path:'); + } else { + expect(result).toBeNull(); + } + }); +}); diff --git a/packages/core/src/utils/path-validator.test.ts b/packages/core/src/utils/path-validator.test.ts index d0cc8c4af5..44b5e0b444 100644 --- a/packages/core/src/utils/path-validator.test.ts +++ b/packages/core/src/utils/path-validator.test.ts @@ -68,10 +68,25 @@ describe('PathValidator', () => { ).toBe(false); expect(validatePath('✓ test passed').isValid).toBe(false); expect(validatePath('× test failed').isValid).toBe(false); + expect( + validatePath('TestingLibraryElementError: Unable to find an element') + .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); }); + + 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 short paths with braces', () => { + expect(validatePath('{a,b}.ts').isValid).toBe(true); + }); });