[IAmJSD]

Closes #991. This follows up #963 (issue #958), which merged at 7a71710 before this commit landed. The commit is cherry-picked unchanged onto main (adcd80c).

Changes (azure-wif-bicep-deploy.sh.tmpl and its tests)

1. } in the email.
   • The template now does CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-}" and then [ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="{{.ContactEmail}}". So the rendered value is never inside a ${...:-default}.
   • The override behaves as before: a set, non-empty CUDLY_CONTACT_EMAIL wins.
   • shellEscape is unchanged. Adding } there would corrupt every other template, because \} stays literal inside double quotes.
2. JSON in the heredoc.
   • A json_escape helper (printf | sed, no jq) escapes \ and ", and turns a tab into \t. net/mail accepts a tab in a quoted local part and rejects every other control character.
   • The heredoc now uses the escaped contact_email, and also account_name, since CUDLY_ACCOUNT_NAME comes from the operator. The other heredoc values are IDs.

Tests

These run in a bare bash: PATH only, an empty temp directory, set -euo pipefail, and az stubbed. Each runs the block from CONTACT_EMAIL= through the end of the heredoc and stops before curl.

• Hostile emails: nine of them, for both bicep and arm, each checked first to pass mail.ParseAddress. They include a}b, "a}b", "a}$(touch pwned)\touch pwned`}b", "{a}${b}", a tab, and the earlier $(), backtick and "/\` cases.
• Assertions for each: CONTACT_EMAIL comes back exactly; the payload parses with json.Unmarshal and has the exact contact_email; no marker file is created; and bash -n passes on the whole script.
• RegistrationEnvOverrides: covers the override being unset, empty and set, plus a hostile CUDLY_ACCOUNT_NAME.

Verification

All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null. Everything here is local; no az, curl or network calls.

• Pass-after: go test ./internal/api/ -run 'Federation|AzureTemplate|Azure' -count=1 -v gave 310 PASS, 0 FAIL, 1 SKIP. The skip is the existing TfvarsAutoLoadedByTerraform/aws-cross-account. It also passes on this branch.
• Fail-before: with the template restored to the sec(api): shell-escape the Bicep/ARM deploy script render (#958) #963 version, the hostile-email subtests fail (all except a\id`b, which contains no }`, quote or tab), as do the three env-override subtests.
• Mutation probes: none survived.

Mutation	Result
Raw CONTACT_EMAIL in the heredoc	Every " and tab case fails
Tab escape dropped	Only the tab cases fail
account_name unescaped	The account-name test fails
Old ${VAR:-default} form restored	Every } case fails
Backslash escape dropped	The \ case fails
Override inverted	Broad failures

