Skip to content

Commit 946f01b

Browse files
luisfelipe-altluisfelipe-alt
committed
fix(security): enforce case-insensitive sensitive path blocklist and vscode hitl
1 parent 4e10a34 commit 946f01b

5 files changed

Lines changed: 210 additions & 18 deletions

File tree

‎packages/core/src/safety/built-in.test.ts‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,4 +240,72 @@ describe('AllowedPathChecker', () => {
240240
const result = await checker.check(input);
241241
expect(result.decision).toBe(SafetyCheckDecision.ALLOW);
242242
});
243+
244+
describe('Security Regression: Case-Insensitive Blocklist & .vscode HITL', () => {
245+
it('should deny sensitive paths like .git, .env, and node_modules case-insensitively, including Windows trailing character and NTFS ADS bypasses', async () => {
246+
const sensitivePaths = [
247+
path.join(mockCwd, '.git', 'config'),
248+
path.join(mockCwd, '.GIT', 'config'),
249+
path.join(mockCwd, '.Git', 'config'),
250+
path.join(mockCwd, '.env'),
251+
path.join(mockCwd, '.Env'),
252+
path.join(mockCwd, '.ENV'),
253+
path.join(mockCwd, 'node_modules', 'package', 'index.js'),
254+
path.join(mockCwd, 'NODE_MODULES', 'package', 'index.js'),
255+
// Windows trailing character bypasses
256+
path.join(mockCwd, '.git ', 'config'),
257+
path.join(mockCwd, '.git.', 'config'),
258+
path.join(mockCwd, '.env ', 'config'),
259+
path.join(mockCwd, '.env.', 'config'),
260+
path.join(mockCwd, 'node_modules ', 'package', 'index.js'),
261+
// NTFS Alternate Data Stream bypasses
262+
path.join(mockCwd, '.git::$DATA', 'config'),
263+
path.join(mockCwd, '.env::$DATA'),
264+
path.join(mockCwd, 'node_modules::$DATA', 'package', 'index.js'),
265+
];
266+
267+
for (const p of sensitivePaths) {
268+
const input = createInput({ path: p });
269+
const result = await checker.check(input);
270+
expect(result.decision).toBe(SafetyCheckDecision.DENY);
271+
expect(result.reason).toContain('Access to sensitive path');
272+
}
273+
});
274+
275+
it('should require ASK_USER for .vscode configuration files inside workspace, but deny them if outside, including NTFS ADS bypasses', async () => {
276+
const vscodePaths = [
277+
path.join(mockCwd, '.vscode', 'settings.json'),
278+
path.join(mockCwd, '.vscode', 'settings.JSON'),
279+
path.join(mockCwd, '.VSCODE', 'settings.json'),
280+
path.join(mockCwd, '.vscode', 'launch.json'),
281+
// Windows trailing character bypasses
282+
path.join(mockCwd, '.vscode ', 'settings.json'),
283+
path.join(mockCwd, '.vscode.', 'settings.json'),
284+
// NTFS Alternate Data Stream bypasses
285+
path.join(mockCwd, '.vscode::$DATA', 'settings.json'),
286+
];
287+
288+
for (const p of vscodePaths) {
289+
const input = createInput({ path: p });
290+
const result = await checker.check(input);
291+
expect(result.decision).toBe(SafetyCheckDecision.ASK_USER);
292+
expect(result.reason).toContain(
293+
'Modifying .vscode configuration files requires explicit user confirmation',
294+
);
295+
}
296+
297+
// Verify that paths outside the workspace containing .vscode are strictly denied
298+
const outsideVscodePaths = [
299+
path.join(testRootDir, 'outside', '.vscode', 'settings.json'),
300+
path.join(testRootDir, 'outside', '.VSCODE', 'settings.json'),
301+
];
302+
303+
for (const p of outsideVscodePaths) {
304+
const input = createInput({ path: p });
305+
const result = await checker.check(input);
306+
expect(result.decision).toBe(SafetyCheckDecision.DENY);
307+
expect(result.reason).toContain('outside of the allowed workspace');
308+
}
309+
});
310+
});
243311
});

‎packages/core/src/safety/built-in.ts‎

Lines changed: 68 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@
55
*/
66

77
import * as path from 'node:path';
8-
import * as fs from 'node:fs';
98
import {
109
SafetyCheckDecision,
1110
type SafetyCheckInput,
1211
type SafetyCheckResult,
1312
} from './protocol.js';
1413
import type { AllowedPathConfig } from '../policy/types.js';
14+
import { resolveToRealPath } from '../utils/paths.js';
1515

