ci: pin GitHub Actions to immutable SHAs#7475
Conversation
|
The five container/cache failures in run 29788961098 are the expected The logs confirm GitHub executed the reusable workflows from base |
|
✅ Final review complete — no blockers (commit c38801d) |
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The PR correctly pins the current remote GitHub Action references to immutable commit SHAs, retains release-version comments, and configures Dependabot to maintain those pins. The new enforcement lint has one completeness gap: valid YAML representations of the uses key can bypass its line-oriented regex, so the intended security invariant is not fully enforced.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/lint/lint-ci-action-pins.py`:
- [SUGGESTION] test/lint/lint-ci-action-pins.py:21-39: Valid YAML syntax can bypass the immutable action pin lint
`USES_RE` only recognizes an unquoted, block-form `uses:` key at the beginning of a line. YAML treats a quoted key such as `- "uses": actions/checkout@v6` as the same mapping key, but this line does not match the regex and therefore passes the lint despite using a mutable action tag. Flow mappings such as `- {uses: actions/checkout@v6}` are skipped for the same reason. No current workflow uses these alternate forms, but the new lint is intended to prevent mutable references from being reintroduced. Parse the workflow mappings recursively for keys named `uses`, or explicitly reject YAML forms the scanner cannot inspect, and add negative fixtures covering quoted keys and flow mappings.
Keep pull_request_target workflow definitions trusted while forcing PR code onto GitHub-hosted runners with read-only permissions. Reuse default-branch containers for PRs and reserve package and cache writes for trusted pushes. Co-Authored-By: Claude <noreply@anthropic.com>
Build labeled pull requests with the trusted default-branch Guix image and read-only permissions. Keep cache publication and artifact attestation in trusted push and schedule callers, and add a lint that enforces the workflow trust boundary. Co-Authored-By: Claude <noreply@anthropic.com>
b8c484e to
195f4f1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
WalkthroughThe PR hardens GitHub Actions workflows by pinning remote actions to commit SHAs, adding explicit permissions, restricting cache writes, and tightening pull-request checkout behavior. Container publication is separated from image selection. Guix builds are moved into a reusable matrix worker with trust-gated cache saving and provenance attestation. New lint scripts validate action pins and workflow security rules, and a runner-selection test now models Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GuixWorkflow
participant GuixWorker
participant BuilderContainer
participant Cache
participant Artifacts
GuixWorkflow->>GuixWorker: invoke with source, image, and trust flag
GuixWorker->>Cache: restore build caches
GuixWorker->>BuilderContainer: execute matrix Guix build
GuixWorker->>Cache: save caches when trusted
GuixWorker->>Artifacts: upload outputs and attest trusted builds
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8).github/dependabot.ymlTraceback (most recent call last): .github/workflows/build-container.ymlTraceback (most recent call last): .github/workflows/build-depends.ymlTraceback (most recent call last):
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-depends.yml:
- Around line 77-83: Pass the reusable workflow input build-target through the
step’s env configuration and reference that environment variable when
constructing CACHE_KEY_PREFIX. Remove the direct `${{ inputs.build-target }}`
interpolation from the generated shell script while preserving the existing
cache-key contents.
In @.github/workflows/build-src.yml:
- Line 57: Enforce immutable digest-only container image inputs before the
container image is used in both .github/workflows/build-src.yml:57 and
.github/workflows/lint.yml:24. Add validation in each reusable workflow, or
resolve the image to a digest in trusted callers before passing
inputs.container-path; reject mutable tags and preserve the existing
container.image usage only for validated digests.
In @.github/workflows/clang-diff-format.yml:
- Around line 11-12: Update the Checkout step to set persist-credentials to
false, preventing PR-controlled code from accessing persisted checkout
credentials. Preserve the existing checkout action and pass authentication
explicitly only to the later git fetch if private refs require it.
In @.github/workflows/guix-build-worker.yml:
- Around line 70-75: Update the Guix build workflow’s cache setup and
corresponding Guix daemon/container configuration so the cached /gnu/store path
is mounted into the guix-daemon container at the same host location. Apply this
consistently to the analogous sections around the cache entries at the
referenced workflow blocks, ensuring builds read and write the persisted runner
cache rather than a container-local store.
In @.github/workflows/guix-build.yml:
- Around line 45-48: Update the resolve-pr-image step to derive the base-branch
image tag using the same `${BASE_BRANCH##*/}` basename rule as build-image, so
slash-containing branches resolve to the matching tag. Preserve repository
normalization and existing output behavior.
In `@test/lint/lint-ci-workflow-security.py`:
- Around line 46-76: The security checks in main must parse each workflow as
YAML before validating permissions, checkout settings, and cache-save guards,
rather than relying on raw text and regex matches. Inspect parsed mappings and
values so equivalent forms such as flow mappings, quoted keys, and inline
comments are enforced consistently; add fixtures covering these bypass forms and
ensure the assertions still require the intended trusted-run and
isolated-checkout settings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9b9852c-b0a5-4e22-9108-524b63e9824c
📒 Files selected for processing (19)
.github/dependabot.yml.github/workflows/build-container.yml.github/workflows/build-depends.yml.github/workflows/build-src.yml.github/workflows/build.yml.github/workflows/cache-depends-sources.yml.github/workflows/clang-diff-format.yml.github/workflows/guix-build-worker.yml.github/workflows/guix-build.yml.github/workflows/label-merge-conflicts.yml.github/workflows/lint.yml.github/workflows/merge-check.yml.github/workflows/predict-conflicts.yml.github/workflows/release_docker_hub.yml.github/workflows/semantic-pull-request.yml.github/workflows/test-src.yml.github/workflows/test_select_dynamic_runner.pytest/lint/lint-ci-action-pins.pytest/lint/lint-ci-workflow-security.py
Use base-branch trusted images for pull requests, share depends caches across equivalent runner providers, and make the policy lint verify positive cache guards and indirect PR checkouts. Remove obsolete event parsing from runner selection. Co-Authored-By: Claude <noreply@anthropic.com>
195f4f1 to
ade664f
Compare
✅ Action performedReviews resumed. |
Grant read-only package access to jobs consuming private GHCR images. Supply job-container credentials without exposing a write token, and pre-pull the Guix builder before checking out untrusted code so registry credentials can be removed first. Co-Authored-By: Claude <noreply@anthropic.com>
Blacksmith runners are ephemeral and equivalent to GitHub-hosted runners for this threat model. Restore label/backlog-based dynamic selection and bootstrap RUNNER_CHECK_SKIP configuration for pull_request_target, while keeping least-privilege permissions and trusted-image boundaries. Co-Authored-By: Claude <noreply@anthropic.com>
Ensure an unlabeled pull_request_target event still follows the dynamic backlog thresholds and may select Blacksmith runners. Co-Authored-By: Claude <noreply@anthropic.com>
Remove the outdated claim that Blacksmith runners retain local cache state and describe both supported runner paths as ephemeral. Co-Authored-By: Claude <noreply@anthropic.com>
Mutable action tags can change CI behavior without a reviewed dependency update. Pin every remote uses: reference to its current full commit SHA and keep the exact release version in a trailing comment. Co-Authored-By: Claude <noreply@anthropic.com>
Propose action pin updates through reviewable Dependabot PRs so future checkout and third-party action releases no longer land by silent tag moves. Co-Authored-By: Claude <noreply@anthropic.com>
Fail CI when remote workflow uses: refs are not full 40-hex SHAs or lack an exact release-version comment, so mutable tags cannot reappear. Co-Authored-By: Claude <noreply@anthropic.com>
ade664f to
c38801d
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This delta review confirms the prior YAML-bypass finding is genuinely fixed via recursive parsing with dedicated negative/positive fixtures. Two narrower gaps remain in the new lint's version-comment enforcement: it validates comment presence per unique ref rather than per uses: occurrence (so a duplicate pin missing its own comment can hide behind a sibling occurrence that has one), and its version regex accepts mutable major/minor-only aliases like v6 alongside exact patch versions. Both are suggestion-level robustness gaps in newly added test infrastructure, not functional regressions in the CI pinning itself.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `test/lint/lint-ci-action-pins.py`:
- [SUGGESTION] test/lint/lint-ci-action-pins.py:81-127: Version-comment check is keyed by ref across the whole file, not per `uses:` occurrence
versioned_pins() scans the entire file once and builds a single ref -> version dict from PIN_WITH_VERSION_RE matches (lines 81-86). check_workflow() then looks up each uses: occurrence from iter_uses() against that same file-wide dict via pins.get(ref) (line 122), with no association back to the specific line/step the occurrence came from. Today this is latent rather than exploited: actions/cache/restore@caa296126883cff596d87d8935842f9db880ef25 appears 5 times in .github/workflows/build-depends.yml and all 5 currently carry their own '# v5.1.0' comment. But the check as written only requires that some occurrence of a given ref anywhere in the file have a valid version comment -- if a future edit adds or duplicates a pinned step and drops the inline comment on just that one line, the lint will not catch it because the dict entry is still satisfied by an unrelated occurrence of the same SHA. None of the self_test() fixtures (lines 154-247) exercise a file with two occurrences of the same ref where only one has a comment, so this gap isn't caught by the added tests either. Tracking per-occurrence source position (e.g., during the YAML load or via a line-scoped lookup) rather than a global ref dict would close this.
- [SUGGESTION] test/lint/lint-ci-action-pins.py:32: VERSION_COMMENT_RE accepts mutable major/minor-only aliases, not just exact release versions
VERSION_COMMENT_RE = re.compile(r"^v?\d+(?:\.\d+){0,3}$") permits zero to three dotted components after the leading digit group, so comments like '# v6' or '# v6.1' pass the check even though the lint's stated purpose (per its own docstring and error message: "must include an exact release-version comment (for example '# v6.1.0')") is to require an exact pinned release, not a mutable major/minor alias. All current annotations in the repo happen to use full major.minor.patch versions, so nothing is broken today, but the regex doesn't enforce that convention -- a future PR could add '# v6' next to a SHA pin and the lint would accept it, defeating part of the intent of documenting exactly which release a SHA corresponds to. Requiring at least major.minor.patch (with the existing optional 4th component) would close this without needing new test fixtures beyond a straightforward regex tightening.
| def versioned_pins(text: str) -> dict[str, str]: | ||
| """Map full remote pin refs to their trailing version-comment token.""" | ||
| found: dict[str, str] = {} | ||
| for match in PIN_WITH_VERSION_RE.finditer(text): | ||
| found[match.group("ref")] = match.group("version") | ||
| return found | ||
|
|
||
|
|
||
| def check_workflow(path: Path, rel: Path, text: str, data: Any) -> list[str]: | ||
| errors: list[str] = [] | ||
| if data is None: | ||
| # Empty YAML documents have nothing to pin. | ||
| return errors | ||
|
|
||
| pins = versioned_pins(text) | ||
| for ref in iter_uses(data): | ||
| if ref.startswith("./"): | ||
| continue | ||
| if ref.startswith("docker://"): | ||
| errors.append( | ||
| f"{rel}: unsupported docker:// action {ref!r}; " | ||
| f"pin remote GitHub Actions with owner/name@<40-hex-sha>" | ||
| ) | ||
| continue | ||
|
|
||
| remote = REMOTE_ACTION_RE.match(ref) | ||
| if remote is None: | ||
| errors.append( | ||
| f"{rel}: unsupported `uses:` reference {ref!r} " | ||
| f"(expected owner/name[/...]@<40-hex-sha> or local ./ path)" | ||
| ) | ||
| continue | ||
|
|
||
| pin = remote.group("ref") | ||
| if not SHA_RE.fullmatch(pin): | ||
| errors.append( | ||
| f"{rel}: mutable GitHub Action ref {ref!r}; " | ||
| f"pin to a full 40-character commit SHA" | ||
| ) | ||
| continue | ||
|
|
||
| version = pins.get(ref) | ||
| if not version or not VERSION_COMMENT_RE.fullmatch(version): | ||
| errors.append( | ||
| f"{rel}: pinned action {ref!r} must include an exact " | ||
| f"release-version comment (for example `# v6.1.0`)" | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: Version-comment check is keyed by ref across the whole file, not per uses: occurrence
versioned_pins() scans the entire file once and builds a single ref -> version dict from PIN_WITH_VERSION_RE matches (lines 81-86). check_workflow() then looks up each uses: occurrence from iter_uses() against that same file-wide dict via pins.get(ref) (line 122), with no association back to the specific line/step the occurrence came from. Today this is latent rather than exploited: actions/cache@caa2961 appears 5 times in .github/workflows/build-depends.yml and all 5 currently carry their own '# v5.1.0' comment. But the check as written only requires that some occurrence of a given ref anywhere in the file have a valid version comment -- if a future edit adds or duplicates a pinned step and drops the inline comment on just that one line, the lint will not catch it because the dict entry is still satisfied by an unrelated occurrence of the same SHA. None of the self_test() fixtures (lines 154-247) exercise a file with two occurrences of the same ref where only one has a comment, so this gap isn't caught by the added tests either. Tracking per-occurrence source position (e.g., during the YAML load or via a line-scoped lookup) rather than a global ref dict would close this.
source: ['claude', 'codex']
| r"^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+(?:/[A-Za-z0-9_.-]+)*@(?P<ref>[^\s#]+)$" | ||
| ) | ||
| SHA_RE = re.compile(r"^[0-9a-f]{40}$") | ||
| VERSION_COMMENT_RE = re.compile(r"^v?\d+(?:\.\d+){0,3}$") |
There was a problem hiding this comment.
🟡 Suggestion: VERSION_COMMENT_RE accepts mutable major/minor-only aliases, not just exact release versions
VERSION_COMMENT_RE = re.compile(r"^v?\d+(?:.\d+){0,3}$") permits zero to three dotted components after the leading digit group, so comments like '# v6' or '# v6.1' pass the check even though the lint's stated purpose (per its own docstring and error message: "must include an exact release-version comment (for example '# v6.1.0')") is to require an exact pinned release, not a mutable major/minor alias. All current annotations in the repo happen to use full major.minor.patch versions, so nothing is broken today, but the regex doesn't enforce that convention -- a future PR could add '# v6' next to a SHA pin and the lint would accept it, defeating part of the intent of documenting exactly which release a SHA corresponds to. Requiring at least major.minor.patch (with the existing optional 4th component) would close this without needing new test fixtures beyond a straightforward regex tightening.
| VERSION_COMMENT_RE = re.compile(r"^v?\d+(?:\.\d+){0,3}$") | |
| VERSION_COMMENT_RE = re.compile(r"^v?\d+\.\d+\.\d+(?:\.\d+)?$") |
source: ['codex']
1 participant
Issue being fixed or feature implemented
Mutable GitHub Action tags can change Dash Core CI behavior without a reviewed repository change. This follow-up pins the current action releases so future updates arrive as explicit dependency pull requests.
This draft is stacked on and depends on #7474. The three commits after
ab7e7973b0bb4bf635d2c5a083619817466cdd0dare the follow-up change; once #7474 merges, its parent commits will drop out of this diff.Tracker: thepastaclaw/tracker#2159
What was done?
github-actionsecosystem so action updates are proposed as reviewable pull requests.How Has This Been Tested?
python3 test/lint/lint-ci-action-pins.pypython3 test/lint/lint-ci-workflow-security.pyactionlint -shellcheck= .github/workflows/*.ymlpython3 test/lint/lint-files.py.githubYAML files with PyYAML..githuband confirmed that all 69 remoteuses:references use full SHAs.git diff --check upstream/pr-7474..HEADBreaking Changes
None.
Checklist