Judge a renamed element by the name the author wrote (#445, #450)

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

owasp-java-html-sanitizer/src/main/java/org/owasp/html/ElementAndAttributePolicyBasedSanitizerPolicy.java

@@ -68,8 +68,9 @@ class ElementAndAttributePolicyBasedSanitizerPolicy
    * <p>
    * 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<String> openElementStack = new ArrayList<>();
   /**
@@ -857,27 +860,53 @@ void writeOpenTag(
       ElementAndAttributePolicies policies, String adjustedElementName,
       List<String> 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);
   }
 

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.
+   * <p>
+   * 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.

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(
+        "<div>hi</div>",
+        apply(
+            new HtmlPolicyBuilder()
+                .allowElements((name, attrs) -> "div", "span")
+                .allowWithoutAttributes("span"),
+            "<span>hi</span>"));
+    assertEquals(
+        "<div>hi</div> <div>x<i>y</i>z</div>",
+        apply(
+            new HtmlPolicyBuilder()
+                .allowElements((name, attrs) -> "div", "b")
+                .allowElements("i"),
+            "<b>hi</b> <b>x<i>y</i>z</b>"));
+  }
+
+  /**
+   * {@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(
+        "<div></div>", apply(styleToDiv, "<style>a<b</style>"));
+    assertEquals(
+        "<div>a&lt;b</div>",
+        apply(styleToDiv.allowTextIn("style"), "<style>a<b</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 <style>} or
+   * a {@code <div>} the policy turned into one.  Otherwise a rename would be
+   * a way to write raw stylesheet or script text without saying so.
+   */
+  @Test
+  void testRenameIntoALiteralContentElementStillNeedsAllowTextInOnTheTarget() {
+    HtmlPolicyBuilder divToStyle = new HtmlPolicyBuilder()
+        .allowElements((name, attrs) -> "style", "div");
+    assertEquals(
+        "<style></style>", apply(divToStyle, "<div>a{b:c}</div>"));
+    assertEquals(
+        "<style>a{b:c}</style>",
+        apply(divToStyle.allowTextIn("style"), "<div>a{b:c}</div>"));
+  }
+
+  /**
+   * 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 = "<p>a<br>b<i>c</i>d</p><p>e</p>";
+    assertEquals(
+        "<p>a<span></span>bcd</p><p>e</p>",
+        apply(
+            new HtmlPolicyBuilder()
+                .allowElements((name, attrs) -> "span", "br")
+                .allowElements("p", "span"),
+            html));
+    assertEquals(
+        "<p>a<span></span>bcd</p><p>e</p>",
+        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 <br>} 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("<br>");
+      expected.append("<span></span>");
+    }
+    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(
+        "<p>x<br />yz</p>", apply(spanToBr, "<p>x<span>y</span>z</p>"));
+    assertEquals(
+        "<p>x<br />z</p>",
+        apply(spanToBr.disallowTextIn("span"), "<p>x<span>y</span>z</p>"));
+  }
+
+  /**
+   * 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(
+        "<span>a<br class=\"x\" />bc</span>",
+        apply(classedSpanToBr, "<span>a<span class=x>b</span>c</span>"));
+  }
+
   /**
    * 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