Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/analyze-action-post.js

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

451 changes: 228 additions & 223 deletions lib/analyze-action.js

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions lib/autobuild-action.js

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

865 changes: 434 additions & 431 deletions lib/init-action-post.js

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions lib/init-action.js

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

1 change: 1 addition & 0 deletions lib/resolve-environment-action.js

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

1 change: 1 addition & 0 deletions lib/setup-codeql-action.js

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

1 change: 1 addition & 0 deletions lib/start-proxy-action-post.js

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

1 change: 1 addition & 0 deletions lib/start-proxy-action.js

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

397 changes: 200 additions & 197 deletions lib/upload-lib.js

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions lib/upload-sarif-action-post.js

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

397 changes: 201 additions & 196 deletions lib/upload-sarif-action.js

Large diffs are not rendered by default.

8 changes: 8 additions & 0 deletions package-lock.json

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

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"@types/js-yaml": "^4.0.9",
"@types/node": "^20.19.9",
"@types/node-forge": "^1.3.14",
"@types/sarif": "^2.1.7",
"@types/semver": "^7.7.1",
"@types/sinon": "^21.0.0",
"ava": "^6.4.1",
Expand Down
3 changes: 2 additions & 1 deletion src/analyze.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { FeatureEnablement, Feature } from "./feature-flags";
import { KnownLanguage, Language } from "./languages";
import { Logger, withGroupAsync } from "./logging";
import { OverlayDatabaseMode } from "./overlay";
import type * as sarif from "./sarif";
import { DatabaseCreationTimings, EventReport } from "./status-report";
import { endTracingForCluster } from "./tracer-config";
import * as util from "./util";
Expand Down Expand Up @@ -594,7 +595,7 @@ export async function runQueries(
function getPerQueryAlertCounts(sarifPath: string): Record<string, number> {
const sarifObject = JSON.parse(
fs.readFileSync(sarifPath, "utf8"),
) as util.SarifFile;
) as sarif.Log;
// We do not need to compute fingerprints because we are not sending data based off of locations.

// Generate the query: alert count object
Expand Down
5 changes: 3 additions & 2 deletions src/fingerprints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import test from "ava";

import * as fingerprints from "./fingerprints";
import { getRunnerLogger } from "./logging";
import * as sarif from "./sarif";
import { setupTests } from "./testing-utils";
import * as util from "./util";

Expand Down Expand Up @@ -201,7 +202,7 @@ test("addFingerprints", async (t) => {
fs
.readFileSync(`${__dirname}/../src/testdata/fingerprinting.input.sarif`)
.toString(),
) as util.SarifFile;
) as sarif.Log;
const expected = JSON.parse(
fs
.readFileSync(
Expand Down Expand Up @@ -229,7 +230,7 @@ test("missingRegions", async (t) => {
fs
.readFileSync(`${__dirname}/../src/testdata/fingerprinting2.input.sarif`)
.toString(),
) as util.SarifFile;
) as sarif.Log;
const expected = JSON.parse(
fs
.readFileSync(
Expand Down
12 changes: 6 additions & 6 deletions src/fingerprints.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Long from "long";

import { DocUrl } from "./doc-url";
import { Logger } from "./logging";
import { SarifFile, SarifResult } from "./util";
import type * as sarif from "./sarif";

const tab = "\t".charCodeAt(0);
const space = " ".charCodeAt(0);
Expand Down Expand Up @@ -138,7 +138,7 @@ export async function hash(callback: hashCallback, filepath: string) {
// Generate a hash callback function that updates the given result in-place
// when it receives a hash for the correct line number. Ignores hashes for other lines.
function locationUpdateCallback(
result: SarifResult,
result: sarif.Result,
location: any,
logger: Logger,
): hashCallback {
Expand Down Expand Up @@ -256,17 +256,17 @@ export function resolveUriToFile(
// Compute fingerprints for results in the given sarif file
// and return an updated sarif file contents.
export async function addFingerprints(
sarif: SarifFile,
sarifLog: sarif.Log,
sourceRoot: string,
logger: Logger,
): Promise<SarifFile> {
): Promise<sarif.Log> {
logger.info(
`Adding fingerprints to SARIF file. See ${DocUrl.TRACK_CODE_SCANNING_ALERTS_ACROSS_RUNS} for more information.`,
);
// 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[] } = {};
for (const run of sarif.runs || []) {
for (const run of sarifLog.runs || []) {
// We may need the list of artifacts to resolve against
const artifacts = run.artifacts || [];

Expand Down Expand Up @@ -316,5 +316,5 @@ export async function addFingerprints(
await hash(teeCallback, filepath);
}

return sarif;
return sarifLog;
}
18 changes: 18 additions & 0 deletions src/sarif/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as fs from "fs";

import test from "ava";

import { setupTests } from "../testing-utils";

import { getToolNames, type Log } from ".";

setupTests(test);

test("getToolNames", (t) => {
const input = fs.readFileSync(
`${__dirname}/../../src/testdata/tool-names.sarif`,
"utf8",
);
const toolNames = getToolNames(JSON.parse(input) as Log);
t.deepEqual(toolNames, ["CodeQL command-line toolchain", "ESLint"]);
});
117 changes: 117 additions & 0 deletions src/sarif/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import * as fs from "fs";

import { Logger } from "../logging";

import * as sarif from "sarif";

export type * from "sarif";

// `automationId` is non-standard.
export type RunKey = sarif.ToolComponent & {
automationId: string | undefined;
};
Comment thread
mbg marked this conversation as resolved.
Outdated

/**
* An error that occurred due to an invalid SARIF upload request.
*/
export class InvalidSarifUploadError extends Error {}

/**
* Get the array of all the tool names contained in the given sarif contents.
*
* Returns an array of unique string tool names.
*/
export function getToolNames(sarifFile: sarif.Log): string[] {
const toolNames = {};

for (const run of sarifFile.runs || []) {
const tool = run.tool || {};
const driver = tool.driver || {};
if (typeof driver.name === "string" && driver.name.length > 0) {
toolNames[driver.name] = true;
}
}

return Object.keys(toolNames);
}

export function readSarifFile(sarifFilePath: string): sarif.Log {
return JSON.parse(fs.readFileSync(sarifFilePath, "utf8")) as sarif.Log;
}

// Takes a list of paths to sarif files and combines them together,
// returning the contents of the combined sarif file.
export function combineSarifFiles(
sarifFiles: string[],
logger: Logger,
): sarif.Log {
logger.info(`Loading SARIF file(s)`);
const combinedSarif: sarif.Log = {
version: "2.1.0",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a potentially breaking change — now we'll only accept SARIF v2.1.0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite. We haven't changed anything about how we parse or validate SARIF files. So although the type from @types/sarif won't allow us to construct a sarif.Log object without a version "2.1.0" string in TypeScript code, we'll happily parse SARIF files without it. Our validator also didn't previously reject such files.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose since code scanning only supports v2.1.0 it's not necessarily a bad thing if we fail if users try to upload a SARIF file with a different version number. Though it does seem like a breaking change if the API would previously tolerate different version numbers.

Could you explain why this combining logic wouldn't fail with the "Different SARIF versions encountered" error message if a SARIF file contains a different version number or a missing version number?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry. I missed that this (and the other comment) were specifically in the context of combineSarifFiles and not the @types/sarif library's requirement for the version to be 2.1.0.

I have re-jigged this function slightly now so that the old behaviour is maintained, unless there is no version property present. In that case, we insert 2.1.0 in the resulting SARIF.

This was supposed to be a fairly small PR to unblock me on something else, so I am not sure I want to deal with a breaking change here.

runs: [],
};

for (const sarifFile of sarifFiles) {
logger.debug(`Loading SARIF file: ${sarifFile}`);
const sarifObject = readSarifFile(sarifFile);
// Check SARIF version
if (combinedSarif.version === null) {
Comment thread
mbg marked this conversation as resolved.
Outdated
combinedSarif.version = sarifObject.version;
} else if (combinedSarif.version !== sarifObject.version) {
throw new InvalidSarifUploadError(
`Different SARIF versions encountered: ${combinedSarif.version} and ${sarifObject.version}`,
);
}

combinedSarif.runs.push(...sarifObject.runs);
}

return combinedSarif;
}

/**
* Checks whether all the runs in the given SARIF files were produced by CodeQL.
* @param sarifObjects The list of SARIF objects to check.
*/
export function areAllRunsProducedByCodeQL(sarifObjects: sarif.Log[]): boolean {
Comment thread
mbg marked this conversation as resolved.
Outdated
return sarifObjects.every((sarifObject) => {
return sarifObject.runs?.every(
(run) => run.tool?.driver?.name === "CodeQL",
);
});
}

function createRunKey(run: sarif.Run): RunKey {
return {
name: run.tool?.driver?.name,
fullName: run.tool?.driver?.fullName,
version: run.tool?.driver?.version,
semanticVersion: run.tool?.driver?.semanticVersion,
guid: run.tool?.driver?.guid,
automationId: run.automationDetails?.id,
};
}

/**
* Checks whether all runs in the given SARIF files are unique (based on the
* criteria used by Code Scanning to determine analysis categories).
* @param sarifObjects The list of SARIF objects to check.
*/
export function areAllRunsUnique(sarifObjects: sarif.Log[]): boolean {
const keys = new Set<string>();

for (const sarifObject of sarifObjects) {
for (const run of sarifObject.runs) {
const key = JSON.stringify(createRunKey(run));

// If the key already exists, the runs are not unique.
if (keys.has(key)) {
return false;
}

keys.add(key);
}
}

return true;
}
Loading
Loading