fix: a red security scan can block a merge here - #78
Merged
Conversation
security.yml runs gitleaks, a Dockerfile misconfiguration scan and a filesystem vulnerability scan on every pull request, and none of them could stop one. The workflow ends at its last scan job with no gate, and branch protection requires a single context produced by ci.yml, so all three could report failure while the PR stayed mergeable. This is the only repository in the org with a security workflow and no gate over it. competitive-intelligence, digest-pipeline and slack-knowledge-bot each close their security.yml with a merge-gate-security job and each requires the resulting `merge gate (security)` context; this adds the same job here, with the same composite action pinned to the same SHA. Two properties are carried over deliberately rather than reinvented. `always()` is load-bearing: without it a failed dependency skips the gate, and GitHub scores a skipped required check as passing, so it would report green precisely when a scan broke. Restricting it to pull_request matches the org's other fourteen consumers — the gate treats a skipped dependency as failure, and jobs that are legitimately skipped on a push to main would otherwise redden every push. The gate's needs list covers all three scan jobs, which the action re-verifies at runtime against the workflow it runs in, so a scan added later without being watched fails the gate rather than slipping past it. Requiring the new context is a protection change and follows once this is on main; until then the job runs and reports without blocking anything.
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
1 participant
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.
The hole
security.ymlruns three jobs on every PR —gitleaks (secret scan),trivy (dockerfile misconfig),trivy (filesystem vuln scan — npm deps)— and none of them can stop a merge. The workflow ends at its last scan job with no gate, and branch protection requires a single context (Merge Gate) produced byci.yml.So a leaked secret could be found, reported, uploaded to code scanning, and merged.
This is the only repo in the org with a security workflow and no gate over it:
merge gate,merge gate (security)merge gate,merge gate (security)merge gate,merge gate (security)Merge GateThe fix
The same
merge-gate-securityjob the three siblings carry, with the same composite action at the same pinned SHA. Two properties carried over deliberately:always()— without it a failed dependency skips the gate, and GitHub scores a skipped required check as passing. It would report green exactly when a scan broke.if: github.event_name == 'pull_request'— the gate treats a skipped dependency as failure, and jobs legitimately skipped on push would otherwise redden every push to main.Verified the gate watches everything it should: jobs are
[gitleaks, trivy-config, trivy-fs, merge-gate-security], the gate'sneedsis the first three, unwatched set is empty. The action re-checks this at runtime, so a scan added later without being watched fails the gate rather than slipping past.Follow-up
Requiring
merge gate (security)is a protection change and follows once this is on main — until then the job runs and reports without blocking.Also noted, deliberately not done here: this repo's ci.yml gate is named
Merge Gatewhere the org standard is lowercasemerge gate. It matches its required context, so nothing is broken. Renaming needs protection and workflow to move together or every PR blocks on a context that no longer reports, so it belongs in its own deliberate step.