Skip to content

Commit 812b2c9

Browse files
data-douserdata-douser
committed
Addres PR review feedback
1 parent 42e9a3d commit 812b2c9

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
@@ -64476,12 +64476,6 @@ function resolvePromptFilePath(filePath, workspaceRoot) {
6447664476
}
6447764477
const effectiveRoot = workspaceRoot ?? getUserWorkspaceDir();
6447864478
const normalizedPath = normalize(filePath);
64479-
if (normalizedPath.includes("..")) {
64480-
return {
64481-
resolvedPath: filePath,
64482-
warning: `\u26A0 **Invalid file path** \u2014 path traversal detected in \`${filePath}\`. Please provide a path within your workspace.`
64483-
};
64484-
}
6448564479
const absolutePath = isAbsolute7(normalizedPath) ? normalizedPath : resolve13(effectiveRoot, normalizedPath);
6448664480
const rel = relative(effectiveRoot, absolutePath);
6448764481
if (rel.startsWith("..") || isAbsolute7(rel)) {
@@ -64532,7 +64526,7 @@ var describeFalsePositivesSchema = external_exports.object({
6453264526
queryPath: external_exports.string().describe("Path to the CodeQL query file")
6453364527
});
6453464528
var explainCodeqlQuerySchema = external_exports.object({
64535-
databasePath: external_exports.string().describe("Path to a CodeQL database for profiling"),
64529+
databasePath: external_exports.string().optional().describe("Path to a CodeQL database for profiling"),
6453664530
language: external_exports.enum(SUPPORTED_LANGUAGES).describe("Programming language of the query"),
6453764531
queryPath: external_exports.string().describe("Path to the CodeQL query file (.ql or .qlref)")
6453864532
});
@@ -64951,16 +64945,21 @@ ${content}`
6495164945
const qpResult = resolvePromptFilePath(queryPath);
6495264946
const resolvedQueryPath = qpResult.resolvedPath;
6495364947
if (qpResult.warning) warnings.push(qpResult.warning);
64954-
const dbResult = resolvePromptFilePath(databasePath);
64955-
const resolvedDatabasePath = dbResult.resolvedPath;
64956-
if (dbResult.warning) warnings.push(dbResult.warning);
64948+
let resolvedDatabasePath = databasePath;
64949+
if (databasePath) {
64950+
const dbResult = resolvePromptFilePath(databasePath);
64951+
resolvedDatabasePath = dbResult.resolvedPath;
64952+
if (dbResult.warning) warnings.push(dbResult.warning);
64953+
}
6495764954
let contextSection = "## Query to Explain\n\n";
6495864955
contextSection += `- **Query Path**: ${resolvedQueryPath}
6495964956
`;
6496064957
contextSection += `- **Language**: ${language}
6496164958
`;
64962-
contextSection += `- **Database Path**: ${resolvedDatabasePath}
64959+
if (resolvedDatabasePath) {
64960+
contextSection += `- **Database Path**: ${resolvedDatabasePath}
6496364961
`;
64962+
}
6496464963
contextSection += "\n";
6496564964
const warningSection = warnings.length > 0 ? warnings.join("\n") + "\n\n" : "";
6496664965
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)
@@ -881,14 +875,19 @@ export function registerWorkflowPrompts(server: McpServer): void {
881875
const resolvedQueryPath = qpResult.resolvedPath;
882876
if (qpResult.warning) warnings.push(qpResult.warning);
883877

884-
const dbResult = resolvePromptFilePath(databasePath);
885-
const resolvedDatabasePath = dbResult.resolvedPath;
886-
if (dbResult.warning) warnings.push(dbResult.warning);
878+
let resolvedDatabasePath = databasePath;
879+
if (databasePath) {
880+
const dbResult = resolvePromptFilePath(databasePath);
881+
resolvedDatabasePath = dbResult.resolvedPath;
882+
if (dbResult.warning) warnings.push(dbResult.warning);
883+
}
887884

888885
let contextSection = '## Query to Explain\n\n';
889886
contextSection += `- **Query Path**: ${resolvedQueryPath}\n`;
890887
contextSection += `- **Language**: ${language}\n`;
891-
contextSection += `- **Database Path**: ${resolvedDatabasePath}\n`;
888+
if (resolvedDatabasePath) {
889+
contextSection += `- **Database Path**: ${resolvedDatabasePath}\n`;
890+
}
892891
contextSection += '\n';
893892

894893
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,
@@ -526,45 +526,56 @@ describe('Workflow Prompts', () => {
526526

527527
it('should accept required queryPath and language', () => {
528528
const result = explainCodeqlQuerySchema.safeParse({
529-
databasePath: '/db',
530529
language: 'javascript',
531530
queryPath: '/q.ql',
532531
});
533532
expect(result.success).toBe(true);
534533
});
535534

536-
it('should reject missing databasePath', () => {
535+
it('should accept optional databasePath', () => {
537536
const result = explainCodeqlQuerySchema.safeParse({
537+
databasePath: '/db',
538538
language: 'python',
539539
queryPath: '/q.ql',
540540
});
541-
expect(result.success).toBe(false);
541+
expect(result.success).toBe(true);
542+
if (result.success) {
543+
expect(result.data.databasePath).toBe('/db');
544+
}
545+
});
546+
547+
it('should leave databasePath as undefined when omitted', () => {
548+
const result = explainCodeqlQuerySchema.safeParse({
549+
language: 'go',
550+
queryPath: '/q.ql',
551+
});
552+
expect(result.success).toBe(true);
553+
if (result.success) {
554+
expect(result.data.databasePath).toBeUndefined();
555+
}
542556
});
543557

544558
it('should reject missing queryPath', () => {
545559
const result = explainCodeqlQuerySchema.safeParse({
546-
databasePath: '/db',
547560
language: 'java',
548561
});
549562
expect(result.success).toBe(false);
550563
});
551564

552565
it('should reject missing language', () => {
553566
const result = explainCodeqlQuerySchema.safeParse({
554-
databasePath: '/db',
555567
queryPath: '/q.ql',
556568
});
557569
expect(result.success).toBe(false);
558570
});
559571

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

565577
it.each([...SUPPORTED_LANGUAGES])('should accept language "%s"', (lang) => {
566578
const result = explainCodeqlQuerySchema.safeParse({
567-
databasePath: '/db',
568579
language: lang,
569580
queryPath: '/q.ql',
570581
});
@@ -820,8 +831,8 @@ describe('Workflow Prompts', () => {
820831
{
821832
name: 'explainCodeqlQuerySchema',
822833
schema: explainCodeqlQuerySchema,
823-
required: ['databasePath', 'language', 'queryPath'],
824-
optional: [],
834+
required: ['language', 'queryPath'],
835+
optional: ['databasePath'],
825836
},
826837
{
827838
name: 'documentCodeqlQuerySchema',
@@ -1054,7 +1065,7 @@ describe('Workflow Prompts', () => {
10541065
const minimalArgs: Record<string, Record<string, string>> = {
10551066
check_for_duplicated_code: { queryPath: '/q.ql' },
10561067
document_codeql_query: { language: 'java', queryPath: '/q.ql' },
1057-
explain_codeql_query: { databasePath: '/db', language: 'java', queryPath: '/q.ql' },
1068+
explain_codeql_query: { language: 'java', queryPath: '/q.ql' },
10581069
find_overlapping_queries: { language: 'cpp', queryDescription: 'detect placement-new on non-trivial types' },
10591070
ql_lsp_iterative_development: { language: 'ruby', queryPath: '/q.ql' },
10601071
ql_tdd_advanced: { language: 'javascript' },
@@ -1266,7 +1277,7 @@ describe('Workflow Prompts', () => {
12661277
expect(result.messages[0].content.text).toContain('Invalid input');
12671278
});
12681279

1269-
it('explain_codeql_query handler should include all required params', async () => {
1280+
it('explain_codeql_query handler should include all params when provided', async () => {
12701281
const handler = getRegisteredHandler(mockServer, 'explain_codeql_query');
12711282
const result: PromptResult = await handler({
12721283
databasePath: '/db/path',
@@ -1279,14 +1290,14 @@ describe('Workflow Prompts', () => {
12791290
expect(text).toContain('**Database Path**: /db/path');
12801291
});
12811292

1282-
it('explain_codeql_query handler should return inline error when databasePath missing', async () => {
1293+
it('explain_codeql_query handler should omit Database Path when not provided', async () => {
12831294
const handler = getRegisteredHandler(mockServer, 'explain_codeql_query');
12841295
const result: PromptResult = await handler({
12851296
language: 'java',
12861297
queryPath: '/q.ql',
12871298
});
12881299
const text = result.messages[0].content.text;
1289-
expect(text).toContain('Invalid input');
1300+
expect(text).not.toContain('**Database Path**');
12901301
});
12911302

12921303
it('document_codeql_query handler should include queryPath and language', async () => {
@@ -1720,7 +1731,7 @@ describe('Workflow Prompts', () => {
17201731
it('should return a warning for path traversal attempts', () => {
17211732
const result = resolvePromptFilePath('../../../etc/passwd', testDir);
17221733
expect(result.warning).toBeDefined();
1723-
expect(result.warning).toContain('path traversal');
1734+
expect(result.warning).toContain('resolves outside the workspace root');
17241735
});
17251736

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

0 commit comments

Comments
 (0)