Ensure artifacts are only uploaded in safe situations

This commit:

Turns on uploading of artifacts again but only if CLI version is
>= 2.20.3. I implemented the check using our feature flag functionality.
I was on the fence about this since it makes the PR more complex.
However, it does give us more flexibility when controlling artifact
uploads.

Also, I renamed the two workflows that were previously disabled. This
way we will not accidentally enable the old workflows for previous
versions of the action.

src/analyze-action-post.ts

@@ -10,8 +10,15 @@ import { getGitHubVersion } from "./api-client";
 import { getConfig } from "./config-utils";
 import * as debugArtifacts from "./debug-artifacts";
 import { EnvVar } from "./environment";
-import { getActionsLogger, withGroup } from "./logging";
-import { checkGitHubVersionInRange, getErrorMessage } from "./util";
+import { Features } from "./feature-flags";
+import { getActionsLogger, Logger, withGroup } from "./logging";
+import { parseRepositoryNwo } from "./repository";
+import {
+  checkGitHubVersionInRange,
+  getErrorMessage,
+  getRequiredEnvParam,
+  GitHubVersion,
+} from "./util";
 
 async function runWrapper() {
   try {
@@ -20,6 +27,8 @@ async function runWrapper() {
     const gitHubVersion = await getGitHubVersion();
     checkGitHubVersionInRange(gitHubVersion, logger);
 
+    const features = createFeatures(gitHubVersion, logger);
+
     // Upload SARIF artifacts if we determine that this is a first-party analysis run.
     // For third-party runs, this artifact will be uploaded in the `upload-sarif-post` step.
     if (process.env[EnvVar.INIT_ACTION_HAS_RUN] === "true") {
@@ -32,6 +41,7 @@ async function runWrapper() {
           debugArtifacts.uploadCombinedSarifArtifacts(
             logger,
             config.gitHubVersion.type,
+            features,
           ),
         );
       }
@@ -43,4 +53,18 @@ async function runWrapper() {
   }
 }
 
+function createFeatures(gitHubVersion: GitHubVersion, logger: Logger) {
+  const repositoryNwo = parseRepositoryNwo(
+    getRequiredEnvParam("GITHUB_REPOSITORY"),
+  );
+
+  const features = new Features(
+    gitHubVersion,
+    repositoryNwo,
+    actionsUtil.getTemporaryDirectory(),
+    logger,
+  );
+  return features;
+}
+
 void runWrapper();

src/debug-artifacts.test.ts

@@ -1,7 +1,9 @@
 import test from "ava";
 
 import * as debugArtifacts from "./debug-artifacts";
+import { Feature } from "./feature-flags";
 import { getActionsLogger } from "./logging";
+import { createFeatures } from "./testing-utils";
 import { GitHubVariant } from "./util";
 
 test("sanitizeArtifactName", (t) => {
@@ -20,16 +22,102 @@ test("sanitizeArtifactName", (t) => {
   );
 });
 
-test("uploadDebugArtifacts", async (t) => {
+test("uploadDebugArtifacts when artifacts empty", 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,
-    ),
-  );
+      true,
+    );
+    t.is(
+      uploaded,
+      "no-artifacts-to-upload",
+      "Should not have uploaded any artifacts",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when true", 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,
+      true,
+    );
+    t.is(
+      uploaded,
+      "upload-failed",
+      "Expect failure to upload artifacts since root dir does not exist",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when false", 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,
+      false,
+    );
+    t.is(
+      uploaded,
+      "upload-not-supported",
+      "Should not have uploaded any artifacts",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when feature enabled", 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,
+      createFeatures([Feature.SafeArtifactUpload]),
+    );
+    t.is(
+      uploaded,
+      "upload-failed",
+      "Expect failure to upload artifacts since root dir does not exist",
+    );
+  });
+});
+
+test("uploadDebugArtifacts when feature disabled", 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,
+      createFeatures([]),
+    );
+    t.is(
+      uploaded,
+      "upload-not-supported",
+      "Expect failure to upload artifacts since root dir does not exist",
+    );
+  });
 });

src/debug-artifacts.ts

@@ -7,11 +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 {
+  Feature,
+  featureConfig,
+  FeatureEnablement,
+  Features,
+} from "./feature-flags";
 import { Language } from "./languages";
 import { Logger, withGroup } from "./logging";
 import {
@@ -34,6 +40,7 @@ export function sanitizeArtifactName(name: string): string {
 export async function uploadCombinedSarifArtifacts(
   logger: Logger,
   gitHubVariant: GitHubVariant,
+  features: Features | boolean,
 ) {
   const tempDir = getTemporaryDirectory();
 
@@ -68,6 +75,7 @@ export async function uploadCombinedSarifArtifacts(
         baseTempDir,
         "combined-sarif-artifacts",
         gitHubVariant,
+        features,
       );
     } catch (e) {
       logger.warning(
@@ -160,6 +168,7 @@ async function tryBundleDatabase(
 export async function tryUploadAllAvailableDebugArtifacts(
   config: Config,
   logger: Logger,
+  features: FeatureEnablement,
 ) {
   const filesToUpload: string[] = [];
   try {
@@ -223,6 +232,7 @@ export async function tryUploadAllAvailableDebugArtifacts(
         config.dbLocation,
         config.debugArtifactName,
         config.gitHubVersion.type,
+        features,
       ),
     );
   } catch (e) {
@@ -238,15 +248,30 @@ export async function uploadDebugArtifacts(
   rootDir: string,
   artifactName: string,
   ghVariant: GitHubVariant,
-) {
+  features: FeatureEnablement | boolean,
+): Promise<
+  | "no-artifacts-to-upload"
+  | "upload-successful"
+  | "upload-failed"
+  | "upload-not-supported"
+> {
   if (toUpload.length === 0) {
-    return;
+    return "no-artifacts-to-upload";
+  }
+  const uploadSupported =
+    typeof features === "boolean"
+      ? features
+      : await features.getValue(Feature.SafeArtifactUpload);
+
+  if (!uploadSupported) {
+    core.info(
+      `Skipping debug artifact upload because the current CLI does not support safe upload. Please upgrade to CLI v${featureConfig.safe_artifact_upload.minimumVersion} 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 +297,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/feature-flags.ts

@@ -54,6 +54,7 @@ export enum Feature {
   PythonDefaultIsToNotExtractStdlib = "python_default_is_to_not_extract_stdlib",
   QaTelemetryEnabled = "qa_telemetry_enabled",
   ZstdBundleStreamingExtraction = "zstd_bundle_streaming_extraction",
+  SafeArtifactUpload = "safe_artifact_upload",
 }
 
 export const featureConfig: Record<
@@ -154,6 +155,18 @@ export const featureConfig: Record<
     legacyApi: true,
     minimumVersion: undefined,
   },
+
+  /**
+   * 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.
+   */
+  [Feature.SafeArtifactUpload]: {
+    defaultValue: true,
+    envVar: "CODEQL_ACTION_SAFE_ARTIFACT_UPLOAD",
+    legacyApi: true,
+    minimumVersion: "2.20.3",
+  },
 };
 
 /**

src/upload-sarif-action-post.ts

@@ -30,7 +30,11 @@ async function runWrapper() {
         return;
       }
       await withGroup("Uploading combined SARIF debug artifact", () =>
-        debugArtifacts.uploadCombinedSarifArtifacts(logger, gitHubVersion.type),
+        debugArtifacts.uploadCombinedSarifArtifacts(
+          logger,
+          gitHubVersion.type,
+          true,
+        ),
       );
     }
   } catch (error) {