[IAmJSD]

Closes #1436

Root cause

Eight generated scripts that self-register with CUDly pre-filled the contact email as the default of a shell expansion:

CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-{{.ContactEmail}}}"

shellEscape escapes \, ", ` and $, but not }, and net/mail accepts }. A } in the session email therefore closed the ${...:-...} early, and the script registered a garbled address: a}b@example.com became ab}@example.com. The registration heredoc also put CONTACT_EMAIL and ACCOUNT_NAME into the JSON raw, so a ", \ or tab produced an invalid payload. Nothing could execute: the shell metacharacters stay escaped. This is a correctness bug, not an injection.

Fix

Ported the Bicep fix from #958 / #992 to the 8 templates under internal/iacfiles/templates/: aws-cfn-deploy, aws-cross-account-cfn-deploy, aws-cross-account-cli, aws-wif-cli, azure-wif-cli, azure-wif-deploy, gcp-sa-impersonation-cli, gcp-wif-cli (all .sh.tmpl).

• The env value is assigned first, and the pre-filled email is the fallback on its own line: CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-}" then [ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="{{.ContactEmail}}". No template value sits inside ${...} any more. The override semantics are unchanged: a set, non-empty CUDLY_CONTACT_EMAIL wins.
• The payload uses json_escape'd copies of the email and account name (CONTACT_EMAIL_JSON, ACCOUNT_NAME_JSON). The helper is the Bicep script's, byte for byte (printf + sed, no jq).
• scripts/generate-federation-iac.go renders these same template files with the same shellEscape rule, so it needs no code change. Its escape test's probe template now uses the new two-line form, and its hostile shell email now includes the } it previously had to leave out.

Verification

All local runs, Linux, bash 5:

