From b05fe6986383c4668ec5e2da14c876844eefffa1 Mon Sep 17 00:00:00 2001 From: Sam Robson Date: Mon, 2 Nov 2026 15:28:08 +0000 Subject: [PATCH] fix: decode SARIF location URIs in diff-informed alert filtering --- lib/entry-points.js | 12 +- src/upload-lib.test.ts | 194 +++++++++++++++++- src/upload-lib.ts | 17 +- .../decode-diff-informed-sarif-uris.md | 1 + 4 files changed, 218 insertions(+), 6 deletions(-) create mode 100644 unreleased-change-notes/decode-diff-informed-sarif-uris.md diff --git a/lib/entry-points.js b/lib/entry-points.js index 9f802ddfac..a515ef9e9c 100644 --- a/lib/entry-points.js +++ b/lib/entry-points.js @@ -188534,11 +188534,19 @@ function filterAlertsByDiffRange(logger2, sarifLog) { return locations.some((physicalLocation) => { const locationUri = physicalLocation?.artifactLocation?.uri; const locationStartLine = physicalLocation?.region?.startLine; - if (!locationUri || locationStartLine === void 0) { + if (typeof locationUri !== "string" || !locationUri || locationStartLine === void 0) { return false; } + let decodedLocationUri = locationUri; + try { + decodedLocationUri = decodeURIComponent(locationUri); + } catch { + logger2.debug( + `Failed to decode location URI "${locationUri}", comparing it as a raw path` + ); + } return diffRanges.some( - (range2) => range2.path === locationUri && (range2.startLine <= locationStartLine && range2.endLine >= locationStartLine || range2.startLine === 0 && range2.endLine === 0) + (range2) => (range2.path === decodedLocationUri || range2.path === locationUri) && (range2.startLine <= locationStartLine && range2.endLine >= locationStartLine || range2.startLine === 0 && range2.endLine === 0) ); }); }); diff --git a/src/upload-lib.test.ts b/src/upload-lib.test.ts index ebfc5cd404..ce1ba57ed4 100644 --- a/src/upload-lib.test.ts +++ b/src/upload-lib.test.ts @@ -12,7 +12,11 @@ import * as api from "./api-client"; import * as diffUtils from "./diff-informed-analysis-utils"; import { Logger } from "./logging"; import * as sarif from "./sarif"; -import { RecordingLogger, setupTests } from "./testing-utils"; +import { + checkExpectedLogMessages, + RecordingLogger, + setupTests, +} from "./testing-utils"; import * as uploadLib from "./upload-lib"; import { UploadPayload } from "./upload-lib/types"; import { GitHubVariant, initializeEnvironment, withTmpDir } from "./util"; @@ -1014,12 +1018,62 @@ for (const analysisKind of analyses.supportedAnalysisKinds) { ); } +function createMockSarifWithResults( + results: sarif.Result[], +): Partial { + return { + version: "2.1.0", + runs: [ + { + tool: { + driver: { + name: "test tool", + }, + }, + results, + }, + ], + }; +} + +function createMockResultWithUri( + uri: string, + startLine = 2, + relatedUri?: string, +): sarif.Result { + return { + ruleId: "test-rule", + message: { text: "test message" }, + locations: [ + { + physicalLocation: { + artifactLocation: { uri }, + region: { startLine }, + }, + }, + ], + ...(relatedUri + ? { + relatedLocations: [ + { + physicalLocation: { + artifactLocation: { uri: relatedUri }, + region: { startLine }, + }, + }, + ], + } + : {}), + }; +} + function runFilterAlertsByDiffRange( input: Partial, diffRanges: diffUtils.DiffThunkRange[], + logger: Logger = new RecordingLogger(), ): Partial { sinon.stub(diffUtils, "readDiffRangesJsonFile").returns(diffRanges); - return uploadLib.filterAlertsByDiffRange(new RecordingLogger(), input); + return uploadLib.filterAlertsByDiffRange(logger, input); } test.serial( @@ -1043,3 +1097,139 @@ test.serial( t.deepEqual(actualOutput, expectedOutput); }, ); + +for (const [filePath, uri] of [ + ["with space/board.js", "with%20space/board.js"], + ["src/[id]/board.js", "src/%5Bid%5D/board.js"], + ["src/file#1.js", "src/file%231.js"], + ["src/100%.js", "src/100%25.js"], + ["src/literal%20.js", "src/literal%2520.js"], + ["src/foo:bar.js", "src/foo%3Abar.js"], + ["src/caf\u00e9.js", "src/caf%C3%A9.js"], + ["src/caf\u00e9.js", "src/caf%c3%a9.js"], +]) { + test.serial( + `filterAlertsByDiffRange matches percent-encoded URI ${uri}`, + (t) => { + const matchingResult = createMockResultWithUri(uri); + const nonMatchingResult = createMockResultWithUri("other.js"); + const input = createMockSarifWithResults([ + matchingResult, + nonMatchingResult, + ]); + + const actualOutput = runFilterAlertsByDiffRange(input, [ + { path: filePath, startLine: 1, endLine: 3 }, + ]); + + t.deepEqual(actualOutput.runs?.[0].results, [matchingResult]); + }, + ); +} + +test.serial( + "filterAlertsByDiffRange filters out percent-encoded URI when outside diff line range", + (t) => { + const outOfRangeResult = createMockResultWithUri( + "with%20space/board.js", + 2, + ); + const inRangeResult = createMockResultWithUri("with%20space/board.js", 15); + const input = createMockSarifWithResults([outOfRangeResult, inRangeResult]); + const actualOutput = runFilterAlertsByDiffRange(input, [ + { path: "with space/board.js", startLine: 10, endLine: 20 }, + ]); + + t.deepEqual(actualOutput.runs?.[0].results, [inRangeResult]); + }, +); + +test.serial( + "filterAlertsByDiffRange filters out percent-encoded URI when path does not match", + (t) => { + const input = createMockSarifWithResults([ + createMockResultWithUri("with%20space/board.js"), + ]); + const actualOutput = runFilterAlertsByDiffRange(input, [ + { path: "other/board.js", startLine: 1, endLine: 3 }, + ]); + + t.deepEqual(actualOutput.runs?.[0].results, []); + }, +); + +test.serial( + "filterAlertsByDiffRange keeps alert when relatedLocations has a matching percent-encoded URI", + (t) => { + const matchingRelatedResult = createMockResultWithUri( + "src/promiseUtils.js", + 2, + "with%20space/ParseObject.js", + ); + const nonMatchingResult = createMockResultWithUri("other.js"); + const input = createMockSarifWithResults([ + matchingRelatedResult, + nonMatchingResult, + ]); + + const actualOutput = runFilterAlertsByDiffRange(input, [ + { path: "with space/ParseObject.js", startLine: 1, endLine: 3 }, + ]); + + t.deepEqual(actualOutput.runs?.[0].results, [matchingRelatedResult]); + }, +); + +for (const uri of ["dir%ZZ/board.js", "src/bad%.js", "src/bad%C3%28.js"]) { + test.serial( + `filterAlertsByDiffRange falls back to raw path for malformed URI ${uri}`, + (t) => { + const logger = new RecordingLogger(); + const matchingResult = createMockResultWithUri(uri); + const input = createMockSarifWithResults([matchingResult]); + const actualOutput = runFilterAlertsByDiffRange( + input, + [{ path: uri, startLine: 1, endLine: 3 }], + logger, + ); + + t.deepEqual(actualOutput.runs?.[0].results, [matchingResult]); + checkExpectedLogMessages(t, logger.messages, [ + `Failed to decode location URI "${uri}", comparing it as a raw path`, + ]); + }, + ); +} + +test.serial( + "filterAlertsByDiffRange filters out malformed URI when raw path does not match", + (t) => { + const logger = new RecordingLogger(); + const input = createMockSarifWithResults([ + createMockResultWithUri("dir%ZZ/board.js"), + ]); + const actualOutput = runFilterAlertsByDiffRange( + input, + [{ path: "src/different.js", startLine: 1, endLine: 3 }], + logger, + ); + + t.deepEqual(actualOutput.runs?.[0].results, []); + checkExpectedLogMessages(t, logger.messages, [ + 'Failed to decode location URI "dir%ZZ/board.js", comparing it as a raw path', + ]); + }, +); + +test.serial( + "filterAlertsByDiffRange matches raw unencoded path containing valid percent-encoding sequence", + (t) => { + const matchingResult = createMockResultWithUri("dir/100%20done.js"); + const input = createMockSarifWithResults([matchingResult]); + const actualOutput = runFilterAlertsByDiffRange(input, [ + { path: "dir/100%20done.js", startLine: 1, endLine: 3 }, + ]); + + t.deepEqual(actualOutput.runs?.[0].results, [matchingResult]); + }, +); diff --git a/src/upload-lib.ts b/src/upload-lib.ts index 4e97c1d1db..52f6fdba66 100644 --- a/src/upload-lib.ts +++ b/src/upload-lib.ts @@ -1099,9 +1099,21 @@ export function filterAlertsByDiffRange( return locations.some((physicalLocation) => { const locationUri = physicalLocation?.artifactLocation?.uri; const locationStartLine = physicalLocation?.region?.startLine; - if (!locationUri || locationStartLine === undefined) { + if ( + typeof locationUri !== "string" || + !locationUri || + locationStartLine === undefined + ) { return false; } + let decodedLocationUri = locationUri; + try { + decodedLocationUri = decodeURIComponent(locationUri); + } catch { + logger.debug( + `Failed to decode location URI "${locationUri}", comparing it as a raw path`, + ); + } // Alert filtering here replicates the same behavior as the restrictAlertsTo // extensible predicate in CodeQL. See the restrictAlertsTo documentation // https://codeql.github.com/codeql-standard-libraries/csharp/codeql/util/AlertFiltering.qll/predicate.AlertFiltering$restrictAlertsTo.3.html @@ -1109,7 +1121,8 @@ export function filterAlertsByDiffRange( // of an alert location. return diffRanges.some( (range) => - range.path === locationUri && + (range.path === decodedLocationUri || + range.path === locationUri) && ((range.startLine <= locationStartLine && range.endLine >= locationStartLine) || (range.startLine === 0 && range.endLine === 0)), diff --git a/unreleased-change-notes/decode-diff-informed-sarif-uris.md b/unreleased-change-notes/decode-diff-informed-sarif-uris.md new file mode 100644 index 0000000000..b233ef4373 --- /dev/null +++ b/unreleased-change-notes/decode-diff-informed-sarif-uris.md @@ -0,0 +1 @@ +- Fixed diff-informed alert filtering incorrectly removing alerts in changed files whose paths contain spaces, special characters, or non-ASCII characters. [#4283](https://github.com/github/codeql-action/pull/4283)