Skip to content

Commit

Permalink
Avoid throwing validation error on invalid URIs
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
aeisenberg committed May 25, 2023
1 parent 3912995 commit 9c5706e
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 8 deletions.
13 changes: 10 additions & 3 deletions lib/upload-lib.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/upload-lib.js.map

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions lib/upload-lib.test.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/upload-lib.test.js.map

Large diffs are not rendered by default.

42 changes: 42 additions & 0 deletions src/testdata/with-invalid-uri.sarif
Original file line number Diff line number Diff line change
@@ -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"
}
}
]
}
17 changes: 17 additions & 0 deletions src/upload-lib.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
21 changes: 18 additions & 3 deletions src/upload-lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 9c5706e

Please sign in to comment.