Re-run fence filter and preserve valid variation sequences

Address review feedback on the post-HTML-entity sanitization pass.

Entity decoding could still smuggle code-fence metadata past the
sanitizer. A first line such as "`​``steal secrets" is not a fence
in the raw input, so FilterCodeFenceMetadata left it alone; decoding the
entity and stripping the zero width space then produced a real fence with
its info string intact. Sanitize now re-runs the fence filter after the
input is fully normalized.

Filtering every variation selector also corrupted legitimate text: VS15
and VS16 select text or emoji presentation, so "✈️" was reduced to "✈",
and the Variation Selectors Supplement encodes registered CJK ideographic
variation sequences. Selectors are now filtered contextually. A selector
is kept when it can apply to the character it follows, and dropped when it
is orphaned, follows a removed or non-graphic character, or continues a
run of selectors. Supplement selectors additionally require a CJK
ideograph base, matching the Ideographic Variation Database. That keeps
the anti-smuggling property, since hidden payloads rely on selector runs,
without rewriting valid Unicode.

Also corrects a lowercase-hex test case that claimed uppercase digits.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: SamMorrowDrums

pkg/sanitize/sanitize.go

@@ -4,6 +4,7 @@ import (
 	"strings"
 	"sync"
 	"unicode"
+	"unicode/utf8"
 
 	"github.com/microcosm-cc/bluemonday"
 )
@@ -12,15 +13,17 @@ var policy *bluemonday.Policy
 var policyOnce sync.Once
 
 func Sanitize(input string) string {
-	// FilterInvisibleCharacters runs both before and after HTML processing.
-	// The first pass strips raw invisible characters so they don't interfere
-	// with code-fence parsing. HTML sanitization (FilterHTMLTags) decodes
-	// character entities (e.g. "​" or "​" become U+200B), which
-	// can introduce invisible or bidirectional characters that were not
-	// present as literal runes in the original input. The second pass
-	// filters the fully normalized output so entity-encoded characters
-	// cannot survive the policy.
-	return FilterInvisibleCharacters(FilterHTMLTags(FilterCodeFenceMetadata(FilterInvisibleCharacters(input))))
+	// The invisible-character and code-fence filters both run before and after
+	// HTML processing. The first pass strips raw invisible characters so they
+	// don't interfere with code-fence parsing. HTML sanitization
+	// (FilterHTMLTags) decodes character entities (e.g. "​" or
+	// "​" become U+200B), which can introduce invisible or
+	// bidirectional characters that were not present as literal runes in the
+	// original input. Those decoded characters can both survive on their own
+	// and splice previously inert text into a code fence, so the second pass
+	// re-applies both filters to the fully normalized output.
+	normalized := FilterHTMLTags(FilterCodeFenceMetadata(FilterInvisibleCharacters(input)))
+	return FilterCodeFenceMetadata(FilterInvisibleCharacters(normalized))
 }
 
 // FilterInvisibleCharacters removes invisible or control characters that should not appear
