FUSI-052: Reviewer finding (2026-09-26 tick, Workflow Merger

.github/workflows/threatcrush-scan.yml

@@ -135,11 +135,56 @@
         # our own iface step so it is not attacker-controlled, but "a workflow
         # expression interpolated into a shell body" is the shape of a template
         # injection and static analysis reads the shape, not the provenance.
+        # The same reasoning applies to the settings-sourced threshold below:
+        # it reaches the shell as an environment variable, never as shell text.
         env:
           NATIVE: ${{ steps.iface.outputs.native }}
+          # Optional, never load-bearing. Unset is the normal case and resolves
+          # to the default in the script body; see the FNXC block there.
+          FAIL_ON_OVERRIDE: ${{ vars.THREATCRUSH_FAIL_ON }}
         run: |
           set -o pipefail
-          FAIL_ON=""
+
+          # FNXC:ThreatCrushFailOn 2026-09-29-11:29:
+          # This job is a gate, so it must be able to fail on a real finding.
+          # It previously hardcoded FAIL_ON="" with no override, so --fail-on
+          # was never passed, the CLI never returned 1, and the `1)` findings
+          # arm below was unreachable — every other exit meant the scan itself
+          # broke, not that a problem was detected. A credential committed in a
+          # PR and a clean tree both concluded "success".
+          #
+          # Threshold `high` is measured, not taste: on the pinned
+          # @profullstack/threatcrush@0.11.0 a committed `postgresql://user:pass@host`
+          # is HIGH (secret-database-url, CWE-798, confidence: evidence) while
+          # a `/tmp/...` string quoted inside a markdown table is MEDIUM
+          # (insecure-temp-file, CWE-377, confidence: pattern). `high` therefore
+          # fails on the credential and passes on the known documentation noise.
+          # `medium` would redden open PRs on that noise; `critical` would keep
+          # reporting green on exactly the credential class we exist to catch.
+          #
+          # The threshold is a FLOOR, not a list. The CLI takes the minimum rank
+          # across the requested severities, so `high` also fails on CRITICAL,
+          # and `high,medium` behaves as `medium`. Read it as "this and worse".
+          #
+          # The default lives here, in the workflow, not only in the override.
+          # An unset repository variable expands to the empty string, so a bare
+          # vars.THREATCRUSH_FAIL_ON with no default would put us straight back
+          # to the defect that this line replaces, one settings-page click away
+          # and with a green check to hide it. The override is additive; the
+          # gate is fail-closed.
+          #
+          # `.github/` is outside ROOTS in scripts/check-fnxc-future-dates.mjs,
+          # so this stamp is convention-honored but not gate-checked.
+          #
+          # Trim before defaulting. ${VAR:-high} alone substitutes only for an
+          # unset or empty variable, not for a whitespace-only one, so a stray
+          # space pasted into the repo variable would reach the CLI as an
+          # unknown severity: exit 2, no SARIF, reported as a broken scan
+          # rather than as the threshold being unset. Trimming makes every
+          # blank value resolve to the documented default.
+          FAIL_ON="$(printf %s "${FAIL_ON_OVERRIDE}" | tr -d '[:space:]')"
+          FAIL_ON="${FAIL_ON:-high}"
+
           SCAN_PATH="."
           code=0
 
@@ -177,10 +222,19 @@
           # failure this whole workflow is arranged to avoid.
           if [ ! -s threatcrush.sarif ]; then
             echo "status=error" >> "$GITHUB_OUTPUT"
-            echo "::error::ThreatCrush produced no SARIF (exit ${code}) — this diff was NOT scanned"
+            # The real scope, not "this diff": SCAN_PATH="." with a default
+            # actions/checkout on pull_request scans the full working tree at
+            # the merge ref. Naming it accurately is the difference between
+            # "your change was examined and is clean" and "the whole tree,
+            # change included, was examined".
+            echo "::error::ThreatCrush produced no SARIF (exit ${code}) — the full working tree at the merge ref was NOT scanned"
             exit 1
           fi
 
+          # The resolved threshold, so the report can name the severity boundary
+          # a red job means instead of leaving it only in the job log.
+          echo "threshold=$FAIL_ON" >> "$GITHUB_OUTPUT"
+
           case "$code" in
             0) echo "status=clean" >> "$GITHUB_OUTPUT" ;;
             # Exit 1 *with* a SARIF file is the documented "findings at or
