From a0cb7d9d396bde0e140d4c9b023ba8e430cf2ce9 Mon Sep 17 00:00:00 2001
From: Jacopo Clark <37738506+clark-ja@users.noreply.github.com>
Date: Tue, 1 Sep 2026 22:30:37 +0200
Subject: [PATCH] fix(domain): make SanitizeString idempotent (#745)

NFC normalisation ran before control and format characters were stripped.
Removing a character can leave a base letter next to a combining mark the
earlier pass never saw as a pair, so a second call composes it:

    input    U+0041 U+0009 U+0300   ("A", tab, combining grave)
    once  -> U+0041 U+0300
    twice -> U+00C0

Found by TestPropertySanitizeStringIdempotent. Category Cf characters
behave the same way.

Strip first, then normalise. Normalisation still precedes truncation
because composing changes the rune count. One side effect: invalid UTF-8
is now dropped by the strip loop instead of surviving as U+FFFD, so an
identifier containing such bytes sanitises differently than before.

Signed-off-by: clark-ja <37738506+clark-ja@users.noreply.github.com>
---
 internal/domain/sanitize.go      | 20 ++++++++++++++----
 internal/domain/sanitize_test.go | 36 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/internal/domain/sanitize.go b/internal/domain/sanitize.go
index 9999a60..23ee874 100644
--- a/internal/domain/sanitize.go
+++ b/internal/domain/sanitize.go
@@ -47,16 +47,25 @@ var reservedUserIdentifiers = map[string]struct{}{
 	CtxSystemDBMigrator: {},
 }
 
-// SanitizeString normalizes to NFC, trims leading and trailing whitespace, strips Unicode
-// control and format characters, drops invalid UTF-8 bytes, and truncates the result to
+// SanitizeString trims leading and trailing whitespace, strips Unicode control and format
+// characters, drops invalid UTF-8 bytes, normalizes to NFC, and truncates the result to
 // maxLen runes. If maxLen <= 0, returns "".
+//
+// The order matters and is load-bearing for idempotency: see the comments in the body.
+// SanitizeString(SanitizeString(s, n), n) == SanitizeString(s, n) for all s and n.
 func SanitizeString(s string, maxLen int) string {
 	if maxLen <= 0 {
 		return ""
 	}
 
-	s = norm.NFC.String(strings.TrimSpace(s))
+	s = strings.TrimSpace(s)
 
+	// Strip control/format characters and invalid UTF-8 *before* normalizing.
+	// Normalizing first is not idempotent: removing a character can leave a
+	// base letter next to a combining mark that the earlier normalization never
+	// saw as a pair. For example "A\t̀" is already NFC (the tab keeps the
+	// letter and the combining grave apart), but stripping the tab yields
+	// "À", which a second call would compose to "À".
 	var b strings.Builder
 	b.Grow(len(s))
 	for len(s) > 0 {
@@ -69,7 +78,10 @@ func SanitizeString(s string, maxLen int) string {
 			b.WriteRune(r)
 		}
 	}
-	s = b.String()
+
+	// Normalize before truncating, not after: composition can change the rune
+	// count, so normalizing afterwards could push the result back over maxLen.
+	s = norm.NFC.String(b.String())
 
 	if utf8.RuneCountInString(s) > maxLen {
 		runes := []rune(s)
diff --git a/internal/domain/sanitize_test.go b/internal/domain/sanitize_test.go
index d3e5b6a..dfd0418 100644
--- a/internal/domain/sanitize_test.go
+++ b/internal/domain/sanitize_test.go
@@ -501,3 +501,39 @@ func TestPropertySanitizeIdentifierRejectsReservedValues(t *testing.T) {
 		}
 	})
 }
+
+// ---------------------------------------------------------------------------
+// Regression: normalization must run after control characters are stripped
+// ---------------------------------------------------------------------------
+
+// Found by TestPropertySanitizeStringIdempotent. Normalizing before stripping
+// control characters is not idempotent: "A\t̀" is already NFC because the
+// tab separates the letter from the combining grave, but removing the tab
+// leaves "À", which a second call composes to "À".
+func TestSanitizeStringNormalizesAfterStripping(t *testing.T) {
+	const input = "A\t\u0300" // "A", tab, combining grave accent
+
+	got := SanitizeString(input, 2)
+
+	if got != "À" {
+		t.Errorf("expected the combining mark to be composed after the tab is stripped, got %q (% x)",
+			got, []rune(got))
+	}
+	if again := SanitizeString(got, 2); again != got {
+		t.Errorf("not idempotent: once=%q twice=%q", got, again)
+	}
+}
+
+// The same hazard exists for format characters (category Cf), not just controls.
+func TestSanitizeStringStripsFormatCharsBeforeNormalizing(t *testing.T) {
+	const input = "A\u00ad\u0300" // "A", soft hyphen (Cf), combining grave accent
+
+	got := SanitizeString(input, 2)
+
+	if again := SanitizeString(got, 2); again != got {
+		t.Errorf("not idempotent: once=%q twice=%q", got, again)
+	}
+	if got != "À" {
+		t.Errorf("expected %q, got %q", "À", got)
+	}
+}