diff --git a/change_log.md b/change_log.md index 2e537b8f..95dc253b 100644 --- a/change_log.md +++ b/change_log.md @@ -2,6 +2,12 @@ Most recent at top. * Next release + * Elements written at the configured nesting limit now receive their end + tags when they close explicitly, implicitly, or with an ancestor. The + previous close checks were one level too strict and could leave the + sanitized output unbalanced. Close decisions for stacked elements also + continue to match what was written if the limit changes mid-document + (#485). * 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 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 a0b15177..66e21933 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 @@ -59,6 +59,13 @@ public class TagBalancingHtmlStreamEventReceiver * When the receiver below cannot report that, the input name is used. */ private final IntVector outputElements = new IntVector(); + /** + * Bit {@code i} is set when the open event for entry {@code i} of + * {@link #openElements} was sent to {@link #underlying}. Resumed formatting + * and returned table entries can remain on the logical stack at or beyond + * the nesting limit without an open event, and the limit may later change. + */ + private final BitSet sentToUnderlying = new BitSet(); private final IntVector toResumeInReverse = new IntVector(); /** * Bit {@code i} is set while the element at {@code i} of @@ -266,14 +273,15 @@ public void openDocument() { } public void closeDocument() { - for (int i = Math.min(nestingLimit, openElements.size()); --i >= 0;) { - if (pushedOut.get(i)) { continue; } // Already closed in the output. + for (int i = openElements.size(); --i >= 0;) { + if (!sentToUnderlying.get(i) || pushedOut.get(i)) { continue; } int elIndex = openElements.get(i); String elname = METADATA.canonNameForIndex(elIndex); underlying.closeTag(elname); } openElements.clear(); outputElements.clear(); + sentToUnderlying.clear(); pushedOut.clear(); reopenedWithoutTable.clear(); foreignRootPendingTableReturn = null; @@ -339,6 +347,7 @@ && isOutputInForeignContent()) { if (!HtmlTextEscapingMode.isVoidElement(canonElementName)) { openElements.add(elIndex); outputElements.add(outputElementIndex); + sentToUnderlying.set(openElements.size() - 1); } } else { if (contentIsSkippable(canonElementName)) { ++droppedSkippableDepth; } @@ -498,6 +507,7 @@ private void prepareForContent(int elIndex) { int outputElementIndex = openElement(impliedElIndex, attrs, true); openElements.add(impliedElIndex); outputElements.add(outputElementIndex); + sentToUnderlying.set(openElements.size() - 1); if (suppressedImpliedTable) { reopenedWithoutTable.set(openElements.size() - 1); } @@ -527,11 +537,12 @@ private void prepareForContent(int elIndex) { // it holds, from the top down. for (int i = openElements.size(); --i >= container;) { int unclosed = openElements.get(i); - if (i + 1 < nestingLimit && !pushedOut.get(i)) { + if (sentToUnderlying.get(i) && !pushedOut.get(i)) { underlying.closeTag(METADATA.canonNameForIndex(unclosed)); } openElements.remove(i); outputElements.remove(i); + sentToUnderlying.clear(i); pushedOut.clear(i); reopenedWithoutTable.clear(i); if (METADATA.resumable(unclosed) && unclosed != elIndex) { @@ -556,11 +567,13 @@ && canContain(elIndex, toResume, nOpen) && (elIndex == A_TAG || hasOpenLinkInFormattingScope()))) { toResumeInReverse.removeLast(); int outputElementIndex = NO_OUTPUT_ELEMENT; - if (openElements.size() < nestingLimit) { + boolean sent = openElements.size() < nestingLimit; + if (sent) { outputElementIndex = openElement(toResume, new ArrayList<>()); } openElements.add(toResume); outputElements.add(outputElementIndex); + sentToUnderlying.set(openElements.size() - 1, sent); } else { break; } @@ -641,7 +654,7 @@ private void pushOutTable(int topIndex) { int elIndex = openElements.get(i); if (!TABLE_CONTEXT.get(elIndex)) { break; } if (!pushedOut.get(i)) { - if (i < nestingLimit) { + if (sentToUnderlying.get(i)) { underlying.closeTag(METADATA.canonNameForIndex(elIndex)); } pushedOut.set(i); @@ -673,9 +686,10 @@ private void returnToPushedOutTable(int elIndex) { for (int i = openElements.size(); --i > top;) { int unclosed = openElements.remove(i); outputElements.remove(i); - if (i < nestingLimit) { + if (sentToUnderlying.get(i)) { underlying.closeTag(METADATA.canonNameForIndex(unclosed)); } + sentToUnderlying.clear(i); if (METADATA.resumable(unclosed)) { toResumeInReverse.add(unclosed); } @@ -685,6 +699,7 @@ private void returnToPushedOutTable(int elIndex) { if (canHold(elIndex, openElements.get(top), top)) { break; } openElements.remove(top); outputElements.remove(top); + sentToUnderlying.clear(top); pushedOut.clear(top); reopenedWithoutTable.clear(top); --top; @@ -698,6 +713,7 @@ private void returnToPushedOutTable(int elIndex) { for (int i = n; --i >= 0;) { run[i] = openElements.remove(start + i); outputElements.remove(start + i); + sentToUnderlying.clear(start + i); pushedOut.clear(start + i); reopenedWithoutTable.clear(start + i); } @@ -708,7 +724,8 @@ private void returnToPushedOutTable(int elIndex) { PushedOutTablePolicy policy = pushedOutTablePolicy(); for (int i = 0; i < n; ++i) { int outputElementIndex = NO_OUTPUT_ELEMENT; - if (openElements.size() < nestingLimit) { + boolean sent = openElements.size() < nestingLimit; + if (sent) { List attrs = new ArrayList<>(); if (run[i] == TABLE_TAG && policy != null) { policy.openReopenedTable(attrs); @@ -719,6 +736,7 @@ private void returnToPushedOutTable(int elIndex) { } openElements.add(run[i]); outputElements.add(outputElementIndex); + sentToUnderlying.set(openElements.size() - 1, sent); if (policy != null && run[i] == TABLE_TAG && outputElementIndex != TABLE_TAG) { reopenedWithoutTable.set(openElements.size() - 1); @@ -877,9 +895,10 @@ && isOutputInForeignContent()) { while (--last > index) { int unclosed = openElements.remove(last); outputElements.remove(last); - if (last + 1 < nestingLimit && !pushedOut.get(last)) { + if (sentToUnderlying.get(last) && !pushedOut.get(last)) { underlying.closeTag(METADATA.canonNameForIndex(unclosed)); } + sentToUnderlying.clear(last); pushedOut.clear(last); reopenedWithoutTable.clear(last); if (METADATA.resumable(unclosed)) { @@ -890,9 +909,10 @@ && isOutputInForeignContent()) { underlying.closeTag(foreignRootPendingTableReturn); foreignRootPendingTableReturn = null; } - if (openElements.size() < nestingLimit && !pushedOut.get(index)) { + if (sentToUnderlying.get(index) && !pushedOut.get(index)) { underlying.closeTag(METADATA.canonNameForIndex(elIndex)); } + sentToUnderlying.clear(index); pushedOut.clear(index); reopenedWithoutTable.clear(index); openElements.remove(index); diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlSanitizerTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlSanitizerTest.java index a1af241b..a30978ff 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlSanitizerTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlSanitizerTest.java @@ -82,6 +82,21 @@ void testTextSurvivesPastTheNestingLimit() { assertEquals(256, deep.split("", -1).length - 1); } + /** The element at the 256th level is emitted and must also be closed. */ + @Test + void testElementAtDefaultNestingLimitRoundTrips() throws Exception { + PolicyFactory p = new HtmlPolicyBuilder() + .allowElements("div", "span", "p") + .allowWithoutAttributes("span") + .toFactory(); + String input = nest("x

y

", 255); + String out = p.sanitize(input); + + assertEquals(input, out); + assertEquals(out, p.sanitize(out)); + assertEquals(parseAsBrowser(input), parseAsBrowser(out)); + } + /** * Issue #205. Text kept past the limit is still text: it is escaped on the * way out and cannot reintroduce markup. 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 06b54624..d6ff6d66 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 @@ -44,14 +44,32 @@ class TagBalancingHtmlStreamRendererTest { StringBuilder htmlOutputBuffer; TagBalancingHtmlStreamEventReceiver balancer; + int emittedOpenElements; + int emittedCloseElements; @BeforeEach void createBalancer() { htmlOutputBuffer = new StringBuilder(); + HtmlStreamEventReceiver renderer = HtmlStreamRenderer.create( + htmlOutputBuffer, + x -> fail("Unexpected renderer error: " + x)); balancer = new TagBalancingHtmlStreamEventReceiver( - HtmlStreamRenderer.create( - htmlOutputBuffer, - x -> fail("Unexpected renderer error: " + x))); + new HtmlStreamEventReceiverWrapper(renderer) { + @Override + public void openTag(String elementName, List attrs) { + super.openTag(elementName, attrs); + if (!HtmlTextEscapingMode.isVoidElement( + HtmlLexer.canonicalElementName(elementName))) { + ++emittedOpenElements; + } + } + + @Override + public void closeTag(String elementName) { + super.closeTag(elementName); + ++emittedCloseElements; + } + }); } @Test @@ -388,6 +406,91 @@ void testNestingLimits() { "
" + "
", htmlOutputBuffer.toString()); + assertEquals(emittedOpenElements, emittedCloseElements); + } + + /** An explicit end tag closes an element written at the nesting limit. */ + @Test + void testExplicitCloseAtNestingLimit() { + balancer.setNestingLimit(3); + balancer.openDocument(); + balancer.openTag("div", j8().listOf()); + balancer.openTag("div", j8().listOf()); + balancer.openTag("span", j8().listOf()); + balancer.text("x"); + balancer.closeTag("span"); + balancer.openTag("p", j8().listOf()); + balancer.text("y"); + balancer.closeDocument(); + + assertEquals( + "
x

y

", + htmlOutputBuffer.toString()); + assertEquals(emittedOpenElements, emittedCloseElements); + } + + /** A sibling implicitly closes an incompatible element at the limit. */ + @Test + void testImplicitCloseAtNestingLimit() { + balancer.setNestingLimit(3); + balancer.openDocument(); + balancer.openTag("div", j8().listOf()); + balancer.openTag("div", j8().listOf()); + balancer.openTag("textarea", j8().listOf()); + balancer.text("x"); + balancer.openTag("p", j8().listOf()); + balancer.text("y"); + balancer.closeDocument(); + + assertEquals( + "

y

", + htmlOutputBuffer.toString()); + assertEquals(emittedOpenElements, emittedCloseElements); + } + + /** Closing an ancestor also closes descendants written at the limit. */ + @Test + void testAncestorCloseAtNestingLimit() { + balancer.setNestingLimit(3); + balancer.openDocument(); + balancer.openTag("div", j8().listOf()); + balancer.openTag("div", j8().listOf()); + balancer.openTag("span", j8().listOf()); + balancer.text("x"); + balancer.closeTag("div"); + balancer.closeDocument(); + + assertEquals( + "
x
", + htmlOutputBuffer.toString()); + assertEquals(emittedOpenElements, emittedCloseElements); + } + + /** Raising the limit must not close a resumed element that was not written. */ + @Test + void testRaisedLimitDoesNotCloseUnemittedResumedElement() { + balancer.setNestingLimit(3); + balancer.openDocument(); + balancer.openTag("table", j8().listOf()); + balancer.openTag("caption", j8().listOf()); + balancer.openTag("strong", j8().listOf()); + balancer.closeTag("caption"); + balancer.text("x"); + balancer.openTag("strong", j8().listOf()); + balancer.openTag("caption", j8().listOf()); + balancer.closeTag("caption"); + balancer.openTag("strong", j8().listOf()); + balancer.openTag("span", j8().listOf()); + balancer.setNestingLimit(4); + balancer.closeTag("table"); + balancer.closeDocument(); + + assertEquals( + "
" + + "x
" + + "", + htmlOutputBuffer.toString()); + assertEquals(emittedOpenElements, emittedCloseElements); } @Test