Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions packages/core/src/utils/pathReader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,113 @@ describe('readPathFromWorkspace', () => {
).rejects.toThrow('Path not found in workspace: not-found.txt');
});

it('should prevent path traversal outside the workspace via relative paths', async () => {
mock({
[CWD]: {},
[OUTSIDE_DIR]: {
'secret.txt': 'secrets',
},
});
const config = createMockConfig(CWD);
// Attempt to traverse out of CWD to OUTSIDE_DIR
const relativeTraversal = path.join('..', 'outside', 'secret.txt');
await expect(
readPathFromWorkspace(relativeTraversal, config),
).rejects.toThrow(`Path not found in workspace: ${relativeTraversal}`);
});

it('should prevent symlink escape outside the workspace', async () => {
mock({
[CWD]: {
'malicious-link': mock.symlink({
path: path.join(OUTSIDE_DIR, 'secret.txt'),
}),
},
[OUTSIDE_DIR]: {
'secret.txt': 'secrets',
},
});
const config = createMockConfig(CWD);
// Even if the link is in the workspace, its target is not.
await expect(
readPathFromWorkspace('malicious-link', config),
).rejects.toThrow('Path not found in workspace: malicious-link');
});

it('should block symlink escape inside a directory expansion (defense-in-depth)', async () => {
mock({
[CWD]: {
'allowed-dir': {
'legit.txt': 'legit content',
'malicious-link.txt': mock.symlink({
path: path.join(OUTSIDE_DIR, 'secret.txt'),
}),
},
},
[OUTSIDE_DIR]: {
'secret.txt': 'secrets',
},
});
const mockFileService = {
filterFiles: vi.fn((files) => files),
} as unknown as FileDiscoveryService;
const config = createMockConfig(CWD, [], mockFileService);
const result = await readPathFromWorkspace('allowed-dir', config);
const resultText = result
.map((p) => {
if (typeof p === 'string') return p;
if (typeof p === 'object' && p && 'text' in p) return p.text;
return '';
})
.join('');

// Legit content should be there
expect(resultText).toContain('legit content');
// Secret content should NOT be there, but a skip message SHOULD be
expect(resultText).not.toContain('secrets');
expect(resultText).toContain(
'--- Skipped malicious-link.txt: traverses outside workspace ---',
);
});

it('should push multiple skip messages if multiple traversals are found in a directory', async () => {
mock({
[CWD]: {
'bad-dir': {
'link1.txt': mock.symlink({ path: path.join(OUTSIDE_DIR, 's1.txt') }),
'link2.txt': mock.symlink({ path: path.join(OUTSIDE_DIR, 's2.txt') }),
'good.txt': 'good content',
},
},
[OUTSIDE_DIR]: {
's1.txt': 'secret1',
's2.txt': 'secret2',
},
});
const mockFileService = {
filterFiles: vi.fn((files) => files),
} as unknown as FileDiscoveryService;
const config = createMockConfig(CWD, [], mockFileService);
const result = await readPathFromWorkspace('bad-dir', config);
const resultText = result
.map((p) => {
if (typeof p === 'string') return p;
if (typeof p === 'object' && p && 'text' in p) return p.text;
return '';
})
.join('');

expect(resultText).toContain('good content');
expect(resultText).toContain(
'--- Skipped link1.txt: traverses outside workspace ---',
);
expect(resultText).toContain(
'--- Skipped link2.txt: traverses outside workspace ---',
);
expect(resultText).not.toContain('secret1');
expect(resultText).not.toContain('secret2');
});

// mock-fs permission simulation is unreliable on Windows.
it.skipIf(process.platform === 'win32')(
'should return an error string if reading a file with no permissions',
Expand Down
15 changes: 15 additions & 0 deletions packages/core/src/utils/pathReader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ export async function readPathFromWorkspace(
const searchDirs = workspace.getDirectories();
for (const dir of searchDirs) {
const potentialPath = path.resolve(dir, pathStr);

// Security check: ensure the resolved path is actually within the workspace.
if (!workspace.isPathWithinWorkspace(potentialPath)) {
Comment thread
galdawave marked this conversation as resolved.
continue;
}

try {
await fs.access(potentialPath);
absolutePath = potentialPath;
Expand Down Expand Up @@ -81,6 +87,15 @@ export async function readPathFromWorkspace(
);

for (const filePath of finalFiles) {
// Defense in depth: validate each file found within the directory.
if (!workspace.isPathWithinWorkspace(filePath)) {
const relativePathForDisplay = path.relative(absolutePath, filePath);
allParts.push({
text: `--- Skipped ${relativePathForDisplay}: traverses outside workspace ---\n\n`,
});
continue;
}
Comment on lines +91 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While this check is important for security (defense in depth), calling isPathWithinWorkspace for every file in a directory expansion can lead to significant performance degradation. isPathWithinWorkspace performs a realpath resolution which involves I/O. For directories with a large number of files, this will result in thousands of synchronous-like I/O operations. Consider optimizing this by checking if the file is a symbolic link first, or by using a more efficient path validation that doesn't require full resolution for every entry if the parent directory has already been validated and the entry is not a symlink.


const relativePathForDisplay = path.relative(absolutePath, filePath);
allParts.push({ text: `--- ${relativePathForDisplay} ---\n` });
const result = await processSingleFileContent(
Expand Down
Loading