fix(iac): keep '}' and JSON metacharacters in the Bicep/ARM contact email intact (#963 follow-ups)

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,46 +47,85 @@ 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 {
 		t.Skip("bash not available")
 	}
 	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) {

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")
 		})
 	}

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 <<JSON || true
 {
   "provider": "azure",
   "external_id": "${SUBSCRIPTION_ID}",
-  "account_name": "${ACCOUNT_NAME}",
-  "contact_email": "${CONTACT_EMAIL}",
+  "account_name": "${ACCOUNT_NAME_JSON}",
+  "contact_email": "${CONTACT_EMAIL_JSON}",
   "azure_auth_mode": "workload_identity_federation",
   "azure_subscription_id": "${SUBSCRIPTION_ID}",
   "azure_tenant_id": "${TENANT_ID}",