Merge pull request 'WI-327: Fix sanitizer entity-decode stored-XSS regression' (#51) from agent-runs/run-116 into main

Reviewed-on: https://codeberg.org/realigned/windshift-core/pulls/51

Author: realigned

internal/sanitize/sanitize.go

@@ -207,13 +207,13 @@ var (
 )
 
 // stripAndCap is the common path for PlainTextField + ShortIdentifier:
-// strip every HTML tag, decode entities so we don't store
-// double-encoded text, trim whitespace, length-cap by rune count.
+// decode entities, strip every HTML tag, preserve safe decoded prose,
+// trim whitespace, length-cap by rune count.
 func stripAndCap(input string, maxRunes int) string {
 	if input == "" {
 		return input
 	}
-	s := html.UnescapeString(strictPolicy.Sanitize(input))
+	s := sanitizeDecoded(input, strictPolicy)
 	s = strings.TrimSpace(s)
 	if maxRunes > 0 && utf8.RuneCountInString(s) > maxRunes {
 		s = string([]rune(s)[:maxRunes])
@@ -240,17 +240,17 @@ const (
 func plainTextField(s string) string  { return stripAndCap(s, PlainTextFieldMaxRunes) }
 func shortIdentifier(s string) string { return stripAndCap(s, ShortIdentifierMaxRunes) }
 
-// brAllowAndCap is the common path for RichText + LongDocument: strip
-// HTML except <br />, decode entities, normalize the bluemonday <br/>
-// output back to <br /> for Milkdown compatibility, neutralize
+// brAllowAndCap is the common path for RichText + LongDocument:
+// decode entities, strip HTML except <br />, normalize bluemonday's
+// break output back to <br /> for Milkdown compatibility, neutralize
 // dangerous URL schemes, byte-cap.
 func brAllowAndCap(input string, maxBytes int) string {
 	if input == "" || input == "null" {
 		return ""
 	}
-	s := brOnlyPolicy.Sanitize(input)
-	s = html.UnescapeString(s)
+	s := sanitizeDecoded(input, brOnlyPolicy)
 	s = strings.ReplaceAll(s, "<br/>", "<br />")
+	s = strings.ReplaceAll(s, "<br>", "<br />")
 	s = markdownURLOnly(s)
 	if maxBytes > 0 && len(s) > maxBytes {
 		s = s[:maxBytes]
@@ -265,13 +265,41 @@ func commentPolicy(s string) string {
 	if s == "" {
 		return ""
 	}
-	out := markdownURLOnly(html.UnescapeString(strictPolicy.Sanitize(s)))
+	out := sanitizeDecoded(s, strictPolicy)
+	out = markdownURLOnly(out)
 	if len(out) > LongTextMaxBytes {
 		out = out[:LongTextMaxBytes]
 	}
 	return out
 }
 
+// sanitizeDecoded fully decodes HTML entities before sanitizing so nested
+// entity payloads (for example "&amp;lt;img ...&amp;gt;") cannot survive as
+// encoded markup that a later decode could reanimate. After sanitizing, it
+// decodes only when doing so is stable under the same policy; that preserves
+// legit prose like "5 < 6 > 4" without turning escaped tags back into HTML.
+func sanitizeDecoded(input string, policy *bluemonday.Policy) string {
+	s := unescapeRepeated(input)
+	s = policy.Sanitize(s)
+
+	decoded := html.UnescapeString(s)
+	if policy.Sanitize(decoded) == s {
+		return decoded
+	}
+	return s
+}
+
+func unescapeRepeated(s string) string {
+	for i := 0; i < 8; i++ {
+		u := html.UnescapeString(s)
+		if u == s {
+			return s
+		}
+		s = u
+	}
+	return s
+}
+
 func markdownURLOnly(s string) string {
 	if s == "" {
 		return ""

internal/sanitize/sanitize_test.go

@@ -0,0 +1,108 @@
+package sanitize
+
+import (
+	"strings"
+	"testing"
+)
+
+// entityPayload is the canonical finding payload: the < and > around an
+// <img onerror> tag are HTML-entity-encoded, so bluemonday tokenizes them
+// as character data and StrictPolicy passes them through untouched. The
+// post-sanitize html.UnescapeString then reconstitutes a live tag.
+const entityPayload = `&lt;img src=x onerror=alert(1)&gt;`
+
+// stripAllTags removes every remaining <...> tag from s; used as an
+// assertion helper (the sanitizer's contract is "no HTML in output").
+func stripAllTags(t *testing.T, s string) string {
+	t.Helper()
+	var b strings.Builder
+	inTag := false
+	for _, r := range s {
+		switch r {
+		case '<':
+			inTag = true
+		case '>':
+			inTag = false
+			continue
+		}
+		if !inTag {
+			b.WriteRune(r)
+		}
+	}
+	return b.String()
+}
+
+func assertNoRawTag(t *testing.T, policy Policy, label string) {
+	t.Helper()
+	out := policy.Sanitize(entityPayload)
+	if out == entityPayload {
+		t.Errorf("%s: payload passed through unmodified: %q", label, out)
+	}
+	if strings.Contains(strings.ToLower(out), "<img") {
+		t.Errorf("%s: live <img tag reconstituted after sanitize: %q", label, out)
+	}
+	if strings.Contains(strings.ToLower(out), "onerror") {
+		t.Errorf("%s: onerror handler survived sanitize: %q", label, out)
+	}
+	// After stripping any tag residue, no raw tag markup should remain.
+	if got := stripAllTags(t, out); got != out && strings.Contains(strings.ToLower(out), "<img") {
+		t.Errorf("%s: output still contained tag markup after tag-strip: %q", label, out)
+	}
+}
+
+func TestPlainTextFieldEntityEncodedXSS(t *testing.T) {
+	assertNoRawTag(t, PlainTextField, "PlainTextField")
+}
+
+func TestShortIdentifierEntityEncodedXSS(t *testing.T) {
+	assertNoRawTag(t, ShortIdentifier, "ShortIdentifier")
+}
+
+func TestRichTextEntityEncodedXSS(t *testing.T) {
+	assertNoRawTag(t, RichText, "RichText")
+}
+
+func TestLongDocumentEntityEncodedXSS(t *testing.T) {
+	assertNoRawTag(t, LongDocument, "LongDocument")
+}
+
+func TestCommentEntityEncodedXSS(t *testing.T) {
+	assertNoRawTag(t, Comment, "Comment")
+}
+
+// Legit entity-encoded prose (e.g. "5 &lt; 6 &gt; 4") must survive as
+// plain decoded text, not be eaten. Regression guard for the
+// second-sanitize pass turning into over-stripping.
+func TestPlainTextFieldPreservesDecodedEntities(t *testing.T) {
+	got := PlainTextField.Sanitize("5 &lt; 6 &gt; 4")
+	if want := "5 < 6 > 4"; got != want {
+		t.Errorf("PlainTextField decoded-prose: got %q want %q", got, want)
+	}
+}
+
+func TestRichTextPreservesDecodedEntities(t *testing.T) {
+	got := RichText.Sanitize("5 &lt; 6 &gt; 4")
+	if want := "5 < 6 > 4"; got != want {
+		t.Errorf("RichText decoded-prose: got %q want %q", got, want)
+	}
+}
+
+// brOnly path must still keep a real <br /> on round-trip (its whole
+// reason for existing — Milkdown blank-line preservation).
+func TestRichTextPreservesBreakTag(t *testing.T) {
+	got := RichText.Sanitize("line one<br />line two")
+	if !strings.Contains(got, "<br />") {
+		t.Errorf("RichText lost <br />: got %q", got)
+	}
+	if strings.Contains(strings.ToLower(got), "<img") {
+		t.Errorf("RichText leaked img: %q", got)
+	}
+}
+
+// A javascript: Markdown link is neutralized regardless of HTML.
+func TestCommentNeutralizesDangerousMarkdownURL(t *testing.T) {
+	got := Comment.Sanitize("[click](javascript:alert(1))")
+	if strings.Contains(strings.ToLower(got), "javascript:") {
+		t.Errorf("Comment kept dangerous scheme: %q", got)
+	}
+}