@@ -245,14 +299,26 @@
               )
 
           status = os.environ.get("SCAN_STATUS", "")
+          threshold = os.environ.get("THRESHOLD", "")
           try:
               with open("threatcrush.sarif") as handle:
                   results = json.load(handle)["runs"][0]["results"]
           except Exception as err:
               results = None
               print(f"::warning::could not read SARIF: {err}")
 
-          lines = ["## ThreatCrush Security Scan", ""]
+          # Answer "what did you look at" in the header, where the reader
+          # arrives, instead of leaving it to be inferred. SCAN_PATH="." with a
+          # default actions/checkout scans the FULL WORKING TREE at the merge
+          # ref, not the diff — a secret that predates the pull request is
+          # reported here too, which is the correct posture for a secret scan.
+          lines = [
+              "## ThreatCrush Security Scan",
+              "",
+              "_Scanned: the full working tree at the merge ref (not just the changed files)._",
+          ]
+          if threshold:
+              lines.append(f"_The job fails when a finding at or above **{threshold}** severity is present._")
 
           # Fail closed: render findings only on positive evidence that a scan
           # completed. Testing for `status == "error"` was fail-open and got
@@ -263,10 +329,10 @@
           # outcome is NOT RUN.
           if status not in ("clean", "findings") or results is None:
               # Never render "no issues found" for a scan that did not finish.
-              # An unexamined diff is not a clean one, and the two are
+              # An unexamined tree is not a clean one, and the two are
               # indistinguishable to whoever reads the comment.
               lines += [
-                  "**NOT RUN** — the scan did not complete, so this diff was not examined.",
+                  "**NOT RUN** — the scan did not complete, so the full working tree at the merge ref was not examined.",
                   "This is not a clean result. See the job log.",
               ]
           else:
@@ -321,6 +387,7 @@
           PYEOF
         env:
           SCAN_STATUS: ${{ steps.scan.outputs.status }}
+          THRESHOLD: ${{ steps.scan.outputs.threshold }}
 
       - name: Write report to job summary
         if: always()

packages/cli/src/__tests__/threatcrush-workflow.test.ts

