fix: decode SARIF location URIs in diff-informed alert filtering

Author: sam-robson

lib/entry-points.js

@@ -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<sarif.Log> {
+  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<sarif.Log>,
   diffRanges: diffUtils.DiffThunkRange[],
+  logger: Logger = new RecordingLogger(),
 ): Partial<sarif.Log> {
   sinon.stub(diffUtils, "readDiffRangesJsonFile").returns(diffRanges);
-  return uploadLib.filterAlertsByDiffRange(new RecordingLogger(), input);
+  return uploadLib.filterAlertsByDiffRange(logger, input);
 }
 
 test.serial(
@@ -1043,3 +1097,136 @@ 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 input = createMockSarifWithResults([
+      createMockResultWithUri("with%20space/board.js", 2),
+    ]);
+    const actualOutput = runFilterAlertsByDiffRange(input, [
+      { path: "with space/board.js", startLine: 10, endLine: 20 },
+    ]);
+
+    t.deepEqual(actualOutput.runs?.[0].results, []);
+  },
+);
+
+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]);
+  },
+);

src/upload-lib.test.ts

@@ -1099,17 +1099,30 @@ 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
           // for more details, such as why the filtering applies only to the first line
           // 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)),

src/upload-lib.ts

@@ -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)