Repository navigation
ci(npm-publish): the gate exempted every event except release - #81
Merged
Merged
Conversation
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 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From the security audit (B-M5). This matters here specifically: publishing v3.8.54 already started an
npm publishof a package upstream owns, and it was cancelled by hand. #44 built this gate in response.The hole
Everything that was not a GitHub Release passed straight through. The only brake on the
workflow_callpath was oneif:inelectron-release.yml.That caller does carry the same condition:
…so nothing is broken today. But the whole guarantee then rests on a single line in another file. A second caller, or one edit to that line, opens a provenance-signed
npm publishwithNPM_TOKENin scope, and nothing in this workflow would object. The audit is right that a gate with its brake somewhere else is not a gate.The change
Everything except
workflow_dispatchnow 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.A subtlety worth writing down
Inside a reusable workflow,
github.event_nameis the caller's event — never the string"workflow_call".electron-release.ymltriggers onpush: tags, so a tag push arrives here aspush, which the old!= "release"test waved through. It is now recorded in the workflow's own comments, because the next person to read that condition will make the same assumption I did.Today's behaviour is unchanged, because the caller already requires the variable. What changes is that it no longer depends on that.
Tests
!= "release"shape;Proven failable: restoring the old condition takes it from 5 pass to 1 fail.
npm-publish-release-gatecheck:workflowsprettier --checkNot fixed here: the two opencode-plugin jobs run
npm installwithout--ignore-scriptsbefore anpm publish --provenance, so a compromised transitive dependency'spostinstallwould get a valid SLSA attestation over poisoned bytes. The mainomniroutejob does it correctly. That is a separate change and is recorded in the audit document.🤖 Generated with Claude Code