@@ -29,18 +32,35 @@ func Sanitize(input string) string {
 // - BiDi control characters: U+202A–U+202E, U+2066–U+2069
 // - BiDi/directional marks: U+200E, U+200F, U+061C
 // - Hidden modifier characters: U+200B, U+200C, U+00AD, U+FEFF, U+180E, U+2060–U+2064
-// - Variation selectors: U+FE00–U+FE0F, U+E0100–U+E01EF
+// - Orphaned variation selectors: U+FE00–U+FE0F, U+E0100–U+E01EF
+//
+// Variation selectors are filtered contextually rather than unconditionally.
+// A selector that forms a plausible variation sequence with the character it
+// follows is preserved, so ordinary content such as "✈️", "1️⃣" and CJK
+// ideographic variation sequences survive unchanged. Selectors that cannot
+// belong to such a sequence — those at the start of the input, those following
+// a removed or non-graphic character, and runs of consecutive selectors — are
+// removed, which is the shape used to smuggle hidden payloads.
 func FilterInvisibleCharacters(input string) string {
 	if input == "" {
 		return input
 	}
 
 	// Filter runes
 	out := make([]rune, 0, len(input))
+	var prev rune
+	var prevKept bool
 	for _, r := range input {
-		if !shouldRemoveRune(r) {
+		keep := false
+		if isVariationSelector(r) {
+			keep = prevKept && isValidVariationSequence(prev, r)
+		} else {
+			keep = !shouldRemoveRune(r)
+		}
+		if keep {
 			out = append(out, r)
 		}
+		prev, prevKept = r, keep
 	}
 	return string(out)
 }
@@ -215,14 +235,43 @@ func shouldRemoveRune(r rune) bool {
 	if r >= 0x2060 && r <= 0x2064 {
 		return true
 	}
-	// Variation selectors: U+FE00–U+FE0F
-	if r >= 0xFE00 && r <= 0xFE0F {
-		return true
+
+	return false
+}
+
+// isVariationSelector reports whether r is a Unicode variation selector, either
+// from the Variation Selectors block (VS1–VS16) or the Variation Selectors
+// Supplement (VS17–VS256).
+func isVariationSelector(r rune) bool {
+	return (r >= 0xFE00 && r <= 0xFE0F) || (r >= 0xE0100 && r <= 0xE01EF)
+}
+
+// isValidVariationSequence reports whether selector can legitimately apply to
+// the base character it immediately follows.
+//
+// A base may carry at most one selector, so a selector following another
+// selector is always rejected; consecutive selectors carry no rendering meaning
+// and are the primary way arbitrary data is hidden in text.
+func isValidVariationSequence(base, selector rune) bool {
+	if isVariationSelector(base) || !unicode.IsGraphic(base) || unicode.IsSpace(base) {
+		return false
 	}
-	// Variation selectors supplement: U+E0100–U+E01EF
-	if r >= 0xE0100 && r <= 0xE01EF {
-		return true
+
+	// The Ideographic Variation Database only registers sequences whose base is
+	// a CJK ideograph, so supplement selectors are meaningless elsewhere.
+	if selector >= 0xE0100 {
+		return unicode.Is(unicode.Han, base)
 	}
 
-	return false
+	// Standardized variation sequences use non-ASCII bases, except for the
+	// keycap bases '#', '*' and the ASCII digits, which take a presentation
+	// selector (VS15/VS16) only.
+	if base < utf8.RuneSelf {
+		if base != '#' && base != '*' && (base < '0' || base > '9') {
+			return false
+		}
+		return selector == 0xFE0E || selector == 0xFE0F
+	}
+
+	return true
 }

pkg/sanitize/sanitize_test.go

@@ -118,19 +118,49 @@ func TestFilterInvisibleCharacters(t *testing.T) {
 			expected: "HelloWorld",
 		},
 		{
-			name:     "text with variation selector",
+			name:     "orphaned variation selector after ascii letter",
 			input:    "Hello\uFE0FWorld",
 			expected: "HelloWorld",
 		},
 		{
-			name:     "text with variation selector supplement",
+			name:     "ideographic variation selector after non-ideograph base",
 			input:    "Hello\U000E0100World",
 			expected: "HelloWorld",
 		},
 		{
-			name:     "emoji variation selector hidden after emoji (steganography)",
-			input:    "\U0001F600\uFE0F\U000E0101Hi",
-			expected: "\U0001F600Hi",
+			name:     "variation selector at start of input has no base",
+			input:    "\uFE0FHello",
+			expected: "Hello",
+		},
+		{
+			name:     "variation selector orphaned by removed zero width space",
+			input:    "\u2708\u200B\uFE0F",
+			expected: "\u2708",
+		},
+		{
+			name:     "smuggled selector run after emoji keeps only the presentation selector",
+			input:    "\U0001F600\uFE0F\U000E0101\U000E0102Hi",
+			expected: "\U0001F600\uFE0FHi",
+		},
+		{
+			name:     "emoji presentation sequence is preserved",
+			input:    "Book a flight \u2708\uFE0F today",
+			expected: "Book a flight \u2708\uFE0F today",
+		},
+		{
+			name:     "text presentation sequence is preserved",
+			input:    "Book a flight \u2708\uFE0E today",
+			expected: "Book a flight \u2708\uFE0E today",
+		},
+		{
+			name:     "keycap sequence is preserved",
+			input:    "Step 1\uFE0F\u20E3 first",
+			expected: "Step 1\uFE0F\u20E3 first",
+		},
+		{
+			name:     "registered cjk ideographic variation sequence is preserved",
+			input:    "\u845B\U000E0100\u57CE",
+			expected: "\u845B\U000E0100\u57CE",
 		},
 	}
 
@@ -189,19 +219,13 @@ func TestShouldRemoveRune(t *testing.T) {
 		// Additional directional mark
 		{name: "arabic letter mark", rune: 0x061C, expected: true},
 
-		// Range tests - Variation selectors: U+FE00–U+FE0F
-		{name: "variation selector range start", rune: 0xFE00, expected: true},
-		{name: "variation selector range middle", rune: 0xFE05, expected: true},
-		{name: "variation selector range end (VS16, emoji presentation)", rune: 0xFE0F, expected: true},
-		{name: "before variation selector range", rune: 0xFDFF, expected: false},
-		{name: "after variation selector range", rune: 0xFE10, expected: false},
-
-		// Range tests - Variation selectors supplement: U+E0100–U+E01EF
-		{name: "variation selector supplement range start", rune: 0xE0100, expected: true},
-		{name: "variation selector supplement range middle", rune: 0xE0150, expected: true},
-		{name: "variation selector supplement range end", rune: 0xE01EF, expected: true},
-		{name: "before variation selector supplement range", rune: 0xE00FF, expected: false},
-		{name: "after variation selector supplement range", rune: 0xE01F0, expected: false},
+		// Variation selectors are filtered contextually by
+		// FilterInvisibleCharacters, so shouldRemoveRune never removes them on
+		// its own. See TestIsValidVariationSequence for that behaviour.
+		{name: "variation selector range start", rune: 0xFE00, expected: false},
+		{name: "variation selector range end (VS16, emoji presentation)", rune: 0xFE0F, expected: false},
+		{name: "variation selector supplement range start", rune: 0xE0100, expected: false},
+		{name: "variation selector supplement range end", rune: 0xE01EF, expected: false},
 
 		// Characters that should NOT be removed
 		{name: "regular ascii letter", rune: 'A', expected: false},
@@ -359,7 +383,7 @@ func TestSanitizeFiltersInvisibleCharactersAfterEntityDecoding(t *testing.T) {
 			expected: "HelloWorld",
 		},
 		{
-			name:     "hexadecimal entity for zero width space (lowercase x, uppercase hex)",
+			name:     "hexadecimal entity for zero width space (lowercase hex digits)",
 			input:    "Hello​World",
 			expected: "HelloWorld",
 		},
@@ -374,15 +398,20 @@ func TestSanitizeFiltersInvisibleCharactersAfterEntityDecoding(t *testing.T) {
 			expected: "HelloWorld",
 		},
 		{
-			name:     "decimal entity for variation selector",
+			name:     "decimal entity for orphaned variation selector",
 			input:    "Hello️World",
 			expected: "HelloWorld",
 		},
 		{
-			name:     "hexadecimal entity for variation selector supplement",
+			name:     "hexadecimal entity for orphaned variation selector supplement",
 			input:    "Hello󠄀World",
 			expected: "HelloWorld",
 		},
+		{
+			name:     "entity encoded selector run after emoji is truncated to one selector",
+			input:    "Ship it \U0001F600️󠄁󠄂",
+			expected: "Ship it \U0001F600\uFE0F",
+		},
 		{
 			name:     "direct invisible rune alongside entity encoded one",
 			input:    "Hello\u200B‎World",
@@ -403,6 +432,57 @@ func TestSanitizeFiltersInvisibleCharactersAfterEntityDecoding(t *testing.T) {
 			input:    "Hello 世界 🌍 αβγ",
 			expected: "Hello 世界 🌍 αβγ",
 		},
+		{
+			name:     "emoji presentation sequence survives the full pipeline",
+			input:    "Book a flight \u2708\uFE0F today",
+			expected: "Book a flight \u2708\uFE0F today",
+		},
+		{
+			name:     "registered cjk ideographic variation sequence survives the full pipeline",
+			input:    "\u845B\U000E0100\u57CE",
+			expected: "\u845B\U000E0100\u57CE",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := Sanitize(tt.input)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+// TestSanitizeRemovesCodeFenceMetadataRevealedByEntityDecoding covers fences
+// that only become fences after HTML entity decoding. A leading "`​“"
+// is not a fence in the raw input, so the first FilterCodeFenceMetadata pass
+// leaves it alone; once the entity is decoded and the zero width space is
+// removed the line is a real fence, so the fence filter has to run again.
+func TestSanitizeRemovesCodeFenceMetadataRevealedByEntityDecoding(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "decimal entity hides fence delimiter",
+			input:    "`​``steal secrets\nfmt.Println(42)\n```",
+			expected: "```\nfmt.Println(42)\n```",
+		},
+		{
+			name:     "hexadecimal entity hides fence delimiter",
+			input:    "``​`steal secrets\nfmt.Println(42)\n```",
+			expected: "```\nfmt.Println(42)\n```",
+		},
+		{
+			name:     "entity hides fence delimiter with disallowed info string",
+			input:    "`​``go;rm -rf /\ncode\n```",
+			expected: "```\ncode\n```",
+		},
+		{
+			name:     "entity encoded fence keeps a safe info string",
+			input:    "`​``go\nfmt.Println(42)\n```",
+			expected: "```go\nfmt.Println(42)\n```",
+		},
 	}
 
 	for _, tt := range tests {
@@ -412,3 +492,40 @@ func TestSanitizeFiltersInvisibleCharactersAfterEntityDecoding(t *testing.T) {
 		})
 	}
 }
+
+func TestIsValidVariationSequence(t *testing.T) {
+	tests := []struct {
+		name     string
+		base     rune
+		selector rune
+		expected bool
+	}{
+		{name: "emoji presentation selector after symbol", base: 0x2708, selector: 0xFE0F, expected: true},
+		{name: "text presentation selector after symbol", base: 0x2708, selector: 0xFE0E, expected: true},
+		{name: "presentation selector after emoji", base: 0x1F600, selector: 0xFE0F, expected: true},
+		{name: "presentation selector after keycap digit", base: '1', selector: 0xFE0F, expected: true},
+		{name: "presentation selector after keycap hash", base: '#', selector: 0xFE0F, expected: true},
+		{name: "presentation selector after keycap asterisk", base: '*', selector: 0xFE0E, expected: true},
+		{name: "non-presentation selector after keycap digit", base: '1', selector: 0xFE00, expected: false},
+		{name: "presentation selector after ascii letter", base: 'a', selector: 0xFE0F, expected: false},
+		{name: "presentation selector after ascii punctuation", base: '.', selector: 0xFE0F, expected: false},
+		{name: "standardized selector after cjk ideograph", base: '葛', selector: 0xFE00, expected: true},
+
+		{name: "ideographic selector after cjk ideograph", base: '葛', selector: 0xE0100, expected: true},
+		{name: "ideographic selector after cjk compatibility ideograph", base: 0xF900, selector: 0xE0101, expected: true},
+		{name: "ideographic selector after emoji", base: 0x1F600, selector: 0xE0100, expected: false},
+		{name: "ideographic selector after ascii letter", base: 'a', selector: 0xE0100, expected: false},
+		{name: "ideographic selector after greek letter", base: 'α', selector: 0xE0100, expected: false},
+
+		{name: "selector after another selector", base: 0xFE0F, selector: 0xFE0F, expected: false},
+		{name: "ideographic selector after another selector", base: 0xE0100, selector: 0xE0101, expected: false},
+		{name: "selector after space", base: ' ', selector: 0xFE0F, expected: false},
+		{name: "selector after newline", base: '\n', selector: 0xFE0F, expected: false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, isValidVariationSequence(tt.base, tt.selector))
+		})
+	}
+}