diff --git a/change_log.md b/change_log.md index 541b88a1..2561171e 100644 --- a/change_log.md +++ b/change_log.md @@ -2,6 +2,14 @@ Most recent at top. * Next release + * 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, + is reported as a discarded tag, as is a tag arriving inside literal + content the renderer is writing; an attribute whose name is not one + HTML allows is reported as a discarded attribute, with its value. The + policy had emitted each, so the listener used to hear nothing of them + (#469). * 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 diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeListener.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeListener.java index 7099921b..2108cf3b 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeListener.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlChangeListener.java @@ -45,7 +45,15 @@ */ public interface HtmlChangeListener { - /** Called when a tag is discarded from the input. */ + /** + * Called when a tag is discarded from the input, by the policy, by the tag + * balancer for nesting past its limit, or by the renderer. The renderer + * writes no tag whose name is not one HTML allows, which an + * {@link ElementPolicy} can produce by renaming, and none that arrives + * inside literal content it is writing, such as the tags inside an element + * a policy renamed into a {@code style}. Its drops are reported under the + * same conditions as its dropped text: see {@link #discardedText}. + */ public void discardedTag(@Nullable T context, String elementName); /** @@ -60,7 +68,9 @@ public interface HtmlChangeListener { * reports the tag, and this method still reports the attributes, since * rejecting them is what the policy did. Attributes on a tag that the * policy did not allow are not reported; {@code discardedTag} covers the - * whole tag. + * whole tag. An attribute the renderer leaves off a tag it writes, because + * its name is not one HTML allows, is reported here too, under the same + * conditions as the renderer's dropped text: see {@link #discardedText}. *

* A repeated attribute name counts once per dropped copy. */ 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 0ec97ef8..073065e0 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 @@ -129,7 +129,7 @@ private static final class InputChannel TagBalancingHtmlStreamEventReceiver.NestingLimitListener, TextSuppressionPolicy, OpenTagOutputPolicy, - HtmlStreamRenderer.DroppedTextListener { + HtmlStreamRenderer.DropListener { HtmlStreamEventReceiver policy; final OutputChannel output; final T context; @@ -177,6 +177,26 @@ public void droppedText(String elementName, String text) { pendingDroppedText.add(text); } + /** + * The renderer likewise refuses a start tag whose name it cannot write + * or that arrives inside literal content, after the policy has opened + * it. No tag came out, so the input tag was discarded, and the report + * below says so; the end tag the renderer refuses later is the same loss. + */ + public void droppedTag(String elementName) { + output.refusedOpenedTag(); + } + + /** + * And it leaves an attribute whose name it cannot write off a tag it + * does write. The policy's own accounting had counted it as emitted; + * this returns it to the discarded. + */ + public void droppedAttribute( + String elementName, String name, String value) { + output.attributeLeftOff(name, value); + } + public @Nullable String outputElementNameForLastOpenTag() { return outputElementNameForLastOpenTag; } @@ -193,11 +213,12 @@ public void openDocument() { .reportDiscardedAttributesTo(output); } // The renderer decides on its own to drop literal content it cannot - // emit, so it has to tell us; any other receiver keeps that to itself. + // emit, a tag it cannot write and an attribute it cannot write, so it + // has to tell us; any other receiver keeps that to itself. // Bound once the renderer has opened the document, which forgets any // earlier listener, and for this document only, so that a renderer // reused without this reporter does not go on reporting to it. - output.listenForDroppedText(this); + output.listenForDrops(this); } public void closeDocument() { @@ -211,7 +232,7 @@ public void closeDocument() { ((DiscardedAttributeSource) policy) .reportDiscardedAttributesTo(null); } - output.listenForDroppedText(null); + output.listenForDrops(null); dispatchDroppedText(); } @@ -226,17 +247,20 @@ public void openTag(String elementName, List attrs) { // listener re-entering the stream event receiver. This shouldn't // occur, but if it does it will be a source of subtle confusing bugs. // - // The tag survived if the policy opened anything in response. Its - // name is not compared with the input name: an ElementPolicy may - // rename the element, and a renamed element was kept, not dropped. + // 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. boolean discarded = output.openedElementName == null; outputElementNameForLastOpenTag = output.openedElementName; output.openedElementName = null; // Attributes go unreported with a tag the policy rejected: the tag // report covers them. Not so when the policy allowed the element and - // dropped it only because none of its attributes survived: rejecting - // them was the policy's decision, and the tag went as a consequence. + // dropped it only because none of its attributes survived, or when + // the renderer refused the tag the policy opened: rejecting them was + // the policy's decision, and the tag went for another reason. boolean attrsRejectedOnTheirOwn = !discarded + || output.tagRefusedByRenderer || (policy instanceof AttributelessSkipPolicy && ((AttributelessSkipPolicy) policy) .skippedLastTagAsAttributeless()); @@ -317,6 +341,13 @@ private static final class OutputChannel final BitSet rejectedAttrs = new BitSet(); /** Input pairs accounted for by attributes the policy emitted. */ final BitSet emittedAttrs = new BitSet(); + /** + * Name and value pairs the policy added to the tag, so that no input + * pair accounts for them, and the renderer then left off. + */ + final List addedThenLeftOffAttrs = new ArrayList<>(); + /** True if the renderer refused the tag the policy opened. */ + boolean tagRefusedByRenderer; OutputChannel(HtmlStreamEventReceiver renderer) { super(renderer); @@ -328,6 +359,31 @@ void expectAttributes(List attrs) { expectedAttrs.addAll(attrs); rejectedAttrs.clear(); emittedAttrs.clear(); + addedThenLeftOffAttrs.clear(); + tagRefusedByRenderer = false; + } + + /** Records that the renderer wrote no tag for the one the policy opened. */ + void refusedOpenedTag() { + openedElementName = null; + tagRefusedByRenderer = true; + } + + /** + * Records that the renderer left an attribute off the tag it wrote. The + * input copy that the policy's emitting it accounted for, if any, is + * discarded after all, with the value the author wrote; a pair the policy + * added is reported as the renderer received it. + */ + void attributeLeftOff(String name, String value) { + for (int i = 0, n = expectedAttrs.size() / 2; i < n; ++i) { + if (emittedAttrs.get(i) && name.equals(expectedAttrs.get(i * 2))) { + emittedAttrs.clear(i); + return; + } + } + addedThenLeftOffAttrs.add(name); + addedThenLeftOffAttrs.add(value); } public void discardedAttribute(String name, String value) { @@ -342,10 +398,14 @@ public void discardedAttribute(String name, String value) { } } - /** Returns original pairs not accounted for by emitted attributes. */ + /** + * Returns original pairs not accounted for by emitted attributes, then + * any pairs the policy added and the renderer left off. + */ String[] discardedAttributes() { int n = expectedAttrs.size() / 2; - int nDiscarded = n - emittedAttrs.cardinality(); + int nDiscarded = n - emittedAttrs.cardinality() + + addedThenLeftOffAttrs.size() / 2; if (nDiscarded == 0) { return InputChannel.ZERO_STRINGS; } String[] discarded = new String[nDiscarded * 2]; int out = 0; @@ -355,6 +415,9 @@ String[] discardedAttributes() { discarded[out++] = expectedAttrs.get(i * 2 + 1); } } + for (String s : addedThenLeftOffAttrs) { + discarded[out++] = s; + } return discarded; } @@ -362,22 +425,24 @@ void clearExpectedAttributes() { expectedAttrs.clear(); rejectedAttrs.clear(); emittedAttrs.clear(); + addedThenLeftOffAttrs.clear(); + tagRefusedByRenderer = false; } /** - * Has the renderer report dropped literal content to {@code listener}, - * or to nobody when null, if it is one that can. The library's own - * decorator, which a postprocessor or a logging wrapper is likely to - * extend, is seen through. + * Has the renderer report what it drops to {@code listener}, or to + * nobody when null, if it is one that can. The library's own decorator, + * which a postprocessor or a logging wrapper is likely to extend, is seen + * through. */ - void listenForDroppedText( - @Nullable HtmlStreamRenderer.DroppedTextListener listener) { + void listenForDrops( + @Nullable HtmlStreamRenderer.DropListener listener) { HtmlStreamEventReceiver r = underlying; while (r instanceof HtmlStreamEventReceiverWrapper) { r = ((HtmlStreamEventReceiverWrapper) r).underlying; } if (r instanceof HtmlStreamRenderer) { - ((HtmlStreamRenderer) r).reportDroppedTextTo(listener); + ((HtmlStreamRenderer) r).reportDropsTo(listener); } } diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlStreamRenderer.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlStreamRenderer.java index de583406..005b14ee 100644 --- a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlStreamRenderer.java +++ b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlStreamRenderer.java @@ -57,7 +57,7 @@ public class HtmlStreamRenderer implements HtmlStreamEventReceiver { private final Handler ioExHandler; private final Handler badHtmlHandler; /** Told about dropped literal content; null while nobody is listening. */ - private @Nullable DroppedTextListener droppedTextListener; + private @Nullable DropListener dropListener; private String lastTagOpened; private StringBuilder pendingUnescaped; private HtmlTextEscapingMode escapingMode = HtmlTextEscapingMode.PCDATA; @@ -83,7 +83,10 @@ public class HtmlStreamRenderer implements HtmlStreamEventReceiver { * @param badHtmlHandler receives alerts when HTML cannot be rendered because * there is not valid HTML tree that results from that series of calls. * E.g. it is not possible to create an HTML {@code "}. + * textual content is {@code ""}. What the renderer leaves out + * on such an alert also reaches an {@link HtmlChangeListener} when the + * renderer is behind an {@link HtmlChangeReporter}, as it is in + * {@link PolicyFactory#sanitize(String, HtmlChangeListener, Object)}. */ public static HtmlStreamRenderer create( @WillCloseWhenClosed Appendable output, @@ -149,12 +152,41 @@ interface DroppedTextListener { } /** - * Sends dropped literal content to {@code listener}, or to nobody, until + * Carries the renderer's other drops to {@link HtmlChangeReporter} as + * well: a start tag it did not write, and an attribute it left off one it + * did. Each also goes to the bad-HTML handler as a message, which is all + * there was before and which {@link PolicyFactory#sanitize} wires to + * nobody, so without this a listener heard nothing of them. + */ + interface DropListener extends DroppedTextListener { + /** + * A start tag the renderer did not write, because the element's name is + * not one HTML allows or because it arrived inside literal content that + * cannot hold a tag. The matching end tag is refused for the same reason + * when it comes, and is not reported: it is the same loss. + * + * @param elementName the element's name as the renderer received it. + */ + void droppedTag(String elementName); + + /** + * An attribute left off a start tag the renderer wrote, because its name + * is not one HTML allows. + * + * @param elementName the element the tag opened. + * @param name the attribute's name, as the renderer received it. + * @param value the attribute's value, as the renderer received it. + */ + void droppedAttribute(String elementName, String name, String value); + } + + /** + * Sends what the renderer drops to {@code listener}, or to nobody, until * the next {@link #openDocument}, which starts a document with nobody * listening. */ - final void reportDroppedTextTo(@Nullable DroppedTextListener listener) { - this.droppedTextListener = listener; + final void reportDropsTo(@Nullable DropListener listener) { + this.dropListener = listener; } public final void openDocument() throws IllegalStateException { @@ -162,7 +194,7 @@ public final void openDocument() throws IllegalStateException { open = true; // A listener is for one document; whoever wants this one's drops // registers after this, so an earlier document's cannot linger. - droppedTextListener = null; + dropListener = null; } public final void closeDocument() throws IllegalStateException { @@ -203,10 +235,12 @@ private void writeOpenTag( String elementName = safeName(unsafeElementName); if (!isValidHtmlName(elementName)) { error("Invalid element name", elementName); + if (dropListener != null) { dropListener.droppedTag(elementName); } return; } if (pendingUnescaped != null) { error("Tag content cannot appear inside CDATA element", elementName); + if (dropListener != null) { dropListener.droppedTag(elementName); } return; } @@ -245,6 +279,9 @@ private void writeOpenTag( name = HtmlLexer.canonicalAttributeName(name); if (!isValidHtmlName(name)) { error("Invalid attr name", name); + if (dropListener != null) { + dropListener.droppedAttribute(elementName, name, value); + } continue; } output.append(' ').append(name).append('=').append('"'); @@ -320,9 +357,8 @@ private final void writeCloseTag(String uncanonElementName) cdataContent.subSequence( problemIndex, Math.min(problemIndex + 10, cdataContent.length()))); - if (droppedTextListener != null) { - droppedTextListener.droppedText( - elementName, cdataContent.toString()); + if (dropListener != null) { + dropListener.droppedText(elementName, cdataContent.toString()); } // Still output the close tag. } diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlChangeReporterTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlChangeReporterTest.java index d7a0de1b..c8063d80 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlChangeReporterTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlChangeReporterTest.java @@ -626,6 +626,77 @@ private static Result sanitize(PolicyFactory policy, String html) { * Like {@link #sanitize} but also records each discarded attribute's value * and any dropped text; see {@link #verboseListener}. */ + /** + * The renderer writes no tag whose name is not one HTML allows, which an + * element policy can produce by renaming (#469). The policy had opened + * the tag, so the reporter counted it as kept and the listener heard + * nothing. The attributes the policy rejected on it were its own + * decisions and are still reported, as for a tag skipped for having none. + */ + @Test + void testTagRenamedToAnInvalidNameIsReported() { + PolicyFactory policy = new HtmlPolicyBuilder() + .allowElements((elementName, attrs) -> "a@b", "b") + .allowElements("p") + .toFactory(); + Result result = sanitizeVerbose(policy, "

hiy

"); + + assertEquals("

hiy

", result.html); + assertEquals(" b.onclick=\"x\" ", result.log); + } + + /** + * The renderer leaves an attribute whose name is not one HTML allows off + * the tag it writes. It is reported with the value the author wrote. + */ + @Test + void testAttributeWithAnInvalidNameIsReported() { + PolicyFactory policy = new HtmlPolicyBuilder() + .allowElements("b") + .allowAttributes("x@y", "title").onElements("b") + .toFactory(); + Result result = sanitizeVerbose(policy, "hi"); + + assertEquals("hi", result.html); + assertEquals(" b.x@y=\"1\" ", result.log); + } + + /** An attribute a policy added is reported as the renderer received it. */ + @Test + void testAddedAttributeWithAnInvalidNameIsReported() { + PolicyFactory policy = new HtmlPolicyBuilder() + .allowElements( + (elementName, attrs) -> { + attrs.add("x@y"); + attrs.add("1"); + return elementName; + }, + "b") + .toFactory(); + Result result = sanitizeVerbose(policy, "hi"); + + assertEquals("hi", result.html); + assertEquals(" b.x@y=\"1\" ", result.log); + } + + /** + * A tag arriving inside literal content the renderer is writing is dropped + * as content that cannot appear there, which a policy that renames an + * element into {@code style} brings about. Reported under the input name. + */ + @Test + void testTagInsideRenamedLiteralContentElementIsReported() { + PolicyFactory policy = new HtmlPolicyBuilder() + .allowElements((elementName, attrs) -> "style", "div") + .allowElements("b") + .allowTextIn("style") + .toFactory(); + Result result = sanitizeVerbose(policy, "
aboldc
"); + + assertEquals("", result.html); + assertEquals(" ", result.log); + } + private static Result sanitizeVerbose(PolicyFactory policy, String html) { return sanitize(policy, html, true); } diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlStreamRendererTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlStreamRendererTest.java index e287fd1b..0963c31d 100644 --- a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlStreamRendererTest.java +++ b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/HtmlStreamRendererTest.java @@ -141,6 +141,58 @@ void testIllegalAttributeName() throws Exception { errors.clear(); } + /** + * What the renderer drops reaches its drop listener as well as the + * bad-HTML handler, so that {@code HtmlChangeReporter} can report it + * (#469): a tag it cannot write, an attribute it cannot write, and a tag + * inside literal content. An end tag refused for the same reason as its + * start tag is the same loss and is not reported again. + */ + @Test + void testDropsReachTheDropListener() throws Exception { + List drops = new ArrayList<>(); + renderer.openDocument(); + renderer.reportDropsTo(new HtmlStreamRenderer.DropListener() { + public void droppedText(String elementName, String text) { + drops.add(elementName + "{" + text + "}"); + } + + public void droppedTag(String elementName) { + drops.add("<" + elementName + ">"); + } + + public void droppedAttribute( + String elementName, String name, String value) { + drops.add(elementName + "." + name + "=" + value); + } + }); + renderer.openTag("a@b", j8().listOf("id", "x")); + renderer.text("t"); + renderer.closeTag("a@b"); + renderer.openTag("div", j8().listOf("x@y", "1", "id", "z")); + renderer.openTag("style", j8().listOf()); + renderer.text("a"); + renderer.openTag("b", j8().listOf()); + renderer.text("bold"); + renderer.closeTag("b"); + renderer.closeTag("style"); + renderer.closeTag("div"); + renderer.closeDocument(); + + assertEquals( + "t
", rendered.toString()); + assertEquals(j8().listOf("", "div.x@y=1", ""), drops); + assertIterableEquals( + j8().listOf( + "Invalid element name : a@b", + "Invalid element name : a@b", + "Invalid attr name : x@y", + "Tag content cannot appear inside CDATA element : b", + "Tag content cannot appear inside CDATA element : b"), + errors); + errors.clear(); + } + @Test void testCdataContainsEndTag1() throws Exception { renderer.openDocument();