Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add deprecation warning for merging SARIF files with non-unique categories #2265

Merged
merged 13 commits into from May 3, 2024
Merged
7 changes: 7 additions & 0 deletions lib/feature-flags.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/feature-flags.js.map

Large diffs are not rendered by default.

66 changes: 61 additions & 5 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.

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

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions src/feature-flags.ts
Expand Up @@ -47,6 +47,7 @@ export interface FeatureEnablement {
export enum Feature {
AutobuildDirectTracingEnabled = "autobuild_direct_tracing_enabled",
CliSarifMerge = "cli_sarif_merge_enabled",
CombineSarifFilesDeprecationWarning = "combine_sarif_files_deprecation_warning_enabled",
CppDependencyInstallation = "cpp_dependency_installation_enabled",
CppTrapCachingEnabled = "cpp_trap_caching_enabled",
DisableJavaBuildlessEnabled = "disable_java_buildless_enabled",
Expand Down Expand Up @@ -91,6 +92,12 @@ export const featureConfig: Record<
minimumVersion: undefined,
defaultValue: false,
},
[Feature.CombineSarifFilesDeprecationWarning]: {
envVar: "CODEQL_ACTION_COMBINE_SARIF_FILES_DEPRECATION_WARNING",
// Independent of the CLI version.
minimumVersion: undefined,
defaultValue: false,
},
[Feature.CppDependencyInstallation]: {
envVar: "CODEQL_EXTRACTOR_CPP_AUTOINSTALL_DEPENDENCIES",
minimumVersion: "2.15.0",
Expand Down
118 changes: 110 additions & 8 deletions src/upload-lib.ts
Expand Up @@ -6,6 +6,7 @@ import * as core from "@actions/core";
import { OctokitResponse } from "@octokit/types";
import fileUrl from "file-url";
import * as jsonschema from "jsonschema";
import * as semver from "semver";

import * as actionsUtil from "./actions-util";
import { getOptionalInput, getRequiredInput } from "./actions-util";
Expand All @@ -24,8 +25,10 @@ import * as util from "./util";
import {
ConfigurationError,
getRequiredEnvParam,
GitHubVariant,
GitHubVersion,
SarifFile,
SarifRun,
wrapError,
} from "./util";

Expand Down Expand Up @@ -65,20 +68,84 @@ function combineSarifFiles(sarifFiles: string[], logger: Logger): SarifFile {

/**
* Checks whether all the runs in the given SARIF files were produced by CodeQL.
* @param sarifFiles The list of SARIF files to check.
* @param sarifObjects The list of SARIF objects to check.
*/
function areAllRunsProducedByCodeQL(sarifFiles: string[]): boolean {
return sarifFiles.every((sarifFile) => {
const sarifObject = JSON.parse(
fs.readFileSync(sarifFile, "utf8"),
) as SarifFile;

function areAllRunsProducedByCodeQL(sarifObjects: SarifFile[]): boolean {
return sarifObjects.every((sarifObject) => {
return sarifObject.runs?.every(
(run) => run.tool?.driver?.name === "CodeQL",
);
});
}

type SarifRunKey = {
name: string | undefined;
fullName: string | undefined;
version: string | undefined;
semanticVersion: string | undefined;
guid: string | undefined;
automationId: string | undefined;
};

function createRunKey(run: SarifRun): SarifRunKey {
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,
koesie10 marked this conversation as resolved.
Show resolved Hide resolved
};
}

/**
* 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.
*/
function areAllRunsUnique(sarifObjects: SarifFile[]): 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;
}

async function showCombineSarifFilesDeprecationWarning(
sarifObjects: util.SarifFile[],
features: Features,
githubVersion: GitHubVersion,
) {
if (!(await features.getValue(Feature.CombineSarifFilesDeprecationWarning))) {
return false;
}

// Do not show this warning on GHES versions before 3.14.0
if (
githubVersion.type === GitHubVariant.GHES &&
semver.lt(githubVersion.version, "3.14.0")
) {
return false;
}

// Only give a deprecation warning when not all runs are unique
koesie10 marked this conversation as resolved.
Show resolved Hide resolved
return (
!areAllRunsUnique(sarifObjects) &&
!process.env.CODEQL_MERGE_SARIF_DEPRECATION_WARNING
);
}

// Takes a list of paths to sarif files and combines them together using the
// CLI `github merge-results` command when all SARIF files are produced by
// CodeQL. Otherwise, it will fall back to combining the files in the action.
Expand All @@ -94,11 +161,33 @@ async function combineSarifFilesUsingCLI(
return JSON.parse(fs.readFileSync(sarifFiles[0], "utf8")) as SarifFile;
}

if (!areAllRunsProducedByCodeQL(sarifFiles)) {
const sarifObjects = sarifFiles.map((sarifFile): SarifFile => {
return JSON.parse(fs.readFileSync(sarifFile, "utf8")) as SarifFile;
});

const deprecationWarningMessage =
gitHubVersion.type === GitHubVariant.GHES
? "and will be removed in GitHub Enterprise Server 3.18"
: "and will be removed on June 4, 2025";

if (!areAllRunsProducedByCodeQL(sarifObjects)) {
logger.debug(
"Not all SARIF files were produced by CodeQL. Merging files in the action.",
);

if (
await showCombineSarifFilesDeprecationWarning(
sarifObjects,
features,
gitHubVersion,
)
) {
logger.warning(
`Uploading multiple SARIF runs with the same category is deprecated ${deprecationWarningMessage}. Please update your workflow to upload a single run per category. For more information, see https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-deprecation-notice`,
);
core.exportVariable("CODEQL_MERGE_SARIF_DEPRECATION_WARNING", "true");
}

// If not, use the naive method of combining the files.
return combineSarifFiles(sarifFiles, logger);
}
Expand Down Expand Up @@ -149,6 +238,19 @@ async function combineSarifFilesUsingCLI(
"The CodeQL CLI does not support merging SARIF files. Merging files in the action.",
);

if (
await showCombineSarifFilesDeprecationWarning(
sarifObjects,
features,
gitHubVersion,
)
) {
logger.warning(
`Uploading multiple CodeQL runs with the same category is deprecated ${deprecationWarningMessage}. Please update your CodeQL CLI version or update your workflow to set a distinct category for each CodeQL run. For more information, see https://github.blog/changelog/2024-05-06-code-scanning-will-stop-combining-runs-deprecation-notice`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the same message as above? If so, probably best to extract to a variable so we don't accidentally change one but not the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the message is not exactly the same. I'll extract the last part ("For more information, see ..."), but the other parts of the message are different. The first message is for third-party tools, while the second message is more specific to CodeQL-only uploads.

);
core.exportVariable("CODEQL_MERGE_SARIF_DEPRECATION_WARNING", "true");
}

return combineSarifFiles(sarifFiles, logger);
}

Expand Down
3 changes: 3 additions & 0 deletions src/util.ts
Expand Up @@ -56,8 +56,11 @@ export interface SarifFile {
export interface SarifRun {
tool?: {
driver?: {
guid?: string;
name?: string;
fullName?: string;
semanticVersion?: string;
version?: string;
};
};
automationDetails?: {
Expand Down