Merge pull request #2487 from rvermeulen/rvermeulen/uri-errors-as-war…

…nings Turn URI errors into warnings
github · Sep 17, 2024 · cb28816 · cb28816
2 parents 34666c1 + 498c508
commit cb28816
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 32 deletions.
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
@@ -8,30 +8,42 @@
           "name": "LGTM.com",
           "organization": "Semmle",
           "version": "1.24.0-SNAPSHOT",
-          "rules": []
+          "rules": [
+            {
+              "id": "js/unused-local-variable",
+              "shortDescription": {
+                "text": "Unused local variable"
+              },
+              "helpUri": "not a valid URI"
+            }
+          ]
         }
       },
-      "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
+      "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",

diff --git a/src/upload-lib.test.ts b/src/upload-lib.test.ts
@@ -317,9 +317,10 @@ test("accept results with invalid artifactLocation.uri value", (t) => {
   const sarifFile = `${__dirname}/../src/testdata/with-invalid-uri.sarif`;
   uploadLib.validateSarifFileSchema(sarifFile, mockLogger);
 
-  t.deepEqual(loggedMessages.length, 2);
+  t.deepEqual(loggedMessages.length, 3);
   t.deepEqual(
     loggedMessages[1],
+    "Warning: 'not a valid URI' is not a valid URI in 'instance.runs[0].tool.driver.rules[0].helpUri'.",
     "Warning: 'not a valid URI' is not a valid URI in 'instance.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri'.",
   );
 });

diff --git a/src/upload-lib.ts b/src/upload-lib.ts
@@ -449,11 +449,20 @@ export function validateSarifFileSchema(sarifFilePath: string, logger: Logger) {
   const result = new jsonschema.Validator().validate(sarif, schema);
   // 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 warningAttributes = ["uri-reference", "uri"];
+  const errors = (result.errors ?? []).filter(
+    (err) =>
+      !(
+        err.name === "format" &&
+        typeof err.argument === "string" &&
+        warningAttributes.includes(err.argument)
+      ),
   );
-  const warnings = (result.errors || []).filter(
-    (err) => err.argument === "uri-reference",
+  const warnings = (result.errors ?? []).filter(
+    (err) =>
+      err.name === "format" &&
+      typeof err.argument === "string" &&
+      warningAttributes.includes(err.argument),
   );
 
   for (const warning of warnings) {