Merge pull request #2726 from github/aeisenberg/reenable-artifact-upload

Ensure artifacts are only uploaded in safe situations

src/analyze-action-post.ts

@@ -7,6 +7,7 @@ import * as core from "@actions/core";
 
 import * as actionsUtil from "./actions-util";
 import { getGitHubVersion } from "./api-client";
+import { getCodeQL } from "./codeql";
 import { getConfig } from "./config-utils";
 import * as debugArtifacts from "./debug-artifacts";
 import { EnvVar } from "./environment";
@@ -28,10 +29,13 @@ async function runWrapper() {
         logger,
       );
       if (config !== undefined) {
+        const codeql = await getCodeQL(config.codeQLCmd);
+        const version = await codeql.getVersion();
         await withGroup("Uploading combined SARIF debug artifact", () =>
           debugArtifacts.uploadCombinedSarifArtifacts(
             logger,
             config.gitHubVersion.type,
+            version.version,
           ),
         );
       }

src/debug-artifacts.test.ts

@@ -20,16 +20,92 @@ test("sanitizeArtifactName", (t) => {
   );
 });
 
-test("uploadDebugArtifacts", async (t) => {
+// These next tests check the correctness of the logic to determine whether or not
+// artifacts are uploaded in debug mode. Since it's not easy to mock the actual
+// call to upload an artifact, we just check that we get an "upload-failed" result,
+// instead of actually uploading the artifact.
+//
+// For tests where we expect artifact upload to be blocked, we check for a different
+// response from the function.
+
+test("uploadDebugArtifacts when artifacts empty should emit 'no-artifacts-to-upload'", async (t) => {
   // Test that no error is thrown if artifacts list is empty.
   const logger = getActionsLogger();
-  await t.notThrowsAsync(
-    debugArtifacts.uploadDebugArtifacts(
+  await t.notThrowsAsync(async () => {
+    const uploaded = await debugArtifacts.uploadDebugArtifacts(
       logger,
       [],
-      "rootDir",
+      "i-dont-exist",
       "artifactName",
       GitHubVariant.DOTCOM,
-    ),
-  );
+      undefined,
+    );
+    t.is(
+      uploaded,
+      "no-artifacts-to-upload",
+      "Should not have uploaded any artifacts",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when no codeql version is used should invoke artifact upload", async (t) => {
+  // Test that the artifact is uploaded.
+  const logger = getActionsLogger();
+  await t.notThrowsAsync(async () => {
+    const uploaded = await debugArtifacts.uploadDebugArtifacts(
+      logger,
+      ["hucairz"],
+      "i-dont-exist",
+      "artifactName",
+      GitHubVariant.DOTCOM,
+      undefined,
+    );
+    t.is(
+      uploaded,
+      // The failure is expected since we don't want to actually upload any artifacts in unit tests.
+      "upload-failed",
+      "Expect failure to upload artifacts since root dir does not exist",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when new codeql version is used should invoke artifact upload", async (t) => {
+  // Test that the artifact is uploaded.
+  const logger = getActionsLogger();
+  await t.notThrowsAsync(async () => {
+    const uploaded = await debugArtifacts.uploadDebugArtifacts(
+      logger,
+      ["hucairz"],
+      "i-dont-exist",
+      "artifactName",
+      GitHubVariant.DOTCOM,
+      "2.20.3",
+    );
+    t.is(
+      uploaded,
+      // The failure is expected since we don't want to actually upload any artifacts in unit tests.
+      "upload-failed",
+      "Expect failure to upload artifacts since root dir does not exist",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when old codeql is used should avoid trying to upload artifacts", async (t) => {
+  // Test that the artifact is not uploaded.
+  const logger = getActionsLogger();
+  await t.notThrowsAsync(async () => {
+    const uploaded = await debugArtifacts.uploadDebugArtifacts(
+      logger,
+      ["hucairz"],
+      "i-dont-exist",
+      "artifactName",
+      GitHubVariant.DOTCOM,
+      "2.20.2",
+    );
+    t.is(
+      uploaded,
+      "upload-not-supported",
+      "Expected artifact upload to be blocked because of old CodeQL version",
+    );
+  });
 });

src/debug-artifacts.ts

@@ -7,13 +7,17 @@ import * as core from "@actions/core";
 import AdmZip from "adm-zip";
 import del from "del";
 
-import { getRequiredInput, getTemporaryDirectory } from "./actions-util";
+import { getOptionalInput, getTemporaryDirectory } from "./actions-util";
 import { dbIsFinalized } from "./analyze";
 import { getCodeQL } from "./codeql";
 import { Config } from "./config-utils";
 import { EnvVar } from "./environment";
 import { Language } from "./languages";
 import { Logger, withGroup } from "./logging";
+import {
+  isSafeArtifactUpload,
+  SafeArtifactUploadVersion,
+} from "./tools-features";
 import {
   bundleDb,
   doesDirectoryExist,
@@ -34,6 +38,7 @@ export function sanitizeArtifactName(name: string): string {
 export async function uploadCombinedSarifArtifacts(
   logger: Logger,
   gitHubVariant: GitHubVariant,
+  codeQlVersion: string | undefined,
 ) {
   const tempDir = getTemporaryDirectory();
 
@@ -68,6 +73,7 @@ export async function uploadCombinedSarifArtifacts(
         baseTempDir,
         "combined-sarif-artifacts",
         gitHubVariant,
+        codeQlVersion,
       );
     } catch (e) {
       logger.warning(
@@ -160,6 +166,7 @@ async function tryBundleDatabase(
 export async function tryUploadAllAvailableDebugArtifacts(
   config: Config,
   logger: Logger,
+  codeQlVersion: string | undefined,
 ) {
   const filesToUpload: string[] = [];
   try {
@@ -223,6 +230,7 @@ export async function tryUploadAllAvailableDebugArtifacts(
         config.dbLocation,
         config.debugArtifactName,
         config.gitHubVersion.type,
+        codeQlVersion,
       ),
     );
   } catch (e) {
@@ -238,15 +246,27 @@ export async function uploadDebugArtifacts(
   rootDir: string,
   artifactName: string,
   ghVariant: GitHubVariant,
-) {
+  codeQlVersion: string | undefined,
+): Promise<
+  | "no-artifacts-to-upload"
+  | "upload-successful"
+  | "upload-failed"
+  | "upload-not-supported"
+> {
   if (toUpload.length === 0) {
-    return;
+    return "no-artifacts-to-upload";
+  }
+  const uploadSupported = isSafeArtifactUpload(codeQlVersion);
+
+  if (!uploadSupported) {
+    core.info(
+      `Skipping debug artifact upload because the current CLI does not support safe upload. Please upgrade to CLI v${SafeArtifactUploadVersion} or later.`,
+    );
+    return "upload-not-supported";
   }
-  logger.info("Uploading debug artifacts is temporarily disabled");
-  return;
 
   let suffix = "";
-  const matrix = getRequiredInput("matrix");
+  const matrix = getOptionalInput("matrix");
   if (matrix) {
     try {
       for (const [, matrixVal] of Object.entries(
@@ -272,9 +292,11 @@ export async function uploadDebugArtifacts(
         retentionDays: 7,
       },
     );
+    return "upload-successful";
   } catch (e) {
     // A failure to upload debug artifacts should not fail the entire action.
     core.warning(`Failed to upload debug artifacts: ${e}`);
+    return "upload-failed";
   }
 }
 

src/init-action-post-helper.ts

@@ -161,7 +161,7 @@ export async function run(
   uploadAllAvailableDebugArtifacts: (
     config: Config,
     logger: Logger,
-    features: FeatureEnablement,
+    codeQlVersion: string,
   ) => Promise<void>,
   printDebugLogs: (config: Config) => Promise<void>,
   config: Config,
@@ -211,7 +211,9 @@ export async function run(
     logger.info(
       "Debug mode is on. Uploading available database bundles and logs as Actions debugging artifacts...",
     );
-    await uploadAllAvailableDebugArtifacts(config, logger, features);
+    const codeql = await getCodeQL(config.codeQLCmd);
+    const version = await codeql.getVersion();
+    await uploadAllAvailableDebugArtifacts(config, logger, version.version);
     await printDebugLogs(config);
   }
 

src/tools-features.ts

@@ -1,3 +1,5 @@
+import * as semver from "semver";
+
 import type { VersionInfo } from "./codeql";
 
 export enum ToolsFeature {
@@ -26,3 +28,21 @@ export function isSupportedToolsFeature(
 ): boolean {
   return !!versionInfo.features && versionInfo.features[feature];
 }
+
+export const SafeArtifactUploadVersion = "2.20.3";
+
+/**
+ * The first version of the CodeQL CLI where artifact upload is safe to use
+ * for failed runs. This is not really a feature flag, but it is easiest to
+ * model the behavior as a feature flag.
+ *
+ * This was not captured in a tools feature, so we need to use semver.
+ *
+ * @param codeQlVersion The version of the CodeQL CLI to check. If not provided, it is assumed to be safe.
+ * @returns True if artifact upload is safe to use for failed runs or false otherwise.
+ */
+export function isSafeArtifactUpload(codeQlVersion?: string): boolean {
+  return !codeQlVersion
+    ? true
+    : semver.gte(codeQlVersion, SafeArtifactUploadVersion);
+}

src/upload-sarif-action-post.ts

@@ -30,7 +30,13 @@ async function runWrapper() {
         return;
       }
       await withGroup("Uploading combined SARIF debug artifact", () =>
-        debugArtifacts.uploadCombinedSarifArtifacts(logger, gitHubVersion.type),
+        debugArtifacts.uploadCombinedSarifArtifacts(
+          logger,
+          gitHubVersion.type,
+          // The codeqlVersion is not applicable for uploading non-codeql sarif.
+          // We can assume all versions are safe to upload.
+          undefined,
+        ),
       );
     }
   } catch (error) {