Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions .github/workflows/npm-publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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:-<unset>})."
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
Expand Down
41 changes: 41 additions & 0 deletions tests/unit/npm-publish-release-gate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
);
});
Loading