Skip to content

Commit 7f7356c

Browse files
committed
Address PR #153 review comments
- resolvePromptFilePath: only enforce workspace-root boundary for relative path inputs; absolute paths pass through since users legitimately reference databases/queries outside the workspace - download-vscode.js: use fileURLToPath() instead of URL.pathname for cross-platform compatibility - integration-test-runner.js: track execution counts separately from pass/fail; runWorkflowIntegrationTests and runPromptIntegrationTests return { executed, passed } objects - Add test for absolute paths outside workspace being allowed
1 parent cb7387b commit 7f7356c

File tree

6 files changed

+62
-38
lines changed

6 files changed

+62
-38
lines changed

client/src/lib/integration-test-runner.js

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -225,11 +225,17 @@ export class IntegrationTestRunner {
225225
}
226226

227227
this.logger.log(`Completed ${totalIntegrationTests} tool-specific integration tests`);
228-
const toolIntegrationSucceeded = totalIntegrationTests > 0;
228+
229+
// Track execution counts and pass/fail separately.
230+
// *Count* tracks whether the suite discovered & ran tests.
231+
// *Succeeded* tracks whether those tests passed.
232+
const toolTestsExecuted = totalIntegrationTests;
233+
const toolTestsPassed = totalIntegrationTests > 0;
229234

