diff --git a/pkg/attachment/attachment.go b/pkg/attachment/attachment.go
index a6f3116d1..a206e0a39 100644
--- a/pkg/attachment/attachment.go
+++ b/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: `>` 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
+// `>`: 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 (``
+// 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.
diff --git a/pkg/attachment/defuse_convergence_test.go b/pkg/attachment/defuse_convergence_test.go
new file mode 100644
index 000000000..ae9a744cc
--- /dev/null
+++ b/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": ``,
+ "plain open": ``,
+ "self closing": ``,
+ "trailing attrs": ``,
+ "trailing bang": ``,
+ "whitespace padded": ``,
+ "upper case": ``,
+ "mixed case": ``,
+ "prefix extending": ``,
+ "residue shape": `>`,
+ "residue deeper": `>>`,
+ "deeply nested": strings.Repeat(``, 20),
+ "split brackets": strings.Repeat(``, 12),
+ "many singles": strings.Repeat(` `, 50),
+ "placeholder first": delimiterPlaceholder + ``,
+ }
+
+ 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
+}