diff --git a/packages/core/src/safety/built-in.test.ts b/packages/core/src/safety/built-in.test.ts index ecfc8e6bd5..3c966c0d2b 100644 --- a/packages/core/src/safety/built-in.test.ts +++ b/packages/core/src/safety/built-in.test.ts @@ -240,4 +240,72 @@ describe('AllowedPathChecker', () => { const result = await checker.check(input); expect(result.decision).toBe(SafetyCheckDecision.ALLOW); }); + + describe('Security Regression: Case-Insensitive Blocklist & .vscode HITL', () => { + it('should deny sensitive paths like .git, .env, and node_modules case-insensitively, including Windows trailing character and NTFS ADS bypasses', async () => { + const sensitivePaths = [ + path.join(mockCwd, '.git', 'config'), + path.join(mockCwd, '.GIT', 'config'), + path.join(mockCwd, '.Git', 'config'), + path.join(mockCwd, '.env'), + path.join(mockCwd, '.Env'), + path.join(mockCwd, '.ENV'), + path.join(mockCwd, 'node_modules', 'package', 'index.js'), + path.join(mockCwd, 'NODE_MODULES', 'package', 'index.js'), + // Windows trailing character bypasses + path.join(mockCwd, '.git ', 'config'), + path.join(mockCwd, '.git.', 'config'), + path.join(mockCwd, '.env ', 'config'), + path.join(mockCwd, '.env.', 'config'), + path.join(mockCwd, 'node_modules ', 'package', 'index.js'), + // NTFS Alternate Data Stream bypasses + path.join(mockCwd, '.git::$DATA', 'config'), + path.join(mockCwd, '.env::$DATA'), + path.join(mockCwd, 'node_modules::$DATA', 'package', 'index.js'), + ]; + + for (const p of sensitivePaths) { + const input = createInput({ path: p }); + const result = await checker.check(input); + expect(result.decision).toBe(SafetyCheckDecision.DENY); + expect(result.reason).toContain('Access to sensitive path'); + } + }); + + it('should require ASK_USER for .vscode configuration files inside workspace, but deny them if outside, including NTFS ADS bypasses', async () => { + const vscodePaths = [ + path.join(mockCwd, '.vscode', 'settings.json'), + path.join(mockCwd, '.vscode', 'settings.JSON'), + path.join(mockCwd, '.VSCODE', 'settings.json'), + path.join(mockCwd, '.vscode', 'launch.json'), + // Windows trailing character bypasses + path.join(mockCwd, '.vscode ', 'settings.json'), + path.join(mockCwd, '.vscode.', 'settings.json'), + // NTFS Alternate Data Stream bypasses + path.join(mockCwd, '.vscode::$DATA', 'settings.json'), + ]; + + for (const p of vscodePaths) { + const input = createInput({ path: p }); + const result = await checker.check(input); + expect(result.decision).toBe(SafetyCheckDecision.ASK_USER); + expect(result.reason).toContain( + 'Modifying .vscode configuration files requires explicit user confirmation', + ); + } + + // Verify that paths outside the workspace containing .vscode are strictly denied + const outsideVscodePaths = [ + path.join(testRootDir, 'outside', '.vscode', 'settings.json'), + path.join(testRootDir, 'outside', '.VSCODE', 'settings.json'), + ]; + + for (const p of outsideVscodePaths) { + const input = createInput({ path: p }); + const result = await checker.check(input); + expect(result.decision).toBe(SafetyCheckDecision.DENY); + expect(result.reason).toContain('outside of the allowed workspace'); + } + }); + }); }); diff --git a/packages/core/src/safety/built-in.ts b/packages/core/src/safety/built-in.ts index 6e1a91773b..a2be24ba36 100644 --- a/packages/core/src/safety/built-in.ts +++ b/packages/core/src/safety/built-in.ts @@ -5,13 +5,13 @@ */ import * as path from 'node:path'; -import * as fs from 'node:fs'; import { SafetyCheckDecision, type SafetyCheckInput, type SafetyCheckResult, } from './protocol.js'; import type { AllowedPathConfig } from '../policy/types.js'; +import { resolveToRealPath } from '../utils/paths.js'; /** * Interface for all in-process safety checkers. @@ -45,6 +45,11 @@ export class AllowedPathChecker implements InProcessChecker { excludedArgs, ); + // Resolve allowed directories once outside the loop to avoid redundant filesystem calls + const resolvedAllowedDirs = allowedDirs + .map((dir) => this.safelyResolvePath(dir, context.environment.cwd)) + .filter((resolvedDir): resolvedDir is string => resolvedDir !== null); + // Check each path for (const { path: p, argName } of pathsToCheck) { const resolvedPath = this.safelyResolvePath(p, context.environment.cwd); @@ -57,15 +62,52 @@ export class AllowedPathChecker implements InProcessChecker { }; } - const isAllowed = allowedDirs.some((dir) => { - // Also resolve allowed directories to handle symlinks - const resolvedDir = this.safelyResolvePath( - dir, - context.environment.cwd, - ); - if (!resolvedDir) return false; - return this.isPathAllowed(resolvedPath, resolvedDir); - }); + // Check for blocked segments case-insensitively + let hasBlockedSegment = false; + let isVscodePath = false; + + for (const resolvedDir of resolvedAllowedDirs) { + if (!this.isPathAllowed(resolvedPath, resolvedDir)) continue; + const relative = path.relative(resolvedDir, resolvedPath); + const segments = relative.split(path.sep); + for (const segment of segments) { + const clean = trimTrailingSpacesAndDots( + segment.split(':')[0], + ).toLowerCase(); + if ( + clean === '.git' || + clean === '.env' || + clean === 'node_modules' + ) { + hasBlockedSegment = true; + } + if (clean === '.vscode') { + isVscodePath = true; + } + } + } + + if (hasBlockedSegment) { + return { + decision: SafetyCheckDecision.DENY, + reason: `Access to sensitive path "${p}" in argument "${argName}" is blocked.`, + }; + } + + if (isVscodePath) { + return { + decision: SafetyCheckDecision.ASK_USER, + reason: `Modifying .vscode configuration files requires explicit user confirmation.`, + }; + } + + let isAllowed = false; + for (const resolvedDir of resolvedAllowedDirs) { + if (this.isPathAllowed(resolvedPath, resolvedDir)) { + isAllowed = true; + break; + } + } if (!isAllowed) { return { @@ -84,14 +126,15 @@ export class AllowedPathChecker implements InProcessChecker { // Walk up the directory tree until we find a path that exists let current = resolved; - // Stop at root (dirname(root) === root on many systems, or it becomes empty/'.' depending on implementation) while (current && current !== path.dirname(current)) { - if (fs.existsSync(current)) { - const canonical = fs.realpathSync(current); + try { + const canonical = resolveToRealPath(current); // Re-construct the full path from this canonical base const relative = path.relative(current, resolved); // path.join handles empty relative paths correctly (returns canonical) return path.join(canonical, relative); + } catch { + // Path does not exist, continue walking up } current = path.dirname(current); } @@ -156,3 +199,15 @@ export class AllowedPathChecker implements InProcessChecker { return paths; } } + +/** + * Trims trailing spaces and dots from a string without using regular expressions + * to completely eliminate any potential ReDoS (Regular Expression Denial of Service) risk. + */ +function trimTrailingSpacesAndDots(str: string): string { + let end = str.length - 1; + while (end >= 0 && (str[end] === ' ' || str[end] === '.')) { + end--; + } + return str.slice(0, end + 1); +} diff --git a/packages/core/src/tools/read-many-files.test.ts b/packages/core/src/tools/read-many-files.test.ts index 249a9970ac..6c630f871c 100644 --- a/packages/core/src/tools/read-many-files.test.ts +++ b/packages/core/src/tools/read-many-files.test.ts @@ -398,7 +398,7 @@ describe('ReadManyFilesTool', () => { }); it('should NOT use default excludes if useDefaultExcludes is false', async () => { - createFile('node_modules/some-lib/index.js', 'lib code'); + createFile('dist/some-lib/index.js', 'lib code'); createFile('src/app.js', 'app code'); const params = { include: ['**/*.js'], useDefaultExcludes: false }; const invocation = tool.build(params); @@ -406,10 +406,7 @@ describe('ReadManyFilesTool', () => { abortSignal: new AbortController().signal, }); const content = result.llmContent as string[]; - const expectedPath1 = path.join( - tempRootDir, - 'node_modules/some-lib/index.js', - ); + const expectedPath1 = path.join(tempRootDir, 'dist/some-lib/index.js'); const expectedPath2 = path.join(tempRootDir, 'src/app.js'); expect( content.some((c) => diff --git a/packages/core/src/utils/workspaceContext.test.ts b/packages/core/src/utils/workspaceContext.test.ts index 8c29819a79..1badf7a136 100644 --- a/packages/core/src/utils/workspaceContext.test.ts +++ b/packages/core/src/utils/workspaceContext.test.ts @@ -492,4 +492,50 @@ describe('WorkspaceContext with optional directories', () => { expect(directories).toEqual([cwd, existingDir1]); expect(debugLogger.warn).not.toHaveBeenCalled(); }); + + describe('Security Regression: Case-Insensitive Sensitive Path Blocklist', () => { + it('should reject sensitive paths like .git, .env, and node_modules case-insensitively, including Windows trailing character and NTFS ADS bypasses', () => { + const workspaceContext = new WorkspaceContext(cwd); + + const sensitivePaths = [ + path.join(cwd, '.git', 'config'), + path.join(cwd, '.GIT', 'config'), + path.join(cwd, '.Git', 'config'), + path.join(cwd, '.env'), + path.join(cwd, '.Env'), + path.join(cwd, '.ENV'), + path.join(cwd, 'node_modules', 'package', 'index.js'), + path.join(cwd, 'NODE_MODULES', 'package', 'index.js'), + // Windows trailing character bypasses + path.join(cwd, '.git ', 'config'), + path.join(cwd, '.git.', 'config'), + path.join(cwd, '.env ', 'config'), + path.join(cwd, '.env.', 'config'), + path.join(cwd, 'node_modules ', 'package', 'index.js'), + // NTFS Alternate Data Stream bypasses + path.join(cwd, '.git::$DATA', 'config'), + path.join(cwd, '.env::$DATA'), + path.join(cwd, 'node_modules::$DATA', 'package', 'index.js'), + ]; + + for (const p of sensitivePaths) { + expect(workspaceContext.isPathWithinWorkspace(p)).toBe(false); + } + }); + + it('should allow standard non-sensitive paths', () => { + const workspaceContext = new WorkspaceContext(cwd); + + const safePaths = [ + path.join(cwd, 'src', 'index.ts'), + path.join(cwd, '.gitignore'), + path.join(cwd, '.env.example'), + path.join(cwd, 'package.json'), + ]; + + for (const p of safePaths) { + expect(workspaceContext.isPathWithinWorkspace(p)).toBe(true); + } + }); + }); }); diff --git a/packages/core/src/utils/workspaceContext.ts b/packages/core/src/utils/workspaceContext.ts index 48c2fa2107..987cb40546 100755 --- a/packages/core/src/utils/workspaceContext.ts +++ b/packages/core/src/utils/workspaceContext.ts @@ -184,6 +184,20 @@ export class WorkspaceContext { for (const dir of this.directories) { if (this.isPathWithinRoot(fullyResolvedPath, dir)) { + // Check for blocked segments case-insensitively + const relative = path.relative(dir, fullyResolvedPath); + const segments = relative.split(path.sep); + const hasBlockedSegment = segments.some((segment) => { + const clean = trimTrailingSpacesAndDots( + segment.split(':')[0], + ).toLowerCase(); + return ( + clean === '.git' || clean === '.env' || clean === 'node_modules' + ); + }); + if (hasBlockedSegment) { + return false; + } return true; } } @@ -248,3 +262,15 @@ export class WorkspaceContext { ); } } + +/** + * Trims trailing spaces and dots from a string without using regular expressions + * to completely eliminate any potential ReDoS (Regular Expression Denial of Service) risk. + */ +function trimTrailingSpacesAndDots(str: string): string { + let end = str.length - 1; + while (end >= 0 && (str[end] === ' ' || str[end] === '.')) { + end--; + } + return str.slice(0, end + 1); +}