Merge pull request #2370 from github/robertbrignull/undefined_codeflows

Avoid crashing when SARIF file contains invalid thread flow locations

Author: robertbrignull

extensions/ql-vscode/src/variant-analysis/markdown-generation.ts

@@ -275,17 +275,19 @@ function generateMarkdownForPathResults(
         threadFlow.highlightedRegion?.startLine,
         threadFlow.highlightedRegion?.endLine,
       );
-      const codeSnippet = generateMarkdownForCodeSnippet(
-        threadFlow.codeSnippet,
-        language,
-        threadFlow.highlightedRegion,
-      );
-      // Indent the snippet to fit with the numbered list.
-      // The indentation is "n + 2" where the list number is an n-digit number.
-      const codeSnippetIndented = codeSnippet.map((line) =>
-        (" ".repeat(listNumber.toString().length + 2) + line).trimEnd(),
-      );
-      pathLines.push(`${listNumber}. ${link}`, ...codeSnippetIndented);
+      pathLines.push(`${listNumber}. ${link}`);
+
+      if (threadFlow.codeSnippet) {
+        const codeSnippet = generateMarkdownForCodeSnippet(
+          threadFlow.codeSnippet,
+          language,
+          threadFlow.highlightedRegion,
+        );
+        const indentation = " ".repeat(listNumber.toString().length + 2);
+        pathLines.push(
+          ...codeSnippet.map((line) => (indentation + line).trimEnd()),
+        );
+      }
     }
     lines.push(...buildExpandableMarkdownSection(title, pathLines));
   }

extensions/ql-vscode/src/variant-analysis/sarif-processing.ts

@@ -168,6 +168,23 @@ export function tryGetRule(
   return undefined;
 }
 
+export function tryGetFilePath(
+  physicalLocation: sarif.PhysicalLocation,
+): string | undefined {
+  const filePath = physicalLocation.artifactLocation?.uri;
+  // We expect the location uri value to be a relative file path, with no scheme.
+  // We only need to support output from CodeQL here, so we can be quite strict,
+  // even though the SARIF spec supports many more types of URI.
+  if (
+    filePath === undefined ||
+    filePath === "" ||
+    filePath.startsWith("file:")
+  ) {
+    return undefined;
+  }
+  return filePath;
+}
+
 function getCodeSnippet(
   contextRegion?: sarif.Region,
   region?: sarif.Region,
@@ -238,13 +255,13 @@ function getCodeFlows(
 
   if (result.codeFlows) {
     for (const codeFlow of result.codeFlows) {
-      const threadFlows = [];
+      const threadFlows: ThreadFlow[] = [];
 
       for (const threadFlow of codeFlow.threadFlows) {
         for (const threadFlowLocation of threadFlow.locations) {
           const physicalLocation =
             threadFlowLocation!.location!.physicalLocation!;
-          const filePath = physicalLocation!.artifactLocation!.uri!;
+          const filePath = tryGetFilePath(physicalLocation);
           const codeSnippet = getCodeSnippet(
             physicalLocation.contextRegion,
             physicalLocation.region,
@@ -253,14 +270,16 @@ function getCodeFlows(
             ? getHighlightedRegion(physicalLocation.region)
             : undefined;
 
-          threadFlows.push({
-            fileLink: {
-              fileLinkPrefix,
-              filePath,
-            },
-            codeSnippet,
-            highlightedRegion,
-          } as ThreadFlow);
+          if (filePath !== undefined) {
+            threadFlows.push({
+              fileLink: {
+                fileLinkPrefix,
+                filePath,
+              },
+              codeSnippet,
+              highlightedRegion,
+            });
+          }
         }
       }
 

extensions/ql-vscode/src/variant-analysis/shared/analysis-result.ts

@@ -42,7 +42,7 @@ export interface CodeFlow {
 
 export interface ThreadFlow {
   fileLink: FileLink;
-  codeSnippet: CodeSnippet;
+  codeSnippet?: CodeSnippet;
   highlightedRegion?: HighlightedRegion;
   message?: AnalysisMessage;
 }

extensions/ql-vscode/test/unit-tests/sarif-processing.test.ts

@@ -1,6 +1,7 @@
 import * as sarif from "sarif";
 import {
   extractAnalysisAlerts,
+  tryGetFilePath,
   tryGetRule,
   tryGetSeverity,
 } from "../../src/variant-analysis/sarif-processing";
@@ -288,6 +289,51 @@ describe("SARIF processing", () => {
     });
   });
 
+  describe("tryGetFilePath", () => {
+    it("should return value when uri is a file path", () => {
+      const physicalLocation: sarif.PhysicalLocation = {
+        artifactLocation: {
+          uri: "foo/bar",
+        },
+      };
+      expect(tryGetFilePath(physicalLocation)).toBe("foo/bar");
+    });
+
+    it("should return undefined when uri has a file scheme", () => {
+      const physicalLocation: sarif.PhysicalLocation = {
+        artifactLocation: {
+          uri: "file:/",
+        },
+      };
+      expect(tryGetFilePath(physicalLocation)).toBe(undefined);
+    });
+
+    it("should return undefined when uri is empty", () => {
+      const physicalLocation: sarif.PhysicalLocation = {
+        artifactLocation: {
+          uri: "",
+        },
+      };
+      expect(tryGetFilePath(physicalLocation)).toBe(undefined);
+    });
+
+    it("should return undefined if artifact location uri is undefined", () => {
+      const physicalLocation: sarif.PhysicalLocation = {
+        artifactLocation: {
+          uri: undefined,
+        },
+      };
+      expect(tryGetFilePath(physicalLocation)).toBe(undefined);
+    });
+
+    it("should return undefined if artifact location is undefined", () => {
+      const physicalLocation: sarif.PhysicalLocation = {
+        artifactLocation: undefined,
+      };
+      expect(tryGetFilePath(physicalLocation)).toBe(undefined);
+    });
+  });
+
   describe("tryGetSeverity", () => {
     it("should return undefined if no rule set", () => {
       const result = {