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