docs(attachment): correct the defuse loop's termination argument

pkg/attachment/attachment.go

@@ -115,9 +115,12 @@ var envelopeTagRe = regexp.MustCompile(`(?i)<[\s/]*(document-[a-z0-9-]+)\b[^>]*>
 // a placeholder in someone else's markup, while the cost of missing it is a
 // break-out — so the check errs toward defusing.
 //
-// Replacement repeats until the output is stable, because one pass can leave a
-// delimiter-shaped residue behind: `</TAG</TAG>>` collapses to `[…removed]>` only
-// after the second pass.
+// Replacement repeats until the output is stable. In practice one pass is
+// enough for every case covered by the tests, including the residue shape
+// `</TAG</TAG>>`: the pattern runs to the FIRST `>`, so that whole prefix is a
+// single match and collapses to `[…removed]>` in one go. The loop stays because
+// stability is the property that matters, and proving "one pass always suffices"
+// for every possible body is harder than simply iterating to a fixed point.
 func defuseDelimiters(body, tag string) string {
 	if body == "" {
 		return body
@@ -140,10 +143,20 @@ func defuseDelimiters(body, tag string) string {
 	return body
 }
 
-// maxDefusePasses bounds the replace-until-stable loop. Each pass strictly
-// shortens the body (a match is always longer than nothing and is replaced by a
-// constant), so this converges quickly; the bound only exists so a pathological
-// input cannot spin.
+// maxDefusePasses bounds the replace-until-stable loop.
+//
+// Termination does NOT come from the body shrinking -- it usually grows, because
+// delimiterPlaceholder is longer than the delimiters it replaces (`</document-x>`
+// is 13 bytes, the placeholder is 41). It comes from the placeholder containing
+// neither `<` nor `>`: it can never form part of a new match, so every pass
+// strictly reduces the number of delimiter-shaped tokens left in the body. That
+// count is a non-negative integer, so the loop reaches a fixed point.
+//
+// The bound is therefore belt-and-braces rather than the real guarantee, and it
+// is generous: every case in TestDefuseDelimitersConverges settles in one pass.
+// Note the loop returns whatever it has if the bound is ever hit, so if a future
+// change to envelopeTagRe or delimiterPlaceholder breaks the no-angle-brackets
+// property, that test is what will catch it.
 const maxDefusePasses = 8
 
 // slugify converts s to a lowercase, alphanumeric-and-hyphens-only string.

pkg/attachment/defuse_convergence_test.go

@@ -0,0 +1,112 @@
+package attachment
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestDelimiterPlaceholderCannotFormAMatch pins the property that actually makes
+// defuseDelimiters terminate.
+//
+// The loop does not converge by shrinking the body -- the placeholder is longer
+// than the delimiters it replaces, so the body usually grows. It converges
+// because the placeholder contains neither "<" nor ">", so it can never become
+// part of a new match, and every pass therefore strictly reduces the number of
+// delimiter-shaped tokens remaining.
+//
+// If someone gives delimiterPlaceholder angle brackets, that argument collapses
+// and the loop could hit maxDefusePasses with a live delimiter still in the body
+// -- which defuseDelimiters would then return as-is. This test is the tripwire.
+func TestDelimiterPlaceholderCannotFormAMatch(t *testing.T) {
+	if strings.ContainsAny(delimiterPlaceholder, "<>") {
+		t.Fatalf("delimiterPlaceholder must contain no angle brackets, or the "+
+			"convergence argument for maxDefusePasses no longer holds; got %q",
+			delimiterPlaceholder)
+	}
+	if envelopeTagRe.MatchString(delimiterPlaceholder) {
+		t.Fatalf("delimiterPlaceholder must not itself match envelopeTagRe; got %q",
+			delimiterPlaceholder)
+	}
+}
+
+// TestDefuseDelimitersConverges checks that adversarial bodies reach a fixed
+// point well inside maxDefusePasses, and that nothing delimiter-shaped for this
+// envelope's tag survives.
+//
+// Convergence is asserted separately from survivor-freedom on purpose: hitting
+// the pass bound is silent (defuseDelimiters returns the body it has), so a
+// regression there would otherwise only show up as a break-out much later.
+func TestDefuseDelimitersConverges(t *testing.T) {
+	const tag = "document-x"
+
+	cases := map[string]string{
+		"plain close":       `</document-x>`,
+		"plain open":        `<document-x>`,
+		"self closing":      `<document-x/>`,
+		"trailing attrs":    `</document-x foo="1">`,
+		"trailing bang":     `</document-x!>`,
+		"whitespace padded": `</   document-x   >`,
+		"upper case":        `</DOCUMENT-X>`,
+		"mixed case":        `</DoCuMeNt-X>`,
+		"prefix extending":  `</document-x-extra>`,
+		"residue shape":     `</document-x</document-x>>`,
+		"residue deeper":    `</document-x</document-x</document-x>>>`,
+		"deeply nested":     strings.Repeat(`</document-x`, 20) + strings.Repeat(`>`, 20),
+		"split brackets":    strings.Repeat(`<document-x`, 12) + strings.Repeat(`>`, 12),
+		"many singles":      strings.Repeat(`</document-x> `, 50),
+		"placeholder first": delimiterPlaceholder + `</document-x>`,
+	}
+
+	for name, body := range cases {
+		t.Run(name, func(t *testing.T) {
+			passes := passesToFixedPoint(body, tag)
+			if passes < 0 {
+				t.Fatalf("did not converge within maxDefusePasses (%d)", maxDefusePasses)
+			}
+			if passes > 1 {
+				// Not a failure -- the loop exists precisely to allow this --
+				// but the corrected comment claims one pass suffices for every
+				// covered case, so surface any case that stops being true.
+				t.Logf("converged in %d passes (comment claims 1 for covered cases)", passes)
+			}
+
+			out := defuseDelimiters(body, tag)
+			for _, m := range envelopeTagRe.FindAllStringSubmatch(out, -1) {
+				if len(m) >= 2 && strings.HasPrefix(strings.ToLower(m[1]), tag) {
+					t.Fatalf("live delimiter survived defusing: %q in %q", m[0], out)
+				}
+			}
+		})
+	}
+}
+
+// passesToFixedPoint mirrors the defuseDelimiters loop and reports how many
+// passes it takes to stabilise, or -1 if it never does within the bound.
+func passesToFixedPoint(body, tag string) int {
+	lowerTag := strings.ToLower(tag)
+	for i := range maxDefusePasses {
+		next := envelopeTagRe.ReplaceAllStringFunc(body, func(match string) string {
+			groups := envelopeTagRe.FindStringSubmatch(match)
+			if len(groups) < 2 || !strings.HasPrefix(strings.ToLower(groups[1]), lowerTag) {
+				return match
+			}
+			return delimiterPlaceholder
+		})
+		if next == body {
+			return i
+		}
+		body = next
+	}
+	// One more comparison: the bound may have been exactly enough.
+	next := envelopeTagRe.ReplaceAllStringFunc(body, func(match string) string {
+		groups := envelopeTagRe.FindStringSubmatch(match)
+		if len(groups) < 2 || !strings.HasPrefix(strings.ToLower(groups[1]), lowerTag) {
+			return match
+		}
+		return delimiterPlaceholder
+	})
+	if next == body {
+		return maxDefusePasses
+	}
+	return -1
+}