Merge pull request #1705 from github/aeisenberg/location-uri-schema-fix

github · May 25, 2023 · d3314cc · d3314cc
2 parents 3912995 + 42add7b
commit d3314cc
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,7 +2,7 @@
 
 ## [UNRELEASED]
 
-No user facing changes.
+- Allow invalid URIs to be used as values to `artifactLocation.uri` properties. This reverses a change from [#1668](https://github.com/github/codeql-action/pull/1668) that inadvertently led to stricter validation of some URI values. [#1705](https://github.com/github/codeql-action/pull/1705)
 
 ## 2.3.4 - 24 May 2023
 

diff --git a/lib/upload-lib.js b/lib/upload-lib.js
diff --git a/lib/upload-lib.js.map b/lib/upload-lib.js.map
diff --git a/lib/upload-lib.test.js b/lib/upload-lib.test.js
diff --git a/lib/upload-lib.test.js.map b/lib/upload-lib.test.js.map
diff --git a/src/testdata/with-invalid-uri.sarif b/src/testdata/with-invalid-uri.sarif
@@ -0,0 +1,42 @@
+{
+  "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
+  "version": "2.1.0",
+  "runs": [
+    {
+      "tool": {
+        "driver": {
+          "name": "LGTM.com",
+          "organization": "Semmle",
+          "version": "1.24.0-SNAPSHOT",
+          "rules": []
+        }
+      },
+      "results" : [ {
+        "ruleId" : "js/unused-local-variable",
+        "ruleIndex" : 0,
+        "message" : {
+          "text" : "Unused variable foo."
+        },
+        "locations" : [ {
+          "physicalLocation" : {
+            "artifactLocation" : {
+              "uri" : "not a valid URI",
+              "uriBaseId" : "%SRCROOT%",
+              "index" : 0
+            },
+            "region" : {
+              "startLine" : 2,
+              "startColumn" : 7,
+              "endColumn" : 10
+            }
+          }
+        } ]
+      } ],
+      "columnKind": "utf16CodeUnits",
+      "properties": {
+        "semmle.formatSpecifier": "2.1.0",
+        "semmle.sourceLanguage": "java"
+      }
+    }
+  ]
+}
diff --git a/src/upload-lib.test.ts b/src/upload-lib.test.ts
@@ -360,6 +360,23 @@ test("pruneInvalidResults", (t) => {
   t.assert(loggedMessages[0].includes("Pruned 2 results"));
 });
 
+test("accept results with invalid artifactLocation.uri value", (t) => {
+  const loggedMessages: string[] = [];
+  const mockLogger = {
+    info: (message: string) => {
+      loggedMessages.push(message);
+    },
+  } as Logger;
+
+  const sarifFile = `${__dirname}/../src/testdata/with-invalid-uri.sarif`;
+  uploadLib.validateSarifFileSchema(sarifFile, mockLogger);
+
+  t.deepEqual(loggedMessages.length, 1);
+  t.deepEqual(
+    loggedMessages[0],
+    "Warning: 'not a valid URI' is not a valid URI in 'instance.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri'."
+  );
+});
 const affectedCodeQLVersion = {
   driver: {
     name: "CodeQL",

diff --git a/src/upload-lib.ts b/src/upload-lib.ts
@@ -228,17 +228,32 @@ export function validateSarifFileSchema(sarifFilePath: string, logger: Logger) {
   const schema = require("../src/sarif-schema-2.1.0.json") as jsonschema.Schema;
 
   const result = new jsonschema.Validator().validate(sarif, schema);
-  if (!result.valid) {
+  // Filter errors related to invalid URIs in the artifactLocation field as this
+  // is a breaking change. See https://github.com/github/codeql-action/issues/1703
+  const errors = (result.errors || []).filter(
+    (err) => err.argument !== "uri-reference"
+  );
+  const warnings = (result.errors || []).filter(
+    (err) => err.argument === "uri-reference"
+  );
+
+  for (const warning of warnings) {
+    logger.info(
+      `Warning: '${warning.instance}' is not a valid URI in '${warning.property}'.`
+    );
+  }
+
+  if (errors.length) {
     // Output the more verbose error messages in groups as these may be very large.
-    for (const error of result.errors) {
+    for (const error of errors) {
       logger.startGroup(`Error details: ${error.stack}`);
       logger.info(JSON.stringify(error, null, 2));
       logger.endGroup();
     }
 
     // Set the main error message to the stacks of all the errors.
     // This should be of a manageable size and may even give enough to fix the error.
-    const sarifErrors = result.errors.map((e) => `- ${e.stack}`);
+    const sarifErrors = errors.map((e) => `- ${e.stack}`);
     throw new Error(
       `Unable to upload "${sarifFilePath}" as it is not valid SARIF:\n${sarifErrors.join(
         "\n"