Union policy skipIfEmpty value issue (#199)

.and properly merges skipIfEmpty giving preference to explicit policy choices over defaults.

Fixes #197

Author: yangbongsoo

src/main/java/org/owasp/html/ElementAndAttributePolicies.java

@@ -43,18 +43,18 @@ final class ElementAndAttributePolicies {
   final String elementName;
   final ElementPolicy elPolicy;
   final ImmutableMap<String, AttributePolicy> attrPolicies;
-  final boolean skipIfEmpty;
+  final HtmlTagSkipType htmlTagSkipType;
 
   ElementAndAttributePolicies(
       String elementName,
       ElementPolicy elPolicy,
       Map<? extends String, ? extends AttributePolicy>
         attrPolicies,
-      boolean skipIfEmpty) {
+      HtmlTagSkipType htmlTagSkipType) {
     this.elementName = elementName;
     this.elPolicy = elPolicy;
     this.attrPolicies = ImmutableMap.copyOf(attrPolicies);
-    this.skipIfEmpty = skipIfEmpty;
+    this.htmlTagSkipType = htmlTagSkipType;
   }
 
   ElementAndAttributePolicies and(ElementAndAttributePolicies p) {
@@ -78,24 +78,11 @@ ElementAndAttributePolicies and(ElementAndAttributePolicies p) {
       }
     }
 
-    // HACK: this is attempting to recognize when skipIfEmpty has been
-    // explicitly set in HtmlPolicyBuilder and can only make a best effort at
-    // that and is also too tightly coupled with HtmlPolicyBuilder.
-    // Maybe go tri-state.
-    boolean combinedSkipIfEmpty;
-    if (HtmlPolicyBuilder.DEFAULT_SKIP_IF_EMPTY.contains(elementName)) {
-      // Either policy explicitly opted out of skip if empty.
-      combinedSkipIfEmpty = skipIfEmpty && p.skipIfEmpty;
-    } else {
-      // Either policy explicitly specified skip if empty.
-      combinedSkipIfEmpty = skipIfEmpty || p.skipIfEmpty;
-    }
-
     return new ElementAndAttributePolicies(
         elementName,
         ElementPolicy.Util.join(elPolicy, p.elPolicy),
         joinedAttrPolicies.build(),
-        combinedSkipIfEmpty);
+        this.htmlTagSkipType.and(p.htmlTagSkipType));
   }
 
   ElementAndAttributePolicies andGlobals(
@@ -130,7 +117,7 @@ ElementAndAttributePolicies andGlobals(
     }
     if (anded == null) { return this; }
     return new ElementAndAttributePolicies(
-        elementName, elPolicy, anded, skipIfEmpty);
+        elementName, elPolicy, anded, htmlTagSkipType);
   }
 
 }

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

