Repository navigation
sec(ci): move the remaining run-block ${{ }} interpolations into env (#155) - #1147
Conversation
Moves the five remaining ${{ }} interpolations out of ci.yml run:
blocks: github.sha in the image scan step goes through a step-level
IMAGE env (same name LeanerCloud#1139 uses for that image ref), and matrix.cloud in
terraform-validate goes through a job-level CLOUD env, now quoted at
each use. No other change to the steps.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d#155) Moves the 25 remaining ${{ }} interpolations (inputs.deploy_to, step outputs, needs.*.outputs and needs.*.result) out of deploy-all.yml run: blocks into step-level env for determine-deployment and a job-level env for notify, referenced quoted. Each env entry keeps the original expression verbatim, including the `|| 'all'` fallback. No logic change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…anerCloud#155) Moves the six remaining ${{ }} interpolations (github.sha in Set image tag, and the prepare output and needs.*.result values in Post summary) into step-level env, referenced quoted. No logic change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oud#155) Moves the eleven remaining ${{ }} interpolation lines out of deploy-azure.yml run: blocks. needs.* values go through step-level env; ${{ env.AZURE_LOCATION }} becomes ${AZURE_LOCATION}, the workflow-level env var the runner already exports to every step. The existing `ENVIRONMENT=` and `APP_URL=` shell assignments are kept, reading from the new env vars, so the rest of each script is untouched. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d#155) Moves the eight remaining ${{ }} interpolation lines out of deploy-gcp.yml run: blocks. needs.* and vars.GCP_PROJECT_ID go through step-level env; ${{ env.GCP_REGION }} becomes $GCP_REGION, the workflow-level env var the runner already exports. The SERVICE_URL= assignment is kept, reading from the new env var. Closes LeanerCloud#155 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oud#155) The workflow-lint comment cited 106 findings including 66 template-injection. With the run-block sweep done, pedantic zizmor reports 35 low findings and no template-injection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reached
This review includes 5 billable files and costs up to $1.25. Or wait 59 minutes for your next included review. View limit details
Comment |
…ation
Keeps the new step clear of the ${{ }}-in-run pattern that LeanerCloud#155 / LeanerCloud#1147
removes from the workflows.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (claude-cc-platform reviewer, cold brief): REQUEST_CHANGES @IAmJSD the conversion itself is correct everywhere I checked. The only blocker is a rebase conflict with main.
Verified:
|
…SQL protection on (#1143) * fix(iac): lock Azure Postgres against deletion and default GCP Cloud SQL protection on The three database modules disagreed on deletion protection. The Azure Flexible Server module had no protection at all, and the GCP module defaulted deletion_protection to false while AWS defaults it to true. - modules/database/azure: new deletion_protection input (default true) gating a CanNotDelete azurerm_management_lock on the server. The lock depends on the server's child and extension resources so Terraform removes it first on destroy or replacement; Azure locks are inherited and would otherwise fail the destroy with ScopeLocked. - modules/database/gcp: deletion_protection now defaults to true. - environments/azure: database_deletion_protection (default true, like AWS and GCP); github-dev and github-staging opt out, github-prod sets true explicitly. - ci-cd-permissions (Azure): deploy role gains Microsoft.Authorization/locks/{read,write,delete}. - terraform test suites for both modules (mock providers), wired into the CI Validate Terraform job; lock files now carry linux_amd64, darwin_arm64 and darwin_amd64 hashes. Closes #293 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * ci: read the database module cloud from env, not a run-block interpolation Keeps the new step clear of the ${{ }}-in-run pattern that #155 / #1147 removes from the workflows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Resolves the conflict in deploy-all.yml's "Check for failures" step with main's LeanerCloud#1127 check_job allowlist. Main's logic is kept as is; its five new ${{ }} run-block interpolations now read the notify job's existing env vars (DEPLOY_<CLOUD>, DEPLOY_<CLOUD>_RESULT and DETERMINE_DEPLOYMENT_RESULT), so the run-block census stays at 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eanerCloud#155) The pinned zizmor 1.29.0 reports 35 low findings. zizmor 1.31 adds a self-repository audit with 4 more on deploy-all.yml's local reusable-workflow calls, which the comment did not mention. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @cristim. Both findings from the review of
I also updated the PR body's "Overlap" section. The #1127 note is now past tense. It also says that #1228 removes the Verification on
|
Brings in main's CI fixes (LeanerCloud#1236, LeanerCloud#1252) and CI changes since, so this PR gets a fresh CI run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB8. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE Blocking:
Non-blocking:
Attacked:
Body-stale:
Reviewer: rB12. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths. |
|
Local verification at |
|
Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on |
Main's LeanerCloud#1147 moved every run-block ${{ }} interpolation in ci.yml into env, giving terraform-validate a job-level CLOUD. The step this branch adds still interpolated matrix.cloud straight into its run block, which the merge brought back. Use the quoted CLOUD env like the neighbouring steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#1255) * fix(iac): require PostgreSQL 15+ in every environment's version input The migrations need PostgreSQL 15 (000134 uses UNIQUE NULLS NOT DISTINCT), but the environment inputs still accepted older majors: Azure's postgres_version allowed 11-16, and AWS database_engine_version and GCP database_version had no validation. A custom deployment on an older engine would plan and apply, then fail at migration time. Validate all three at plan time: Azure "15"/"16", GCP POSTGRES_15/POSTGRES_16, and AWS "<major>" or "<major>.<minor>" with a major of 15 or more (try() makes a malformed value a validation failure rather than an evaluation error). Every shipped tfvars pins 16, so no deployed environment changes. Add a mocked tftest per environment (old major rejected, 15 and 16 accepted), run by a new "Test environment inputs" step in the terraform-validate CI job, and state the minimum in docs/DEPLOYMENT.md. Closes #1233 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(iac): allowlist the mocked caller-identity account ID in the PG15 tftest The combined-tree git-secrets run flagged terraform/environments/aws/tests/postgres_version.tftest.hcl:10, a bare account_id = "123456789012" in the mocked aws_caller_identity. The repo allows that placeholder only on AccountID lines in _test.go files, so add an anchored whole-line entry for this one mock line (bracketed digits so the entry does not match itself), like the other terraform/ entries. Refs #1233 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(ci): read the cloud from CLOUD in the environment-inputs test step Main's #1147 moved every run-block ${{ }} interpolation in ci.yml into env, giving terraform-validate a job-level CLOUD. The step this branch adds still interpolated matrix.cloud straight into its run block, which the merge brought back. Use the quoted CLOUD env like the neighbouring steps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Conflict: .github/workflows/deploy-all.yml, three hunks against LeanerCloud#1147, which moved every run-block ${{ }} into env:. Kept main's env-variable form throughout (the injection hardening must stay) and dropped the AWS Fargate lines LeanerCloud#1147 added to the plan summary, the result summary and check_job, plus the three DEPLOY_AWS_FARGATE* env entries that pointed at the deploy-aws-fargate job this PR removes. This is the resolution the PR body describes. actionlint and zizmor are clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
Finishes the run-block sweep from #155. On current
mainthere were 55${{ }}interpolation lines left insiderun:blocks across 5 workflow files (the issue's table of 159 is out of date:deploy-aws-lambda.yml,database-migration.ymland part of the deploy-* files have been cleaned up on main since it was written). This PR moves every one of them intoenv:and references the shell variable quoted. Afterwards the issue's parser reports 0, and zizmor'stemplate-injectionaudit goes from 58 findings to 0.deploy-all.ymldeploy-azure.ymldeploy-gcp.ymldeploy-aws-fargate.ymlci.ymlThere's one commit per workflow file, so each can be reviewed by inspection. #155 suggested one PR per file. I'm happy to split this into five PRs if you'd prefer that.
How (mechanical, no logic changes)
env:entry with the original expression copied exactly, including${{ inputs.deploy_to || 'all' }}, so fallbacks and empty values evaluate the same way as before.env:is used everywhere except in two places that use job-levelenv::deploy-all.ymlnotify(both steps read the same nineneeds.*values).ci.ymlterraform-validate, whereCLOUD: ${{ matrix.cloud }}is the same name and value that fix(ops): alert on failed purchases and scheduled tasks; wire monitoring per environment #1094 uses for its new step.ENVIRONMENT="${{ needs.prepare.outputs.target_environment }}"becomesENVIRONMENT="$TARGET_ENVIRONMENT". The rest of each script is byte-identical, and the variable stays unexported, just as it was.${{ env.AZURE_LOCATION }}and${{ env.GCP_REGION }}become${AZURE_LOCATION}and$GCP_REGION. Both are workflow-levelenv:entries that the runner already exports to every step, and nothing writes them to$GITHUB_ENV.env:andscripts/scan-shipped-image.sh, which reads noIMAGEvariable.set -euo pipefailwas added, per the issue's 2026-08-03 note.Deliberate quoting change (2 sites):
scripts/scan-shipped-image.sh cudly:${{ github.sha }}andcd terraform/environments/${{ matrix.cloud }}were unquoted, and they're now"$IMAGE"and"terraform/environments/$CLOUD". The result only differs for a value containing whitespace, glob or shell metacharacters.github.shais always 40 hex characters andmatrix.cloudis the fixed list[aws, gcp, azure], so neither can reach that case.ci.yml's workflow-lint comment cited "106 findings … 66 template-injection" below the zizmor threshold. It now says the pinned zizmor 1.29.0 reports 35 low findings (24undocumented-permissions, 11concurrency-limits), that zizmor 1.31 adds 4 lowself-repositoryfindings ondeploy-all.yml's local reusable-workflow calls, and that template-injection is at zero (separate commits).Verification
All of this was run locally on macOS. No workflow was run, so the behavioral evidence below comes from a local simulation of the runner, not from Actions.
TOTAL 55, HEAD reportsTOTAL 0. The origin/main run doubles as the negative control: the same parser finds the 55 hits on the pre-fix files.zizmor --offline --persona=pedantic --min-severity=medium .github/workflows/(the CI gate): "No findings" both before and after. Ignored findings drop from 93 to 35.--format=jsonat--persona=pedantic:template-injection58 → 0. At--persona=auditor: 0.undocumented-permissionsandconcurrency-limits. They're out of scope here.run:step that had an interpolation (19 steps), the harness renders the origin/main version by splicing values into the script text, as the runner does. It runs the HEAD version with the values passed through workflow, job and stepenv:. Both run underbash --noprofile --norc -ewith stubbedterraform,az,bashandsleep.$GITHUB_OUTPUT,$GITHUB_STEP_SUMMARYand the exact argv of each stubbed command.deploy_to, emptyvars.*); all-empty; a value unique to each expression (catches swapped env names); and multi-line values.github.shaquoting change above, triggered by the multi-line and metacharacter scenarios thatgithub.shacan't produce.$(touch PWNED_S)and a quote-breaking payload, the pre-fix scripts executed the payload in 28 runs. The post-fix scripts executed it in 0 of 30. The 4matrix.cloudsteps were not given payloads because the matrix is a static list.AZURE_LOCATION, wiringproject_idto the wrong variable, hard-coding the cloud, dropping|| 'all', and changing theIMAGE/IMAGE_TAGsource.--register-aws, repo.gitallowed) on the changed files in a fresh temporary repo: clean. trailing-whitespace and end-of-file checks: clean.Equivalence harness (run:
python3 equiv.py <repo> origin/main .github/workflows/{ci,deploy-all,deploy-aws-fargate,deploy-azure,deploy-gcp}.yml, needs PyYAML)Overlap with open PRs
I tested each of these with
git merge-treeagainst this branch's head:Check for failuresstep. I merged origin/main into this branch and kept fix: six small scheduled-auth, CI, OpenAPI and IaC fixes #1127'scheck_joballowlist unchanged, with its 5 new${{ }}lines reading thenotifyjob-level env instead:check_job "$DEPLOY_AWS_LAMBDA" "$DEPLOY_AWS_LAMBDA_RESULT" || FAILED=true(and the same forAWS_FARGATE,GCPandAZURE), plusif [[ "$DETERMINE_DEPLOYMENT_RESULT" != "success" ]]. A simulation of that step over 1,296 result/selection combinations matched main's version exactly.deploy-aws-fargatecaller job and itscheck_jobline. Whichever of sec(ci): move the remaining run-block ${{ }} interpolations into env (#155) #1147 and chore(ci): drop the unreachable Fargate caller job from deploy-all (#314) #1228 merges second needs a trivial merge in that step.d=terraform/modules/database/${{ matrix.cloud }}run line, which would bring the census back to 1. After this PR merges, that line can use the job-level$CLOUD(d="terraform/modules/database/$CLOUD").ci.ymland merge cleanly with no new run-block interpolations: fix(scripts): make init-backend.sh write the state key CI uses #1142, ci: build and check the Docker image natively for amd64 and arm64 #1139, fix(scripts): require GCP_PROJECT_ID and TF_BACKEND_GCP in gcp-import-dev-state.sh #1119, fix(iac/azure): schedule the server tasks on AKS with CronJobs #1114, fix(ci): verify the rollback image in AWS_ACCOUNT_ID's registry #1112, sec(iac/aws): restrict the Fargate ALB to CloudFront when enable_cdn is on #1106, fix(ops): alert on failed purchases and scheduled tasks; wire monitoring per environment #1094, fix(iac/aws): route private egress through a persistent fck-nat ENI #1090, chore(iac/aws): add multi_az support and run prod RDS Multi-AZ on a non-burstable class #1086, fix(ci): bound prod down migrations so a large steps cannot run down -all #1084.env:blocks, I placed them so they don't land next to fix(ops): alert on failed purchases and scheduled tasks; wire monitoring per environment #1094'sTF_VAR_alert_emailslines.rollback.ymlis not touched. It is already clean, and fix(ci): verify the rollback image in AWS_ACCOUNT_ID's registry #1112 edits it.Out of scope
template-injectionfindings, because they score low or informational and the gate's threshold is--min-severity=medium.set -euo pipefail(a separate pass, per the issue).undocumented-permissions,concurrency-limits).Closes #155
Could a maintainer please mirror the issue's triage labels onto this PR:
priority/p2,severity/medium,impact/internal,type/chore,triaged,urgency/this-quarter,effort/m.🤖 Generated with Claude Code