diff --git a/change_log.md b/change_log.md index 2561171e..2e537b8f 100644 --- a/change_log.md +++ b/change_log.md @@ -2,6 +2,30 @@ Most recent at top. * Next release + * Content that cannot go inside a table, such as a `div` between its + rows, no longer stays open until the end of the document, taking the + rows and everything after the table with it (#342). A browser puts + such content in front of the table and keeps the table open, so that + the next row pops the content and carries on in the same table. The + tag balancer now keeps the table, and any row group and row, on its + stack while closing them in the output, closes the content when a part + of the table arrives, and writes the table again for that part. The + output cannot put anything in front of a tag already written, so the + table is written twice, once empty and once with the later rows, and + text pushed out of a table follows it rather than preceding it as in a + browser; a browser reads the rest as it reads the input. Such content + is judged by the element that holds the table, which is where a + browser puts it: that element closes if it cannot hold the content, + and supplies the elements a browser would imply around it, such as the + `select` around an `option`. A link is no longer written again around + or inside another link, with or without a table involved, since a + browser's parse unnests links and the output would read back as a + different tree. Table-part names in SVG and MathML stay in foreign + content, including across stray end tags, while names at HTML integration + points still return to the table. Implied table structure observes the + nesting limit. If an element policy drops the table written again for + later rows, or renames it, the synthetic replacement and its row + structure are suppressed while allowed cell text survives. * What `HtmlStreamRenderer` leaves out now reaches an `HtmlChangeListener` as well as the renderer's bad-HTML handler: a start tag whose name is not one HTML allows, which an `ElementPolicy` can produce by renaming, 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 2d0d64fc..7322263f 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 @@ -49,6 +49,7 @@ class ElementAndAttributePolicyBasedSanitizerPolicy implements HtmlSanitizer.Policy, TagBalancingHtmlStreamEventReceiver.TextSuppressionPolicy, TagBalancingHtmlStreamEventReceiver.OpenTagOutputPolicy, + TagBalancingHtmlStreamEventReceiver.PushedOutTablePolicy, HtmlChangeReporter.AttributelessSkipPolicy, HtmlChangeReporter.DroppedTextSource, HtmlChangeReporter.DiscardedAttributeSource { @@ -105,6 +106,9 @@ class ElementAndAttributePolicyBasedSanitizerPolicy * emits. {@link #isLiteralContentElement} says why it matters. */ private boolean inForeignContent; + /** Browser tree-construction context for the tags actually emitted. */ + private HtmlSanitizer.ForeignContentContext outputForeignContent + = new HtmlSanitizer.ForeignContentContext(); /** * The last few characters emitted for the kept literal-content element that * is open. Text arrives in chunks, and {@link #stripTags} needs them to see @@ -187,6 +191,7 @@ public void openDocument() { inKeptCdataElement = false; keptCdataElementName = null; inForeignContent = false; + outputForeignContent = new HtmlSanitizer.ForeignContentContext(); literalTextTail = ""; droppedTextListener = null; discardedAttributeListener = null; @@ -204,6 +209,7 @@ public void closeDocument() { for (int i = openElementStack.size() - 1; i >= 0; i -= 2) { String tagNameToClose = openElementStack.get(i); if (tagNameToClose != null) { + outputForeignContent.processEndTag(tagNameToClose); out.closeTag(tagNameToClose); } } @@ -234,6 +240,22 @@ public void reportDiscardedAttributesTo( return outputElementNameForLastOpenTag; } + public boolean supportsPushedOutTableOperations() { return true; } + + public boolean isOutputInForeignContent() { + return outputForeignContent.isInForeignContent(); + } + + public @Nullable String outputForeignContentRootName() { + return outputForeignContent.outermostForeignElementName(); + } + + public boolean outputStartTagUsesForeignContentRules( + String elementName, List attrs) { + return outputForeignContent.startTagUsesForeignContentRules( + elementName, attrs); + } + public void text(String textChunk) { if (!skipText) { // The renderer emits the text of a kept literal-content element as it @@ -762,13 +784,40 @@ private boolean isLiteralContentElement(String adjustedElementName) { } public void openTag(String elementName, List attrs) { + openTag(elementName, attrs, OpenTagMode.NORMAL); + } + + public void openTagWithoutOutput(String elementName, List attrs) { + openTag(elementName, attrs, OpenTagMode.SUPPRESS); + } + + public void openReopenedTable(List attrs) { + openTag("table", attrs, OpenTagMode.REOPENED_TABLE); + } + + private void openTag( + String elementName, List attrs, OpenTagMode mode) { outputElementNameForLastOpenTag = null; ElementAndAttributePolicies policies = elAndAttrPolicies.get(elementName); String adjustedElementName = applyPolicies(elementName, attrs, policies); skippedLastTagAsAttributeless = false; if (adjustedElementName != null) { if (!(attrs.isEmpty() && policies.htmlTagSkipType.skipAvailability())) { - writeOpenTag(policies, adjustedElementName, attrs); + if (mode == OpenTagMode.NORMAL + || (mode == OpenTagMode.REOPENED_TABLE + && "table".equals(adjustedElementName))) { + writeOpenTag(policies, adjustedElementName, attrs); + } else if (!HtmlTextEscapingMode.isVoidElement(elementName)) { + push(elementName, null); + skipText = !allowedTextContainers.contains(elementName) + || disallowedTextContainers.contains(elementName) + // An emitted HTML breakout can leave the renderer's lexical + // SVG/Math nesting open after the browser context has left it. + // Text from a suppressed table part cannot be placed safely in + // that stale lexical context, so fail closed for that text. + || (inForeignContent + && !outputForeignContent.isInForeignContent()); + } return; } // The element was allowed; it goes only because no attribute survived. @@ -777,6 +826,12 @@ public void openTag(String elementName, List attrs) { deferOpenTag(elementName); } + private enum OpenTagMode { + NORMAL, + REOPENED_TABLE, + SUPPRESS, + } + public boolean skippedLastTagAsAttributeless() { return skippedLastTagAsAttributeless; } @@ -841,6 +896,7 @@ public void closeTag(String elementName) { for (int j = n - 1; j > i; j -= 2) { String tagNameToClose = openElementStack.get(j); if (tagNameToClose != null) { + outputForeignContent.processEndTag(tagNameToClose); out.closeTag(tagNameToClose); } } @@ -866,10 +922,15 @@ void writeOpenTag( // 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)) { + boolean adjustedIsVoid = + HtmlTextEscapingMode.isVoidElement(adjustedElementName); + outputForeignContent.processStartTag( + adjustedElementName, attrs, adjustedIsVoid); out.openTag(adjustedElementName, attrs); - if (!HtmlTextEscapingMode.isVoidElement(adjustedElementName)) { + if (!adjustedIsVoid) { // Renamed to an element that needs closing, which nothing upstream // will do: closed at once, so it does not swallow what follows. + outputForeignContent.processEndTag(adjustedElementName); out.closeTag(adjustedElementName); } return; @@ -881,6 +942,7 @@ void writeOpenTag( // a dropped element. push(elementName, null); skipText = skipText || suppressesTextWhenDropped(elementName); + outputForeignContent.processStartTag(adjustedElementName, attrs, true); out.openTag(adjustedElementName, attrs); return; } @@ -907,6 +969,7 @@ void writeOpenTag( inForeignContent = inForeignContent || HtmlStreamRenderer.FOREIGN_CONTENT_ROOT_ELEMENT_NAMES.contains( adjustedElementName); + outputForeignContent.processStartTag(adjustedElementName, attrs, false); out.openTag(adjustedElementName, attrs); } diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java index 073065e0..91d8e4b0 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeReporter.java @@ -33,8 +33,9 @@ import javax.annotation.Nullable; -import org.owasp.html.TagBalancingHtmlStreamEventReceiver.TextSuppressionPolicy; import org.owasp.html.TagBalancingHtmlStreamEventReceiver.OpenTagOutputPolicy; +import org.owasp.html.TagBalancingHtmlStreamEventReceiver.PushedOutTablePolicy; +import org.owasp.html.TagBalancingHtmlStreamEventReceiver.TextSuppressionPolicy; /** * Sits between the HTML parser, the policy, and the renderer so that it @@ -127,8 +128,9 @@ void reportDiscardedAttributesTo( private static final class InputChannel implements HtmlSanitizer.Policy, TagBalancingHtmlStreamEventReceiver.NestingLimitListener, - TextSuppressionPolicy, OpenTagOutputPolicy, + PushedOutTablePolicy, + TextSuppressionPolicy, HtmlStreamRenderer.DropListener { HtmlStreamEventReceiver policy; final OutputChannel output; @@ -139,6 +141,12 @@ private static final class InputChannel /** Output name produced in response to the most recent input start tag. */ private @Nullable String outputElementNameForLastOpenTag; + private enum OpenTagMode { + NORMAL, + REOPENED_TABLE, + SUPPRESS, + } + InputChannel( OutputChannel output, HtmlChangeListener listener, @Nullable T context) { @@ -201,6 +209,36 @@ public void droppedAttribute( return outputElementNameForLastOpenTag; } + public boolean supportsPushedOutTableOperations() { + PushedOutTablePolicy tablePolicy = pushedOutTablePolicy(); + return tablePolicy != null + && tablePolicy.supportsPushedOutTableOperations(); + } + + public boolean isOutputInForeignContent() { + PushedOutTablePolicy tablePolicy = pushedOutTablePolicy(); + return tablePolicy != null && tablePolicy.isOutputInForeignContent(); + } + + public @Nullable String outputForeignContentRootName() { + PushedOutTablePolicy tablePolicy = pushedOutTablePolicy(); + return tablePolicy != null + ? tablePolicy.outputForeignContentRootName() : null; + } + + public boolean outputStartTagUsesForeignContentRules( + String elementName, List attrs) { + PushedOutTablePolicy tablePolicy = pushedOutTablePolicy(); + return tablePolicy != null + && tablePolicy.outputStartTagUsesForeignContentRules( + elementName, attrs); + } + + private @Nullable PushedOutTablePolicy pushedOutTablePolicy() { + return policy instanceof PushedOutTablePolicy + ? (PushedOutTablePolicy) policy : null; + } + public void openDocument() { pendingDroppedText.clear(); outputElementNameForLastOpenTag = null; @@ -237,11 +275,43 @@ public void closeDocument() { } public void openTag(String elementName, List attrs) { + openTag(elementName, attrs, OpenTagMode.NORMAL); + } + + public void openTagWithoutOutput( + String elementName, List attrs) { + openTag(elementName, attrs, OpenTagMode.SUPPRESS); + } + + public void openReopenedTable(List attrs) { + openTag("table", attrs, OpenTagMode.REOPENED_TABLE); + } + + private void openTag( + String elementName, List attrs, OpenTagMode mode) { output.openedElementName = null; // Copied before the policy runs: it removes rejected attributes from // attrs in place, and their values are wanted for the report. output.expectAttributes(attrs); - policy.openTag(elementName, attrs); + if (mode == OpenTagMode.REOPENED_TABLE) { + PushedOutTablePolicy tablePolicy = pushedOutTablePolicy(); + if (tablePolicy == null + || !tablePolicy.supportsPushedOutTableOperations()) { + throw new IllegalStateException( + "Policy cannot safely reopen a table"); + } + tablePolicy.openReopenedTable(attrs); + } else if (mode == OpenTagMode.SUPPRESS) { + PushedOutTablePolicy tablePolicy = pushedOutTablePolicy(); + if (tablePolicy == null + || !tablePolicy.supportsPushedOutTableOperations()) { + throw new IllegalStateException( + "Policy cannot suppress a table-structure tag"); + } + tablePolicy.openTagWithoutOutput(elementName, attrs); + } else { + policy.openTag(elementName, attrs); + } { // Gather the notification details to avoid any problems with the // listener re-entering the stream event receiver. This shouldn't @@ -249,8 +319,10 @@ public void openTag(String elementName, List attrs) { // // The tag survived if the policy opened anything in response and // the renderer wrote it. Its name is not compared with the input - // name: an ElementPolicy may rename the element, and a renamed - // element was kept, not dropped. + // name: an ElementPolicy may rename an ordinary element, and that + // renamed element was kept. A synthetic table reopen is the exception: + // its policy result is deliberately suppressed unless it remains a + // table, so it is reported as discarded here. boolean discarded = output.openedElementName == null; outputElementNameForLastOpenTag = output.openedElementName; output.openedElementName = null; diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlSanitizer.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlSanitizer.java index ce81370e..0e9fa523 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlSanitizer.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlSanitizer.java @@ -251,7 +251,7 @@ public static void sanitize( } /** Tracks the tree-construction context needed for self-closing flags. */ - private static final class ForeignContentContext { + static final class ForeignContentContext { /** Match the sanitizer's output nesting limit without growing unchecked. */ private static final int MAX_DEPTH = 256; @@ -292,12 +292,64 @@ private static final class ForeignContentContext { */ private boolean unknown; + /** Whether the most recently processed tag used foreign-content rules. */ + private boolean lastTagUsedForeignContentRules; + + /** + * True when the most recent start or end tag was processed in SVG or + * MathML rather than under the HTML tree-building rules. + */ + boolean lastTagUsedForeignContentRules() { + return lastTagUsedForeignContentRules; + } + + /** Whether a known current node is in SVG or MathML. */ + boolean isInForeignContent() { + OpenElement current = currentElement(); + return !unknown && current != null && current.namespace != Namespace.HTML; + } + + /** Whether this start tag would use foreign-content tree-building rules. */ + boolean startTagUsesForeignContentRules( + String elementName, List attrs) { + if (unknown) { return false; } + OpenElement current = currentElement(); + return !usesHtmlRulesForStartTag(current, elementName) + && !breaksOutOfForeignContent(elementName, attrs); + } + + /** The outermost SVG or MathML element in the tracked foreign region. */ + @Nullable String outermostForeignElementName() { + if (unknown) { return null; } + for (OpenElement open : openElements) { + if (open.namespace != Namespace.HTML) { return open.elementName; } + } + return null; + } + + /** Whether the foreign end-tag walk would find this local name. */ + boolean hasForeignElementNamed(String elementName) { + if (!isInForeignContent()) { return false; } + for (int i = openElements.size(); --i >= 0;) { + OpenElement open = openElements.get(i); + if (open.namespace == Namespace.HTML) { return false; } + if (asciiEqualsIgnoreCase(open.elementName, elementName)) { return true; } + } + return false; + } + + /** Records an HTML end tag known to be ignored without changing context. */ + void ignoreEndTagUnderHtmlRules() { + lastTagUsedForeignContentRules = false; + } + /** * Updates the context for a start tag and returns whether its self-closing * flag is honored by tree construction. */ boolean processStartTag( String elementName, List attrs, boolean selfClosing) { + lastTagUsedForeignContentRules = false; if (unknown) { return selfClosing && isForeignContentRoot(elementName); } @@ -316,6 +368,7 @@ && breaksOutOfForeignContent(elementName, attrs)) { // Any other start tag in foreign content inherits the current // namespace, even one named "svg" or "math". + lastTagUsedForeignContentRules = true; if (!selfClosing) { push(new OpenElement(elementName, current.namespace, attrs)); } @@ -324,6 +377,7 @@ && breaksOutOfForeignContent(elementName, attrs)) { /** Updates the context using the foreign-content or HTML end-tag rules. */ void processEndTag(String elementName) { + lastTagUsedForeignContentRules = false; if (unknown) { return; } if (openElements.isEmpty()) { if ("form".equals(elementName)) { @@ -353,6 +407,7 @@ void processEndTag(String elementName) { OpenElement open = openElements.get(i); if (open.namespace == Namespace.HTML) { break; } if (asciiEqualsIgnoreCase(open.elementName, elementName)) { + lastTagUsedForeignContentRules = true; openElements.subList(i, openElements.size()).clear(); return; } diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java index 4b948eae..a0b15177 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/TagBalancingHtmlStreamEventReceiver.java @@ -48,6 +48,10 @@ public class TagBalancingHtmlStreamEventReceiver implements HtmlStreamEventReceiver { private final HtmlStreamEventReceiver underlying; private int nestingLimit = Integer.MAX_VALUE; + private HtmlSanitizer.ForeignContentContext foreignContent + = new HtmlSanitizer.ForeignContentContext(); + /** Input name whose rendered foreign root closes before the table reopens. */ + private @Nullable String foreignRootPendingTableReturn; private final IntVector openElements = new IntVector(); /** * The element each entry in {@link #openElements} became after policy @@ -56,12 +60,66 @@ public class TagBalancingHtmlStreamEventReceiver */ private final IntVector outputElements = new IntVector(); private final IntVector toResumeInReverse = new IntVector(); + /** + * Bit {@code i} is set while the element at {@code i} of + * {@link #openElements} is closed in the output but still open here. + *

+ * A browser keeps a table, and any row group and row open in it, on its + * stack when content arrives that cannot go inside a table: it puts the + * content in front of the table instead, foster parenting, and later table + * content pops that content and carries on in the same table. The output + * cannot put anything in front of a tag already written, so the table is + * closed there and the content written after it, and the table is written + * again, as a new table, when its content resumes (#342). Meanwhile its + * entries stay here, so that table content finds them and comes back to + * them, and so that they bound end tags for elements below them as they do + * in a browser. Such entries are contiguous, from a {@code table} up, and + * are the only pushed-out entries below the content pushed out of them. + */ + private final BitSet pushedOut = new BitSet(); + /** + * Bit {@code i} is set when the policy did not emit a synthetic table while + * returning from {@link #pushedOut}. Its table-structure descendants cannot + * be emitted in that output context without changing when a browser reparses + * them. + */ + private final BitSet reopenedWithoutTable = new BitSet(); private static final HtmlElementTables METADATA = HtmlElementTables.get(); private static final int UNRECOGNIZED_TAG = METADATA.indexForName(HtmlElementNames.CUSTOM_ELEMENT_NAME); private static final int A_TAG = METADATA.indexForName("a"); private static final int BODY_TAG = METADATA.indexForName("body"); + private static final int TABLE_TAG = METADATA.indexForName("table"); private static final int NO_OUTPUT_ELEMENT = -1; + /** + * The elements a browser keeps open, and later clears its stack back to, + * when it foster-parents content that arrives inside them: a table and its + * row groups and rows. Not the cell, caption or template, inside which + * content nests normally. + */ + private static final BitSet TABLE_CONTEXT = new BitSet(); + /** + * The elements whose arrival, in a browser, ends foster-parented content + * and returns to the table: its parts, which clear the stack back to the + * table, and a table itself, which pops the open table and takes its + * place. + *

+ * These are the same elements whose end tags are scoped to a table in + * {@link #SCOPE_FOR_END_TAG}, which is built far below; the two lists are + * written out separately because that one is not initialized yet here. + */ + private static final BitSet TABLE_PARTS = new BitSet(); + static { + for (String name : new String[] { "table", "tbody", "tfoot", "thead", "tr" }) { + TABLE_CONTEXT.set(METADATA.indexForName(name)); + } + for (String name : new String[] { + "caption", "col", "colgroup", "table", "tbody", "td", "tfoot", + "th", "thead", "tr", + }) { + TABLE_PARTS.set(METADATA.indexForName(name)); + } + } /** * Elements on entering which a browser puts a marker on its list of * active formatting elements, so that an {@code a} opened inside one of @@ -128,6 +186,29 @@ interface OpenTagOutputPolicy { @Nullable String outputElementNameForLastOpenTag(); } + /** + * Optional policy operations used to keep output around a pushed-out table + * in a browser context that matches the logical input context. + */ + interface PushedOutTablePolicy { + /** Whether the operations below are available for the current policy. */ + boolean supportsPushedOutTableOperations(); + + /** Applies element and text policy while deliberately emitting no tag. */ + void openTagWithoutOutput(String elementName, List attrs); + + /** Emits a synthetic table only if policy keeps it as a table. */ + void openReopenedTable(List attrs); + + /** Reports the browser context of the elements actually emitted. */ + boolean isOutputInForeignContent(); + + @Nullable String outputForeignContentRootName(); + + boolean outputStartTagUsesForeignContentRules( + String elementName, List attrs); + } + /** * How many elements whose content the policy would suppress -- {@code * " + + "" + + "ytail"; + String out = p.sanitize(input); + + assertEquals( + "

" + + "" + + "
" + + "
y
tail", + out); + assertEquals(out, p.sanitize(out)); + } + + /** + * A policy may drop or rename the attribute-free table written when table + * content resumes. A renamed synthetic table and its row structure cannot + * safely be emitted, but allowed cell text still survives. This includes + * replacements with table, void, select, raw-text and foreign parsing rules. + */ + @Test + void testTableStructureIsSuppressedUnderRenamedReopenedTable() { + String input = "
x
" + + "a</script><svg onload=x>b" + + "
tail"; + String[] replacements = { + "", "div", "tbody", "tr", "td", "caption", "colgroup", "col", + "br", "select", "option", "script", "style", "textarea", + "noscript", "xmp", "plaintext", "iframe", "svg", "math", + }; + HtmlChangeListener ignore = new HtmlChangeListener() { + public void discardedTag(Object context, String elementName) { + // Output is under test, notifications are not. + } + + public void discardedAttributes( + Object context, String tagName, String... attributeNames) { + // Output is under test, notifications are not. + } + }; + for (String c : replacements) { + final String replacement = c.isEmpty() ? null : c; + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements( + (elementName, attrs) -> attrs.isEmpty() + ? replacement : elementName, + "table") + .allowElements("tbody", "tr", "td", "div") + .allowAttributes("class").onElements("table") + .allowTextIn("table") + .toFactory(); + String out = p.sanitize(input); + + assertEquals( + "
x
" + + "a</script><svg onload=x>btail", + out, c); + assertEquals(out, p.sanitize(out), c); + assertEquals(out, p.sanitize(input, ignore, null), c); + } + } + + /** + * A link pushed out of a table is closed when the table resumes, and is + * not written again around another link or inside one: nested links do + * not survive a browser's parse. Text is pushed out likewise, and needs + * nothing closed. + */ + @Test + void testPushedOutLinkIsNotResumedAroundAnotherLink() throws Exception { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("table", "tbody", "tr", "td", "a") + .allowAttributes("href").onElements("a") + .allowWithoutAttributes("a") + .toFactory(); + String out = p.sanitize( + "x
yz
w"); + + assertEquals( + "
x" + + "" + + "
yz
w", + out); + assertEquals(out, p.sanitize(out)); + assertEquals( + "
x
y
", + p.sanitize("x
y
")); + } + + /** + * A browser looks for the table to return to within table scope only, so + * a table or row inside a {@code template} in the pushed-out content + * stays in the template, and the pushed-out table waits for a part + * arriving outside it. + */ + @Test + void testReturnToAPushedOutTableStopsAtATableScopeBoundary() { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("table", "tbody", "tr", "td", "div", "template") + .toFactory(); + String out = p.sanitize( + "
" + + "z
w
v
u"); + + assertEquals( + "
z
w" + + "
v
u", + out); + assertEquals(out, p.sanitize(out)); + } + + /** + * Content is judged by what holds the table it was pushed out of, since + * that is where a browser puts it. An element that cannot hold the + * content closes instead of nesting it: a heading inside a heading does + * not survive a browser's parse, so the output would read back as a + * different tree. + */ + @Test + void testPushedOutContentIsJudgedByWhatHoldsTheTable() { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("table", "tbody", "tr", "td", "h1", "button") + .toFactory(); + String heading = p.sanitize( + "

b

a
"); + assertEquals( + "

a

" + + "

b

", + heading); + assertEquals(heading, p.sanitize(heading)); + + String button = p.sanitize( + "" + + "", + button); + assertEquals(button, p.sanitize(button)); + } + + /** + * And it gets the elements a browser would imply around it there: an + * option needs its select whether or not a table was pushed out of the + * way, which the tables never leave to chance, since the sanitizer does + * not know what the output will be embedded in. + */ + @Test + void testContentBesideAPushedOutTableStillGetsItsImpliedWrapper() { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("table", "tbody", "tr", "td", "select", "option") + .toFactory(); + String out = p.sanitize("x
z"); + + assertEquals("
xz", out); + assertEquals(out, p.sanitize(out)); + } + + /** + * Text that follows a pushed-out table reaches the output even where the + * element holding the table cannot hold text: that element closes, as it + * would for text arriving anywhere else, rather than the text going into + * it and being dropped. The table inside the {@code colgroup} here is a + * separate, older mis-implication (#483); what this pins is that + * {@code tail} survives. + */ + @Test + void testTextAfterAPushedOutTableSurvivesAContainerThatCannotHoldIt() { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("table", "colgroup", "col", "tbody", "tr", "td") + .toFactory(); + String out = p.sanitize("tail
a
"); + + assertEquals( + "" + + "
a
" + + "tail", + out); + } + + /** + * A cell returning to a pushed-out table lands in the table, not in a + * fresh one: the row group it arrives in cannot hold a cell without a row + * between, and the path to that row runs through a table, so the row group + * goes and the table takes the cell. + */ + @Test + void testACellReturningToAPushedOutSectionDoesNotOpenANewTable() { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("table", "thead", "tfoot", "tbody", "tr", "td", "th") + .toFactory(); + String head = p.sanitize("x
y
tail"); + assertEquals( + "
x" + + "
y
tail", + head); + assertEquals(head, p.sanitize(head)); + + String foot = p.sanitize("x
h
tail"); + assertEquals( + "
x" + + "
h
tail", + foot); + assertEquals(foot, p.sanitize(foot)); + } + + /** + * The rule that a link is not written again around or inside another link + * is not about tables: an end tag that closes the element a link was + * misnested in queues the link for resumption, and resuming it around the + * next link used to produce nested links, which a browser's parse + * unnests, so the output read back as a different tree. + */ + @Test + void testLinkIsNotResumedAroundAnotherLinkWithoutATable() { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("div", "p", "a") + .allowAttributes("href").onElements("a") + .allowUrlProtocols("http") + .allowWithoutAttributes("a") + .toFactory(); + String out = p.sanitize( + "y"); + assertEquals( + "y", + out); + assertEquals(out, p.sanitize(out)); + + String nested = p.sanitize( + "
x

y

"); + assertEquals( + "
x

" + + "y
", + nested); + assertEquals(nested, p.sanitize(nested)); + } + + /** Pushed-out content is bounded by the nesting limit like any other. */ + @Test + void testPushedOutContentRespectsTheNestingLimit() { + PolicyFactory p = tablePolicy(); + StringBuilder sb = new StringBuilder(""); + for (int i = 0; i < 300; ++i) { sb.append("
"); } + sb.append("
y
z"); + String out = p.sanitize(sb.toString()); + assertTrue( + out.endsWith("
y
z"), + out.substring(out.length() - 80)); + assertEquals(out, p.sanitize(out)); + } + + /** + * A table arriving beside a pushed-out table replaces it, so a long run + * of them leaves nothing behind: the stack stays flat however many there + * are, where the content used to nest one run inside the next. + */ + @Test + void testRepeatedPushOutAndReturnDoesNotAccumulate() { + PolicyFactory p = tablePolicy(); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 300; ++i) { sb.append("
"); } + sb.append("
y
z"); + String out = p.sanitize(sb.toString()); + + StringBuilder expected = new StringBuilder(); + for (int i = 0; i < 300; ++i) { expected.append("
"); } + expected.setLength(expected.length() - "
".length()); + expected.append("
") + .append("
y
z"); + assertEquals(expected.toString(), out); + assertEquals(out, p.sanitize(out)); + } + + private static PolicyFactory tablePolicy() { + return new HtmlPolicyBuilder() + .allowElements( + "table", "caption", "thead", "tbody", "tfoot", "tr", "td", "th", + "div", "p", "span") + .allowAttributes("class").onElements("table", "div") + .allowWithoutAttributes("span") + .toFactory(); + } + /** The tree a browser builds from html, one node per line. */ private static String parseAsBrowser(String html) throws Exception { Node fragment = new HtmlDocumentBuilder().parseFragment( diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java index 88302e7d..06b54624 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/TagBalancingHtmlStreamRendererTest.java @@ -246,6 +246,130 @@ void testTableNesting() { htmlOutputBuffer.toString()); } + /** + * A browser puts content that cannot go inside a table in front of the + * table and keeps the table open, so that the next row pops the content + * and carries on in the same table (#342). The output cannot put anything + * in front of a tag already written, so the table is closed there and + * written again for the row; the content is closed when the row comes, + * where it used to stay open and swallow the row and everything after. + */ + @Test + void testContentPushedOutOfATableIsClosedWhenTheTableResumes() { + balancer.openDocument(); + balancer.openTag("table", j8().listOf()); + balancer.openTag("div", j8().listOf()); + balancer.text("x"); + balancer.openTag("tr", j8().listOf()); + balancer.openTag("td", j8().listOf()); + balancer.text("y"); + balancer.closeTag("td"); + balancer.closeTag("tr"); + balancer.closeTag("div"); // Ignored: no div in table scope. + balancer.closeTag("table"); + balancer.text("tail"); + balancer.closeDocument(); + + assertEquals( + "
x
" + + "
y
tail", + htmlOutputBuffer.toString()); + } + + /** The row group and row the content was pushed out of return as well. */ + @Test + void testPushedOutRowAndRowGroupReturnWithTheTable() { + balancer.openDocument(); + balancer.openTag("table", j8().listOf()); + balancer.openTag("tbody", j8().listOf()); + balancer.openTag("tr", j8().listOf()); + balancer.openTag("td", j8().listOf()); + balancer.text("a"); + balancer.closeTag("td"); + balancer.openTag("div", j8().listOf()); + balancer.text("x"); + balancer.openTag("td", j8().listOf()); + balancer.text("b"); + balancer.closeTag("td"); + balancer.closeTag("tr"); + balancer.closeTag("table"); + balancer.closeDocument(); + + assertEquals( + "
a
x
" + + "
b
", + htmlOutputBuffer.toString()); + } + + /** The table's own end tag ends the content pushed out of it. */ + @Test + void testPushedOutTableEndsWithItsEndTag() { + balancer.openDocument(); + balancer.openTag("table", j8().listOf()); + balancer.openTag("div", j8().listOf()); + balancer.text("x"); + balancer.closeTag("table"); + balancer.text("tail"); + balancer.closeDocument(); + + assertEquals( + "
x
tail", htmlOutputBuffer.toString()); + } + + /** + * A pushed-out table is closed in the output but still counts toward the + * nesting limit, which is a conservative reading of the limit. + */ + @Test + void testPushedOutTableCountsTowardTheNestingLimit() { + balancer.setNestingLimit(3); + balancer.openDocument(); + balancer.openTag("table", j8().listOf()); + balancer.openTag("div", j8().listOf()); + balancer.openTag("p", j8().listOf()); + balancer.openTag("span", j8().listOf()); // Past the limit. + balancer.text("x"); + balancer.closeDocument(); + + assertEquals( + "

x

", htmlOutputBuffer.toString()); + } + + /** Implied table structure never opens past a small nesting limit. */ + @Test + void testPushedOutTableImpliedElementsRespectSmallNestingLimits() { + String[] expected = { + "xytail", + "
x
ytail", + "
x
" + + "
ytail", + "
x
" + + "
ytail", + }; + for (int limit = 0; limit < expected.length; ++limit) { + StringBuilder out = new StringBuilder(); + TagBalancingHtmlStreamEventReceiver limited = + new TagBalancingHtmlStreamEventReceiver( + HtmlStreamRenderer.create( + out, x -> fail("Unexpected renderer error: " + x))); + limited.setNestingLimit(limit); + limited.openDocument(); + limited.openTag("table", j8().listOf()); + limited.openTag("div", j8().listOf()); + limited.text("x"); + limited.openTag("tr", j8().listOf()); + limited.openTag("td", j8().listOf()); + limited.text("y"); + limited.closeTag("td"); + limited.closeTag("tr"); + limited.closeTag("table"); + limited.text("tail"); + limited.closeDocument(); + + assertEquals(expected[limit], out.toString(), "limit " + limit); + } + } + @Test void testNestingLimits() { // Some browsers can be DoSed by deeply nested structures.