@@ -104,7 +104,7 @@ public void openTag(String elementName, List<String> attrs) {
     ElementAndAttributePolicies policies = elAndAttrPolicies.get(elementName);
     String adjustedElementName = applyPolicies(elementName, attrs, policies);
     if (adjustedElementName != null
-        && !(attrs.isEmpty() && policies.skipIfEmpty)) {
+        && !(attrs.isEmpty() && policies.htmlTagSkipType.skipAvailability())) {
       writeOpenTag(policies, adjustedElementName, attrs);
       return;
     }

src/main/java/org/owasp/html/HtmlPolicyBuilder.java

@@ -168,6 +168,15 @@ public class HtmlPolicyBuilder {
   public static final ImmutableSet<String> DEFAULT_SKIP_IF_EMPTY
       = ImmutableSet.of("a", "font", "img", "input", "span");
 
+  static final ImmutableMap<String, HtmlTagSkipType> DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR;
+  static {
+    ImmutableMap.Builder<String, HtmlTagSkipType> b = ImmutableMap.builder();
+    for (String elementName : DEFAULT_SKIP_IF_EMPTY) {
+      b.put(elementName, HtmlTagSkipType.SKIP_BY_DEFAULT);
+    }
+    DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR = b.build();
+  }
+
   /**
    * These
    * <a href="https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types"
@@ -190,8 +199,7 @@ public class HtmlPolicyBuilder {
   private final Map<String, AttributePolicy> globalAttrPolicies
       = Maps.newLinkedHashMap();
   private final Set<String> allowedProtocols = Sets.newLinkedHashSet();
-  private final Set<String> skipIfEmpty = Sets.newLinkedHashSet(
-      DEFAULT_SKIP_IF_EMPTY);
+  private final Map<String, HtmlTagSkipType> skipIssueTagMap = Maps.newLinkedHashMap(DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR);
   private final Map<String, Boolean> textContainers = Maps.newLinkedHashMap();
   private HtmlStreamEventProcessor postprocessor =
       HtmlStreamEventProcessor.Processors.IDENTITY;
@@ -307,29 +315,29 @@ public HtmlPolicyBuilder disallowTextIn(String... elementNames) {
    * Assuming the given elements are allowed, allows them to appear without
    * attributes.
    *
-   * @see #DEFAULT_SKIP_IF_EMPTY
+   * @see #DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR
    * @see #disallowWithoutAttributes
    */
   public HtmlPolicyBuilder allowWithoutAttributes(String... elementNames) {
     invalidateCompiledState();
     for (String elementName : elementNames) {
       elementName = HtmlLexer.canonicalName(elementName);
-      skipIfEmpty.remove(elementName);
+      skipIssueTagMap.put(elementName, HtmlTagSkipType.DO_NOT_SKIP);
     }
     return this;
   }
 
   /**
    * Disallows the given elements from appearing without attributes.
    *
-   * @see #DEFAULT_SKIP_IF_EMPTY
+   * @see #DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR
    * @see #allowWithoutAttributes
    */
   public HtmlPolicyBuilder disallowWithoutAttributes(String... elementNames) {
     invalidateCompiledState();
     for (String elementName : elementNames) {
       elementName = HtmlLexer.canonicalName(elementName);
-      skipIfEmpty.add(elementName);
+      skipIssueTagMap.put(elementName, HtmlTagSkipType.SKIP);
     }
     return this;
   }
@@ -835,13 +843,29 @@ private CompiledState compilePolicies() {
           elementName,
           new ElementAndAttributePolicies(
               elementName,
-              elPolicy, attrs.build(), skipIfEmpty.contains(elementName)));
+              elPolicy, attrs.build(),
+              getHtmlTagSkipType(elementName)
+          )
+      );
     }
     compiledState = new CompiledState(
         globalAttrPolicies, policiesBuilder.build());
     return compiledState;
   }
 
+  private HtmlTagSkipType getHtmlTagSkipType(String elementName) {
+    HtmlTagSkipType htmlTagSkipType = skipIssueTagMap.get(elementName);
+    if (htmlTagSkipType == null) {
+      if (DEFAULT_SKIP_TAG_MAP_IF_EMPTY_ATTR.containsKey(elementName)) {
+        return HtmlTagSkipType.SKIP_BY_DEFAULT;
+      } else {
+        return HtmlTagSkipType.DO_NOT_SKIP_BY_DEFAULT;
+      }
+    }
+
+    return htmlTagSkipType;
+  }
+
   /**
    * Builds the relationship between attributes, the values that they may have,
    * and the elements on which they may appear.

src/main/java/org/owasp/html/HtmlTagSkipType.java

@@ -0,0 +1,34 @@
+package org.owasp.html;
+
+public enum HtmlTagSkipType {
+  SKIP(true),
+  SKIP_BY_DEFAULT(true),
+  DO_NOT_SKIP(false),
+  DO_NOT_SKIP_BY_DEFAULT(false);
+
+  private final boolean skipAvailability;
+
+  HtmlTagSkipType(boolean skipAvailability) {
+    this.skipAvailability = skipAvailability;
+  }
+
+  public HtmlTagSkipType and(HtmlTagSkipType s) {
+    if (this == s || s == SKIP_BY_DEFAULT) {
+      return this;
+    }
+
+    if (s == DO_NOT_SKIP) {
+      return s;
+    }
+
+    if (s == DO_NOT_SKIP_BY_DEFAULT) {
+      return this;
+    }
+
+    return SKIP;
+  }
+
+  public boolean skipAvailability() {
+    return this.skipAvailability;
+  }
+}