fix(ci): stop interpolating the release tag into a run block - #1657
fix(ci): stop interpolating the release tag into a run block#1657cristim wants to merge 2 commits into
Conversation
`deploy-aws-lambda.yml` pasted `${{ github.event.release.tag_name }}`
straight into the shell source of the `prepare` job. A git ref name may
contain `;`, `$`, backtick, `(`, `)` and `|` — it only forbids spaces,
which `$IFS` works around — so cutting a release on a crafted tag was
arbitrary code execution.
`prepare` had no `environment:` binding yet inherited workflow-level
`id-token: write`. Since the AWS trust policy accepts the subject
`repo:<org/repo>:ref:refs/heads/main` independently of the
`environment:*` subjects, injected code there could mint an OIDC token
and assume the full production deploy role.
The job looked gated because it declared `outputs: environment:` — an
output *named* environment, not an `environment:` binding. That shape
reads as a gate at a glance while gating nothing, so the output is
renamed rather than merely commented.
Changes:
- Pass the release tag, event name, commit SHA and `inputs.environment`
through `env:` and reference the quoted shell variables. No `${{ }}`
referencing `inputs.*` or `github.event.*` remains in any `run:` block.
- Constrain the tag to the OCI tag grammar, checked against a shell
variable rather than an already-expanded expression, so unlike the
original guard it actually validates something.
- Allowlist the resolved environment to dev|staging|prod. The
`workflow_call` input is a free-form string, unlike the
`workflow_dispatch` `choice`, so it was previously unchecked.
- Drop `id-token: write` to job level, granting it only to
`build-and-deploy` and `test-deployment` — the two jobs that assume the
deploy role, both of which are bound to an environment. `prepare` and
`summary` authenticate to nothing and now hold `contents: read`.
- Rename the `environment` output to `target_environment` (11 consumers)
so it cannot be misread as a binding.
- Replace the unquoted `cat <<EOF > deployment-info.json` heredoc with
`jq -n --arg`. The tag is charset-validated upstream, but that heredoc
sits in a job holding `id-token: write` and would evaluate `$(...)` in
any value reaching it.
- Route the function URL and the `-var-file` environment through `env:`
for consistency with the rest of the file.
Verified with `actionlint` (shellcheck available): no new findings, and
the six the rewritten `prepare` job used to produce are gone (SC2086
28 -> 22). The 26 that remain are pre-existing, sit outside the changed
hunks, and are tracked in #1646.
Closes #1649
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 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 (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Addresses three findings from the independent review of this PR.
The tag guard rejected valid tags. `^[a-zA-Z0-9_][a-zA-Z0-9._-]{0,127}$`
excludes `/` and `+`, so two of this repo's four existing tags fail it,
as do `release/1.0` and `v1.2.3+build.5`. A release cut on a
slash-namespaced tag would have hard-failed `prepare` and blocked the
whole production deploy.
Filtering shell metacharacters was the wrong instinct anyway: they are
already inert, because the value arrives via `env:` and is only ever
referenced quoted. The structural threat is a NEWLINE, since the tag is
written to `$GITHUB_OUTPUT` as a single `tag=<value>` line and a newline
would append attacker-chosen output keys that the two credentialed
downstream jobs then consume. So the guard now rejects empty and control
characters and allows the rest of the git ref charset, with a note that
an OCI-grammar check belongs here if the value is ever wired to
Terraform's `custom_image_tag` (it is not today — the real image tag is
derived from the git commit inside the build module).
`summary` still interpolated six expressions directly into its `run:`
block, including `needs.prepare.outputs.image_tag`, whose origin is the
release tag and which was safe only because a guard in a *different* job
had run. That makes the operative rule "raw interpolation is fine when
someone upstream validated", an implicit invariant that breaks silently
the moment the upstream guard moves. All six now go through `env:`, so
the file has one uniform rule: no `${{ }}` in any `run:` block.
Also quote the `$GITHUB_OUTPUT` redirect targets in the two steps that
still wrote to them unquoted, which is the same output-integrity concern
this PR is about.
actionlint (with shellcheck) now reports **0** findings on this file,
down from 32 before the PR and 26 at the previous commit.
Closes #1649
Found during the #1542 / PR #1641 sweep of
.github/workflows/. Verified live on currentorigin/main(887d51f) before changing anything.The defect
GitHub substitutes
${{ github.event.release.tag_name }}into the shell source before bash parses it.git check-ref-formatforbids spaces,~,^,:,?,*,[,\— but permits;,$,`,(,),&,|,<,>,!. So a tag likeis a valid ref, and cutting a release on it is arbitrary code execution.
$IFSdefeats the no-spaces restriction.Correction: the escalation I originally claimed is NOT reachable
The first version of this PR body claimed the chain: RCE in
prepare→ mint an OIDC token → assumecudly-terraform-deployviarepo:<org>/<repo>:ref:refs/heads/main. That does not connect, and the corrected impact is below.The payload only executes on a
releaseevent, and on a release eventGITHUB_REFisrefs/tags/<tag>— so the ungated job's OIDC subject isrepo:LeanerCloud/CUDly:ref:refs/tags/<tag>, which matches none ofrole.tf's four entries (ref:refs/heads/mainplusenvironment:{dev,staging,prod}). Verified: the default subject template is in force (gh api repos/.../actions/oidc/customization/sub→use_default: true), GCP pinsassertion.ref == 'refs/heads/main', and Azure allows onlymainandpull_request.What is actually reachable, and why this is still p0-shaped:
prepare, under that job'sGITHUB_TOKEN.$GITHUB_OUTPUT—prepare's outputs are consumed bybuild-and-deployandtest-deployment, both of which holdid-token: write. Controlling what those two credentialed jobs read is the real prize, and it is what the guard in this PR now protects.Honest caveat on severity: cutting a release already requires write access, and this repo's
mainis unprotected (gh api .../branches/main/protection→ 404). A write-capable attacker could simply push tomainand obtain theref:refs/heads/mainsubject directly, with no injection needed. So the marginal capability this bug adds to an attacker who already has write access is small. It is still worth fixing — defence in depth, and the$GITHUB_OUTPUTpath into credentialed jobs is real — but the original "escalates to the full production deploy role" framing was wrong and is retracted.After
The value never appears in the script's source text. It arrives as a process environment variable, and
"$TAG"is a quoted parameter expansion — bash performs no further parsing on the result, so;,$(...), backticks and newlines are inert data. The charset guard now runs against a shell variable rather than an already-expanded expression, so unlike the originalimage_tagguard it actually validates something.The reviewer trap, fixed rather than commented
The job declared:
That is an output named
environment, not anenvironment:binding. It reads as gated at a glance and gates nothing — which is very likely why an ungated job holdingid-token: writesurvived review. The apparent gating was cosmetic. A comment alone would not stop the next person making the same misreading, so the output is renamed totarget_environment(11 consumer sites) and a note tells future readers not to rename it back.Changes
inputs.environmentmoved intoenv:, referenced as quoted shell variables. No${{ }}referencinginputs.*orgithub.event.*remains in anyrun:block.dev|staging|prod— theworkflow_callinput is a free-formstring, unlike theworkflow_dispatchchoice, and was previously unchecked.id-token: writedropped to job level, granted only tobuild-and-deployandtest-deployment(the two jobs that assume the deploy role, both environment-bound).prepareandsummaryauthenticate to nothing and now holdcontents: read.cat <<EOF > deployment-info.jsonheredoc inbuild-and-deployreplaced withjq -n --arg. The tag is charset-validated upstream, but that heredoc sits in a job holdingid-token: writeand would evaluate$(...)in any value reaching it — the same shape sec(ci): rollback.yml interpolates the free-text reason input into run blocks in a job holding id-token write #1542 called its worst case.-var-fileenvironment routed throughenv:for consistency.Verification
actionlint(shellcheck available), changed fileid-token: writewithoutenvironment:${{ inputs.* }}/${{ github.event.* }}inside anyrun:needs.prepare.outputs.environmentrefsI am not claiming
actionlintexit 0 here: this file was already failing onmainand the residue is unquoted$GITHUB_STEP_SUMMARYredirect targets in jobs this PR does not touch. Fixing them would mix an unrelated cleanup into a p0 security change; they are tracked in #1646.Correction: my original regression claim was false
The first version of this body said "
git tag -lreturns only four backup tags, all of which pass it, so nothing previously valid is now rejected." That was wrong, and I had not run it. Running the original guard against the repo's actual tags:Two of four fail, and so do
release/1.0,v1.2.3+20260728andv1.2.3+build.5. A release cut on a slash-namespaced tag — this repo's own existing convention — would have hard-failedprepareand blocked the entire production deploy. That is an availability regression I would have introduced while claiming I had checked for exactly that.The guard is redesigned rather than merely widened. Filtering shell metacharacters was the wrong instinct:
;,$, backtick and|are already inert here, because the value arrives viaenv:and is only ever referenced quoted. The structural threat is a newline, because the tag is written to$GITHUB_OUTPUTas a singletag=<value>line, and a newline lets a crafted tag append attacker-chosen output keys — which the two credentialed downstream jobs consume. So the guard now rejects empty and control characters, and allows the rest of the git ref charset:The guarded value is cosmetic today —
image_tagreaches onlydeployment-info.json(viajq --arg) and the step summary;custom_image_tagis never wired from this workflow, andterraform/modules/build/main.tf:24derives the real tag from the git commit. A comment at the guard says an OCI-grammar check belongs there if that ever changes.actwas not run; no dry run is claimed.Important caveat — the
environment:binding is not currently a gateThis fix leans on
environment:to scope the OIDC subject. Worth stating plainly, because it also affects PR #1641:No Environment in this repo has any protection rules, and
staging/proddo not exist at all. GitHub auto-creates a referenced Environment on first use with no rules. Soenvironment:today only scopes secrets and changes the OIDCsubclaim — it is not a reviewer gate until required reviewers are configured in repo settings. The comments in this PR say so rather than claiming a gate that does not exist. Tracked in #1648.Two side observations from that output: the
azure-andgcp-environments are the residue of anenvironment:name interpolating to empty, and thedev-only list is consistent with #1648's finding that the<cloud>-<env>-rollbacknames are absent from the trust policy.Sibling workflows — same shape, no injectable value
A structural sweep (parsing each workflow's YAML for
id-tokeninheritance vsenvironment:bindings, not grepping) found the ungated-prepare-with-inherited-id-tokenpattern indeploy-aws-fargate.yml,deploy-gcp.ymlanddeploy-azure.ymltoo. Those three are not exploitable today: the only untrusted value reaching theirrun:blocks isinputs.environment, which ischoice-constrained onworkflow_dispatch, and theirworkflow_callstring input is fed only bydeploy-all.yml, whose own input is alsochoice-constrained.deploy-aws-lambda.ymlwas the only one with an attacker-settable value (release.tag_name). Filed separately rather than bundled.github.actorreachesrun:blocks in all four deploy workflows; GitHub usernames are[A-Za-z0-9-], so those are not injectable and are noted only so a reader does not re-flag them.Filed from this PR: #1659 (the sibling deploy/sanity workflows carry the same ungated-credentialed shape;
deploy-gcpanddeploy-azureare worse in that theirbuild-and-deployjobs actually authenticate while ungated — but none has an injectable value, so it is hardening, not a live exploit).Post-review changes (commit
be909c41d)Three findings from the independent review, all addressed:
summarystill had six raw${{ }}in itsrun:block, includingimage_tag, safe only because a guard in a different job had run. That makes the operative rule "raw interpolation is fine when someone upstream validated" — the same implicit-invariant species thetarget_environmentrename exists to kill. All six now route throughenv:, so the file has one uniform rule: zero${{ }}in anyrun:block.$GITHUB_OUTPUTredirect targets in two steps, quoted — same output-integrity concern as the rest of the PR. This is what took actionlint to exit 0.Also filed from this review: #1665 —
deploy-all.ymldeclares nopermissions:on its caller jobs, so a called deploy workflow likely cannot obtain an OIDC token at all. Pre-existing and identical before and after this PR (permission intersection with the caller caps both the old workflow-level and the new job-level form equally), so it is not a bisect target for this change.