Report the renderer's tag and attribute drops to HtmlChangeListener (#469)

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

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

@@ -45,7 +45,15 @@
  */
 public interface HtmlChangeListener<T> {
 
-  /** 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<T> {
    * 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}.
    * <p>
    * A repeated attribute name counts once per dropped copy.
    */

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

@@ -129,7 +129,7 @@ private static final class InputChannel<T>
                  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<String> 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<String> 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<String> 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,29 +415,34 @@ String[] discardedAttributes() {
           discarded[out++] = expectedAttrs.get(i * 2 + 1);
         }
       }
+      for (String s : addedThenLeftOffAttrs) {
+        discarded[out++] = s;
+      }
       return discarded;
     }
 
     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);
       }
     }
 

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

@@ -57,7 +57,7 @@ public class HtmlStreamRenderer implements HtmlStreamEventReceiver {
   private final Handler<? super IOException> ioExHandler;
   private final Handler<? super String> 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 <style>} element whose
-   *    textual content is {@code "</style>"}.
+   *    textual content is {@code "</style>"}.  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,20 +152,49 @@ 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 {
     if (open) { throw new 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.
       }