Merge pull request #4283 from github/sam-robson/decode-diff-informed-sarif-uris

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

lib/entry-points.js

@@ -188776,11 +188776,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)
           );
         });
       });

src/upload-lib.test.ts

@@ -13,7 +13,11 @@ import * as diffUtils from "./diff-informed-analysis-utils";
 import { getEnv } from "./environment";
 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, withTmpDir } from "./util";
@@ -1015,12 +1019,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(
@@ -1044,3 +1098,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]);
+  },
+);

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

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)