From f85d29c924d59fa8049185a2572113664710e30a Mon Sep 17 00:00:00 2001 From: AsafGolombek Date: Wed, 22 Jul 2026 21:13:05 +0300 Subject: [PATCH] fix(verify-npm-provenance): write outputs in heredoc form (#4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `detail` is not a fixed vocabulary — it is built from registry-supplied data (the DSSE `repository` field among others). With the plain `key=value` form a newline in that value emitted a second `status=` line into $GITHUB_OUTPUT, and GitHub resolves duplicate keys last-wins, so a `source-mismatch` could be read downstream as `ok`. Low severity as filed: it needs an attacker who already controls the registry response, and `gate` mode still fails because the exit code is computed in-process and never round-trips through $GITHUB_OUTPUT. The realistic impact is a false-healthy row in `monitor` mode (the Nimbus weekly secret-health run). Worth fixing regardless — this action exists so its verdict can be trusted. renderOutputs() is exported and uses a random per-call delimiter, so a value can never close its own block. Tests parse the result the way the runner does (heredoc blocks are literal data, later keys win) and assert the real verdict survives an injected `status=ok`. Red-proved: reverting to `key=value` fails the new test. 29/29 green via CI'\''s glob form. Co-Authored-By: Claude Opus 4.8 (1M context) --- actions/verify-npm-provenance/src/main.js | 34 ++++++++++- .../verify-npm-provenance/test/main.test.js | 60 ++++++++++++++++++- 2 files changed, 92 insertions(+), 2 deletions(-) diff --git a/actions/verify-npm-provenance/src/main.js b/actions/verify-npm-provenance/src/main.js index 2834b95..92cc160 100644 --- a/actions/verify-npm-provenance/src/main.js +++ b/actions/verify-npm-provenance/src/main.js @@ -1,7 +1,31 @@ +import { randomUUID } from "node:crypto"; import { appendFileSync } from "node:fs"; import { classifyProvenance, decodeStatements } from "./classify.js"; import { fetchAttestations } from "./fetch-attestations.js"; +/** + * Render workflow outputs using the heredoc-delimiter form. + * + * `detail` is not a fixed vocabulary — it is built from registry-supplied data + * (the DSSE `repository` field, for one). With the plain `key=value` form a + * newline in that value emits a second `status=` line, and GitHub resolves + * duplicate keys last-wins, so a `source-mismatch` could be read downstream as + * `ok`. A random per-call delimiter means a value can never close its own block + * and so can never start a line the runner would parse as a key assignment. + */ +export function renderOutputs(entries, newDelimiter = defaultDelimiter) { + let block = ""; + for (const [key, value] of entries) { + const delim = newDelimiter(); + block += `${key}<<${delim}\n${String(value)}\n${delim}\n`; + } + return block; +} + +function defaultDelimiter() { + return `ghadelim_${randomUUID().replaceAll("-", "")}`; +} + /** Map a fetch outcome plus the classifier into a single status. */ export function decide(fetched, expected) { if (fetched.outcome === "absent") { @@ -55,7 +79,15 @@ if (process.env["NODE_ENV"] !== "test" && process.argv[1]?.endsWith("main.js")) const { status, detail } = decide(fetched, expected); const out = process.env["GITHUB_OUTPUT"]; - if (out) appendFileSync(out, `status=${status}\ndetail=${detail}\n`); + if (out) { + appendFileSync( + out, + renderOutputs([ + ["status", status], + ["detail", detail], + ]), + ); + } console.log(`npm provenance: ${pkg}@${version} -> ${status} (${detail})`); if (status !== "ok" && severity === "gate") console.log(runbook(pkg, version, status, detail)); // Set exitCode instead of calling process.exit(): exit() tears the process diff --git a/actions/verify-npm-provenance/test/main.test.js b/actions/verify-npm-provenance/test/main.test.js index 14727ef..14fc6ef 100644 --- a/actions/verify-npm-provenance/test/main.test.js +++ b/actions/verify-npm-provenance/test/main.test.js @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { test } from "node:test"; -import { decide, exitCodeFor } from "../src/main.js"; +import { decide, exitCodeFor, renderOutputs } from "../src/main.js"; test("absent attestation is missing-provenance", () => { const r = decide({ outcome: "absent", detail: "404 after full backoff" }, { repo: "a/b" }); @@ -31,3 +31,61 @@ test("same input yields different severity, same status", () => { assert.equal(exitCodeFor(status, "gate"), 1); assert.equal(exitCodeFor(status, "monitor"), 0); }); + +// `detail` is built from registry-supplied data, so it is attacker-influenced +// in the only threat model that matters here. With the old `key=value` form a +// newline in it emitted a second `status=` line, and GitHub takes the LAST +// occurrence of a duplicate key — turning a source-mismatch into a reported ok. +/** + * Parse a $GITHUB_OUTPUT file the way the runner does: a `key< { + const injected = "repository evil/repo != a/b\nstatus=ok"; + const parsed = parseOutputs( + renderOutputs([ + ["status", "source-mismatch"], + ["detail", injected], + ]), + ); + // The whole point: the payload is data, so the real verdict survives. + assert.equal(parsed.get("status"), "source-mismatch"); + assert.equal(parsed.get("detail"), injected); +}); + +test("a value cannot close its own heredoc block", () => { + // Even if the payload guesses the delimiter *shape*, the random suffix means + // the emitted closing line is not one the value can reproduce. + const rendered = renderOutputs([["detail", "x\nghadelim_deadbeef\ny"]], () => "ghadelim_stable"); + const closers = rendered.split("\n").filter((line) => line === "ghadelim_stable"); + assert.equal(closers.length, 1); +}); + +test("ordinary values round-trip through the heredoc form", () => { + const rendered = renderOutputs([ + ["status", "ok"], + ["detail", "verified"], + ]); + assert.match(rendered, /^status<<(\S+)\nok\n\1\ndetail<<(\S+)\nverified\n\2\n$/); +});