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

internal/api/handler_accounts.go

@@ -361,6 +361,12 @@ func validateCloudAccountRequest(req CloudAccountRequest) error {
 		return NewClientError(400, "external_id is required")
 	}
 
+	if err := validateNoControlChars(
+		namedField{"name", req.Name}, namedField{"external_id", req.ExternalID},
+	); err != nil {
+		return err
+	}
+
 	if err := validateEmailFormat(req.ContactEmail); err != nil {
 		return NewClientError(400, "invalid contact_email format")
 	}

internal/api/handler_accounts_controlchar_test.go

@@ -0,0 +1,157 @@
+package api
+
+import (
+	"context"
+	"encoding/json"
+	"testing"
+
+	"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// accountControlChars are the control characters the account write paths
+// must reject in single-line identifiers (#970). A tab is included: unlike
+// the render guard, which lets one through for the session email, a name or
+// external ID has no use for it.
+var accountControlChars = map[string]string{
+	"LF":   "\n",
+	"CRLF": "\r\n",
+	"CR":   "\r",
+	"TAB":  "\t",
+	"NUL":  "\x00",
+	"ESC":  "\x1b",
+	"DEL":  "\x7f",
+	"NEL":  "\u0085",
+}
+
+// accountBody returns a create/update body for an Azure account (no AWS
+// external-ID charset rule applies), with name and external_id as given.
+func accountBody(t *testing.T, name, externalID string) string {
+	t.Helper()
+	b, err := json.Marshal(map[string]string{"name": name, "provider": "azure", "external_id": externalID})
+	require.NoError(t, err)
+	return string(b)
+}
+
+func requireClientError400(t *testing.T, err error, field string) {
+	t.Helper()
+	require.Error(t, err)
+	ce, ok := IsClientError(err)
+	require.True(t, ok, "expected a client error, got %v", err)
+	assert.Equal(t, 400, ce.code)
+	assert.Contains(t, ce.message, field+" must not contain control characters")
+}
+
+// TestCreateAccount_RejectsControlChars drives POST /api/accounts with a
+// control character in name or external_id: the request is refused with a
+// 400 naming the field and nothing is stored.
+func TestCreateAccount_RejectsControlChars(t *testing.T) {
+	ctx := context.Background()
+	for label, c := range accountControlChars {
+		for _, field := range []string{"name", "external_id"} {
+			t.Run(field+"/"+label, func(t *testing.T) {
+				mockAuth := new(MockAuthService)
+				setupAdminAuth(ctx, mockAuth)
+				store := new(MockConfigStore)
+				handler := &Handler{auth: mockAuth, config: store}
+
+				name, ext := "Acme", "acme-prod"
+				if field == "name" {
+					name = "Acme" + c + "touch PWNED"
+				} else {
+					ext = "acme" + c + "touch PWNED"
+				}
+				result, err := handler.createAccount(ctx, adminRequest(accountBody(t, name, ext)))
+				assert.Nil(t, result)
+				requireClientError400(t, err, field)
+				store.AssertNotCalled(t, "CreateCloudAccount")
+			})
+		}
+	}
+}
+
+// TestUpdateAccount_RejectsControlChars is the PUT /api/accounts/:id
+// counterpart: the update is refused before the account is loaded or saved.
+func TestUpdateAccount_RejectsControlChars(t *testing.T) {
+	ctx := context.Background()
+	const id = "11111111-1111-1111-1111-111111111111"
+	for label, c := range accountControlChars {
+		for _, field := range []string{"name", "external_id"} {
+			t.Run(field+"/"+label, func(t *testing.T) {
+				mockAuth := new(MockAuthService)
+				setupAdminAuth(ctx, mockAuth)
+				store := new(MockConfigStore)
+				handler := &Handler{auth: mockAuth, config: store}
+
+				name, ext := "Acme", "acme-prod"
+				if field == "name" {
+					name = "Acme" + c + "touch PWNED"
+				} else {
+					ext = "acme" + c + "touch PWNED"
+				}
+				req := adminRequest(accountBody(t, name, ext))
+				req.RequestContext.HTTP.Method = "PUT"
+				req.RequestContext.HTTP.Path = "/api/accounts/" + id
+				result, err := handler.updateAccount(ctx, req, id)
+				assert.Nil(t, result)
+				requireClientError400(t, err, field)
+				store.AssertNotCalled(t, "UpdateCloudAccount")
+			})
+		}
+	}
+}
+
+// TestCreateAccount_AcceptsPrintableNames guards against over-rejection:
+// spaces, punctuation and non-ASCII letters stay valid and are stored as sent.
+func TestCreateAccount_AcceptsPrintableNames(t *testing.T) {
+	ctx := context.Background()
+	for _, name := range []string{"Acme Production", "Acme (EU) – Zürich #2", "日本 本番"} {
+		mockAuth := new(MockAuthService)
+		setupAdminAuth(ctx, mockAuth)
+		handler := &Handler{auth: mockAuth, config: new(MockConfigStore)}
+		result, err := handler.createAccount(ctx, adminRequest(accountBody(t, name, "acme-prod")))
+		require.NoError(t, err, name)
+		assert.Equal(t, name, result.(*config.CloudAccount).Name)
+	}
+}
+
+// TestSubmitRegistration_RejectsControlChars drives the public POST
+// /api/register handler. CR and LF in account_name are still stripped (#544),
+// but any other control character in account_name, and any control character
+// in external_id, is refused, since approval would reject it anyway.
+func TestSubmitRegistration_RejectsControlChars(t *testing.T) {
+	ctx := context.Background()
+	submit := func(t *testing.T, name, ext string) error {
+		t.Helper()
+		store := new(MockConfigStore)
+		h := &Handler{config: store}
+		b, err := json.Marshal(map[string]string{
+			"provider": "azure", "external_id": ext, "account_name": name, "contact_email": "ops@example.com",
+		})
+		require.NoError(t, err)
+		_, err = h.submitRegistration(ctx, adminRequest(string(b)))
+		if err != nil {
+			store.AssertNotCalled(t, "CreateAccountRegistration")
+		}
+		return err
+	}
+	for label, c := range accountControlChars {
+		t.Run("external_id/"+label, func(t *testing.T) {
+			requireClientError400(t, submit(t, "Acme", "acme"+c+"touch PWNED"), "external_id")
+		})
+		if label == "LF" || label == "CRLF" || label == "CR" {
+			continue // stripped from account_name, covered below
+		}
+		t.Run("account_name/"+label, func(t *testing.T) {
+			requireClientError400(t, submit(t, "Acme"+c+"touch PWNED", "acme-prod"), "account_name")
+		})
+	}
+	t.Run("account_name CR/LF still stripped", func(t *testing.T) {
+		req := RegistrationRequest{
+			Provider: "azure", ExternalID: "acme-prod", AccountName: "Acme\r\ntouch PWNED", ContactEmail: "ops@example.com",
+		}
+		require.NoError(t, validateRegistrationRequest(&req))
+		assert.Equal(t, "Acmetouch PWNED", req.AccountName)
+	})
+}

internal/api/handler_federation.go

@@ -12,6 +12,8 @@ import (
 	"regexp"
 	"strings"
 	"text/template"
+	"unicode"
+	"unicode/utf8"
 
 	"github.com/google/uuid"
 
@@ -101,7 +103,18 @@ func gcpOIDCIssuerURI(source, tenantID string) string {
 }
 
 // renderTemplate renders a named template from the embedded iacfiles.Templates FS.
+// It refuses data holding a line break or other control character; see
+// checkRenderDataSingleLine.
 func renderTemplate(tmplPath string, data federationIaCData) (string, error) {
+	if err := checkRenderDataSingleLine(data); err != nil {
+		return "", fmt.Errorf("render template %s: %w", tmplPath, err)
+	}
+	return executeTemplate(tmplPath, data)
+}
+
+// executeTemplate parses and executes a template with no checks on data.
+// Only renderTemplate calls it outside tests.
+func executeTemplate(tmplPath string, data federationIaCData) (string, error) {
 	tmplBytes, err := iacfiles.Templates.ReadFile(tmplPath)
 	if err != nil {
 		return "", fmt.Errorf("read template %s: %w", tmplPath, err)
@@ -117,6 +130,41 @@ func renderTemplate(tmplPath string, data federationIaCData) (string, error) {
 	return buf.String(), nil
 }
 
+// checkRenderDataSingleLine fails closed when any string field of data holds
+// a control character other than a horizontal tab. The shell and tfvars
+// templates print several fields on "#" comment lines, and shellEscape and
+// hclEscape escape quoting metacharacters but not line breaks, so a CR or LF
+// there would end the comment and run the rest of the value as shell or HCL
+// (#970). The generic bundles fill no stored account fields today, and the
+// account write paths reject control characters, so this guard only trips on
+// data that bypassed them (a record stored before that validation, a future
+// caller that fills account fields, or a bad config value); an error is safer
+// than a script that runs something unintended.
+// A tab is allowed because it cannot end a line, and the session email can
+// legitimately hold one in a quoted local part. It walks the struct by
+// reflection so a new field cannot be missed.
+func checkRenderDataSingleLine(data federationIaCData) error {
+	v := reflect.ValueOf(data)
+	for i := 0; i < v.NumField(); i++ {
+		f := v.Field(i)
+		if f.Kind() != reflect.String {
+			continue
+		}
+		if at := strings.IndexFunc(f.String(), isRenderControlChar); at >= 0 {
+			r, _ := utf8.DecodeRuneInString(f.String()[at:])
+			return fmt.Errorf("field %s contains control character %U", v.Type().Field(i).Name, r)
+		}
+	}
+	return nil
+}
+
+// isRenderControlChar reports whether r is a control character that
+// checkRenderDataSingleLine refuses: every C0 and C1 control and DEL except
+// the horizontal tab.
+func isRenderControlChar(r rune) bool {
+	return r != '\t' && unicode.IsControl(r)
+}
+
 // getFederationIaC handles GET /api/federation/iac
 // Query params: target, source, format — all required.
 //
@@ -404,9 +452,8 @@ func shellEscape(s string) string {
 // hclEscape escapes a string for use inside a double-quoted HCL string. HCL
 // also starts an interpolation at "${" and a template directive at "%{", so
 // both are doubled into their literal "$${" and "%%{" forms (#952). Line
-// breaks are not handled: the one user-supplied value, the session email,
-// cannot hold one (mail.ParseAddress rejects CR and LF), and the rest come
-// from server configuration or are derived by the server.
+// breaks are not escaped: renderTemplate refuses any value holding one
+// (checkRenderDataSingleLine, #970).
 func hclEscape(s string) string {
 	return strings.NewReplacer(`\`, `\\`, `"`, `\"`, "${", "$${", "%{", "%%{").Replace(s)
 }