[IAmJSD]

Root cause

scripts/generate-federation-iac.go copies its flag values into iacData and renders the shared internal/iacfiles/templates/*.tmpl with text/template, which does no escaping. --account-id, --source-account-id, --oidc-subject-claim and (for Azure) --cudly-api-url are validated against strict allowlists. The other free-form flags are not: --account-name, --account-slug, --contact-email, --cudly-api-url (non-Azure), --tenant-id, --project-id and --service-account-email. A quote, ${/%{ or a newline in any of them could change the structure of the generated tfvars, CloudFormation params JSON or Bash file.

Reproduced on main (18ee477):

$ go run scripts/generate-federation-iac.go --target aws --source aws \
    --account-name $'prod"\nrole_name = "pwned' --account-id 999888777666 \
    --source-account-id 111122223333 --output -
# Target AWS account : prod"
role_name = "pwned (999888777666)
...
account_name  = "prod"
role_name = "pwned"

Fix

I chose escaping by output format plus one generic rejection, not a per-field validator for each flag. A display name like --account-name has no grammar to validate against, and the same value lands in HCL, Bash and JSON, so a per-field rule would still need escaping behind it.

1. Escape per output grammar (script). renderTmpl picks an escaper from the template's file suffix and applies it to every string field of iacData (walked by reflection, so a field added later is covered):
   • *.tfvars.tmpl: HCL string escaping (\ to \\, " to \", ${ to $${, %{ to %%{).
   • *.sh.tmpl: the same rule as the server's shellEscape (\, ", `, $).
   • *.json.tmpl: JSON string escaping (encoding/json, HTML escaping off so <AZURE_TENANT_ID> stays as is).
   • Any other suffix is an error, so a new template type can't render raw values.
2. Reject control characters (script). Values are also rendered into # comment lines, where only a line break can change the structure, and no flag can legitimately contain one. After the existing field validators run (so they still give their more specific errors), any flag whose value contains a control character (unicode.IsControl: newline, CR, tab, ESC and others) is rejected with an error that names the flag. Nothing is written.
3. Server bundle tfvars. I checked whether GET /api/federation/iac can feed user-controlled values into its tfvars. It can, but only one: ContactEmail is session.Email. When an admin creates or edits a user, or a user changes their own email with their current password, the stored email only has to pass mail.ParseAddress and is kept as typed. A quoted local part such as "o\"q${x}%{y}"@example.com therefore reaches contact_email = "..." raw. That produces broken HCL or template evaluation inside that one string. It can't add attributes, because mail.ParseAddress rejects CR/LF. The other tfvars fields are server-derived (AccountSlug: "target", the allowlisted source, OIDC constants, the STS account ID) or come from configuration (dashboardURL or the Lambda Function URL domain, not the Host header). addBundleTerraform now renders the tfvars with hclEscapeData(data). The CLI scripts and the bundle's CFN deploy-cfn.sh keep their existing shellEscapeData and are not double-escaped. A test asserts this.

Output is byte-identical for values with no metacharacters. I diffed every target/source tfvars combination plus cf-params, rendered with benign values, between main and this branch, and there were no differences. Hostile values now also show escaped inside the # header comments (e.g. # Target AWS account : pr\"od). That only changes how the comment looks.

Verification

All Go commands ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, on macOS (Darwin 24.6, Go 1.26). Nothing touched a cloud API. Terraform was used only locally, as terraform console on scratch modules that declare variables and nothing else (no providers).

Fail-before. The new tests were run on main with the fix stashed (git stash push internal/api/handler_federation.go scripts/generate-federation-iac.go) and the new test file in place:

--- FAIL: TestGetFederationIaC_BundleTfvarsEscapesSessionEmail
      contact_email = ""o\"q${x}%{y}"@example.com"   (raw email in tfvars)
--- FAIL: TestGenerator_TfvarsValuesCannotChangeStructure (aws-wif, aws-cross-account, azure-wif, gcp-sa-impersonation, gcp-wif)
--- FAIL: TestGenerator_CFParamsValuesCannotChangeStructure
--- FAIL: TestGenerator_ShellTemplatesEscapeValues
--- FAIL: TestGenerator_RejectsControlCharacters (all 10 subtests, including the issue's own example)

TestGenerator_PlainValuesRenderVerbatim (the positive control) passes both before and after, as it should.

Pass-after:

$ go build ./... && go vet ./... && go vet scripts/generate-federation-iac.go       # OK
$ go test -short -race -count=1 ./scripts/ ./internal/api/ ./internal/iacfiles/    # ok x3
$ golangci-lint run --allow-parallel-runners ./internal/api/... ./scripts/...       # v2.10.1: 0 issues
$ golangci-lint run --allow-parallel-runners scripts/generate-federation-iac.go     # no new findings vs main (the file has 11 pre-existing ones)
$ gocyclo -over 10 scripts/generate-federation-iac.go internal/api/handler_federation.go   # silent
$ bash scripts/gosec-hook.sh <changed files>                                         # clean
$ git secrets --register-aws (temp global config) && git secrets --scan <changed files>  # clean
$ bash scripts/test-git-secrets-allowlist.sh                                         # 42 PASS, 0 FAIL
$ git diff --check                                                                   # clean

Mutation probes on the script. Each one was applied by hand and reverted. Every one fails at least one new test:

Mutation	Caught by
drop the ${ rule from hclEscape	TfvarsValuesCannotChangeStructure
drop the %{ rule	TfvarsValuesCannotChangeStructure
use hclEscape for .sh.tmpl	ShellTemplatesEscapeValues
use hclEscape or shellEscape for .json.tmpl	CFParamsValuesCannotChangeStructure
disable checkNoControlChars	RejectsControlCharacters
skip escapeData entirely	tfvars, JSON and shell tests
server: render tfvars without hclEscapeData	BundleTfvarsEscapesSessionEmail

Real-parser check. I generated hostile tfvars for gcp/azure, azure/aws and gcp/gcp and loaded each with terraform console -var-file. Terraform returned the exact original --account-name and --contact-email strings, including ", \, ${x} and %{y}. It also decoded the server test's expected contact_email literal back to the original email.

How the tests work: the tfvars test checks that every non-comment line is exactly one quoted HCL attribute. It then checks that the set of attributes matches a benign run and that each value decodes back to its input; an unescaped ${/%{ fails the decode. The JSON test unmarshals the output and checks there are exactly four parameters. The Bash test actually runs the rendered script with bash and compares what Bash sees against the literal inputs, which include $(printf INJECTED) and backticks.

Out of scope / notes for follow-up

• chore(scripts): generate-federation-iac.go iacData has drifted out of sync with the templates it renders #153 (iacData drift). The script's iacData has no OIDCIssuerHost, so --format bundle for an AWS target with a non-AWS source fails to render aws-cfn-deploy.sh.tmpl on main today. Because of that, the script cannot currently produce any .sh output. TestGenerator_ShellTemplatesEscapeValues therefore uses a probe aws-cfn-deploy.sh.tmpl (passed via --templates-dir) that has the same quoting shapes as the real templates. The .sh escaping is in place now so that fixing chore(scripts): generate-federation-iac.go iacData has drifted out of sync with the templates it renders #153 doesn't reopen this issue.
• shellEscape does not handle } inside "${VAR:-default}". This applies to both the server and the script, since the script mirrors the server's rule. A } in the value ends the expansion early and corrupts the value. It can't run a command or break out of the quotes, because $, backticks, " and \ are escaped. A fix needs a context-aware rule for the server's CLI templates too, so I left it alone. The shell test avoids } in that position and a comment explains why.
• Server Bicep/ARM deploy.sh is rendered unescaped. writeAzureTemplateFiles renders azure-wif-bicep-deploy.sh.tmpl with raw data instead of shellEscapeData(data), unlike writeCFNFiles. The template embeds ContactEmail in CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-...}", and mail.ParseAddress accepts "a$(id)b"@example.com. This looks like a separate shell-injection bug in a server path that generate-federation-iac: CLI flag values reach tfvars/shell templates unescaped (HCL injection via --account-name) #952 doesn't cover, and it probably deserves its own issue.
• --account-slug in output file names. The script uses --account-slug verbatim in the default output file name (and in zip entry names), so a value with / or .. can write outside the current directory. The operator controls this flag; I didn't change it.
• handler_federation.go (now about 865 lines) and handler_federation_test.go were already over the 500-line guideline before this change. The new script tests are in a new file, scripts/generate_federation_iac_escape_test.go, because both existing script test files are at the limit.

Labels: I can't apply labels. Could a maintainer apply the triage labels? The issue has none yet; the issue suggests type/security, type/bug, priority/p3, severity/low.

Closes #952

🤖 Generated with Claude Code

[coderabbitai]

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 3146b167-db7a-45ab-b4d8-d6d6880ca54e

📥 Commits

Reviewing files that changed from the base of the PR and between 4a3fa9c and 61be15a.

📒 Files selected for processing: 4

• internal/api/handler_federation.go
• internal/api/handler_federation_test.go
• scripts/generate-federation-iac.go
• scripts/generate_federation_iac_escape_test.go

+---------------------------+
| I refactor in bunny hops. |
+---------------------------+
  \
   \     /)/)
        ( '')
        /  |
      o(___)

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[IAmJSD]

The Unit Tests failure on this PR is TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh (scheduler_test.go:1003). It isn't caused by this change. The same test fails on main: the CI - Build & Test runs on main went red starting at 18ee477 (#938), and the run just before it (7138188) was green. #959's agent also reproduced it locally on a clean origin/main. This PR doesn't touch internal/scheduler. Every other check on this head that has finished has passed.

🤖 Generated with Claude Code

[cristim]

Independent review of 69d52ae: draft: blocks ready

No BLOCKING code findings. The script half of #952 is correct for the formats it can reach. The blocks below are gate items.

Findings

1. NON-BLOCKING (security, follow-up, pre-existing): internal/api/handler_federation.go:558-562 renders azure-wif-bicep-deploy.sh.tmpl with raw data for format=bicep|arm; only cli and the CFN deploy script go through shellEscapeData. Probe: session email "a$(id)id\"; touch X; \""@example.com passes mail.ParseAddress and lands as CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-"a$(id)id\"; touch X; \""@example.com}" in deploy-azure.sh, so $(id) runs when the downloader runs the script. The victim is the owner of that email, so impact is low, but #952 asked the PR to confirm the server side and this path is not covered. Fix: shellEscapeData(data) at line 558, plus a test beside the new bundle test.
2. NON-BLOCKING: --account-slug is unvalidated and used raw in output and zip entry names (scripts/generate-federation-iac.go:297-313, 356, 384). --account-slug ../../evil is accepted (exit 0). Escaping cannot fix this; file separately.
3. NON-BLOCKING: the Bash rule leaves } unescaped inside ${V:-...}, so a}b renders as ab} (the test file documents this). On macOS /bin/bash 3.2 an odd number of ' inside "${V:-...}" is a syntax error. Both fail loud or corrupt only the value; I found no command execution (probed '"; touch X; "', $(), backticks on 3.2).
4. NON-BLOCKING: server hclEscape (handler_federation.go:~414) does not reject control characters, unlike the script. Safe today because mail.ParseAddress rejects CR/LF (probed) and the other fields are server-derived; note for later. hclEscape, shellEscape and the reflect walker are duplicated between script and server (script is //go:build ignore, so sharing is not trivial); the cross-reference comment is acceptable.
5. NON-BLOCKING: aws-cfn-deploy.sh.tmpl needs .OIDCIssuerHost, which the script's iacData lacks, so --target aws --source gcp|azure --format bundle fails on main as well as on this head (pre-existing). The script's Bash escaper is therefore only reachable through that broken path; unit tests cover it.

What I attacked

• HCL: built --account-name/--contact-email payloads with ", \, ${, %{, $${, %%{ and $(...), rendered tfvars for all 5 target/source pairs, ran terraform fmt and terraform apply -var-file on the aws-wif output; the variable round-trips byte for byte. The $${/%%{ doubling stays correct when the input already contains $${.
• JSON: slug a"b\c</>é rendered through cf-params, jq parses it and the value is intact.
• Shell: simulated "${V:-<escaped>}" under bash for quote, $(), backtick, backslash and ! payloads; no breakout. Findings 3.
• Control chars: newline, CR, tab and ESC are rejected before any render; checkNoControlChars runs after populateData, so the #930 validators (account ID shapes, subject claim, source account ID, Azure URL) still fire first. Escaping is defense in depth.
• Server: renderTemplate(tfvars, hclEscapeData(data)) only affects tfvars; the CFN deploy script keeps its single shell escaping (the test asserts no $${). Finding 1 for the uncovered path.
• Fail-before/pass-after: at the merge-base production code with the PR tests, TestGenerator_Tfvars..., CFParams..., ShellTemplates..., RejectsControlCharacters and the server BundleTfvarsEscapesSessionEmail all fail; at the head go test -count=1 ./scripts/ ./internal/api/ passes. Note go test ./scripts can report a stale (cached) pass because the tests exec the generator; use -count=1.

Gate status (origin/main Review gate)

• Independent review: this comment, SHA above.
• CI: red on this SHA. Unit Tests fails on TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh (author reports the same failure on main); Integration Tests also failed and CI Success rolls up. Needs a re-run once main is green; not caused by this diff. Fork runs otherwise passed, pre-commit passed.
• CodeRabbit: skipped (draft), no threads. Not pinged.
• Local verification: macOS, real generator binary and terraform, as above. No real-account verification.
• Labels: PR has none; #952 itself has no triage labels, so there is nothing to mirror. Maintainer triage needed.
• Size: +537 (non-blocking, external contributor).

Blocks ready

• CI green on this SHA (rerun after the main scheduler flake is fixed)
• Labels: triage #952, then mirror to the PR
• Mark ready for review (draft now)
• Optional before merge: finding 1 (one-line shellEscapeData at line 558) or a follow-up issue for it and finding 2

[IAmJSD]

Thanks @cristim. I merged origin/main (2ef2f5d, which includes #960) into this branch at 61be15a, instead of rebasing, to avoid force-pushing over the reviewed commits. The PR's own commits are unchanged: git diff origin/main 61be15a8 matches git diff $(git merge-base origin/main 69d52aef) 69d52aef, and the merge auto-resolved with no conflicts. Locally at 61be15a, the build is ok, and go test passes for the touched packages plus ./internal/scheduler/, including the stale-refresh test. The fork CI run for the new head needs approval. (This PR hadn't been reviewed yet; the merge is only to pick up the scheduler fix so CI can go green.)

🤖 Generated with Claude Code

[cristim]

Independent delta review of 61be15a (previous: 69d52ae): no actionable findings

Delta: OLD_SHA is an ancestor of NEW_SHA. The only new commit is 61be15a, a merge of origin/main (parents: OLD_SHA and 2ef2f5d, picks up #960). No rebase, so range-diff is not needed. git diff OLD NEW on the PR's four files (internal/api/handler_federation.go, handler_federation_test.go, scripts/generate-federation-iac.go, scripts/generate_federation_iac_escape_test.go) is empty, so the escaping grammar for every output format and the injection payload tests are byte-identical to the reviewed code. The other 10 changed files in the delta come from main (scheduler/middleware/negative-cap tests). git merge-tree of origin/main and NEW_SHA is clean, and the PR diff against origin/main is still those 4 files (537 additions).

Previous non-blocking items, all unchanged and none addressed in this delta:

• handler_federation.go:558-562 raw bicep/arm deploy-azure.sh: NOT RESOLVED, tracked as #964.
• --account-slug ../../evil accepted: NOT RESOLVED (non-blocking).
• } in a shell value, odd ' on bash 3.2: NOT RESOLVED (non-blocking).
• --target aws --source gcp|azure --format bundle fails on main too: NOT RESOLVED (pre-existing).

What I attacked: re-ran go test -count=1 ./scripts/ ./internal/api/ at NEW_SHA in a detached worktree: 3554 passed in 2 packages, which includes the escape payload tests.

CI on NEW_SHA: pending, not waited on. Run 38902343704 (CI - Build & Test) is queued/pending; Lint Workflows, Validate Terraform (azure) and the AWS Terraform state namespace check already pass. Run 38902343705 (pre-commit) is in progress. No action_required runs. The previous SHA's CI was green.

Gate: independent review covers this SHA (delta is main-only); local verification is tests above, labels unchanged from the prior review. The PR is still a draft and CI on NEW_SHA must go green before merge.

[cristim]

Gate record for 61be15a (origin/main CLAUDE.md "Review gate"):

• Independent review of 69d52ae: no blocking code findings. Escaping is correct per output format:
  • tfvars round-trips ", \, ${, %{, $${ and $(...) through terraform apply -var-file;
  • the JSON parses in jq;
  • there is no shell breakout;
  • control characters are rejected before rendering;
  • the fix(scripts): validate --account-id per target in generate-federation-iac #930 validators still run first.
• Independent delta review of 61be15a: the only new commit merges origin/main (picks up test(scheduler): kick the stale refresh with an unrestricted reader #960), and the PR's own 4 files are unchanged.
• Non-blocking follow-ups: the pre-existing unescaped bicep/arm deploy-azure.sh render (filed as fix(api): escape template data in the rendered deploy-azure.sh (bicep/arm) #964), --account-slug traversal, } and odd-' handling in shell values, and the size of +537.
• CI on 61be15a: CI Success and Run pre-commit hooks pass. This was a fresh run after the main-wide scheduler fix.
• Local verification (macOS):
  • Fail-before/pass-after on the merge-base generator and server code.
  • go test -count=1 ./scripts/ ./internal/api/ passes 3554 tests.
• CR skipped: draft and quota-limited; covered by the independent reviews plus local verification.
• Labels mirrored from generate-federation-iac: CLI flag values reach tfvars/shell templates unescaped (HCL injection via --account-name) #952, now triaged.

Marking ready (owner-authorized) and merging at 61be15a with --match-head-commit.