Skip to content

Commit 9c5706e

Browse files
aeisenbergaeisenberg
committed
Avoid throwing validation error on invalid URIs
The recent update of jsonschema inadvertently caused extra validation of `uri-reference` formatted properties. This change ensures that these errors are converted to warnings. Note that we cannot revert the change to jsonschema since the old version does not handle `uniqueItems` correctly.
1 parent 3912995 commit 9c5706e

File tree

7 files changed

+101
-8
lines changed

7 files changed

+101
-8
lines changed

‎lib/upload-lib.js‎

Lines changed: 10 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎lib/upload-lib.js.map‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎lib/upload-lib.test.js‎

Lines changed: 12 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎lib/upload-lib.test.js.map‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/testdata/with-invalid-uri.sarif‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
{
2+
"$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json",
3+
"version": "2.1.0",
4+
"runs": [
5+
{
6+
"tool": {
7+
"driver": {
8+
"name": "LGTM.com",
9+
"organization": "Semmle",
10+
"version": "1.24.0-SNAPSHOT",
11+
"rules": []
12+
}
13+
},
14+
"results" : [ {
15+
"ruleId" : "js/unused-local-variable",
16+
"ruleIndex" : 0,
17+
"message" : {
18+
"text" : "Unused variable foo."
19+
},
20+
"locations" : [ {
21+
"physicalLocation" : {
22+
"artifactLocation" : {
23+
"uri" : "not a valid URI",
24+
"uriBaseId" : "%SRCROOT%",
25+
"index" : 0
26+
},
27+
"region" : {
28+
"startLine" : 2,
29+
"startColumn" : 7,
30+
"endColumn" : 10
31+
}
32+
}
33+
} ]
34+
} ],
35+
"columnKind": "utf16CodeUnits",
36+
"properties": {
37+
"semmle.formatSpecifier": "2.1.0",
38+
"semmle.sourceLanguage": "java"
39+
}
40+
}
41+
]
42+
}

‎src/upload-lib.test.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,23 @@ test("pruneInvalidResults", (t) => {
360360
t.assert(loggedMessages[0].includes("Pruned 2 results"));
361361
});
362362

363+
test("accept results with invalid artifactLocation.uri value", (t) => {
364+
const loggedMessages: string[] = [];
365+
const mockLogger = {
366+
info: (message: string) => {
367+
loggedMessages.push(message);
368+
},
369+
} as Logger;
370+
371+
const sarifFile = `${__dirname}/../src/testdata/with-invalid-uri.sarif`;
372+
uploadLib.validateSarifFileSchema(sarifFile, mockLogger);
373+
374+
t.deepEqual(loggedMessages.length, 1);
375+
t.deepEqual(
376+
loggedMessages[0],
377+
"Warning: 'not a valid URI' is not a valid URI in 'instance.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri'."
378+
);
379+
});
363380
const affectedCodeQLVersion = {
364381
driver: {
365382
name: "CodeQL",

‎src/upload-lib.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -228,17 +228,32 @@ export function validateSarifFileSchema(sarifFilePath: string, logger: Logger) {
228228
const schema = require("../src/sarif-schema-2.1.0.json") as jsonschema.Schema;
229229

230230
const result = new jsonschema.Validator().validate(sarif, schema);
231-
if (!result.valid) {
231+
// Filter errors related to invalid URIs in the artifactLocation field as this
232+
// is a breaking change. See https://github.com/github/codeql-action/issues/1703
233+
const errors = (result.errors || []).filter(
234+
(err) => err.argument !== "uri-reference"
235+
);
236+
const warnings = (result.errors || []).filter(
237+
(err) => err.argument === "uri-reference"
238+
);
239+
240+
for (const warning of warnings) {
241+
logger.info(
242+
`Warning: '${warning.instance}' is not a valid URI in '${warning.property}'.`
243+
);
244+
}
245+
246+
if (errors.length) {
232247
// Output the more verbose error messages in groups as these may be very large.
233-
for (const error of result.errors) {
248+
for (const error of errors) {
234249
logger.startGroup(`Error details: ${error.stack}`);
235250
logger.info(JSON.stringify(error, null, 2));
236251
logger.endGroup();
237252
}
238253

239254
// Set the main error message to the stacks of all the errors.
240255
// This should be of a manageable size and may even give enough to fix the error.
241-
const sarifErrors = result.errors.map((e) => `- ${e.stack}`);
256+
const sarifErrors = errors.map((e) => `- ${e.stack}`);
242257
throw new Error(
243258
`Unable to upload "${sarifFilePath}" as it is not valid SARIF:\n${sarifErrors.join(
244259
"\n"

0 commit comments

Comments
 (0)