Skip to content

Commit 30c4379

Browse files
data-douserdata-douser
committed
Addres PR review feedback
1 parent 2b1a3c9 commit 30c4379

File tree

6 files changed

+55
-48
lines changed

6 files changed

+55
-48
lines changed

‎client/src/lib/mcp-test-suite.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ export class MCPTestSuite {
148148
const result = await this.client.getPrompt({
149149
name: "explain_codeql_query",
150150
arguments: {
151+
databasePath: "nonexistent/path/to/database",
151152
queryPath: "nonexistent/path/to/query.ql",
152153
language: "javascript"
153154
}

‎extensions/vscode/test/suite/mcp-prompt-e2e.integration.test.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,6 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
126126
const result = await client.getPrompt({
127127
name: 'explain_codeql_query',
128128
arguments: {
129-
databasePath: 'nonexistent/database',
130129
queryPath: 'nonexistent/path/to/query.ql',
131130
language: 'javascript',
132131
},
@@ -154,7 +153,6 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
154153
const result = await client.getPrompt({
155154
name: 'explain_codeql_query',
156155
arguments: {
157-
databasePath: existingFile,
158156
queryPath: existingFile,
159157
language: 'javascript',
160158
},
@@ -185,7 +183,6 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
185183
const result = await client.getPrompt({
186184
name: 'explain_codeql_query',
187185
arguments: {
188-
databasePath: '/some/db',
189186
queryPath: '/some/query.ql',
190187
language: 'rust',
191188
},
@@ -247,7 +244,7 @@ suite('MCP Prompt Error Handling Integration Tests', () => {
247244

248245
const text = getFirstMessageText(result);
249246
assert.ok(
250-
text.includes('path traversal') || text.includes('Invalid file path'),
247+
text.includes('resolves outside the workspace root') || text.includes('does not exist'),
251248
`Response should warn about path traversal. Got:\n${text.slice(0, 500)}`,
252249
);
253250

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

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -64347,12 +64347,6 @@ function resolvePromptFilePath(filePath, workspaceRoot) {
6434764347
}
6434864348
const effectiveRoot = workspaceRoot ?? getUserWorkspaceDir();
6434964349
const normalizedPath = normalize(filePath);
64350-
if (normalizedPath.includes("..")) {
64351-
return {
64352-
resolvedPath: filePath,
64353-
warning: `\u26A0 **Invalid file path** \u2014 path traversal detected in \`${filePath}\`. Please provide a path within your workspace.`
64354-
};
64355-
}
6435664350
const absolutePath = isAbsolute7(normalizedPath) ? normalizedPath : resolve13(effectiveRoot, normalizedPath);
6435764351
const rel = relative(effectiveRoot, absolutePath);
6435864352
if (rel.startsWith("..") || isAbsolute7(rel)) {
@@ -64403,7 +64397,7 @@ var describeFalsePositivesSchema = external_exports.object({
6440364397
queryPath: external_exports.string().describe("Path to the CodeQL query file")
6440464398
});
6440564399
var explainCodeqlQuerySchema = external_exports.object({
64406-
databasePath: external_exports.string().describe("Path to a CodeQL database for profiling"),
64400+
databasePath: external_exports.string().optional().describe("Path to a CodeQL database for profiling"),
6440764401
language: external_exports.enum(SUPPORTED_LANGUAGES).describe("Programming language of the query"),
6440864402
queryPath: external_exports.string().describe("Path to the CodeQL query file (.ql or .qlref)")
6440964403
});
@@ -64809,16 +64803,21 @@ ${content}`
6480964803
const qpResult = resolvePromptFilePath(queryPath);
6481064804
const resolvedQueryPath = qpResult.resolvedPath;
6481164805
if (qpResult.warning) warnings.push(qpResult.warning);
64812-
const dbResult = resolvePromptFilePath(databasePath);
64813-
const resolvedDatabasePath = dbResult.resolvedPath;
64814-
if (dbResult.warning) warnings.push(dbResult.warning);
64806+
let resolvedDatabasePath = databasePath;
64807+
if (databasePath) {
64808+
const dbResult = resolvePromptFilePath(databasePath);
64809+
resolvedDatabasePath = dbResult.resolvedPath;
64810+
if (dbResult.warning) warnings.push(dbResult.warning);
64811+
}
6481564812
let contextSection = "## Query to Explain\n\n";
6481664813
contextSection += `- **Query Path**: ${resolvedQueryPath}
6481764814
`;
6481864815
contextSection += `- **Language**: ${language}
6481964816
`;
64820-
contextSection += `- **Database Path**: ${resolvedDatabasePath}
64817+
if (resolvedDatabasePath) {
64818+
contextSection += `- **Database Path**: ${resolvedDatabasePath}
6482164819
`;
64820+
}
6482264821
contextSection += "\n";
6482364822
const warningSection = warnings.length > 0 ? warnings.join("\n") + "\n\n" : "";
6482464823
return {

‎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: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -70,21 +70,14 @@ export function resolvePromptFilePath(
7070
// Normalise first to collapse any . or .. segments.
7171
const normalizedPath = normalize(filePath);
7272

73-
// Detect path traversal attempts.
74-
if (normalizedPath.includes('..')) {
75-
return {
76-
resolvedPath: filePath,
77-
warning: `⚠ **Invalid file path** — path traversal detected in \`${filePath}\`. Please provide a path within your workspace.`,
78-
};
79-
}
80-
8173
// Resolve to absolute path.
8274
const absolutePath = isAbsolute(normalizedPath)
8375
? normalizedPath
8476
: resolve(effectiveRoot, normalizedPath);
8577

86-
// If a workspace root was given (or inferred), verify the resolved path
87-
// stays within it.
78+
// Verify the resolved path stays within the workspace root.
79+
// This catches path traversal (e.g. "../../etc/passwd") after full
80+
// resolution rather than relying on a fragile substring check.
8881
const rel = relative(effectiveRoot, absolutePath);
8982
if (rel.startsWith('..') || isAbsolute(rel)) {
9083
return {
@@ -253,12 +246,13 @@ export const describeFalsePositivesSchema = z.object({
253246
/**
254247
* Schema for explain_codeql_query prompt parameters.
255248
*
256-
* All three parameters are **required** – the prompt needs a query, its
257-
* language, and a database to produce meaningful profiling output.
249+
* - `queryPath` and `language` are **required**.
250+
* - `databasePath` is optional – a database may also be derived from tests.
258251
*/
259252
export const explainCodeqlQuerySchema = z.object({
260253
databasePath: z
261254
.string()
255+
.optional()
262256
.describe('Path to a CodeQL database for profiling'),
263257
language: z
264258
.enum(SUPPORTED_LANGUAGES)
@@ -838,14 +832,19 @@ export function registerWorkflowPrompts(server: McpServer): void {
838832
const resolvedQueryPath = qpResult.resolvedPath;
839833
if (qpResult.warning) warnings.push(qpResult.warning);
840834

841-
const dbResult = resolvePromptFilePath(databasePath);
842-
const resolvedDatabasePath = dbResult.resolvedPath;
843-
if (dbResult.warning) warnings.push(dbResult.warning);
835+
let resolvedDatabasePath = databasePath;
836+
if (databasePath) {
837+
const dbResult = resolvePromptFilePath(databasePath);
838+
resolvedDatabasePath = dbResult.resolvedPath;
839+
if (dbResult.warning) warnings.push(dbResult.warning);
840+
}
844841

845842
let contextSection = '## Query to Explain\n\n';
846843
contextSection += `- **Query Path**: ${resolvedQueryPath}\n`;
847844
contextSection += `- **Language**: ${language}\n`;
848-
contextSection += `- **Database Path**: ${resolvedDatabasePath}\n`;
845+
if (resolvedDatabasePath) {
846+
contextSection += `- **Database Path**: ${resolvedDatabasePath}\n`;
847+
}
849848
contextSection += '\n';
850849

851850
const warningSection = warnings.length > 0

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

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* 7. All SUPPORTED_LANGUAGES are accepted by every schema with a language field.
1313
*/
1414

15-
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
15+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
1616
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
1717
import {
1818
buildToolsQueryContext,
@@ -524,45 +524,56 @@ describe('Workflow Prompts', () => {
524524

525525
it('should accept required queryPath and language', () => {
526526
const result = explainCodeqlQuerySchema.safeParse({
527-
databasePath: '/db',
528527
language: 'javascript',
529528
queryPath: '/q.ql',
530529
});
531530
expect(result.success).toBe(true);
532531
});
533532

534-
it('should reject missing databasePath', () => {
533+
it('should accept optional databasePath', () => {
535534
const result = explainCodeqlQuerySchema.safeParse({
535+
databasePath: '/db',
536536
language: 'python',
537537
queryPath: '/q.ql',
538538
});
539-
expect(result.success).toBe(false);
539+
expect(result.success).toBe(true);
540+
if (result.success) {
541+
expect(result.data.databasePath).toBe('/db');
542+
}
543+
});
544+
545+
it('should leave databasePath as undefined when omitted', () => {
546+
const result = explainCodeqlQuerySchema.safeParse({
547+
language: 'go',
548+
queryPath: '/q.ql',
549+
});
550+
expect(result.success).toBe(true);
551+
if (result.success) {
552+
expect(result.data.databasePath).toBeUndefined();
553+
}
540554
});
541555

542556
it('should reject missing queryPath', () => {
543557
const result = explainCodeqlQuerySchema.safeParse({
544-
databasePath: '/db',
545558
language: 'java',
546559
});
547560
expect(result.success).toBe(false);
548561
});
549562

550563
it('should reject missing language', () => {
551564
const result = explainCodeqlQuerySchema.safeParse({
552-
databasePath: '/db',
553565
queryPath: '/q.ql',
554566
});
555567
expect(result.success).toBe(false);
556568
});
557569

558-
it('should reject empty object (all three fields required)', () => {
570+
it('should reject empty object (both required fields missing)', () => {
559571
const result = explainCodeqlQuerySchema.safeParse({});
560572
expect(result.success).toBe(false);
561573
});
562574

563575
it.each([...SUPPORTED_LANGUAGES])('should accept language "%s"', (lang) => {
564576
const result = explainCodeqlQuerySchema.safeParse({
565-
databasePath: '/db',
566577
language: lang,
567578
queryPath: '/q.ql',
568579
});
@@ -818,8 +829,8 @@ describe('Workflow Prompts', () => {
818829
{
819830
name: 'explainCodeqlQuerySchema',
820831
schema: explainCodeqlQuerySchema,
821-
required: ['databasePath', 'language', 'queryPath'],
822-
optional: [],
832+
required: ['language', 'queryPath'],
833+
optional: ['databasePath'],
823834
},
824835
{
825836
name: 'documentCodeqlQuerySchema',
@@ -1035,7 +1046,7 @@ describe('Workflow Prompts', () => {
10351046
// Build a minimal-valid args map for each prompt to invoke its handler.
10361047
const minimalArgs: Record<string, Record<string, string>> = {
10371048
document_codeql_query: { language: 'java', queryPath: '/q.ql' },
1038-
explain_codeql_query: { databasePath: '/db', language: 'java', queryPath: '/q.ql' },
1049+
explain_codeql_query: { language: 'java', queryPath: '/q.ql' },
10391050
ql_lsp_iterative_development: { language: 'ruby', queryPath: '/q.ql' },
10401051
ql_tdd_advanced: { language: 'javascript' },
10411052
ql_tdd_basic: { language: 'javascript' },
@@ -1246,7 +1257,7 @@ describe('Workflow Prompts', () => {
12461257
expect(result.messages[0].content.text).toContain('Invalid input');
12471258
});
12481259

1249-
it('explain_codeql_query handler should include all required params', async () => {
1260+
it('explain_codeql_query handler should include all params when provided', async () => {
12501261
const handler = getRegisteredHandler(mockServer, 'explain_codeql_query');
12511262
const result: PromptResult = await handler({
12521263
databasePath: '/db/path',
@@ -1259,14 +1270,14 @@ describe('Workflow Prompts', () => {
12591270
expect(text).toContain('**Database Path**: /db/path');
12601271
});
12611272

1262-
it('explain_codeql_query handler should return inline error when databasePath missing', async () => {
1273+
it('explain_codeql_query handler should omit Database Path when not provided', async () => {
12631274
const handler = getRegisteredHandler(mockServer, 'explain_codeql_query');
12641275
const result: PromptResult = await handler({
12651276
language: 'java',
12661277
queryPath: '/q.ql',
12671278
});
12681279
const text = result.messages[0].content.text;
1269-
expect(text).toContain('Invalid input');
1280+
expect(text).not.toContain('**Database Path**');
12701281
});
12711282

12721283
it('document_codeql_query handler should include queryPath and language', async () => {
@@ -1700,7 +1711,7 @@ describe('Workflow Prompts', () => {
17001711
it('should return a warning for path traversal attempts', () => {
17011712
const result = resolvePromptFilePath('../../../etc/passwd', testDir);
17021713
expect(result.warning).toBeDefined();
1703-
expect(result.warning).toContain('path traversal');
1714+
expect(result.warning).toContain('resolves outside the workspace root');
17041715
});
17051716

17061717
it('should return the original path as resolvedPath even for invalid paths', () => {

0 commit comments

Comments
 (0)