From df915176bacac843179ecca80f119751cb3dd0dd Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 16:00:54 +0100 Subject: [PATCH] fix(iac): keep "}" and JSON metacharacters in the Bicep/ARM contact email intact (#958) deploy-azure.sh pre-filled the contact email as the default of ${CUDLY_CONTACT_EMAIL:-...}. A "}" in the email (net/mail accepts it) closed the expansion early and reordered the value, e.g. a}b@example.com became ab}@example.com. Assign the env value first and fall back to the pre-filled email on its own line; the override semantics are unchanged (a set, non-empty CUDLY_CONTACT_EMAIL wins). The self-registration heredoc also put CONTACT_EMAIL and ACCOUNT_NAME into the JSON raw, so a '"', '\' or tab (all accepted by net/mail in a quoted local part) produced a malformed payload. Add a json_escape helper (printf + sed, portable to macOS bash 3.2 and Linux, no jq) and use the escaped values in the heredoc. The other heredoc values are the subscription, tenant and client GUIDs. Tests run the registration block in the sandboxed bash with az stubbed, for hostile emails with "}", "$", backticks, quotes, backslashes and a tab, and check that CONTACT_EMAIL round-trips, that the payload parses as JSON with the exact email, and that nothing executes. They also pin the env-override cases and JSON-escaping of CUDLY_ACCOUNT_NAME. Co-Authored-By: Claude Opus 5.5 --- .../handler_federation_azure_escape_test.go | 127 ++++++++++++++---- internal/api/handler_federation_test.go | 3 +- .../templates/azure-wif-bicep-deploy.sh.tmpl | 18 ++- 3 files changed, 120 insertions(+), 28 deletions(-) diff --git a/internal/api/handler_federation_azure_escape_test.go b/internal/api/handler_federation_azure_escape_test.go index 0d66efb33..31feb5e07 100644 --- a/internal/api/handler_federation_azure_escape_test.go +++ b/internal/api/handler_federation_azure_escape_test.go @@ -5,11 +5,13 @@ import ( "bytes" "context" "encoding/base64" + "encoding/json" "net/mail" "os" "os/exec" "path/filepath" "regexp" + "strings" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/iacfiles" @@ -45,32 +47,77 @@ func azureZipEntry(t *testing.T, zipBytes []byte, name string) string { return "" } -// evalShellWord evaluates one line of shell in a fresh bash, in an empty -// temporary directory and with an empty environment, and returns what it -// printed plus the directory, so callers can check that no command -// substitution created a file there. Only printf and variable assignments run. -func evalShellWord(t *testing.T, bashPath, script string) (out, dir string) { +// evalShellWord evaluates a snippet of shell in a fresh bash, in an empty +// temporary directory and with an environment of PATH plus env, and returns +// what it printed plus the directory, so callers can check that no command +// substitution created a file there. +func evalShellWord(t *testing.T, bashPath, script string, env ...string) (out, dir string) { t.Helper() dir = t.TempDir() cmd := exec.Command(bashPath, "--noprofile", "--norc", "-c", script) cmd.Dir = dir - cmd.Env = []string{"PATH=" + os.Getenv("PATH")} + cmd.Env = append([]string{"PATH=" + os.Getenv("PATH")}, env...) got, err := cmd.Output() require.NoError(t, err, "bash must evaluate %q", script) return string(got), dir } var ( - contactEmailLineRe = regexp.MustCompile(`(?m)^CONTACT_EMAIL=.*$`) - registerURLWordRe = regexp.MustCompile(`(?m)-X POST ("[^\n]*/api/register") \\$`) + // registrationBlockRe matches deploy-azure.sh from the CONTACT_EMAIL + // assignment through the end of the PAYLOAD heredoc: the part that + // turns the pre-filled email into the registration JSON, stopping + // before the curl call. + registrationBlockRe = regexp.MustCompile(`(?ms)^CONTACT_EMAIL=.*?^JSON$`) + registerURLWordRe = regexp.MustCompile(`(?m)-X POST ("[^\n]*/api/register") \\$`) ) +// registration is what evalRegistration observed. +type registration struct { + contactEmail string + payload map[string]string + dir string +} + +// evalRegistration runs the registration block of deploy-azure.sh in the +// evalShellWord sandbox, with az stubbed by a shell function (no Azure +// CLI, no network; curl is past the end of the block). It returns the +// resulting CONTACT_EMAIL and the PAYLOAD heredoc parsed as JSON. +func evalRegistration(t *testing.T, bashPath, script string, env ...string) registration { + t.Helper() + block := registrationBlockRe.FindString(script) + require.NotEmpty(t, block, "deploy-azure.sh must build the registration payload") + stub := "set -euo pipefail\naz() { printf '%s\\n' 00000000-0000-0000-0000-0000000000aa; }\n" + out, dir := evalShellWord(t, bashPath, + stub+block+"\nprintf '%s\\0%s' \"$CONTACT_EMAIL\" \"$PAYLOAD\"", env...) + email, payload, ok := strings.Cut(out, "\x00") + require.True(t, ok, "sandbox output must hold CONTACT_EMAIL and PAYLOAD: %q", out) + var fields map[string]string + require.NoError(t, json.Unmarshal([]byte(payload), &fields), "PAYLOAD must be valid JSON: %s", payload) + return registration{contactEmail: email, payload: fields, dir: dir} +} + +// hostileContactEmails all pass mail.ParseAddress. They cover command +// substitution (#958), a "}" that would close a ${VAR:-default} early, and +// the characters JSON needs escaped ("\", '"', tab). +var hostileContactEmails = []string{ + "\"a`touch pwned`b\"@example.com", + `"a$(touch pwned)b"@example.com`, + "a`id`b@example.com", + `"a\"b\\c$HOME"@example.com`, + `a}b@example.com`, + `"a}b"@example.com`, + "\"a}$(touch pwned)`touch pwned`}b\"@example.com", + `"{a}${b}"@example.com`, + "\"a\tb\"@example.com", +} + // TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail is the // regression test for #958. deploy-azure.sh in the Bicep and ARM bundles // embeds the session user's email inside a double-quoted bash string. The -// email only has to pass mail.ParseAddress, which accepts backticks and -// "$(...)", so the script must carry it shell-escaped: running the -// CONTACT_EMAIL assignment must yield the email verbatim and run nothing. +// email only has to pass mail.ParseAddress, which accepts backticks, +// "$(...)", "}", quotes, backslashes and tabs. Running the registration +// block must leave CONTACT_EMAIL equal to the email verbatim, put it into +// valid JSON unchanged, and run nothing. func TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail(t *testing.T) { bashPath, err := exec.LookPath("bash") if err != nil { @@ -78,13 +125,7 @@ func TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail(t *testing.T) { } t.Setenv("CUDLY_SOURCE_CLOUD", "gcp") - emails := []string{ - "\"a`touch pwned`b\"@example.com", - `"a$(touch pwned)b"@example.com`, - "a`id`b@example.com", - `"a\"b\\c$HOME"@example.com`, - } - for _, email := range emails { + for _, email := range hostileContactEmails { _, err := mail.ParseAddress(email) require.NoError(t, err, "fixture %q must be an address the auth service accepts", email) @@ -112,17 +153,55 @@ func TestGetFederationIaC_AzureDeployScriptEscapesSessionEmail(t *testing.T) { out, err := syntax.CombinedOutput() require.NoError(t, err, "deploy-azure.sh must parse: %s", out) - assign := contactEmailLineRe.FindString(script) - require.NotEmpty(t, assign, "deploy-azure.sh must assign CONTACT_EMAIL") - got, dir := evalShellWord(t, bashPath, assign+`; printf '%s' "$CONTACT_EMAIL"`) - assert.Equal(t, email, got, "CONTACT_EMAIL must evaluate to the email verbatim") - assert.NoFileExists(t, filepath.Join(dir, "pwned"), + got := evalRegistration(t, bashPath, script) + assert.Equal(t, email, got.contactEmail, "CONTACT_EMAIL must evaluate to the email verbatim") + assert.Equal(t, email, got.payload["contact_email"], "the payload must carry the email verbatim") + assert.NoFileExists(t, filepath.Join(got.dir, "pwned"), "a command substitution in the email must not run") }) } } } +// TestBuildAzureTemplateZip_RegistrationEnvOverrides pins the env-override +// semantics of the restructured CONTACT_EMAIL assignment (a set, non-empty +// CUDLY_CONTACT_EMAIL wins; unset or empty falls back to the pre-filled +// email, as ${VAR:-default} did) and that an operator-supplied +// CUDLY_ACCOUNT_NAME is JSON-escaped too. +func TestBuildAzureTemplateZip_RegistrationEnvOverrides(t *testing.T) { + bashPath, err := exec.LookPath("bash") + if err != nil { + t.Skip("bash not available") + } + const prefilled = `"pre}filled"@example.com` + data := buildGenericIaCData("azure", "gcp", "https://cudly.example.com") + data.ContactEmail = prefilled + zipBytes, _, err := buildAzureTemplateZip("bicep", data, "azure", "target") + require.NoError(t, err) + script := azureDeployScriptFromZip(t, zipBytes) + + cases := []struct { + name, wantEmail string + env []string + }{ + {"unset", prefilled, nil}, + {"empty", prefilled, []string{"CUDLY_CONTACT_EMAIL="}}, + {"set", `"o}p\s"@example.com`, []string{`CUDLY_CONTACT_EMAIL="o}p\s"@example.com`}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := evalRegistration(t, bashPath, script, tc.env...) + assert.Equal(t, tc.wantEmail, got.contactEmail) + assert.Equal(t, tc.wantEmail, got.payload["contact_email"]) + assert.Equal(t, "Azure 00000000-0000-0000-0000-0000000000aa", got.payload["account_name"]) + }) + } + + const name = "Ops \"prod\" \\ team\tB" + got := evalRegistration(t, bashPath, script, "CUDLY_ACCOUNT_NAME="+name) + assert.Equal(t, name, got.payload["account_name"], "account_name must be carried verbatim as JSON") +} + // TestBuildAzureTemplateZip_DeployScriptEscapesEveryField covers the other // interpolation in deploy-azure.sh, the CUDly API URL in the registration // curl call, with the builder called directly on hostile data. @@ -166,7 +245,7 @@ func TestBuildAzureTemplateZip_BenignDataRendersUnchanged(t *testing.T) { data.ContactEmail = "admin@example.com" want, err := renderTemplate("templates/azure-wif-bicep-deploy.sh.tmpl", data) require.NoError(t, err) - require.Contains(t, want, `CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-admin@example.com}"`) + require.Contains(t, want, `CONTACT_EMAIL="admin@example.com"`) for _, format := range []string{"bicep", "arm"} { t.Run(format, func(t *testing.T) { diff --git a/internal/api/handler_federation_test.go b/internal/api/handler_federation_test.go index 07240ae6d..3117d7437 100644 --- a/internal/api/handler_federation_test.go +++ b/internal/api/handler_federation_test.go @@ -1002,7 +1002,8 @@ func TestBuildAzureTemplateZip_IncludesRenderedDeployScript(t *testing.T) { "deploy script must handle success codes") assert.Contains(t, script, "409", "deploy script must handle 409 already-pending") - assert.Contains(t, script, `CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-`, + assert.Contains(t, script, `CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-}"`+"\n"+ + `[ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="admin@example.com"`, "deploy script must pre-fill CONTACT_EMAIL with env-override support") }) } diff --git a/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl b/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl index fc2bcfd61..5c9016524 100644 --- a/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl +++ b/internal/iacfiles/templates/azure-wif-bicep-deploy.sh.tmpl @@ -61,18 +61,30 @@ echo "Set azure_auth_mode=workload_identity_federation in CUDly." # --------------------------------------------------------------------------- echo "" echo "Registering account with CUDly..." -CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-{{.ContactEmail}}}" +# The pre-filled email is assigned on its own line, not as a ${VAR:-default}, +# so a "}" in it cannot close the expansion early (#958). +CONTACT_EMAIL="${CUDLY_CONTACT_EMAIL:-}" +[ -n "$CONTACT_EMAIL" ] || CONTACT_EMAIL="{{.ContactEmail}}" SUBSCRIPTION_ID="${AZURE_SUBSCRIPTION_ID:-$(az account show --query id -o tsv)}" TENANT_ID=$(az account show --query tenantId -o tsv) ACCOUNT_NAME="${CUDLY_ACCOUNT_NAME:-Azure ${SUBSCRIPTION_ID}}" # AZURE_CLIENT_ID is written by the preceding azure-wif-cli.sh step. CLIENT_ID="${AZURE_CLIENT_ID:-}" +# json_escape prints $1 escaped for use inside a JSON string: \ and " are +# backslash-escaped and a tab becomes \t. CR and LF are not handled: the +# email cannot hold them (net/mail rejects them), and CUDLY_ACCOUNT_NAME is +# set by the operator running this script. +json_escape() { + printf '%s' "$1" | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g' -e "s/$(printf '\t')/\\\\t/g" +} +CONTACT_EMAIL_JSON=$(json_escape "$CONTACT_EMAIL") +ACCOUNT_NAME_JSON=$(json_escape "$ACCOUNT_NAME") read -r -d '' PAYLOAD <