1616
/**
1717
* Interface for all in-process safety checkers.
@@ -45,6 +45,11 @@ export class AllowedPathChecker implements InProcessChecker {
4545
excludedArgs,
4646
);
4747

48+
// Resolve allowed directories once outside the loop to avoid redundant filesystem calls
49+
const resolvedAllowedDirs = allowedDirs
50+
.map((dir) => this.safelyResolvePath(dir, context.environment.cwd))
51+
.filter((resolvedDir): resolvedDir is string => resolvedDir !== null);
52+
4853
// Check each path
4954
for (const { path: p, argName } of pathsToCheck) {
5055
const resolvedPath = this.safelyResolvePath(p, context.environment.cwd);
@@ -57,15 +62,52 @@ export class AllowedPathChecker implements InProcessChecker {
5762
};
5863
}
5964

60-
const isAllowed = allowedDirs.some((dir) => {
61-
// Also resolve allowed directories to handle symlinks
62-
const resolvedDir = this.safelyResolvePath(
63-
dir,
64-
context.environment.cwd,
65-
);
66-
if (!resolvedDir) return false;
67-
return this.isPathAllowed(resolvedPath, resolvedDir);
68-
});
65+
// Check for blocked segments case-insensitively
66+
let hasBlockedSegment = false;
67+
let isVscodePath = false;
68+
69+
for (const resolvedDir of resolvedAllowedDirs) {
70+
if (!this.isPathAllowed(resolvedPath, resolvedDir)) continue;
71+
const relative = path.relative(resolvedDir, resolvedPath);
72+
const segments = relative.split(path.sep);
73+
for (const segment of segments) {
74+
const clean = trimTrailingSpacesAndDots(
75+
segment.split(':')[0],
76+
).toLowerCase();
77+
if (
78+
clean === '.git' ||
79+
clean === '.env' ||
80+
clean === 'node_modules'
81+
) {
82+
hasBlockedSegment = true;
83+
}
84+
if (clean === '.vscode') {
85+
isVscodePath = true;
86+
}
87+
}
88+
}
89+
90+
if (hasBlockedSegment) {
91+
return {
92+
decision: SafetyCheckDecision.DENY,
93+
reason: `Access to sensitive path "${p}" in argument "${argName}" is blocked.`,
94+
};
95+
}
96+
97+
if (isVscodePath) {
98+
return {
99+
decision: SafetyCheckDecision.ASK_USER,
100+
reason: `Modifying .vscode configuration files requires explicit user confirmation.`,
101+
};
102+
}
103+
104+
let isAllowed = false;
105+
for (const resolvedDir of resolvedAllowedDirs) {
106+
if (this.isPathAllowed(resolvedPath, resolvedDir)) {
107+
isAllowed = true;
108+
break;
109+
}
110+
}
69111

70112
if (!isAllowed) {
71113
return {
@@ -84,14 +126,15 @@ export class AllowedPathChecker implements InProcessChecker {
84126

85127
// Walk up the directory tree until we find a path that exists
86128
let current = resolved;
87-
// Stop at root (dirname(root) === root on many systems, or it becomes empty/'.' depending on implementation)
88129
while (current && current !== path.dirname(current)) {
89-
if (fs.existsSync(current)) {
90-
const canonical = fs.realpathSync(current);
130+
try {
131+
const canonical = resolveToRealPath(current);
91132
// Re-construct the full path from this canonical base
92133
const relative = path.relative(current, resolved);
93134
// path.join handles empty relative paths correctly (returns canonical)
94135
return path.join(canonical, relative);
136+
} catch {
137+
// Path does not exist, continue walking up
95138
}
96139
current = path.dirname(current);
97140
}
@@ -156,3 +199,15 @@ export class AllowedPathChecker implements InProcessChecker {
156199
return paths;
157200
}
158201
}
202+
203+
/**
204+
* Trims trailing spaces and dots from a string without using regular expressions
205+
* to completely eliminate any potential ReDoS (Regular Expression Denial of Service) risk.
206+
*/
207+
function trimTrailingSpacesAndDots(str: string): string {
208+
let end = str.length - 1;
209+
while (end >= 0 && (str[end] === ' ' || str[end] === '.')) {
210+
end--;
211+
}
212+
return str.slice(0, end + 1);
213+
}

‎packages/core/src/tools/read-many-files.test.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -398,18 +398,15 @@ describe('ReadManyFilesTool', () => {
398398
});
399399