@@ -0,0 +1,221 @@
+import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync, mkdirSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { spawnSync } from "node:child_process";
+import { afterAll, beforeAll, describe, expect, it } from "vitest";
+import { parse } from "yaml";
+
+const workspaceRoot = join(import.meta.dirname!, "..", "..", "..", "..");
+
+/*
+FNXC:ThreatCrushWorkflowTest 2026-09-29-11:29:
+A workflow edit that leaves a guard unreachable has fixed nothing, and a test that
+only reads the YAML cannot tell a working gate from a broken one. So this suite
+executes the real `Scan` step body under bash with a contract-faithful
+`threatcrush` stub on PATH and asserts on the step's exit code and its
+`status=` output — never on the workflow's text. The stub is the acceptance
+gate: it returns exit 1 only when a `--fail-on` threshold was actually received,
+so on the pre-fix workflow (empty FAIL_ON) the findings case reports `clean` and
+this suite goes red. A stub that exits 1 unconditionally would pass both before
+and after the fix, which is exactly the "assert a string is present" weakness
+this card exists to remove.
+
+FNXC:ThreatCrushWorkflowTest 2026-09-29-11:29:
+Path-only fixtures must use mkdtemp rather than a literal /tmp/... name. ThreatCrush
+flags predictable temp paths (CWE-377) even when the tests never create them.
+*/
+
+type StubOptions = {
+  /** Severity the stubbed scan "finds" in the tree. */
+  findingSeverity?: string;
+  /** Overrides the default, mimicking the workflow env. */
+  failOnOverride?: string;
+};
+
+/*
+Contract-faithful stub for @profullstack/threatcrush@0.11.0.
+  rank():  info=0 low=1 medium=2 high=3 critical=4  (SEVERITY_ORDER)
+  - Always writes SARIF on the scan path: the real CLI emits it at
+    dist/index.js:11954 *before* setting process.exitCode=1 at :11958-11966,
+    so a findings run still leaves a non-empty SARIF and the workflow's
+    `if [ ! -s threatcrush.sarif ]` guard passes. A stub that wrote SARIF only
+    on the clean path would let that guard fire first and every case would
+    report `status=error` — measuring the guard, not the gate.
+  - Exit 1 ONLY when a --fail-on threshold was received and the finding
+    severity meets it (Math.min over requested ranks => the threshold is a
+    floor). Absent --fail-on the floor stays 99, no finding meets it, and the
+    step correctly reports `status=clean` — the pre-fix behaviour.
+  - An unknown severity exits 2 and writes no SARIF, matching the measured CLI
+    behaviour on a bad threshold (exit 2, no SARIF). This ordering matters: a
+    stub that wrote SARIF first on every path would turn that case into
+    `status=findings` and hide the distinction.
+*/
+const STUB = `#!/usr/bin/env bash
+rank() { case "$1" in
+  info) echo 0 ;; low) echo 1 ;; medium) echo 2 ;; high) echo 3 ;;
+  critical) echo 4 ;; *) echo -1 ;;
+esac; }
+out=""; prev=""; values=""
+for a in "$@"; do
+  if [ "$prev" = "--output" ]; then out="$a"; fi
+  if [ "$prev" = "--fail-on" ]; then values="$values$a,"; fi
+  prev="$a"
+done
+printf '%s\\n' "$*" >> "\${STUB_ARGV_FILE:?}"
+floor=99
+IFS=,
+for v in $values; do
+  r=$(rank "$v")
+  if [ "$r" -lt 0 ]; then exit 2; fi
+  if [ "$r" -lt "$floor" ]; then floor=$r; fi
+done
+printf '{"version":"2.1.0","runs":[{"results":[]}]}' > "$out"
+if [ "$(rank "\${STUB_FINDING_SEVERITY:-high}")" -ge "$floor" ]; then exit 1; fi
+exit 0
+`;
+
+function loadScanStep(): { run: string; env: Record<string, string> } {
+  const content = readFileSync(join(workspaceRoot, ".github", "workflows", "threatcrush-scan.yml"), "utf8");
+  const doc = parse(content) as any;
+  const steps = doc.jobs.scan.steps as any[];
+  const scan = steps.find((step) => step.id === "scan");
+  if (!scan) throw new Error("threatcrush-scan.yml has no step with id: scan");
+  return { run: scan.run as string, env: (scan.env ?? {}) as Record<string, string> };
+}
+
+type StepResult = { status: string; exitCode: number; argv: string };
+
+describe.skipIf(process.platform === "win32")("ThreatCrush scan step (executed)", () => {
+  let tmpRoot: string;
+  let binDir: string;
+  let body: string;
+
+  beforeAll(() => {
+    tmpRoot = mkdtempSync(join(tmpdir(), "fusion-threatcrush-workflow-"));
+    binDir = join(tmpRoot, "bin");
+    mkdirSync(binDir, { recursive: true });
+    const stubPath = join(binDir, "threatcrush");
+    writeFileSync(stubPath, STUB, { mode: 0o755 });
+    chmodSync(stubPath, 0o755);
+    body = loadScanStep().run;
+  });
+
+  afterAll(() => {
+    rmSync(tmpRoot, { recursive: true, force: true });
+  });
+
+  // Runs the real step body with the stub first on PATH, in a throwaway cwd
+  // (the body writes threatcrush.sarif relative to cwd), and returns the
+  // step's exit code, its `status=` output, and the stub's received argv.
+  function runScan({ findingSeverity = "high", failOnOverride }: StubOptions = {}): StepResult {
+    const workDir = join(tmpRoot, `run-${Math.random().toString(36).slice(2)}`);
+    mkdirSync(workDir, { recursive: true });
+    const bodyPath = join(workDir, "body.sh");
+    writeFileSync(bodyPath, body);
+
+    const argvFile = join(workDir, "argv.txt");
+    const outputFile = join(workDir, "github_output");
+    writeFileSync(outputFile, "");
+
+    const env: NodeJS.ProcessEnv = {
+      ...process.env,
+      PATH: `${binDir}:${process.env.PATH}`,
+      NATIVE: "true",
+      GITHUB_OUTPUT: outputFile,
+      STUB_ARGV_FILE: argvFile,
+      STUB_FINDING_SEVERITY: findingSeverity,
+    };
+    if (failOnOverride !== undefined) env.FAIL_ON_OVERRIDE = failOnOverride;
+    else delete env.FAIL_ON_OVERRIDE;
+
+    // bash --noprofile --norc -eo pipefail, body from a file (not -c) so it
+    // stays byte-identical to the workflow and is not re-quoted by the parent.
+    const proc = spawnSync("bash", ["--noprofile", "--norc", "-eo", "pipefail", bodyPath], {
+      cwd: workDir,
+      env,
+      encoding: "utf8",
+    });
+
+    const outputs = readFileSync(outputFile, "utf8");
+    const statusMatch = /^status=(.*)$/m.exec(outputs);
+    const argv = readFileSync(argvFile, "utf8");
+    rmSync(workDir, { recursive: true, force: true });
+
+    return { status: statusMatch ? statusMatch[1].trim() : "", exitCode: proc.status ?? -1, argv };
+  }
+
+  it("fails with status=findings on a HIGH finding and passes the high threshold (the arm that was dead)", () => {
+    const result = runScan({ findingSeverity: "high" });
+    expect(result.status).toBe("findings");
+    expect(result.exitCode).not.toBe(0);
+    expect(result.argv).toContain("--fail-on high");
+  });
+
+  it("stays clean on a MEDIUM-only tree (the CWE-377 documentation class)", () => {
+    const result = runScan({ findingSeverity: "medium" });
+    expect(result.status).toBe("clean");
+    expect(result.exitCode).toBe(0);
+  });
+
+  it("honours a `medium` override — the override is additive, not ignored", () => {
+    const result = runScan({ findingSeverity: "high", failOnOverride: "medium" });
+    expect(result.status).toBe("findings");
+    expect(result.exitCode).not.toBe(0);
+    expect(result.argv).toContain("--fail-on medium");
+  });
+
+  it("resolves a blank override to the default `high` (fail-closed default)", () => {
+    const result = runScan({ findingSeverity: "high", failOnOverride: "" });
+    expect(result.status).toBe("findings");
+    expect(result.argv).toContain("--fail-on high");
+  });
+
+  it("resolves a whitespace-only override to `high`, not to an unknown severity", () => {
+    // ${VAR:-high} alone would pass `--fail-on "  "` here, which the CLI rejects
+    // (exit 2, no SARIF) => status=error. The trim keeps this fail-closed.
+    const result = runScan({ findingSeverity: "high", failOnOverride: "  " });
+    expect(result.status).toBe("findings");
+    expect(result.argv).toContain("--fail-on high");
+  });
+
+  it("reports status=error for a broken scan (stub writes no SARIF), distinct from findings", () => {
+    // A typo'd threshold makes the stub exit 2 without writing SARIF, tripping
+    // the workflow's `if [ ! -s threatcrush.sarif ]` guard. That is a broken
+    // scan, not a finding, and the two must not be conflated.
+    const result = runScan({ findingSeverity: "high", failOnOverride: "hgh" });
+    expect(result.status).toBe("error");
+    expect(result.exitCode).not.toBe(0);
+  });
+
+  it("treats the threshold as a floor, not a list (high,medium behaves as medium)", () => {
+    // The CLI/converter take the MIN rank across the requested severities, so
+    // `high,medium` is equivalent to `medium` and a MEDIUM finding trips it. This
+    // pins that an operator passing a list cannot get "only these severities".
+    const multi = runScan({ findingSeverity: "medium", failOnOverride: "high,medium" });
+    expect(multi.status).toBe("findings");
+    expect(multi.exitCode).not.toBe(0);
+
+    // And a single `high` floor still fails on a CRITICAL finding (>= the floor).
+    const critical = runScan({ findingSeverity: "critical" });
+    expect(critical.status).toBe("findings");
+    expect(critical.exitCode).not.toBe(0);
+  });
+});
+
+describe("ThreatCrush scan step (structural)", () => {
+  it("resolves the threshold through a defaulted shell expansion, not a bare literal", () => {
+    // Platform-independent guard for the invariant the executed block above
+    // covers on POSIX: a re-break to a bare `FAIL_ON=""` literal removes the
+    // default, and the `${FAIL_ON:-high}` expansion is what fails closed.
+    const { run } = loadScanStep();
+    expect(run).toMatch(/FAIL_ON="\$\{FAIL_ON:-high\}"/);
+  });
+
+  it("sources the optional override from a repository variable through env, not shell interpolation", () => {
+    const { run, env } = loadScanStep();
+    // The override must arrive as an env var (template-injection shape avoided)
+    // and be named in the body via the env reference, not inlined.
+    expect(env.FAIL_ON_OVERRIDE).toContain("vars.THREATCRUSH_FAIL_ON");
+    expect(run).toContain("${FAIL_ON_OVERRIDE}");
+  });
+});