From af335a81d7cc5ee9e9a0d40444336ca94c747725 Mon Sep 17 00:00:00 2001 From: zodyp Date: Sun, 20 Sep 2026 04:53:24 -0300 Subject: [PATCH] ci(npm-publish): the gate exempted every event except `release` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the security audit (B-M5). The gate read: if [ "$EVENT_NAME" != "release" ] || [ "$ENABLED" = "true" ] so everything that was not a GitHub Release passed straight through, and the only brake on the `workflow_call` path was one `if:` in electron-release.yml. That caller does carry the same `vars.ENABLE_NPM_PUBLISH == 'true'` condition — but the whole guarantee then rests on a single line in another file. A second caller, or one edit to it, opens a provenance-signed `npm publish` with NPM_TOKEN in scope, and nothing in this workflow would object. Now everything except `workflow_dispatch` requires the variable. Dispatch stays exempt: it is already a deliberate human action and it is the emergency route if the variable is ever wrong. Worth knowing, and now written down: inside a reusable workflow `github.event_name` is the CALLER's event, never "workflow_call". electron-release.yml triggers on `push: tags`, so a tag push arrives here as `push` — which the old test waved through and this one does not. Today's behaviour is unchanged, because the caller already requires the variable; what changes is that it no longer depends on that. Two tests added. The first pins the condition and refuses the "not a release" shape. The second asserts the CALLER still carries its own guard — removing it would leave only the callee's, which is the mirror of the situation this fixes, and defence in depth means both. Proven failable: restoring the old condition takes it to 1 fail. npm-publish-release-gate 5 pass / 0 fail check:workflows no new findings; YAML parses; prettier clean Co-Authored-By: Claude Opus 5 --- .github/workflows/npm-publish.yml | 22 +++++++++-- tests/unit/npm-publish-release-gate.test.ts | 41 +++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/.github/workflows/npm-publish.yml b/.github/workflows/npm-publish.yml index fcdf6d14b93..c7ac519809f 100644 --- a/.github/workflows/npm-publish.yml +++ b/.github/workflows/npm-publish.yml @@ -69,9 +69,23 @@ jobs: # `workflow_call` path INTO this workflow. A GitHub Release published by hand, or by # any other workflow, enters through `release:` and never meets it. # - # So the same variable now gates the `release` path here. `workflow_dispatch` is left + # So the same variable now gates every path except `workflow_dispatch`, which is left # alone: it is already a deliberate human action, and it is the emergency route if the - # variable is ever wrong. `workflow_call` is left alone too — its only caller gates it. + # variable is ever wrong. + # + # `workflow_call` used to be exempt here, on the reasoning that its only caller gates + # it. That caller does — electron-release.yml's job carries the same + # `vars.ENABLE_NPM_PUBLISH == 'true'` condition — but then the whole brake lives in one + # `if:` in another file. A second caller, or one edit to that line, would open a full + # provenance-signed publish with NPM_TOKEN in scope, and nothing here would object. + # Gating both sides costs nothing (the caller already requires the variable, so today's + # behaviour is unchanged) and removes the single point of failure. + # + # Note what `github.event_name` is inside a reusable workflow: the CALLER's event, never + # the string "workflow_call". electron-release.yml triggers on `push: tags`, so a tag + # push arrives here as `push` — which the old `!= "release"` test waved through and this + # one does not. A dispatch of either workflow still arrives as `workflow_dispatch` and + # stays exempt, which is the emergency route working as intended. # # To publish to npm from a release, set repository variable ENABLE_NPM_PUBLISH=true. gate: @@ -86,12 +100,12 @@ jobs: ENABLED: ${{ vars.ENABLE_NPM_PUBLISH }} run: | set -euo pipefail - if [ "$EVENT_NAME" != "release" ] || [ "$ENABLED" = "true" ]; then + if [ "$EVENT_NAME" = "workflow_dispatch" ] || [ "$ENABLED" = "true" ]; then echo "allowed=true" >> "$GITHUB_OUTPUT" echo "✅ npm publishing allowed (event=$EVENT_NAME, ENABLE_NPM_PUBLISH=${ENABLED:-})." else echo "allowed=false" >> "$GITHUB_OUTPUT" - echo "⛔ npm publishing is NOT authorised for a release trigger." + echo "⛔ npm publishing is NOT authorised for a $EVENT_NAME trigger." echo " This fork does not own the 'omniroute' name on npm; upstream publishes it." echo " Set repository variable ENABLE_NPM_PUBLISH=true to allow it." fi diff --git a/tests/unit/npm-publish-release-gate.test.ts b/tests/unit/npm-publish-release-gate.test.ts index 8323b2484dd..dc09571108b 100644 --- a/tests/unit/npm-publish-release-gate.test.ts +++ b/tests/unit/npm-publish-release-gate.test.ts @@ -114,3 +114,44 @@ test("the release trigger is the one the gate is there to catch", () => { "if that trigger is removed, revisit whether the gate is still the right shape" ); }); + +test("only a deliberate dispatch is exempt; a tag push and a release are not", () => { + const workflow = loadWorkflow(); + const decide = workflow.jobs?.gate?.steps?.find((step) => step.id === "decide"); + const script = decide?.run ?? ""; + + assert.ok(script, "the gate must still decide in a script step"); + + // `workflow_call` used to be exempt on the reasoning that its only caller gates it. + // It does — but then the entire brake is one `if:` in another file, and a second + // caller or one edit opens a provenance-signed publish with NPM_TOKEN in scope. + assert.doesNotMatch( + script, + /\[\s*"\$EVENT_NAME"\s*!=\s*"release"\s*\]/, + 'a "not a release" test waves through every other event, including the tag push ' + + "that electron-release.yml arrives with" + ); + assert.match( + script, + /\[\s*"\$EVENT_NAME"\s*=\s*"workflow_dispatch"\s*\]\s*\|\|\s*\[\s*"\$ENABLED"\s*=\s*"true"\s*\]/, + "everything except a deliberate dispatch must require ENABLE_NPM_PUBLISH" + ); +}); + +test("the caller still carries its own guard — this is defence in depth, not a move", () => { + const caller = parse( + readFileSync(WORKFLOW.replace("npm-publish.yml", "electron-release.yml"), "utf-8") + ) as Workflow; + + const publishJob = Object.values(caller.jobs ?? {}).find((job) => + String((job as { uses?: string }).uses ?? "").includes("npm-publish.yml") + ) as { if?: string } | undefined; + + assert.ok(publishJob, "electron-release.yml must still be the caller this reasoning is about"); + assert.match( + String(publishJob?.if ?? ""), + /vars\.ENABLE_NPM_PUBLISH\s*==\s*'true'/, + "removing the caller's guard would leave only the callee's — the mirror of the " + + "situation this PR fixes" + ); +});