400400
it('should NOT use default excludes if useDefaultExcludes is false', async () => {
401-
createFile('node_modules/some-lib/index.js', 'lib code');
401+
createFile('dist/some-lib/index.js', 'lib code');
402402
createFile('src/app.js', 'app code');
403403
const params = { include: ['**/*.js'], useDefaultExcludes: false };
404404
const invocation = tool.build(params);
405405
const result = await invocation.execute({
406406
abortSignal: new AbortController().signal,
407407
});
408408
const content = result.llmContent as string[];
409-
const expectedPath1 = path.join(
410-
tempRootDir,
411-
'node_modules/some-lib/index.js',
412-
);
409+
const expectedPath1 = path.join(tempRootDir, 'dist/some-lib/index.js');
413410
const expectedPath2 = path.join(tempRootDir, 'src/app.js');
414411
expect(
415412
content.some((c) =>

‎packages/core/src/utils/workspaceContext.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -492,4 +492,50 @@ describe('WorkspaceContext with optional directories', () => {
492492
expect(directories).toEqual([cwd, existingDir1]);
493493
expect(debugLogger.warn).not.toHaveBeenCalled();
494494
});
495+
496+
describe('Security Regression: Case-Insensitive Sensitive Path Blocklist', () => {
497+
it('should reject sensitive paths like .git, .env, and node_modules case-insensitively, including Windows trailing character and NTFS ADS bypasses', () => {
498+
const workspaceContext = new WorkspaceContext(cwd);
499+
500+
const sensitivePaths = [
501+
path.join(cwd, '.git', 'config'),
502+
path.join(cwd, '.GIT', 'config'),
503+
path.join(cwd, '.Git', 'config'),
504+
path.join(cwd, '.env'),
505+
path.join(cwd, '.Env'),
506+
path.join(cwd, '.ENV'),
507+
path.join(cwd, 'node_modules', 'package', 'index.js'),
508+
path.join(cwd, 'NODE_MODULES', 'package', 'index.js'),
509+
// Windows trailing character bypasses
510+
path.join(cwd, '.git ', 'config'),
511+
path.join(cwd, '.git.', 'config'),
512+
path.join(cwd, '.env ', 'config'),
513+
path.join(cwd, '.env.', 'config'),
514+
path.join(cwd, 'node_modules ', 'package', 'index.js'),
515+
// NTFS Alternate Data Stream bypasses
516+
path.join(cwd, '.git::$DATA', 'config'),
517+
path.join(cwd, '.env::$DATA'),
518+
path.join(cwd, 'node_modules::$DATA', 'package', 'index.js'),
519+
];
520+
521+
for (const p of sensitivePaths) {
522+
expect(workspaceContext.isPathWithinWorkspace(p)).toBe(false);
523+
}
524+
});
525+
526+
it('should allow standard non-sensitive paths', () => {
527+
const workspaceContext = new WorkspaceContext(cwd);
528+
529+
const safePaths = [
530+
path.join(cwd, 'src', 'index.ts'),
531+
path.join(cwd, '.gitignore'),
532+
path.join(cwd, '.env.example'),
533+
path.join(cwd, 'package.json'),
534+
];
535+
536+
for (const p of safePaths) {
537+
expect(workspaceContext.isPathWithinWorkspace(p)).toBe(true);
538+
}
539+
});
540+
});
495541
});

‎packages/core/src/utils/workspaceContext.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,20 @@ export class WorkspaceContext {
184184

185185
for (const dir of this.directories) {
186186
if (this.isPathWithinRoot(fullyResolvedPath, dir)) {
187+
// Check for blocked segments case-insensitively
188+
const relative = path.relative(dir, fullyResolvedPath);
189+
const segments = relative.split(path.sep);
190+
const hasBlockedSegment = segments.some((segment) => {
191+
const clean = trimTrailingSpacesAndDots(
192+
segment.split(':')[0],
193+
).toLowerCase();
194+
return (
195+
clean === '.git' || clean === '.env' || clean === 'node_modules'
196+
);
197+
});
198+
if (hasBlockedSegment) {
199+
return false;
200+
}
187201
return true;
188202
}
189203
}
@@ -248,3 +262,15 @@ export class WorkspaceContext {
248262
);
249263
}
250264
}
265+
266+
/**
267+
* Trims trailing spaces and dots from a string without using regular expressions
268+
* to completely eliminate any potential ReDoS (Regular Expression Denial of Service) risk.
269+
*/
270+
function trimTrailingSpacesAndDots(str: string): string {
271+
let end = str.length - 1;
272+
while (end >= 0 && (str[end] === ' ' || str[end] === '.')) {
273+
end--;
274+
}
275+
return str.slice(0, end + 1);
276+
}

0 commit comments

Comments
 (0)