Skip to content

Commit 8f055f5

Browse files
Copilotdata-douser
Copilot
and
data-douser
authored
[UPDATE PRIMITIVE] sarif_diff_by_commits — refRange validation, file path improvement, test mock fix (#242)
* Initial plan * 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> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: data-douser <70299490+data-douser@users.noreply.github.com>
1 parent 7a7f8df commit 8f055f5

File tree

6 files changed

+104
-50
lines changed

6 files changed

+104
-50
lines changed

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189548,7 +189548,7 @@ function diffSarifByCommits(sarif, diffFiles, refRange, granularity = "file") {
189548189548
}
189549189549
}
189550189550
const classified = {
189551-
file: normalizeUri(uri),
189551+
file: matchingDiff ? matchingDiff.path : normalizeUri(uri),
189552189552
line: startLine,
189553189553
resultIndex: i,
189554189554
ruleId: result.ruleId
@@ -200832,6 +200832,14 @@ function registerSarifDiffByCommitsTool(server) {
200832200832
sarifPath: external_exports.string().optional().describe("Path to the SARIF file.")
200833200833
},
200834200834
async ({ sarifPath, cacheKey: cacheKey2, refRange, repoPath, granularity }) => {
200835+
if (/^\s*-/.test(refRange) || /\s/.test(refRange)) {
200836+
return {
200837+
content: [{
200838+
type: "text",
200839+
text: 'Invalid refRange: must not start with "-" or contain whitespace.'
200840+
}]
200841+
};
200842+
}
200835200843
const loaded = loadSarif({ sarifPath, cacheKey: cacheKey2 });
200836200844
if (loaded.error) {
200837200845
return { content: [{ type: "text", text: loaded.error }] };

‎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/lib/sarif-utils.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -946,7 +946,7 @@ export function diffSarifByCommits(
946946
}
947947

948948
const classified: ClassifiedResult = {
949-
file: normalizeUri(uri),
949+
file: matchingDiff ? matchingDiff.path : normalizeUri(uri),
950950
line: startLine,
951951
resultIndex: i,
952952
ruleId: result.ruleId,

‎server/src/tools/sarif-tools.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -373,6 +373,16 @@ function registerSarifDiffByCommitsTool(server: McpServer): void {
373373
sarifPath: z.string().optional().describe('Path to the SARIF file.'),
374374
},
375375
async ({ sarifPath, cacheKey, refRange, repoPath, granularity }) => {
376+
// Validate refRange to prevent git option injection
377+
if (/^\s*-/.test(refRange) || /\s/.test(refRange)) {
378+
return {
379+
content: [{
380+
type: 'text' as const,
381+
text: 'Invalid refRange: must not start with "-" or contain whitespace.',
382+
}],
383+
};
384+
}
385+
376386
// Load SARIF
377387
const loaded = loadSarif({ sarifPath, cacheKey });
378388
if (loaded.error) {

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1550,6 +1550,8 @@ describe('diffSarifByCommits', () => {
15501550
const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'file');
15511551

15521552
expect(result.newResults).toHaveLength(1);
1553+
// When matched, file should use the diff path, not the normalized URI
1554+
expect(result.newResults[0].file).toBe('src/db.js');
15531555
});
15541556
});
15551557

@@ -1652,6 +1654,26 @@ describe('diffSarifByCommits', () => {
16521654
expect(newResult.resultIndex).toBe(0);
16531655
});
16541656

1657+
it('should use normalized URI as file when result does not match any diff entry', () => {
1658+
const sarif: SarifDocument = {
1659+
version: '2.1.0',
1660+
runs: [{
1661+
tool: { driver: { name: 'CodeQL', rules: [{ id: 'r1' }] } },
1662+
results: [{
1663+
ruleId: 'r1',
1664+
message: { text: 'result' },
1665+
locations: [{ physicalLocation: { artifactLocation: { uri: 'file:///home/user/project/src/unmatched.js' }, region: { startLine: 1 } } }],
1666+
}],
1667+
}],
1668+
};
1669+
const diffFiles: DiffFileEntry[] = [{ path: 'src/other.js', hunks: [] }];
1670+
1671+
const result = diffSarifByCommits(sarif, diffFiles, 'main..HEAD', 'file');
1672+
1673+
expect(result.preExistingResults).toHaveLength(1);
1674+
expect(result.preExistingResults[0].file).toBe('home/user/project/src/unmatched.js');
1675+
});
1676+
16551677
it('should default granularity to file when not specified', () => {
16561678
const sarif = createDiffTestSarif();
16571679
const diffFiles: DiffFileEntry[] = [{ path: 'src/db.js', hunks: [] }];

‎server/test/src/tools/sarif-tools.test.ts‎

Lines changed: 60 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,13 @@ import { registerSarifTools } from '../../../src/tools/sarif-tools';
1111
import { sessionDataManager } from '../../../src/lib/session-data-manager';
1212
import { createProjectTempDir } from '../../../src/utils/temp-dir';
1313

14+
// Module-scope mock for cli-executor so the dynamic import in the handler
15+
// always resolves to the same controllable mock (prevents module-cache flakiness).
16+
const mockExecuteCLICommand = vi.fn();
17+
vi.mock('../../../src/lib/cli-executor', () => ({
18+
executeCLICommand: mockExecuteCLICommand,
19+
}));
20+
1421
// ---------------------------------------------------------------------------
1522
// Test fixtures
1623
// ---------------------------------------------------------------------------
@@ -507,20 +514,17 @@ describe('SARIF Tools', () => {
507514

508515
describe('sarif_diff_by_commits', () => {
509516
it('should classify results as new when their files appear in git diff', async () => {
510-
// Mock executeCLICommand to return a simulated git diff output
511-
vi.doMock('../../../src/lib/cli-executor', () => ({
512-
executeCLICommand: vi.fn().mockResolvedValue({
513-
success: true,
514-
stdout: [
515-
'diff --git a/src/db.js b/src/db.js',
516-
'--- a/src/db.js',
517-
'+++ b/src/db.js',
518-
'@@ -40,5 +40,5 @@',
519-
' some context',
520-
].join('\n'),
521-
stderr: '',
522-
}),
523-
}));
517+
mockExecuteCLICommand.mockResolvedValue({
518+
success: true,
519+
stdout: [
520+
'diff --git a/src/db.js b/src/db.js',
521+
'--- a/src/db.js',
522+
'+++ b/src/db.js',
523+
'@@ -40,5 +40,5 @@',
524+
' some context',
525+
].join('\n'),
526+
stderr: '',
527+
});
524528

525529
const result = await handlers.sarif_diff_by_commits({
526530
sarifPath: testSarifPath,
@@ -538,18 +542,16 @@ describe('SARIF Tools', () => {
538542
});
539543

540544
it('should classify all results as pre-existing when diff has no matching files', async () => {
541-
vi.doMock('../../../src/lib/cli-executor', () => ({
542-
executeCLICommand: vi.fn().mockResolvedValue({
543-
success: true,
544-
stdout: [
545-
'diff --git a/unrelated.txt b/unrelated.txt',
546-
'--- a/unrelated.txt',
547-
'+++ b/unrelated.txt',
548-
'@@ -1,1 +1,1 @@',
549-
].join('\n'),
550-
stderr: '',
551-
}),
552-
}));
545+
mockExecuteCLICommand.mockResolvedValue({
546+
success: true,
547+
stdout: [
548+
'diff --git a/unrelated.txt b/unrelated.txt',
549+
'--- a/unrelated.txt',
550+
'+++ b/unrelated.txt',
551+
'@@ -1,1 +1,1 @@',
552+
].join('\n'),
553+
stderr: '',
554+
});
553555

554556
const result = await handlers.sarif_diff_by_commits({
555557
sarifPath: testSarifPath,
@@ -569,14 +571,12 @@ describe('SARIF Tools', () => {
569571
});
570572

571573
it('should return error when git diff fails', async () => {
572-
vi.doMock('../../../src/lib/cli-executor', () => ({
573-
executeCLICommand: vi.fn().mockResolvedValue({
574-
success: false,
575-
stdout: '',
576-
stderr: 'fatal: bad revision',
577-
error: 'fatal: bad revision',
578-
}),
579-
}));
574+
mockExecuteCLICommand.mockResolvedValue({
575+
success: false,
576+
stdout: '',
577+
stderr: 'fatal: bad revision',
578+
error: 'fatal: bad revision',
579+
});
580580

581581
const result = await handlers.sarif_diff_by_commits({
582582
sarifPath: testSarifPath,
@@ -586,18 +586,16 @@ describe('SARIF Tools', () => {
586586
});
587587

588588
it('should support line-level granularity', async () => {
589-
vi.doMock('../../../src/lib/cli-executor', () => ({
590-
executeCLICommand: vi.fn().mockResolvedValue({
591-
success: true,
592-
stdout: [
593-
'diff --git a/src/db.js b/src/db.js',
594-
'--- a/src/db.js',
595-
'+++ b/src/db.js',
596-
'@@ -42,1 +42,1 @@',
597-
].join('\n'),
598-
stderr: '',
599-
}),
600-
}));
589+
mockExecuteCLICommand.mockResolvedValue({
590+
success: true,
591+
stdout: [
592+
'diff --git a/src/db.js b/src/db.js',
593+
'--- a/src/db.js',
594+
'+++ b/src/db.js',
595+
'@@ -42,1 +42,1 @@',
596+
].join('\n'),
597+
stderr: '',
598+
});
601599

602600
const result = await handlers.sarif_diff_by_commits({
603601
sarifPath: testSarifPath,
@@ -612,6 +610,22 @@ describe('SARIF Tools', () => {
612610
expect(newInDb).toHaveLength(1);
613611
expect(newInDb[0].line).toBe(42);
614612
});
613+
614+
it('should return error for refRange starting with a dash', async () => {
615+
const result = await handlers.sarif_diff_by_commits({
616+
sarifPath: testSarifPath,
617+
refRange: '--option-injection',
618+
});
619+
expect(result.content[0].text).toContain('Invalid refRange');
620+
});
621+
622+
it('should return error for refRange containing whitespace', async () => {
623+
const result = await handlers.sarif_diff_by_commits({
624+
sarifPath: testSarifPath,
625+
refRange: 'main HEAD',
626+
});
627+
expect(result.content[0].text).toContain('Invalid refRange');
628+
});
615629
});
616630
});
617631
});

0 commit comments

Comments
 (0)