230235
// Also run workflow integration tests
231-
const workflowIntegrationSucceeded = await this.runWorkflowIntegrationTests(baseDir);
232-
if (!workflowIntegrationSucceeded) {
236+
const { executed: workflowTestsExecuted, passed: workflowTestsPassed } =
237+
await this.runWorkflowIntegrationTests(baseDir);
238+
if (!workflowTestsPassed) {
233239
this.logger.logTest(
234240
"Workflow integration tests",
235241
false,
@@ -238,27 +244,27 @@ export class IntegrationTestRunner {
238244
}
239245

240246
// Also run prompt integration tests
241-
const promptIntegrationSucceeded = await this.runPromptIntegrationTests(baseDir);
242-
if (!promptIntegrationSucceeded) {
247+
const { executed: promptTestsExecuted, passed: promptTestsPassed } =
248+
await this.runPromptIntegrationTests(baseDir);
249+
if (!promptTestsPassed) {
243250
this.logger.logTest(
244251
"Prompt integration tests",
245252
false,
246253
new Error("Prompt integration tests did not complete successfully")
247254
);
248255
}
249256

250-
const anyTestsExecuted =
251-
toolIntegrationSucceeded || workflowIntegrationSucceeded || promptIntegrationSucceeded;
257+
const totalTestsExecuted = toolTestsExecuted + workflowTestsExecuted + promptTestsExecuted;
252258

253-
if (!anyTestsExecuted) {
259+
if (totalTestsExecuted === 0) {
254260
this.logger.log(
255-
"No integration tests completed successfully across tool, workflow, or prompt suites.",
261+
"No integration tests were executed across tool, workflow, or prompt suites.",
256262
"ERROR"
257263
);
258264
return false;
259265
}
260266

261-
return toolIntegrationSucceeded && workflowIntegrationSucceeded && promptIntegrationSucceeded;
267+
return toolTestsPassed && workflowTestsPassed && promptTestsPassed;
262268
} catch (error) {
263269
this.logger.log(`Error running integration tests: ${error.message}`, "ERROR");
264270
return false;
@@ -1183,10 +1189,13 @@ export class IntegrationTestRunner {
11831189
}
11841190

11851191
this.logger.log(`Total prompt integration tests executed: ${totalPromptTests}`);
1186-
return totalPromptTests > 0 ? allPromptTestsPassed : true;
1192+
return {
1193+
executed: totalPromptTests,
1194+
passed: totalPromptTests > 0 ? allPromptTestsPassed : true
1195+
};
11871196
} catch (error) {
11881197
this.logger.log(`Error running prompt integration tests: ${error.message}`, "ERROR");
1189-
return false;
1198+
return { executed: 0, passed: false };
11901199
}
11911200
}
11921201

@@ -1285,7 +1294,7 @@ export class IntegrationTestRunner {
12851294

12861295
if (!fs.existsSync(workflowTestsDir)) {
12871296
this.logger.log("No workflow integration tests directory found", "INFO");
1288-
return true;
1297+
return { executed: 0, passed: true };
12891298
}
12901299

12911300
// Discover workflow test directories
@@ -1296,7 +1305,7 @@ export class IntegrationTestRunner {
12961305

12971306
if (workflowDirs.length === 0) {
12981307
this.logger.log("No workflow test directories found", "INFO");
1299-
return true;
1308+
return { executed: 0, passed: true };
13001309
}
13011310

13021311
this.logger.log(`Found ${workflowDirs.length} workflow test(s): ${workflowDirs.join(", ")}`);
@@ -1309,10 +1318,10 @@ export class IntegrationTestRunner {
13091318
}
13101319

13111320
this.logger.log(`Total workflow integration tests executed: ${totalWorkflowTests}`);
1312-
return true;
1321+
return { executed: totalWorkflowTests, passed: true };
13131322
} catch (error) {
13141323
this.logger.log(`Error running workflow integration tests: ${error.message}`, "ERROR");
1315-
return false;
1324+
return { executed: 0, passed: false };
13161325
}
13171326
}
13181327

extensions/vscode/scripts/download-vscode.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import { existsSync, rmSync } from 'node:fs';
1616
import { join } from 'node:path';
17+
import { fileURLToPath } from 'node:url';
1718
import { downloadAndUnzipVSCode } from '@vscode/test-electron';
1819

1920
const MAX_RETRIES = 3;
@@ -34,7 +35,7 @@ process.on('unhandledRejection', (reason) => {
3435
* fresh instead of hitting a checksum mismatch on a truncated archive.
3536
*/
3637
function cleanPartialDownload() {
37-
const cacheDir = join(new URL('..', import.meta.url).pathname, '.vscode-test');
38+
const cacheDir = join(fileURLToPath(new URL('..', import.meta.url)), '.vscode-test');
3839
if (existsSync(cacheDir)) {
3940
console.log(' Cleaning .vscode-test/ to remove partial downloads...');
4041
rmSync(cacheDir, { recursive: true, force: true });

server/dist/codeql-development-mcp-server.js

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64506,14 +64506,17 @@ async function resolvePromptFilePath(filePath, workspaceRoot) {
6450664506
}
6450764507
const effectiveRoot = workspaceRoot ?? getUserWorkspaceDir();
6450864508
const normalizedPath = normalize(effectivePath);
64509-
const absolutePath = isAbsolute7(normalizedPath) ? normalizedPath : resolve13(effectiveRoot, normalizedPath);
64510-
const rel = relative(effectiveRoot, absolutePath);
64511-
if (rel === ".." || rel.startsWith(`..${sep2}`) || isAbsolute7(rel)) {
64512-
return {
64513-
blocked: true,
64514-
resolvedPath: "",
64515-
warning: "\u26A0 **File path resolves outside the workspace root.** The path has been blocked for security."
64516-
};
64509+
const inputWasAbsolute = isAbsolute7(normalizedPath);
64510+
const absolutePath = inputWasAbsolute ? normalizedPath : resolve13(effectiveRoot, normalizedPath);
64511+
if (!inputWasAbsolute) {
64512+
const rel = relative(effectiveRoot, absolutePath);
64513+
if (rel === ".." || rel.startsWith(`..${sep2}`) || isAbsolute7(rel)) {
64514+
return {
64515+
blocked: true,
64516+
resolvedPath: "",
64517+
warning: "\u26A0 **File path resolves outside the workspace root.** The path has been blocked for security."
64518+
};
64519+
}
6451764520
}
6451864521
try {
6451964522
await access2(absolutePath);

server/dist/codeql-development-mcp-server.js.map

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

server/src/prompts/workflow-prompts.ts

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -128,20 +128,23 @@ export async function resolvePromptFilePath(
128128
const normalizedPath = normalize(effectivePath);
129129

130130
// Resolve to absolute path.
131-
const absolutePath = isAbsolute(normalizedPath)
131+
const inputWasAbsolute = isAbsolute(normalizedPath);
132+
const absolutePath = inputWasAbsolute
132133
? normalizedPath
133134
: resolve(effectiveRoot, normalizedPath);
134135

135-
// Verify the resolved path stays within the workspace root.
136-
// This catches path traversal (e.g. "../../etc/passwd") after full
137-
// resolution rather than relying on a fragile substring check.
138-
const rel = relative(effectiveRoot, absolutePath);
139-
if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
140-
return {
141-
blocked: true,
142-
resolvedPath: '',
143-
warning: '⚠ **File path resolves outside the workspace root.** The path has been blocked for security.',
144-
};
136+
// Only enforce the workspace-root boundary for relative inputs.
137+
// Absolute paths are allowed through — users legitimately reference
138+
// databases, queries, and pack roots outside the workspace.
139+
if (!inputWasAbsolute) {
140+
const rel = relative(effectiveRoot, absolutePath);
141+
if (rel === '..' || rel.startsWith(`..${sep}`) || isAbsolute(rel)) {
142+
return {
143+
blocked: true,
144+
resolvedPath: '',
145+
warning: '⚠ **File path resolves outside the workspace root.** The path has been blocked for security.',
146+
};
147+
}
145148
}
146149

147150
// Check existence on disk (advisory only — the resolved path is always

server/test/src/prompts/workflow-prompts.test.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1787,6 +1787,14 @@ describe('Workflow Prompts', () => {
17871787
expect(result.warning).toContain('resolves outside the workspace root');
17881788
});
17891789

1790+
it('should allow absolute paths outside the workspace root', async () => {
1791+
const result = await resolvePromptFilePath('/tmp/external-db', testDir);
1792+
expect(result.blocked).toBeUndefined();
1793+
expect(result.resolvedPath).toBe('/tmp/external-db');
1794+
// The file likely does not exist, so a warning is expected.
1795+
expect(result.warning).toContain('does not exist');
1796+
});
1797+
17901798
it('should not flag filenames containing consecutive dots as traversal', async () => {
17911799
const filePath = 'my..query.ql';
17921800
writeFileSync(join(testDir, filePath), 'select 1');

0 commit comments

Comments
 (0)