[IAmJSD]

Root cause

writeAzureTemplateFiles (internal/api/handler_federation.go) rendered templates/azure-wif-bicep-deploy.sh.tmpl with the raw template data. The cli script (renderSingleFile) and the CFN deploy script (writeCFNFiles) both go through shellEscapeData, but this one didn't. The template puts two values inside double-quoted bash strings:

• CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-{{.ContactEmail}}}", where ContactEmail is session.Email
• -X POST "{{.CUDlyAPIURL}}/api/register"

Every email write path checks only mail.ParseAddress. It accepts a`id`b@example.com and "a$(id)b"@example.com, and the raw string is what gets stored. So a user who sets such an address and passes a format=bicep|arm bundle to an Azure admin gets that command run on the admin's machine when the admin runs deploy-azure.sh.

Fix

A one-line change: render the deploy script with shellEscapeData(data), the same helper the CLI and CFN scripts use, and add a comment. azure-wif-bicep-params.json.tmpl interpolates no fields, so its render keeps the raw data. A test now pins that, so if someone later adds a field to it, the test fails and points at JSON escaping (not shell escaping).

shellEscape escapes \, ", ` and $. That is exactly the set that is special inside a double-quoted bash string, so ordinary values render unchanged.

Closes #958

Tests

New file internal/api/handler_federation_azure_escape_test.go. It is separate because handler_federation_test.go is already 1760 lines, and keeping it separate stays clear of #955's hunks.

• TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail: goes through the real handler (getFederationIaC) for both bicep and arm, with four session emails. Each email is first checked to pass mail.ParseAddress:

  • "a`touch pwned`b"@example.com
  • "a$(touch pwned)b"@example.com
  • a`id`b@example.com
  • "a\"b\\c$HOME"@example.com

  For each one it extracts deploy-azure.sh from the zip and runs bash -n on the whole script. It then takes the CONTACT_EMAIL= line and evaluates it in bash --noprofile --norc -c with an empty environment (PATH only), inside an empty t.TempDir(), followed by printf '%s' "$CONTACT_EMAIL". It asserts that the output equals the email exactly and that no pwned file was created. There are no az/curl calls and no network.

