robojerk/codeql-action

Merge pull request #2221 from github/nickfyson/upload-logging

Browse source 
improve logging coverage during sarif upload
This commit is contained in:
Nick Fyson 2024-04-04 17:30:55 +01:00 • committed by GitHub
commit 5f535debfe
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 54 additions and 24 deletions
Download patch file Download diff file Expand all files Collapse all files

4
lib/actions-util.test.js generated
View file

@ -227,7 +227,9 @@ const util_1 = require("./util");
const infoStub = sinon.stub(core, "info");
process.env["GITHUB_EVENT_NAME"] = "pull_request";
process.env["GITHUB_SHA"] = "100912429fab4cb230e66ffb11e738ac5194e73a";
await actionsUtil.determineMergeBaseCommitOid(path.join(__dirname, "../.."));
await (0, util_1.withTmpDir)(async (tmpDir) => {
await actionsUtil.determineMergeBaseCommitOid(tmpDir);
});
t.deepEqual(1, infoStub.callCount);
t.assert(infoStub.firstCall.args[0].startsWith("The checkout path provided to the action does not appear to be a git repository."));
infoStub.restore();

2
lib/actions-util.test.js.map
View file

File diff suppressed because one or more lines are too long

1
lib/fingerprints.js generated
View file

@ -238,6 +238,7 @@ exports.resolveUriToFile = resolveUriToFile;
// Compute fingerprints for results in the given sarif file
// and return an updated sarif file contents.
async function addFingerprints(sarif, sourceRoot, logger) {
logger.info("Adding fingerprints to SARIF file. For more information, see https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs");
// Gather together results for the same file and construct
// callbacks to accept hashes for that file and update the location
const callbacksByFile = {};

2
lib/fingerprints.js.map
View file

File diff suppressed because one or more lines are too long

20
lib/upload-lib.js generated
View file

@ -51,12 +51,14 @@ const GENERIC_403_MSG = "The repo on which this action is running has not opted-
const GENERIC_404_MSG = "The CodeQL code scanning feature is forbidden on this repository.";
// Takes a list of paths to sarif files and combines them together,
// returning the contents of the combined sarif file.
function combineSarifFiles(sarifFiles) {
function combineSarifFiles(sarifFiles, logger) {
logger.info(`Loading SARIF file(s)`);
const combinedSarif = {
version: null,
runs: [],
};
for (const sarifFile of sarifFiles) {
logger.debug(`Loading SARIF file: ${sarifFile}`);
const sarifObject = JSON.parse(fs.readFileSync(sarifFile, "utf8"));
// Check SARIF version
if (combinedSarif.version === null) {
@ -84,13 +86,14 @@ function areAllRunsProducedByCodeQL(sarifFiles) {
// CodeQL. Otherwise, it will fall back to combining the files in the action.
// Returns the contents of the combined sarif file.
async function combineSarifFilesUsingCLI(sarifFiles, gitHubVersion, features, logger) {
logger.info("Combining SARIF files using the CodeQL CLI");
if (sarifFiles.length === 1) {
return JSON.parse(fs.readFileSync(sarifFiles[0], "utf8"));
}
if (!areAllRunsProducedByCodeQL(sarifFiles)) {
logger.debug("Not all SARIF files were produced by CodeQL. Merging files in the action.");
// If not, use the naive method of combining the files.
return combineSarifFiles(sarifFiles);
return combineSarifFiles(sarifFiles, logger);
}
// Initialize CodeQL, either by using the config file from the 'init' step,
// or by initializing it here.
@ -116,7 +119,7 @@ async function combineSarifFilesUsingCLI(sarifFiles, gitHubVersion, features, lo
}
if (!(await codeQL.supportsFeature(tools_features_1.ToolsFeature.SarifMergeRunsFromEqualCategory))) {
logger.warning("The CodeQL CLI does not support merging SARIF files. Merging files in the action.");
return combineSarifFiles(sarifFiles);
return combineSarifFiles(sarifFiles, logger);
}
const baseTempDir = path.resolve(tempDir, "combined-sarif");
fs.mkdirSync(baseTempDir, { recursive: true });
@ -255,6 +258,7 @@ function countResultsInSarif(sarif) {
// Validates that the given file path refers to a valid SARIF file.
// Throws an error if the file is invalid.
function validateSarifFileSchema(sarifFilePath, logger) {
logger.info(`Validating ${sarifFilePath}`);
let sarif;
try {
sarif = JSON.parse(fs.readFileSync(sarifFilePath, "utf8"));
@ -287,7 +291,8 @@ function validateSarifFileSchema(sarifFilePath, logger) {
exports.validateSarifFileSchema = validateSarifFileSchema;
// buildPayload constructs a map ready to be uploaded to the API from the given
// parameters, respecting the current mode and target GitHub instance version.
function buildPayload(commitOid, ref, analysisKey, analysisName, zippedSarif, workflowRunID, workflowRunAttempt, checkoutURI, environment, toolNames, mergeBaseCommitOid) {
function buildPayload(commitOid, ref, analysisKey, analysisName, zippedSarif, workflowRunID, workflowRunAttempt, checkoutURI, environment, toolNames, mergeBaseCommitOid, logger) {
logger.info(`Combining SARIF files using CLI`);
const payloadObj = {
commit_oid: commitOid,
ref,
@ -337,15 +342,18 @@ async function uploadFiles(sarifFiles, repositoryNwo, commitOid, ref, analysisKe
}
let sarif = (await features.getValue(feature_flags_1.Feature.CliSarifMerge))
? await combineSarifFilesUsingCLI(sarifFiles, gitHubVersion, features, logger)
: combineSarifFiles(sarifFiles);
: combineSarifFiles(sarifFiles, logger);
sarif = await fingerprints.addFingerprints(sarif, sourceRoot, logger);
sarif = populateRunAutomationDetails(sarif, category, analysisKey, environment);
const toolNames = util.getToolNames(sarif);
logger.debug(`Validating that each SARIF run has a unique category`);
validateUniqueCategory(sarif);
logger.debug(`Serializing SARIF for upload`);
const sarifPayload = JSON.stringify(sarif);
logger.debug(`Compressing serialized SARIF`);
const zippedSarif = zlib_1.default.gzipSync(sarifPayload).toString("base64");
const checkoutURI = (0, file_url_1.default)(sourceRoot);
const payload = buildPayload(commitOid, ref, analysisKey, analysisName, zippedSarif, workflowRunID, workflowRunAttempt, checkoutURI, environment, toolNames, await actionsUtil.determineMergeBaseCommitOid());
const payload = buildPayload(commitOid, ref, analysisKey, analysisName, zippedSarif, workflowRunID, workflowRunAttempt, checkoutURI, environment, toolNames, await actionsUtil.determineMergeBaseCommitOid(), logger);
// Log some useful debug info about the info
const rawUploadSizeBytes = sarifPayload.length;
logger.debug(`Raw upload size: ${rawUploadSizeBytes} bytes`);

2
lib/upload-lib.js.map
View file

File diff suppressed because one or more lines are too long

10
lib/upload-lib.test.js generated
View file

@ -47,7 +47,7 @@ ava_1.default.beforeEach(() => {
});
(0, ava_1.default)("validate correct payload used for push, PR merge commit, and PR head", async (t) => {
process.env["GITHUB_EVENT_NAME"] = "push";
const pushPayload = uploadLib.buildPayload("commit", "refs/heads/master", "key", undefined, "", 1234, 1, "/opt/src", undefined, ["CodeQL", "eslint"], "mergeBaseCommit");
const pushPayload = uploadLib.buildPayload("commit", "refs/heads/master", "key", undefined, "", 1234, 1, "/opt/src", undefined, ["CodeQL", "eslint"], "mergeBaseCommit", (0, logging_1.getRunnerLogger)(true));
// Not triggered by a pull request
t.falsy(pushPayload.base_ref);
t.falsy(pushPayload.base_sha);
@ -55,11 +55,11 @@ ava_1.default.beforeEach(() => {
process.env["GITHUB_SHA"] = "commit";
process.env["GITHUB_BASE_REF"] = "master";
process.env["GITHUB_EVENT_PATH"] = `${__dirname}/../src/testdata/pull_request.json`;
const prMergePayload = uploadLib.buildPayload("commit", "refs/pull/123/merge", "key", undefined, "", 1234, 1, "/opt/src", undefined, ["CodeQL", "eslint"], "mergeBaseCommit");
const prMergePayload = uploadLib.buildPayload("commit", "refs/pull/123/merge", "key", undefined, "", 1234, 1, "/opt/src", undefined, ["CodeQL", "eslint"], "mergeBaseCommit", (0, logging_1.getRunnerLogger)(true));
// Uploads for a merge commit use the merge base
t.deepEqual(prMergePayload.base_ref, "refs/heads/master");
t.deepEqual(prMergePayload.base_sha, "mergeBaseCommit");
const prHeadPayload = uploadLib.buildPayload("headCommit", "refs/pull/123/head", "key", undefined, "", 1234, 1, "/opt/src", undefined, ["CodeQL", "eslint"], "mergeBaseCommit");
const prHeadPayload = uploadLib.buildPayload("headCommit", "refs/pull/123/head", "key", undefined, "", 1234, 1, "/opt/src", undefined, ["CodeQL", "eslint"], "mergeBaseCommit", (0, logging_1.getRunnerLogger)(true));
// Uploads for the head use the PR base
t.deepEqual(prHeadPayload.base_ref, "refs/heads/master");
t.deepEqual(prHeadPayload.base_sha, "f95f852bd8fca8fcc58a9a2d6c842781e32a215e");
@ -192,8 +192,8 @@ ava_1.default.beforeEach(() => {
};
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'.");
t.deepEqual(loggedMessages.length, 2);
t.deepEqual(loggedMessages[1], "Warning: 'not a valid URI' is not a valid URI in 'instance.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri'.");
});
function createMockSarif(id, tool) {
return {

2
lib/upload-lib.test.js.map
View file

File diff suppressed because one or more lines are too long

7
src/actions-util.test.ts
View file

@ -286,14 +286,17 @@ test("determineMergeBaseCommitOid no error", async (t) => {
process.env["GITHUB_EVENT_NAME"] = "pull_request";
process.env["GITHUB_SHA"] = "100912429fab4cb230e66ffb11e738ac5194e73a";
await actionsUtil.determineMergeBaseCommitOid(path.join(__dirname, "../.."));
await withTmpDir(async (tmpDir) => {
await actionsUtil.determineMergeBaseCommitOid(tmpDir);
});
t.deepEqual(1, infoStub.callCount);
t.assert(
infoStub.firstCall.args[0].startsWith(
"The checkout path provided to the action does not appear to be a git repository.",
),
);
infoStub.restore();
});

3
src/fingerprints.ts
View file

@ -259,6 +259,9 @@ export async function addFingerprints(
sourceRoot: string,
logger: Logger,
): Promise<SarifFile> {
logger.info(
"Adding fingerprints to SARIF file. For more information, see https://docs.github.com/en/enterprise-cloud@latest/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning#providing-data-to-track-code-scanning-alerts-across-runs",
);
// Gather together results for the same file and construct
// callbacks to accept hashes for that file and update the location
const callbacksByFile: { [filename: string]: hashCallback[] } = {};

7
src/upload-lib.test.ts
View file

@ -42,6 +42,7 @@ test("validate correct payload used for push, PR merge commit, and PR head", asy
undefined,
["CodeQL", "eslint"],
"mergeBaseCommit",
getRunnerLogger(true),
);
// Not triggered by a pull request
t.falsy(pushPayload.base_ref);
@ -65,6 +66,7 @@ test("validate correct payload used for push, PR merge commit, and PR head", asy
undefined,
["CodeQL", "eslint"],
"mergeBaseCommit",
getRunnerLogger(true),
);
// Uploads for a merge commit use the merge base
t.deepEqual(prMergePayload.base_ref, "refs/heads/master");
@ -82,6 +84,7 @@ test("validate correct payload used for push, PR merge commit, and PR head", asy
undefined,
["CodeQL", "eslint"],
"mergeBaseCommit",
getRunnerLogger(true),
);
// Uploads for the head use the PR base
t.deepEqual(prHeadPayload.base_ref, "refs/heads/master");
@ -317,9 +320,9 @@ 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, 1);
t.deepEqual(loggedMessages.length, 2);
t.deepEqual(
loggedMessages[0],
loggedMessages[1],
"Warning: 'not a valid URI' is not a valid URI in 'instance.runs[0].results[0].locations[0].physicalLocation.artifactLocation.uri'.",
);
});

18
src/upload-lib.ts
View file

@ -36,13 +36,15 @@ const GENERIC_404_MSG =
// Takes a list of paths to sarif files and combines them together,
// returning the contents of the combined sarif file.
function combineSarifFiles(sarifFiles: string[]): SarifFile {
function combineSarifFiles(sarifFiles: string[], logger: Logger): SarifFile {
logger.info(`Loading SARIF file(s)`);
const combinedSarif: SarifFile = {
version: null,
runs: [],
};
for (const sarifFile of sarifFiles) {
logger.debug(`Loading SARIF file: ${sarifFile}`);
const sarifObject = JSON.parse(
fs.readFileSync(sarifFile, "utf8"),
) as SarifFile;
@ -87,6 +89,7 @@ async function combineSarifFilesUsingCLI(
features: Features,
logger: Logger,
): Promise<SarifFile> {
logger.info("Combining SARIF files using the CodeQL CLI");
if (sarifFiles.length === 1) {
return JSON.parse(fs.readFileSync(sarifFiles[0], "utf8")) as SarifFile;
}
@ -97,7 +100,7 @@ async function combineSarifFilesUsingCLI(
);
// If not, use the naive method of combining the files.
return combineSarifFiles(sarifFiles);
return combineSarifFiles(sarifFiles, logger);
}
// Initialize CodeQL, either by using the config file from the 'init' step,
@ -146,7 +149,7 @@ async function combineSarifFilesUsingCLI(
"The CodeQL CLI does not support merging SARIF files. Merging files in the action.",
);
return combineSarifFiles(sarifFiles);
return combineSarifFiles(sarifFiles, logger);
}
const baseTempDir = path.resolve(tempDir, "combined-sarif");
@ -356,6 +359,7 @@ function countResultsInSarif(sarif: string): number {
// Validates that the given file path refers to a valid SARIF file.
// Throws an error if the file is invalid.
export function validateSarifFileSchema(sarifFilePath: string, logger: Logger) {
logger.info(`Validating ${sarifFilePath}`);
let sarif;
try {
sarif = JSON.parse(fs.readFileSync(sarifFilePath, "utf8")) as SarifFile;
@ -415,7 +419,9 @@ export function buildPayload(
environment: string | undefined,
toolNames: string[],
mergeBaseCommitOid: string | undefined,
logger: Logger,
) {
logger.info(`Combining SARIF files using CLI`);
const payloadObj = {
commit_oid: commitOid,
ref,
@ -497,7 +503,7 @@ async function uploadFiles(
features,
logger,
)
: combineSarifFiles(sarifFiles);
: combineSarifFiles(sarifFiles, logger);
sarif = await fingerprints.addFingerprints(sarif, sourceRoot, logger);
sarif = populateRunAutomationDetails(
@ -509,8 +515,11 @@ async function uploadFiles(
const toolNames = util.getToolNames(sarif);
logger.debug(`Validating that each SARIF run has a unique category`);
validateUniqueCategory(sarif);
logger.debug(`Serializing SARIF for upload`);
const sarifPayload = JSON.stringify(sarif);
logger.debug(`Compressing serialized SARIF`);
const zippedSarif = zlib.gzipSync(sarifPayload).toString("base64");
const checkoutURI = fileUrl(sourceRoot);
@ -526,6 +535,7 @@ async function uploadFiles(
environment,
toolNames,
await actionsUtil.determineMergeBaseCommitOid(),
logger,
);
// Log some useful debug info about the info