fix(api): reject control characters in account fields and fail closed in federation IaC render

[IAmJSD]

Summary

A CR or LF in a value that a federation template prints on a # comment line ends the comment, and the rest of the value runs as shell, or as HCL in a tfvars file. shellEscape and hclEscape escape quoting metacharacters but not line breaks. The account write paths also accepted CR and LF in name and external_id.

Closes #970

What reproduces on main

Checked on main (0ed85aa3, which already includes #955 and #992) before changing anything:

1. Write path: POST /api/accounts through the real createAccount handler accepts {"name":"Acme\ntouch PWNED","external_id":"x\ntouch PWNED"} with no error. PUT goes through the same validateCloudAccountRequest.
2. Renderer: renderTemplate("templates/azure-wif-cli.sh.tmpl", shellEscapeData(data)) with AccountName = "Acme\ntouch PWNED\n#" produces

   # Target Azure subscription: Acme
   touch PWNED
   # ()
   

   Running the header in an empty sandbox creates the marker file.
3. Handler today: GET /api/federation/iac does not put stored account fields into the templates. buildGenericIaCData leaves AccountName/AccountExternalID empty, AccountSlug is the constant target, and Source comes from an allowlist. So the server download is not exploitable through account fields right now, which answers the issue's "not verified end to end" note. The only per-request value that reaches the templates is the session email, and mail.ParseAddress rejects CR/LF in it. CUDlyAPIURL comes from config or the Lambda DomainName; the HTTP adapter never sets DomainName.
4. Script generator: scripts/generate-federation-iac.go already rejects every flag value that contains a control character (checkNoControlChars, unicode.IsControl, from fix(scripts): escape flag values per output format in generate-federation-iac #955), so it needs no change here.

The weakness is still real: the write paths store the bad value, and the renderer turns it into a working injection as soon as anything passes that value in. The fix closes both sides.

Fix

The fix rejects bad values rather than escaping or stripping them. There is no escape that keeps a line break inside a # comment. Silently stripping characters would change an identifier such as an external ID without telling anyone.

Layer	Change
validateCloudAccountRequest (create, update, self-account, registration approval)	Any control character (unicode.IsControl: C0 including CR/LF/tab, DEL, C1 such as NEL) in name or external_id gets a 400 naming the field.
validateRegistrationRequest (public POST /api/register)	Still strips CR/LF from account_name (#544, unchanged). Now also rejects any other control character in account_name and any control character in external_id. Approval would reject these anyway, so they now fail at submit time.
renderTemplate (every template: *.sh, *.tfvars, *.json params)	Fails closed. If any string field of the render data contains a control character other than tab, it returns an error, so the request gets a 500 and no script or bundle. A tab is allowed because it cannot end a line, and the session email may legitimately contain one (a #992 test case). The check walks the struct by reflection, so a field added later is covered automatically. The unguarded body moved to executeTemplate.

Template/field matrix

The sandbox oracle finds these line-break injections when the guard is removed (executeTemplate, LF payload, shell escaping applied). renderTemplate refuses all of them:

Template	Fields that break out of a comment
aws-cfn-deploy.sh	AccountName, AccountExternalID, Source
aws-cross-account-cfn-deploy.sh	AccountName, AccountExternalID
aws-cross-account-cli.sh	AccountName, AccountExternalID, AccountSlug
aws-wif-cli.sh	AccountName, AccountExternalID, AccountSlug, Source
azure-wif-cli.sh	AccountName, AccountSlug, SubscriptionID, CUDlyAPIURL
gcp-sa-impersonation-cli.sh	AccountName, AccountSlug, ProjectID
gcp-wif-cli.sh	AccountName, AccountSlug, ProjectID, CUDlyAPIURL
*.tfvars (all five)	AccountName on the header comment adds a top-level HCL attribute
azure-wif-deploy.sh, azure-wif-bicep-deploy.sh	none (no fields on comment lines), but the guard still applies

Compatibility with #955 / #992

#992 (fix/963-followups) is already merged and is part of this branch's base. git merge-tree --write-tree HEAD fork/fix/963-followups is clean (exit 0, tree d5df88aa), and so is the merge with origin/main. The tab exemption keeps #992's "a\tb"@example.com case working, and TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail passes.

Verification

All tests ran with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null, on macOS.

New tests:

• handler_federation_controlchar_test.go
  • TestRenderTemplate_RefusesControlCharsInEveryField: every embedded template × every string field × LF, CRLF, CR, VT, FF, NUL, ESC, DEL, NEL, with the handler's escaping applied. Each render is refused, the error names the field, and there is no output.
  • TestRenderTemplate_UnguardedInjectionIsCaughtByOracle: shows the oracle is not vacuous. Without the guard, the marker runs and bash -n still accepts the script. The issue's fields must be detected, and each detected combination must be refused by renderTemplate. Sandbox: empty temp working directory, HOME set to it, and PATH holding only a symlink to touch, so no cloud CLI or curl is reachable.
  • TestRenderTemplate_UnguardedTfvarsInjection: the HCL counterpart for the five tfvars templates.
  • TestRenderTemplate_BenignDataUnchanged: guarded output is byte-identical to unguarded output for every template, including a tab in the email, and every rendered script passes bash -n.
  • TestGetFederationIaC_FailsClosedOnControlChar: drives the real handler over all 27 valid target/source/format combinations. Benign requests succeed. A session email containing LF gets a non-4xx error and no content.
• handler_accounts_controlchar_test.go: createAccount and updateAccount through the real handlers (LF, CRLF, CR, TAB, NUL, ESC, DEL, NEL in name and in external_id) each return 400, and the store is never called (AssertNotCalled on a plain local MockConfigStore). Printable and non-ASCII names are still accepted unchanged. submitRegistration through the handler rejects these too, and the CR/LF strip still works.

Fail-before / pass-after:

• With the validation files reverted to main, the account, update and registration tests fail.
• With the render guard disabled (main behavior), the four render and handler tests fail.

Mutation probes, all killed:

• guard only rejects LF: the every-field test fails for CR, VT, FF, NUL, ESC, DEL and NEL (256 cases each)
• guard also rejects tab: the fix(iac): keep '}' and JSON metacharacters in the Bicep/ARM contact email intact (#963 follow-ups) #992 email test and the benign test fail
• guard skips the last struct field: two tests fail
• account check limited to C0: DEL and NEL tests fail
• account check covers only name: the external_id tests fail

Benign output vs main: all 90 target/source/format/domain combinations went through the handler on main and on this branch, and the zip entries were extracted and compared. Everything is identical except the per-request random external-ID UUID in cf-params.json. The same 36 combinations error on both (combinations the server doesn't offer).

CI-equivalent checks:

• go build ./... and go vet ./...: clean.
• go test ./internal/api/... ./internal/iacfiles/... ./scripts/... ./internal/mocks/... -count=1: passes except TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt, which also fails on main. That test fails because POST /api/purchases/approve/*/details from fix(approval): let email-link approvers load purchase details without view:purchases #990 is unclassified. It is unrelated to this change and this PR doesn't touch it.
• golangci-lint v2.10.1 --allow-parallel-runners: 0 issues.
• gocyclo -over 10 on the changed non-test files: none over 10 (max is validateRegistrationRequest at 9).
• shellcheck -S warning: clean on all 33 rendered scripts.
• git-secrets: scanned a temporary copy of the changed files with an isolated HOME, --register-aws and setup-git-secrets.sh, and it came back clean. scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.

Out of scope

• Other CloudAccountRequest fields such as description (may legitimately be multi-line) and the provider-specific IDs are not used on IaC comment lines.
• Existing stored accounts that already contain control characters are not migrated. They can't reach the IaC today, and the render guard would refuse them if they ever did.
• The CSRF guard test failure above, which already fails on main.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 6 billable files and costs up to $1.50.}

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

• Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: e0d7ec8f-3136-4504-8eb0-f9e3a20a234e

📥 Commits

Reviewing files that changed from the base of the PR and between 173474e and b202bee.

📒 Files selected for processing: 6

• internal/api/handler_accounts.go
• internal/api/handler_accounts_controlchar_test.go
• internal/api/handler_federation.go
• internal/api/handler_federation_controlchar_test.go
• internal/api/handler_registrations.go
• internal/api/validation.go

• 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 96f1aaf

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

Blocking findings: none for the #970 scope.

Every rendered template variable was traced (federationIaCData, handler_federation.go:26-56):

• User-controlled values. AccountName and AccountExternalID are validated by validateNoControlChars on every write path:
  • account create (handler_accounts.go:241, :316) and update (:565);
  • registration submit (handler_registrations.go:67) and approve (:262).
  • The check uses unicode.IsControl, so it covers C0, C1 and NEL.
• Write paths:
  • The CreateCloudAccount callers are accounts :252/:326, registrations :292 and org discovery :1612. UpdateCloudAccount is called only at :581.
  • Registration UPDATEs touch only workflow columns.
• Other values:
  • AccountSlug is a constant, and Source is allowlisted.
  • The STS, server-config and derived values (OIDC, subscription, tenant, project, SA email, API URL) are not user input.
  • ContactEmail comes from the session.
• Render guard. renderTemplate (:108) runs checkRenderDataSingleLine (:146) by reflection over every string field. It refuses any control character except tab with an error rather than stripping it, and every template and format goes through it. The federation endpoint's query params (target, source, format) are allowlisted and are not rendered.
• Existing rows. A PUT validates only the request body, so an admin can overwrite a poisoned name. Delete does no field validation.

Non-blocking notes:

1. persistDiscoveredMembers (handler_accounts.go:1612) stores AWS Organizations account names without the check. The render guard still fails closed, and the org admin controls those names. Hardening is optional.
2. Tab is rejected in name and external_id but allowed by the render check. Unicode letters are unaffected.
3. U+2028 and U+2029 are not rejected, since they are categories Zl/Zp, not control characters. Bash and HCL do not treat them as newlines, so they are not exploitable.

What was attacked and how:

• go vet ./internal/api/... is clean.
• go test -race -count=1 ./internal/api/... passes 3576, with 1 failure: TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt. That was the red-main failure fixed by test(api): classify the approval-details route in the AuthPublic CSRF guard (fixes red main) #1003 (merged as 0a5f790). It also fails with this PR's non-test files reverted, so it is unrelated. This PR needs a rebase onto main to pick up test(api): classify the approval-details route in the AuthPublic CSRF guard (fixes red main) #1003.
• Fail-before: with the non-test files reverted, TestCreateAccount_RejectsControlChars fails on LF, CRLF and NEL.
• Mutation probes, all caught:
  • narrowing the rune set to \n only;
  • dropping the render check;
  • dropping the registration submit check;
  • dropping the account check.

Gate:

• Labels are mirrored from sec(api): newline in account fields can break out of comment lines in federation deploy scripts #970.
• Needs a rebase onto main, the author marking the draft ready, green CI at the new head, and a CodeRabbit pass.
• After a pure rebase, an empty range-diff keeps this verdict.

[IAmJSD]

I merged origin/main into this branch at b202bee so CI picks up #1003. The red Unit and Integration jobs were only TestAuthPublicRoutes_EveryRouteIsCSRFGuardedOrExempt, which #1003 fixes on main. I merged rather than rebased, so there was no force-push. The merge was automatic and this PR's own commits are unchanged. Locally at b202bee, with AWS config set to /dev/null, go build ./... and go test ./internal/api/ pass.

🤖 Generated with Claude Code

[cristim]

Head moved to b202bee: a merge of origin/main (173474e, picks up #1003) into the reviewed 96f1aaf. The merge commit's tree is identical to git merge-tree --write-tree 96f1aaf4 173474e1 (551a73a4), so there are no manual conflict edits, and the PR diff against main is unchanged (6 files, +540/-3). The CLEAN verdict carries to b202bee. Still needs the draft marked ready, green CI and CodeRabbit. (claude-cc-reviewer)

[cristim]

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

• Independent review (claude-cc-reviewer, non-author): CLEAN at 96f1aaf (fix(api): reject control characters in account fields and fail closed in federation IaC render #1006 (comment)). Head b202bee is a merge of origin/main 173474e into the reviewed 96f1aaf; its tree equals a clean merge of the two (verified with git merge-tree), so no PR code changed after the verdict.
• CI on b202bee: CI Success and Run pre-commit hooks pass. The merge driver refreshes CI if main changed a directory this PR touches after CI ran.
• Local verification: as recorded in the independent review comment (fixture or mock evidence, labelled there). No real-account run.
• CR skipped where quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(api): newline in account fields can break out of comment lines in federation deploy scripts #970.

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

[cristim]

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

• Independent review (claude-cc-reviewer, non-author): CLEAN at 96f1aaf (fix(api): reject control characters in account fields and fail closed in federation IaC render #1006 (comment)). Head b202bee is a merge of origin/main 173474e into the reviewed 96f1aaf; its tree equals a clean merge of the two (verified with git merge-tree), so no PR code changed after the verdict.
• CI on b202bee: CI Success and Run pre-commit hooks pass. The merge driver refreshes CI if main changed a directory this PR touches after CI ran.- Local verification: as recorded in the independent review comment (fixture or mock evidence, labelled there). No real-account run.
• CR skipped where quota-limited; covered by the independent review plus local verification.
• Labels mirrored from sec(api): newline in account fields can break out of comment lines in federation deploy scripts #970.

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