• New internal/api/handler_federation_registration_escape_test.go, reusing the Bicep/ARM federation bundle deploy.sh embeds the user's email without shell escaping (command injection) #958 sandbox: evalRegistration became evalRegistrationWith, which takes a stub prelude.
  • TestRegistrationScripts_KeepContactEmailAndPayloadIntact fetches each of the 7 shipped scripts through getFederationIaC, the real download path (format=cli for the 5 CLI scripts, format=cfn for the 2 CFN deploy scripts), with the session email set to each of the 9 Bicep/ARM federation bundle deploy.sh embeds the user's email without shell escaping (command injection) #958 hostile emails: }, ${}, quotes, backslashes, a tab, $(...) and backticks. Each script must pass bash -n, and its registration block, run with stubbed aws/az and no network, must leave CONTACT_EMAIL equal to the email, produce a payload that parses as JSON with the exact email, and create no pwned file. No download format ships azure-wif-deploy.sh.tmpl today; only the iacfiles tests render it. It goes through the same shellEscapeData + renderTemplate the handler uses.
  • TestRegistrationScripts_EnvOverridesAndAccountName checks, for all 8 scripts, that unset or empty CUDLY_CONTACT_EMAIL falls back to a pre-filled email containing }, that a set value wins, and that a CUDLY_ACCOUNT_NAME of Ops "prod" \ team<TAB>B reaches account_name verbatim.
  • TestRegistrationTemplates_NoDrift is the drift test. It scans every templates/*.sh.tmpl that POSTs to /api/register and fails if one is missing from the tested list, puts {{.ContactEmail}} inside a ${VAR:-...}, writes "contact_email"/"account_name" without the _JSON variables, or carries a json_escape that differs from the Bicep script's.
• Fail-before: with the 8 templates restored from origin/main, 72 of the 80 subtests fail, and so does the drift test. The 8 passes are the a`id`b@example.com case, which has no } or JSON metacharacter.
• Pass-after: all pass.
• Two existing assertions were updated for the new form: the PII-guard needle in TestGetFederationIaC_CLIMatrix, and the generator probe and hostile email noted above. The Bicep/ARM federation bundle deploy.sh embeds the user's email without shell escaping (command injection) #958 Bicep/ARM tests, the shell-escaping tests (nothing executes through $, `, " or \), the iacfiles template tests and the generator drift tests (scripts/) all still pass.
• go build ./..., go vet ./..., go vet -tags integration ./..., (cd tests/e2e && go vet -tags e2e ./...): clean.
• golangci-lint v2.10.1 on ./internal/api/ ./internal/iacfiles/... ./scripts/, with and without --build-tags integration: 0 issues. No non-test Go files changed, so gocyclo had nothing to check.
• go test -race -short -count=1 on those packages: ok. go test -race -tags integration -count=1 on the same packages: internal/iacfiles and scripts passed. In the full internal/api run, 27 Postgres-backed tests (ladder, marketplace, planned purchases and others, none touching federation) failed after the shared testcontainer stalled for 364s on this busy machine. Rerun alone with -run '^(<those 27>)$', all 27 passed. None of them touch the changed files.
• secrets-scan.sh on the changed files: "clean; planted key flagged". scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.

Not verified locally: running the scripts end to end against real AWS, Azure or GCP accounts and a live /api/register. Only the registration block runs, with the cloud CLIs stubbed. No Terraform files changed. Out of scope and unchanged: other ${VAR:-{{.Field}}} defaults (ProjectID, ServiceAccountEmail, CUDlyAPIURL, ...). Those values are server-derived rather than user-supplied.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 12 billable files and costs up to $3.00.}

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: c8bb869
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: ad607853-0baa-44bf-b27e-566dcd3c9cd8

Commits

Reviewing files that changed from the base of the PR and between c8bb869 and 6077c1e.

Files selected for processing: 12

• internal/api/handler_federation_azure_escape_test.go
• internal/api/handler_federation_registration_escape_test.go
• internal/api/handler_federation_test.go
• internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl
• internal/iacfiles/templates/aws-cross-account-cfn-deploy.sh.tmpl
• internal/iacfiles/templates/aws-cross-account-cli.sh.tmpl
• internal/iacfiles/templates/aws-wif-cli.sh.tmpl
• internal/iacfiles/templates/azure-wif-cli.sh.tmpl
• internal/iacfiles/templates/azure-wif-deploy.sh.tmpl
• internal/iacfiles/templates/gcp-sa-impersonation-cli.sh.tmpl
• internal/iacfiles/templates/gcp-wif-cli.sh.tmpl
• scripts/generate_federation_iac_escape_test.go

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

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

[IAmJSD]

Opened at head 6077c1e: the #958 Bicep pattern (CONTACT_EMAIL default on its own line, json_escape for the payload) ported to the 8 other registering deploy scripts, with a regression test over all of them and a drift test; fail-before/pass-after and local check results are in the PR body.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 6077c1ed49ef7859ac52e52af0c1599a4a418ad9

Blocking:

• none

Non-blocking:

• The issue suggested CONTACT_EMAIL_DEFAULT=...; CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-$CONTACT_EMAIL_DEFAULT}". The PR uses the Bicep script's two-line form instead. The two behave the same: unset or empty falls back, and a set value wins. TestRegistrationScripts_EnvOverridesAndAccountName pins this, and the form is consistent with Bicep/ARM federation bundle deploy.sh embeds the user's email without shell escaping (command injection) #958. Fine as is.
• json_escape does not escape control characters other than tab. A CR, LF or other control byte in an operator-set CUDLY_ACCOUNT_NAME still produces invalid JSON, and the server rejects the registration. Nothing runs. The email can't carry these characters: net/mail allows only VCHAR/WSP, and renderTemplate refuses line breaks (sec(api): newline in account fields can break out of comment lines in federation deploy scripts #970). The helper's comment says this, so it's acceptable.
• azure-wif-deploy.sh.tmpl is fixed and tested, but as the PR body says, no download format ships it. It may be dead. That's worth a separate look, not something for this PR.

Attacked:

• Read all 8 template diffs and confirmed they are byte-identical hunks. No {{.ContactEmail}} is left inside a ${...} in any .sh.tmpl. The only other occurrences are in the tfvars templates, which hclEscape covers. The heredoc expands ${CONTACT_EMAIL_JSON} once and does not re-parse the result, so backslashes survive into valid JSON. The [ -n ... ] || ... line is safe under set -euo pipefail. Other template fields can't break out of the # Target ...: {{.AccountName}} comment lines, because renderTemplate rejects newlines.
• Fail-before: restored the 8 templates from origin/main. go test ./internal/api -run TestRegistration then failed 72 of the 80 hostile-email subtests, plus EnvOverridesAndAccountName and NoDrift. The 8 passes are the a`id`b@example.com case, as the body says. Restored afterwards, and all pass at head.
• go build ./..., and go vet with and without -tags integration on ./internal/api ./internal/iacfiles/... ./scripts: OK. go test -race -short -count=1 on the same packages: ok. go test -race -tags integration -count=1 on ./internal/iacfiles/... ./scripts: ok. On ./internal/api I ran -run 'Federation|Registration|Escape|IaC': ok. I didn't run the full internal/api integration suite because only test files and templates changed in that package. golangci-lint with and without integration tags: 0 issues.

Body-stale:

• none

_{Reviewer: rC2. 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.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE 6077c1ed49ef7859ac52e52af0c1599a4a418ad9

Blocking:

• none

Non-blocking:

• internal/iacfiles/templates/*.sh.tmpl (json_escape): the helper doesn't handle CR/LF or other control characters, as its comment says. The email can't carry them (net/mail rejects them). A CUDLY_ACCOUNT_NAME the operator sets with a newline would still produce invalid JSON. That is operator input on their own machine, so it's acceptable.
• The issue suggested a CONTACT_EMAIL_DEFAULT variable. The PR uses the Bicep script's [ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="..." form instead. The semantics are the same as ${VAR:-default} (unset or empty falls back), and TestRegistrationScripts_EnvOverridesAndAccountName pins them. Keeping all nine scripts identical is the better choice.
• From an earlier review, and I agree: no download format ships azure-wif-deploy.sh.tmpl (no non-test reference in internal/api or scripts/), so it may be dead. That's a follow-up, not something for this PR.

Attacked:

• Escaping and injection: every CLI, CFN and bundle render path for .sh templates goes through shellEscapeData (handler_federation.go:322, 612, 720), and the generator goes through escaperFor(".sh.tmpl") = shellEscape. The pre-filled email now sits only inside a plain double-quoted assignment, where } is inert and \ " $ and backtick stay escaped. All 8 templates use set -euo pipefail. [ -n ] || assign is exempt from -e, and ${CUDLY_CONTACT_EMAIL:-} is safe under -u. The $(printf '\t') sed expression is the same as the Bicep helper's (the drift test enforces it). shellcheck -S warning counts on the templates are unchanged from main.
• The tests exercise the real thing: they extract the registration block from the script as actually downloaded (via getFederationIaC, cli and cfn zip) and run it in bash with stubbed aws/az. Fail-before: with internal/iacfiles/templates/ checked out from origin/main, 64 KeepContactEmailAndPayloadIntact subtests, all 8 EnvOverridesAndAccountName subtests and NoDrift fail. Restored, all pass.
• go build ./...; go vet (plain and -tags integration) on ./internal/api/ ./internal/iacfiles/... ./scripts/; go test -race -short -count=1 on the same: ok. go test -race -tags integration -count=1 ./internal/iacfiles/... ./scripts/: ok. ./internal/api/ -run 'Federation|Registration': ok. golangci-lint, with and without integration tags: 0 issues.

Body-stale:

• none

_{Reviewer: rC3. 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.}

[IAmJSD]

Local verification at 6077c1ed49ef7859ac52e52af0c1599a4a418ad9 merged onto main (d87b96ff), together with #1437, #1438, #1439, #1440, #1441 in that order (combined tree 37f4d0bb, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

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 6077c1ed (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly 6077c1ed49ef7859ac52e52af0c1599a4a418ad9.