• Other checks:
  • go vet ./internal/api/ is clean, golangci-lint v2.10.1 reports 0 issues, and gofmt is clean.
  • shellcheck 0.11.0 on a script rendered with a hostile email exits 0.
  • git-secrets (AWS patterns plus the repo's custom ones) is clean, and bash scripts/test-git-secrets-allowlist.sh gives 42 PASS, 0 FAIL.

Out of scope

• The same pattern in 8 other templates, listed in Follow-ups from #963 review: '}' and JSON metacharacters in the Bicep/ARM contact email #991. A follow-up could reuse this pattern and test helper.
• Newlines on comment lines, tracked in sec(api): newline in account fields can break out of comment lines in federation deploy scripts #970.
• CR/LF in values, which the helper comment notes it doesn't handle. net/mail rejects them in emails.

Maintainer: please apply labels to #991 and this PR. #958's suggested set is type/security, type/bug, priority/p2, severity/medium.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 3 billable files and costs up to $0.75.}

Or wait 59 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

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: c67a0b4b-e9ed-402e-aa5d-9a3e700192f2

📥 Commits

Reviewing files that changed from the base of the PR and between adcd80c and df91517.

📒 Files selected for processing: 3

• internal/api/handler_federation_azure_escape_test.go
• internal/api/handler_federation_test.go
• internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl

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

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

[cristim]

Independent review of df91517: no actionable findings

Findings

None BLOCKING. One NON-BLOCKING note on #964 below.

NON-BLOCKING, issue bookkeeping: open issue #964 (unescaped deploy-azure.sh render) is already fixed on main by #963. internal/api/handler_federation.go:561 renders the template with shellEscapeData(data). This PR does not change that. What it adds is the residual } case (#991) and JSON escaping of the payload. Suggest closing #964 as done, since #992 does not close it either (the PR closes only #991).

What I attacked

• Rendered deploy-azure.sh for both bicep and arm with 11 emails, each valid for the auth service: a}b, "a\"b\\c$HOME", backtick and $(...) substitutions, a } combined with $(...) and backticks, ', tab, &|;, trailing backslash, %s, !#. Checked bash -n on each (all pass).
• Ran each script end to end in env -i with only a stub az and a stub curl first on PATH, an empty cwd, and dummy template files. All 22 runs exited 0. The captured curl -d body parsed as JSON, and contact_email equaled the input byte for byte. No marker file was created (no command ran).
• Consistency with the other escapers: shellEscape (handler_federation.go:399) is unchanged and still correct for the other templates. } is not added to it, which is right because \} is literal inside double quotes. The ${VAR:-default} form is gone from this template, so no escaper has to handle } here. hclEscape (#955) targets a different format and is not touched.
• json_escape (template lines 74-78): the sed expressions are POSIX, the tab pattern comes from printf '\t', and the replacement has no &. CR/LF is not handled, and the helper comment says so honestly. The email cannot hold them (net/mail), and CUDLY_ACCOUNT_NAME is operator-supplied. A newline in that env var would give invalid JSON and a failed registration, not an injection.
• Override semantics: unset, empty and set CUDLY_CONTACT_EMAIL behave as ${VAR:-default} did (tests plus my runs).
• Fail-before: restored the parent template with the new tests. TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail fails for 16 of 18 subtests (the aidb case has no }/quote/tab, and the first backtick case fails on the JSON escape), and RegistrationEnvOverrides fails. At the SHA, go test ./internal/api -run 'Azure|Federation' gives 314 passed. Reverted, worktree clean.
• Scope: 3 files, +120/-28, no other call site of ContactEmail in the template. The 8 sibling templates are explicitly deferred to #991.

Gate status (origin/main Review gate)

• Independent adversarial review naming the SHA: this comment.
• CI on the SHA: not complete at review time. 8 pass, 13 pending, 0 failed, 0 action_required.
• CodeRabbit: no actionable threads from me to count (not pinged).
• Labels: match #991 (triaged, priority/p1, severity/high, urgency/this-sprint, impact/few, effort/xs, type/security).
• Local verification: real bash runs of the rendered script with stubbed az/curl (macOS), fail-before/pass-after shown. No real Azure account was used.
• Merge condition: all CI green on this SHA.

[cristim]

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

• Independent review of df91517: no actionable findings. The bicep and arm deploy-azure.sh are rendered with 11 hostile contact emails (}, ", \, $(...), backticks, ', tab and others).
  • bash -n passes on all of them.
  • All 22 scripts run with az and curl stubbed under env -i: each exits 0, no command executes, and the JSON passed to az keeps contact_email intact.
• CI on df91517: CI Success and Run pre-commit hooks pass.
• Local verification (macOS):
  • Fail-before: against the parent template, 16 of 18 escape subtests and the env-override tests fail.
  • Pass-after: 314 Azure|Federation tests pass.
  • No real Azure call.
• fix(api): escape template data in the rendered deploy-azure.sh (bicep/arm) #964 was already fixed by sec(api): shell-escape the Bicep/ARM deploy script render (#958) #963 and is closed. This PR covers the remaining }/JSON-escaping gap from Follow-ups from #963 review: '}' and JSON metacharacters in the Bicep/ARM contact email #991.
• CR skipped: quota-limited; covered by the independent review plus local verification.
• Labels mirrored from Follow-ups from #963 review: '}' and JSON metacharacters in the Bicep/ARM contact email #991.

Merging at df91517 with --match-head-commit.