From 5a9d28b9d9df13b62340a36916adea385b57e1db Mon Sep 17 00:00:00 2001 From: Jim Manico Date: Thu, 10 Sep 2026 19:48:48 -1000 Subject: [PATCH] Judge a renamed element by the name the author wrote ElementAndAttributePolicyBasedSanitizerPolicy.writeOpenTag decided two things by the name an ElementPolicy emitted an element under, while the lexer, the tag balancer, deferOpenTag and disallowTextIn all go by the name the author wrote. Text: allowElements(ElementPolicy, String...) and allowTextIn register the input name as a text container, but the gate looked up the adjusted name, so a rename to a name not allowed in its own right kept the element and dropped its text (#445). The gate now follows the input name. A rename into an element whose content a browser reads literally, such as style, still needs allowTextIn on that name, so a rename is not a way around the bar the builder sets for raw-text elements. Void-ness: the push onto the open-element stack keyed on the adjusted name, so a void element renamed to a non-void one left an entry that no close tag ever popped (#450). The renamed element stayed open until its parent closed, swallowing every sibling after it, and since the balancer never counts a void input, such entries nested past its limit. The push now keys on the input name: a void input renamed to a non-void output is emitted and closed at once, and a non-void input renamed to a void output gets an entry with nothing to close, so its own close tag pops it rather than an outer element of the same input name. Fixes #445 Fixes #450 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_014Ydng7gBm6Ax5zfwt4vZip --- change_log.md | 11 ++ ...ndAttributePolicyBasedSanitizerPolicy.java | 73 ++++++--- .../org/owasp/html/HtmlPolicyBuilder.java | 11 ++ .../org/owasp/html/HtmlPolicyBuilderTest.java | 146 ++++++++++++++++++ 4 files changed, 219 insertions(+), 22 deletions(-) diff --git a/change_log.md b/change_log.md index 73047306..541b88a1 100644 --- a/change_log.md +++ b/change_log.md @@ -2,6 +2,17 @@ Most recent at top. * Next release + * An element an `ElementPolicy` renames is judged for text by the name + the author wrote, which is the name `allowElements`, `allowTextIn` and + `disallowTextIn` take, so `span` renamed to `div` keeps its text + whether or not `div` is allowed in its own right (#445). A rename + into an element whose content a browser reads literally, such as + `style`, still needs `allowTextIn` on that name. A void element + renamed to one that is not, such as `br` to `span`, is closed at once + rather than left open until its parent closes, which also kept the + renamed elements from nesting past the balancer's limit (#450). An + element renamed to a void one no longer lets its close tag end an outer + element of the same name. * Ordinary script and style text survives the filter on kept literal-content elements. A tag now needs a well-formed name -- an ASCII letter and then letters, digits, `-`, `_`, `:` or `.` -- so diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java index a98e1622..2d0d64fc 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java @@ -68,8 +68,9 @@ class ElementAndAttributePolicyBasedSanitizerPolicy *

* While a document is open, this is the gate {@link #openElementStack} * implies. Text belongs to the nearest enclosing element the policy kept, - * and is emitted only if that element is an allowed text container whose - * input name is not one text was disallowed in. A dropped element between + * and is emitted only if that element is an allowed text container under + * the name the author wrote, and, where the policy emitted it under a name + * a browser reads literally, under that name too. A dropped element between * the text and that container is not a container in the output, so it does * not decide -- unless its content is never meant to be read as text * ({@link #SKIPPABLE_ELEMENT_CONTENT}) or text in it was disallowed, either @@ -113,7 +114,9 @@ class ElementAndAttributePolicyBasedSanitizerPolicy private String literalTextTail = ""; /** * Alternating input names and adjusted names of elements opened by the - * caller. + * caller, for the input names a close tag can come for: the ones that are + * not void. The adjusted name is null where there is nothing to close in + * the output, because the element was dropped or emitted as a void one. */ private final List openElementStack = new ArrayList<>(); /** @@ -857,27 +860,53 @@ void writeOpenTag( ElementAndAttributePolicies policies, String adjustedElementName, List attrs) { outputElementNameForLastOpenTag = adjustedElementName; - if (!HtmlTextEscapingMode.isVoidElement(adjustedElementName)) { - push(policies.elementName, adjustedElementName); - // A kept element is the container for the text inside it. It is judged - // by the name it was kept under, and by the name the author wrote when - // text was disallowed in that. - skipText = !allowedTextContainers.contains(adjustedElementName) - || disallowedTextContainers.contains(policies.elementName); - boolean enteringKeptCdata = isLiteralContentElement(adjustedElementName) - && allowedTextContainers.contains(adjustedElementName); - if (!inKeptCdataElement && enteringKeptCdata) { - keptCdataElementName = adjustedElementName; - literalTextTail = ""; + String elementName = policies.elementName; + // Whether a close tag will come for this element depends on the name the + // author wrote, which is the one the lexer and the tag balancer see, not + // on the name the policy emitted it under. The stack follows suit, so + // that every entry on it is one a close tag can pop. + if (HtmlTextEscapingMode.isVoidElement(elementName)) { + out.openTag(adjustedElementName, attrs); + if (!HtmlTextEscapingMode.isVoidElement(adjustedElementName)) { + // Renamed to an element that needs closing, which nothing upstream + // will do: closed at once, so it does not swallow what follows. + out.closeTag(adjustedElementName); } - inKeptCdataElement = inKeptCdataElement || enteringKeptCdata; - // Judged before this element is in foreign content itself: a browser - // parses the content of an svg or math element as markup, but the - // element's own tag sits in whatever contains it. - inForeignContent = inForeignContent - || HtmlStreamRenderer.FOREIGN_CONTENT_ROOT_ELEMENT_NAMES.contains( - adjustedElementName); + return; } + if (HtmlTextEscapingMode.isVoidElement(adjustedElementName)) { + // Renamed to a void element. The close tag that comes for the input + // name has nothing to close in the output, and the text between is + // not inside the element there, so the gate stays as it was, as after + // a dropped element. + push(elementName, null); + skipText = skipText || suppressesTextWhenDropped(elementName); + out.openTag(adjustedElementName, attrs); + return; + } + push(elementName, adjustedElementName); + // A kept element is the container for the text inside it. Whether it may + // hold text was said of the name the author wrote, which is the name the + // builder's methods take, so that is the name judged. An element the + // policy emits under a name a browser reads literally, such as style, is + // held to the same bar as one written under that name: text in it needs + // allowTextIn on that name too. + boolean literal = isLiteralContentElement(adjustedElementName); + skipText = !allowedTextContainers.contains(elementName) + || disallowedTextContainers.contains(elementName) + || (literal && !allowedTextContainers.contains(adjustedElementName)); + boolean enteringKeptCdata = literal && !skipText; + if (!inKeptCdataElement && enteringKeptCdata) { + keptCdataElementName = adjustedElementName; + literalTextTail = ""; + } + inKeptCdataElement = inKeptCdataElement || enteringKeptCdata; + // Judged before this element is in foreign content itself: a browser + // parses the content of an svg or math element as markup, but the + // element's own tag sits in whatever contains it. + inForeignContent = inForeignContent + || HtmlStreamRenderer.FOREIGN_CONTENT_ROOT_ELEMENT_NAMES.contains( + adjustedElementName); out.openTag(adjustedElementName, attrs); } diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java index 8ce1d77d..31994eb2 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java @@ -256,6 +256,17 @@ public HtmlPolicyBuilder disallowElements(String... elementNames) { private static HtmlElementTables METADATA = HtmlElementTables.get(); /** * Allow the given elements with the given policy. + *

+ * Whether text may appear inside an element the policy renames follows the + * name given here, which {@link #allowTextIn} and {@link #disallowTextIn} + * take as well, not the name the policy returns: a policy that turns + * {@code span} into {@code div} keeps the text of the span whether or not + * {@code div} is allowed in its own right. The one exception is a rename + * into an element whose content a browser reads literally, such as + * {@code style} or {@code script}, which keeps text only if that name was + * also passed to {@code allowTextIn}, as it would have to be if written. + * A void element renamed to one that is not void, such as {@code br} to + * {@code span}, is closed at once, since no close tag will come for it. * * @param policy May remove or add attributes, change the element name, or * deny the element. diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java index a914f2f6..5efa2281 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlPolicyBuilderTest.java @@ -2000,6 +2000,152 @@ void testTextInADroppedCellOfAKeptRowIsDropped() { table)); } + /** + * Whether an element may hold text is said of the name the author wrote, + * which is the name {@code allowElements(ElementPolicy, String...)} takes, + * so a rename to a name the policy does not allow in its own right keeps + * the text (#445). The gate used to follow the emitted name, so the span + * survived as an empty div. + */ + @Test + void testTextSurvivesARenameToAnUnallowedName() { + assertEquals( + "

hi
", + apply( + new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "div", "span") + .allowWithoutAttributes("span"), + "hi")); + assertEquals( + "
hi
xyz
", + apply( + new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "div", "b") + .allowElements("i"), + "hi xyz")); + } + + /** + * {@code allowTextIn} takes the name the author wrote too, so it reaches a + * renamed element, and without it the text of a raw-text element written + * under that name stays out, whatever the policy renames it to. + */ + @Test + void testAllowTextInFollowsTheInputNameOfARenamedElement() { + HtmlPolicyBuilder styleToDiv = new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "div", "style"); + assertEquals( + "
", apply(styleToDiv, "")); + assertEquals( + "
a<b
", + apply(styleToDiv.allowTextIn("style"), "")); + } + + /** + * A rename into an element whose content a browser reads literally is held + * to the bar the builder sets for that name: text in a {@code style} needs + * {@code allowTextIn("style")} whether the author wrote {@code ", apply(divToStyle, "
a{b:c}
")); + assertEquals( + "", + apply(divToStyle.allowTextIn("style"), "
a{b:c}
")); + } + + /** + * A void element renamed to one that is not void is closed at once (#450). + * The lexer never produces a close tag for {@code br}, and the balancer, + * which also goes by the input name, never synthesizes one, so the renamed + * element used to stay open until its parent closed and swallowed every + * sibling after it. Same result whether or not the target may hold text: + * the text after the void element belongs to the paragraph. + */ + @Test + void testVoidElementRenamedToANonVoidOneIsClosedAtOnce() { + String html = "

a
bcd

e

"; + assertEquals( + "

abcd

e

", + apply( + new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "span", "br") + .allowElements("p", "span"), + html)); + assertEquals( + "

abcd

e

", + apply( + new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "span", "br") + .allowElements("p"), + html)); + } + + /** + * The balancer caps how deep the output nests, but it never counts a void + * input element, so the phantom entries of #450 nested past the cap: three + * hundred {@code
} renamed to {@code span} came out as three hundred + * spans inside one another. + */ + @Test + void testVoidElementRenamedToANonVoidOneDoesNotNest() { + StringBuilder html = new StringBuilder(); + StringBuilder expected = new StringBuilder(); + for (int i = 0; i < 300; ++i) { + html.append("
"); + expected.append(""); + } + assertEquals( + expected.toString(), + apply( + new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "span", "br") + .allowElements("span"), + html.toString())); + } + + /** + * The reverse rename, to a void element, has nothing to close in the + * output, and the text after the void element belongs to the enclosing + * container, as it does after a dropped element. Text was disallowed in + * the element by the name the author wrote, so that still holds. + */ + @Test + void testNonVoidElementRenamedToAVoidOne() { + HtmlPolicyBuilder spanToBr = new HtmlPolicyBuilder() + .allowElements((name, attrs) -> "br", "span") + .allowWithoutAttributes("span") + .allowElements("p"); + assertEquals( + "

x
yz

", apply(spanToBr, "

xyz

")); + assertEquals( + "

x
z

", + apply(spanToBr.disallowTextIn("span"), "

xyz

")); + } + + /** + * The close tag of an element renamed to a void one pops that element and + * nothing else. Before #450 the policy pushed nothing for it, so the close + * tag found the nearest open element of the same input name instead and + * ended the outer span early, leaving {@code c} outside it. + */ + @Test + void testCloseTagOfAnElementRenamedToAVoidOnePopsOnlyThatElement() { + HtmlPolicyBuilder classedSpanToBr = new HtmlPolicyBuilder() + .allowElements( + (name, attrs) -> attrs.isEmpty() ? "span" : "br", "span") + .allowWithoutAttributes("span") + .allowAttributes("class").onElements("span"); + assertEquals( + "a
bc", + apply(classedSpanToBr, "abc")); + } + /** * A factory is typically parked in a static final for the life of the JVM, * so nothing it holds may point back at the throwaway builder. The value