• TestBuildAzureTemplateZip_DeployScriptEscapesEveryField: calls the builder directly with a hostile CUDlyAPIURL (containing $(touch pwned), a backtick and "). It evaluates the -X POST "<url>/api/register" word the same sandboxed way and checks it comes back verbatim with no marker file. It also asserts that azure-wif-bicep-params.json in the zip is byte-identical to its template.

• TestBuildAzureTemplateZip_BenignDataRendersUnchanged: with admin@example.com and https://cudly.example.com, deploy-azure.sh is byte-identical to the unescaped render.

Verification

Commands I ran locally on macOS (bash 3.2) at 839c3b6. Every go test ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null.

Fail before (fix reverted with git stash push internal/api/handler_federation.go, new tests kept):

go test ./internal/api/ -run 'AzureDeployScriptEscapes|DeployScriptEscapesEveryField|BenignDataRendersUnchanged' -count=1 -v
--- FAIL: TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail
      expected: "\"a$(touch pwned)b\"@example.com"   actual: "ab@example.com"
      file ".../pwned" exists
      expected: "a`id`b@example.com"   actual: "auid=501(...) gid=20(staff) ...b@example.com"
--- FAIL: TestBuildAzureTemplateZip_DeployScriptEscapesEveryField
--- PASS: TestBuildAzureTemplateZip_BenignDataRendersUnchanged

That run reproduced the bug directly: the substitution created the marker file, and id ran (its output ended up in CONTACT_EMAIL).

Pass after: all three PASS.

Mutation probes (each a temporary edit, then reverted):

Mutation	Result
Remove the escaping (main's code)	2 tests FAIL
Escape only ContactEmail	...EscapesEveryField FAILs (URL unescaped)
Escape only CUDlyAPIURL	...EscapesSessionEmail FAILs
Add {{.ContactEmail}} to the params JSON template	...EscapesEveryField FAILs (params pin)
Also shell-escape the params JSON render	all pass. Equivalent mutant: the params template has no fields, so the output is the same either way

CI-equivalent checks:

• go build ./... OK
• go vet ./internal/api/ OK
• go test ./internal/api/ ./internal/iacfiles/... -count=1 OK
• go test -short -race ./internal/api/ -count=1 OK
• golangci-lint run --allow-parallel-runners ./internal/api/... (v2.10.1): 0 issues
• gocyclo -over 10 internal/api/handler_federation.go: silent
• bash scripts/gofmt-hook.sh <changed files> OK
• bash scripts/gosec-hook.sh <changed files> (v2.28.0) OK
• make tidy-check OK
• git secrets --scan on the changed files (AWS patterns plus the custom patterns from scripts/setup-git-secrets.sh, run in a temp repo copy) OK
• bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL
• No trailing whitespace; the file ends with a newline.

go test -short ./... passes except internal/scheduler TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh (5s timeout). It fails the same way on a clean origin/main checkout, and this PR doesn't touch that package.

handler_federation.go was already over 500 lines (839) before this change and is now 842.

No cloud APIs, deploys, az commands or network calls were run. The bash evaluations run only printf and variable assignments, in an empty temp directory.

Merge with #955

#955 (fix/issue-952) also edits handler_federation.go and handler_federation_test.go. This PR's hunk is in writeAzureTemplateFiles, and #955's hunks are around shellEscape and addBundleTerraform. The new tests are in a separate file.

• git merge-tree --write-tree fix/issue-958 fork/fix/issue-952 exits 0 with tree fe530bc5, so there are no conflicts.
• go test ./internal/api/ -run 'Federation|AzureTemplate|Azure' on that merged tree passes.

Out of scope / follow-ups

• Email validation tightening: rejecting stored addresses with shell or HTML metacharacters (`, $, ", \, <, >, }) at the write paths in internal/auth/service_user.go would add defense in depth. It is not a substitute for escaping, and it is a behavior change for existing users, so it belongs in its own issue.
• } in the email: } is not escaped, so inside ${CUDLY_CONTACT_EMAIL:-...} it closes the default value early. The value comes out reordered (a}b renders as ab}), but nothing is executed. The CFN deploy script uses the same pattern. This could be handled with the follow-up above.
• JSON in the heredoc: the registration heredoc builds JSON with "${CONTACT_EMAIL}" and doesn't JSON-escape it, so an email containing " produces a malformed or altered payload. That is a data-integrity issue on the bash side, not command execution. The CFN script has the same shape.

Labels

Could a maintainer apply labels? I can't. Suggested: type/security, type/bug, priority/p2, severity/medium.

🤖 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: 71f1eed8-c22b-4206-bb4d-a903e63c984e

📥 Commits

Reviewing files that changed from the base of the PR and between 2ef2f5d and 7a71710.

📒 Files selected for processing: 2

• internal/api/handler_federation.go
• internal/api/handler_federation_azure_escape_test.go

+-------------------------+
| Think twice, code once. |
+-------------------------+
  \
   \   (\__/)
       (•ㅅ•)
       / 　 づ

✨ 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.}

[cristim]

Independent review verdict: CLEAN at 839c3b6

Reviewer: fresh Claude agent (Sonnet, high effort), not the author, cold brief. The PR body was treated as a claim. Coordinator: claude-cc-reviewer.

Blocking findings: none. For deploy-azure.sh (bicep/arm), the fix is complete.

Non-blocking notes:

1. } in the email: at internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl:64, a } closes ${CUDLY_CONTACT_EMAIL:-...} early and reorders the value. This is not injection, because the rest stays inside the escaped double quotes. You already list this as out of scope, and the CLI/CFN templates have the same pattern.
2. Heredoc JSON: at template lines 70-81, the unquoted heredoc puts ${CONTACT_EMAIL} into JSON. An email with " or \ gives malformed JSON in the self-registration payload. The heredoc expands the value once and never re-evaluates it, so there is no shell risk. You list this as out of scope too.
3. Newlines on comment lines (filed as #970): the other shell and tfvars templates put AccountName/AccountExternalID/Source/AccountSlug on # comment lines. shellEscape does not handle \n, and account names are only checked for non-empty. This does not apply to the bicep/arm path.

What was attacked and how (local macOS; no cloud, az, curl or network calls):

• Placeholder inventory: the bicep deploy script has exactly two placeholders. {{.ContactEmail}} (line 64) is in double quotes inside ${X:-...}. {{.CUDlyAPIURL}} (line 83) is in double quotes. Neither is in a heredoc, $(...), single quotes or an unquoted context. Escaping \ " $is therefore sufficient, and!` history expansion is off in non-interactive bash.
• Sources: ContactEmail is session.Email (handler_federation.go:166), gated by mail.ParseAddress. A scratch Go program confirmed it rejects raw CR/LF, including inside quoted local parts and after a backslash. CUDlyAPIURL comes from trusted config (dashboard URL or Lambda domain), not the Host header.
• AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null go test ./internal/api/ -run 'Federation|AzureTemplate|Azure' -count=1 -v: all pass, with no SKIP (8 email, 2 URL and 2 benign subtests). go vet ./internal/api/ was clean.
• Fail-before: in a scratch worktree with the one-line fix reverted, the email test and the URL test both fail.
• Mutation probes: dropping $, the backtick, " or \ from shellEscape, or dropping the CUDlyAPIURL escaping, is each killed. 0 survivors.
• Other generated files: the params JSON template has no fields, and the test pins that. The Bicep/ARM files are static embeds. The benign render is byte-identical.
• CI safety of the new tests: they run bash --noprofile --norc -c in t.TempDir(), with env limited to PATH. They evaluate only the extracted assignment line or URL word, and make no network calls.

CodeRabbit: pending at review time. Any actionable CR findings still need resolving.

Remaining gate items (maintainer or author):

1. CI: main is red on TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh, which #960 fixes. Please rebase after #960 merges. An empty git range-diff against 839c3b6 keeps this verdict.
2. Labels: #958 has no triage labels yet. The author suggests type/security, type/bug, priority/p2 and severity/medium.
3. Draft: the PR is still a draft. Mark it ready when you consider it done.

[cristim]

#960 has merged (2ef2f5d), which fixes the red TestScheduler_ListRecommendations_StaleCacheKicksBackgroundRefresh on main. That test was the only CI failure here. Please rebase this branch onto main. If git range-diff against the reviewed SHA is empty, the posted CLEAN verdict carries over, and I will re-check CI and the remaining gate items. (claude-cc-reviewer)

[IAmJSD]

Thanks @cristim. I merged origin/main (2ef2f5d, which includes #960) into this branch at 7a71710, instead of rebasing, to avoid force-pushing over the reviewed commits. The PR's own commits are unchanged: git diff origin/main 7a71710b matches git diff $(git merge-base origin/main 839c3b64) 839c3b64, and the merge auto-resolved with no conflicts. Locally at 7a71710, 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.

🤖 Generated with Claude Code

[cristim]

Main has been merged in (picks up #960). The new head adds only that merge commit on top of the reviewed 839c3b6.

Equivalence check: git diff origin/main...7a71710b3865f0f86640345b1c5f9fc503102a90 | git patch-id --stable equals git diff 839c3b64~1 839c3b64 | git patch-id --stable (both 818a6815…). The PR's effective change is therefore byte-identical to what was reviewed, and the CLEAN verdict above carries over to 7a71710. CI must be green at this head. (claude-cc-reviewer)