Fix PR #236 review comments: refRange validation, ClassifiedResult.file, test mocking

1. Validate refRange in sarif_diff_by_commits to reject strings starting
   with '-' or containing whitespace (prevents git option injection).

2. Use matchingDiff.path for ClassifiedResult.file when a diff match exists,
   falling back to normalizeUri(uri) only for unmatched results (produces
   repo-relative paths instead of long file:// URI paths).

3. Replace vi.doMock with module-scope vi.mock + shared mockExecuteCLICommand
   to prevent module-cache flakiness in sarif_diff_by_commits handler tests.

Agent-Logs-Url: https://github.com/advanced-security/codeql-development-mcp-server/sessions/1960960b-9658-44b5-87d8-bc29cc55a5ef

Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>

Author: Copilot

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

@@ -189548,7 +189548,7 @@ function diffSarifByCommits(sarif, diffFiles, refRange, granularity = "file") {
       }
     }
     const classified = {
-      file: normalizeUri(uri),
+      file: matchingDiff ? matchingDiff.path : normalizeUri(uri),
       line: startLine,
       resultIndex: i,
       ruleId: result.ruleId
@@ -200832,6 +200832,14 @@ function registerSarifDiffByCommitsTool(server) {
       sarifPath: external_exports.string().optional().describe("Path to the SARIF file.")
     },
     async ({ sarifPath, cacheKey: cacheKey2, refRange, repoPath, granularity }) => {
+      if (/^\s*-/.test(refRange) || /\s/.test(refRange)) {
+        return {
+          content: [{
+            type: "text",
+            text: 'Invalid refRange: must not start with "-" or contain whitespace.'
+          }]
+        };
+      }
       const loaded = loadSarif({ sarifPath, cacheKey: cacheKey2 });
       if (loaded.error) {
         return { content: [{ type: "text", text: loaded.error }] };

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

@@ -946,7 +946,7 @@ export function diffSarifByCommits(
     }
 
     const classified: ClassifiedResult = {
-      file: normalizeUri(uri),
+      file: matchingDiff ? matchingDiff.path : normalizeUri(uri),
       line: startLine,
       resultIndex: i,
       ruleId: result.ruleId,

server/src/lib/sarif-utils.ts

@@ -373,6 +373,16 @@ function registerSarifDiffByCommitsTool(server: McpServer): void {
       sarifPath: z.string().optional().describe('Path to the SARIF file.'),
     },
     async ({ sarifPath, cacheKey, refRange, repoPath, granularity }) => {
+      // Validate refRange to prevent git option injection
+      if (/^\s*-/.test(refRange) || /\s/.test(refRange)) {
+        return {
+          content: [{
+            type: 'text' as const,
+            text: 'Invalid refRange: must not start with "-" or contain whitespace.',
+          }],
+        };
+      }
+
       // Load SARIF
       const loaded = loadSarif({ sarifPath, cacheKey });
       if (loaded.error) {

server/src/tools/sarif-tools.ts

@@ -1550,6 +1550,8 @@ describe('diffSarifByCommits', () => {
       const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'file');
 
       expect(result.newResults).toHaveLength(1);
+      // When matched, file should use the diff path, not the normalized URI
+      expect(result.newResults[0].file).toBe('src/db.js');
     });
   });
 
@@ -1652,6 +1654,26 @@ describe('diffSarifByCommits', () => {
       expect(newResult.resultIndex).toBe(0);
     });
 
+    it('should use normalized URI as file when result does not match any diff entry', () => {
+      const sarif: SarifDocument = {
+        version: '2.1.0',
+        runs: [{
+          tool: { driver: { name: 'CodeQL', rules: [{ id: 'r1' }] } },
+          results: [{
+            ruleId: 'r1',
+            message: { text: 'result' },
+            locations: [{ physicalLocation: { artifactLocation: { uri: 'file:///home/user/project/src/unmatched.js' }, region: { startLine: 1 } } }],
+          }],
+        }],
+      };
+      const diffFiles: DiffFileEntry[] = [{ path: 'src/other.js', hunks: [] }];
+
+      const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'file');
+
+      expect(result.preExistingResults).toHaveLength(1);
+      expect(result.preExistingResults[0].file).toBe('home/user/project/src/unmatched.js');
+    });
+
     it('should default granularity to file when not specified', () => {
       const sarif = createDiffTestSarif();
       const diffFiles: DiffFileEntry[] = [{ path: 'src/db.js', hunks: [] }];

server/test/src/lib/sarif-utils.test.ts

@@ -11,6 +11,13 @@ import { registerSarifTools } from '../../../src/tools/sarif-tools';
 import { sessionDataManager } from '../../../src/lib/session-data-manager';
 import { createProjectTempDir } from '../../../src/utils/temp-dir';
 
+// Module-scope mock for cli-executor so the dynamic import in the handler
+// always resolves to the same controllable mock (prevents module-cache flakiness).
+const mockExecuteCLICommand = vi.fn();
+vi.mock('../../../src/lib/cli-executor', () => ({
+  executeCLICommand: mockExecuteCLICommand,
+}));
+
 // ---------------------------------------------------------------------------
 // Test fixtures
 // ---------------------------------------------------------------------------
@@ -507,20 +514,17 @@ describe('SARIF Tools', () => {
 
     describe('sarif_diff_by_commits', () => {
       it('should classify results as new when their files appear in git diff', async () => {
-        // Mock executeCLICommand to return a simulated git diff output
-        vi.doMock('../../../src/lib/cli-executor', () => ({
-          executeCLICommand: vi.fn().mockResolvedValue({
-            success: true,
-            stdout: [
-              'diff --git a/src/db.js b/src/db.js',
-              '--- a/src/db.js',
-              '+++ b/src/db.js',
-              '@@ -40,5 +40,5 @@',
-              ' some context',
-            ].join('\n'),
-            stderr: '',
-          }),
-        }));
+        mockExecuteCLICommand.mockResolvedValue({
+          success: true,
+          stdout: [
+            'diff --git a/src/db.js b/src/db.js',
+            '--- a/src/db.js',
+            '+++ b/src/db.js',
+            '@@ -40,5 +40,5 @@',
+            ' some context',
+          ].join('\n'),
+          stderr: '',
+        });
 
         const result = await handlers.sarif_diff_by_commits({
           sarifPath: testSarifPath,
@@ -538,18 +542,16 @@ describe('SARIF Tools', () => {
       });
 
       it('should classify all results as pre-existing when diff has no matching files', async () => {
-        vi.doMock('../../../src/lib/cli-executor', () => ({
-          executeCLICommand: vi.fn().mockResolvedValue({
-            success: true,
-            stdout: [
-              'diff --git a/unrelated.txt b/unrelated.txt',
-              '--- a/unrelated.txt',
-              '+++ b/unrelated.txt',
-              '@@ -1,1 +1,1 @@',
-            ].join('\n'),
-            stderr: '',
-          }),
-        }));
+        mockExecuteCLICommand.mockResolvedValue({
+          success: true,
+          stdout: [
+            'diff --git a/unrelated.txt b/unrelated.txt',
+            '--- a/unrelated.txt',
+            '+++ b/unrelated.txt',
+            '@@ -1,1 +1,1 @@',
+          ].join('\n'),
+          stderr: '',
+        });
 
         const result = await handlers.sarif_diff_by_commits({
           sarifPath: testSarifPath,
@@ -569,14 +571,12 @@ describe('SARIF Tools', () => {
       });
 
       it('should return error when git diff fails', async () => {
-        vi.doMock('../../../src/lib/cli-executor', () => ({
-          executeCLICommand: vi.fn().mockResolvedValue({
-            success: false,
-            stdout: '',
-            stderr: 'fatal: bad revision',
-            error: 'fatal: bad revision',
-          }),
-        }));
+        mockExecuteCLICommand.mockResolvedValue({
+          success: false,
+          stdout: '',
+          stderr: 'fatal: bad revision',
+          error: 'fatal: bad revision',
+        });
 
         const result = await handlers.sarif_diff_by_commits({
           sarifPath: testSarifPath,
@@ -586,18 +586,16 @@ describe('SARIF Tools', () => {
       });
 
       it('should support line-level granularity', async () => {
-        vi.doMock('../../../src/lib/cli-executor', () => ({
-          executeCLICommand: vi.fn().mockResolvedValue({
-            success: true,
-            stdout: [
-              'diff --git a/src/db.js b/src/db.js',
-              '--- a/src/db.js',
-              '+++ b/src/db.js',
-              '@@ -42,1 +42,1 @@',
-            ].join('\n'),
-            stderr: '',
-          }),
-        }));
+        mockExecuteCLICommand.mockResolvedValue({
+          success: true,
+          stdout: [
+            'diff --git a/src/db.js b/src/db.js',
+            '--- a/src/db.js',
+            '+++ b/src/db.js',
+            '@@ -42,1 +42,1 @@',
+          ].join('\n'),
+          stderr: '',
+        });
 
         const result = await handlers.sarif_diff_by_commits({
           sarifPath: testSarifPath,
@@ -612,6 +610,22 @@ describe('SARIF Tools', () => {
         expect(newInDb).toHaveLength(1);
         expect(newInDb[0].line).toBe(42);
       });
+
+      it('should return error for refRange starting with a dash', async () => {
+        const result = await handlers.sarif_diff_by_commits({
+          sarifPath: testSarifPath,
+          refRange: '--option-injection',
+        });
+        expect(result.content[0].text).toContain('Invalid refRange');
+      });
+
+      it('should return error for refRange containing whitespace', async () => {
+        const result = await handlers.sarif_diff_by_commits({
+          sarifPath: testSarifPath,
+          refRange: 'main HEAD',
+        });
+        expect(result.content[0].text).toContain('Invalid refRange');
+      });
     });
   });
 });