am 9c0798e0: issue 28: fix Sanitizer.STYLES by changing PolicyFactory to store globals and apply its globals to the other factory when combining PolicyFactories via PolicyFactory.and
* commit '9c0798e090ee7db347657ed2b8604ce26fbe74d1':
issue 28: fix Sanitizer.STYLES by changing PolicyFactory to store globals and apply its globals to the other factory when combining PolicyFactories via PolicyFactory.and
diff --git a/src/main/org/owasp/html/AttributePolicy.java b/src/main/org/owasp/html/AttributePolicy.java
index ee57fe0..7a1f2d9 100644
--- a/src/main/org/owasp/html/AttributePolicy.java
+++ b/src/main/org/owasp/html/AttributePolicy.java
@@ -28,6 +28,11 @@
package org.owasp.html;
+import com.google.common.collect.ImmutableList;
+import java.util.Collection;
+import java.util.Set;
+import java.util.LinkedHashSet;
+import javax.annotation.CheckReturnValue;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;
@@ -60,38 +65,26 @@
* An attribute policy equivalent to applying all the given policies in
* order, failing early if any of them fails.
*/
+ @CheckReturnValue
public static final AttributePolicy join(AttributePolicy... policies) {
-
- class PolicyJoiner {
- AttributePolicy last = null;
- AttributePolicy out = null;
-
- void join(AttributePolicy p) {
- if (REJECT_ALL_ATTRIBUTE_POLICY.equals(p)) {
- out = p;
- } else if (!REJECT_ALL_ATTRIBUTE_POLICY.equals(out)) {
- if (p instanceof JoinedAttributePolicy) {
- JoinedAttributePolicy jap = (JoinedAttributePolicy) p;
- join(jap.first);
- join(jap.second);
- } else if (p != last) {
- last = p;
- if (out == null || IDENTITY_ATTRIBUTE_POLICY.equals(out)) {
- out = p;
- } else if (!IDENTITY_ATTRIBUTE_POLICY.equals(p)) {
- out = new JoinedAttributePolicy(out, p);
- }
- }
- }
+ Set<AttributePolicy> uniq = new LinkedHashSet<AttributePolicy>();
+ for (AttributePolicy p : policies) {
+ if (p instanceof JoinedAttributePolicy) {
+ uniq.addAll(((JoinedAttributePolicy) p).policies);
+ } else if (p != null) {
+ uniq.add(p);
}
}
- PolicyJoiner pu = new PolicyJoiner();
- for (AttributePolicy policy : policies) {
- if (policy == null) { continue; }
- pu.join(policy);
+ if (uniq.contains(REJECT_ALL_ATTRIBUTE_POLICY)) {
+ return REJECT_ALL_ATTRIBUTE_POLICY;
}
- return pu.out != null ? pu.out : IDENTITY_ATTRIBUTE_POLICY;
+ uniq.remove(IDENTITY_ATTRIBUTE_POLICY);
+ switch (uniq.size()) {
+ case 0: return IDENTITY_ATTRIBUTE_POLICY;
+ case 1: return uniq.iterator().next();
+ default: return new JoinedAttributePolicy(uniq);
+ }
}
}
@@ -116,17 +109,29 @@
@Immutable
final class JoinedAttributePolicy implements AttributePolicy {
- final AttributePolicy first, second;
+ final ImmutableList<AttributePolicy> policies;
- JoinedAttributePolicy(AttributePolicy first, AttributePolicy second) {
- this.first = first;
- this.second = second;
+ JoinedAttributePolicy(Collection<? extends AttributePolicy> policies) {
+ this.policies = ImmutableList.copyOf(policies);
}
public @Nullable String apply(
- String elementName, String attributeName, String value) {
- value = first.apply(elementName, attributeName, value);
- return value != null
- ? second.apply(elementName, attributeName, value) : null;
+ String elementName, String attributeName, @Nullable String value) {
+ for (AttributePolicy p : policies) {
+ if (value == null) { break; }
+ value = p.apply(elementName, attributeName, value);
+ }
+ return value;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return o != null && this.getClass() == o.getClass()
+ && policies.equals(((JoinedAttributePolicy) o).policies);
+ }
+
+ @Override
+ public int hashCode() {
+ return policies.hashCode();
}
}
diff --git a/src/main/org/owasp/html/ElementAndAttributePolicies.java b/src/main/org/owasp/html/ElementAndAttributePolicies.java
index 3cc841e..343f2ac 100644
--- a/src/main/org/owasp/html/ElementAndAttributePolicies.java
+++ b/src/main/org/owasp/html/ElementAndAttributePolicies.java
@@ -30,6 +30,7 @@
import java.util.Map;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Maps;
import javax.annotation.concurrent.Immutable;
/**
@@ -98,4 +99,40 @@
joinedAttrPolicies.build(),
combinedSkipIfEmpty);
}
+
+ ElementAndAttributePolicies andGlobals(
+ Map<String, AttributePolicy> globalAttrPolicies) {
+ if (globalAttrPolicies.isEmpty()) { return this; }
+ Map<String, AttributePolicy> anded = null;
+ for (Map.Entry<String, AttributePolicy> e : this.attrPolicies.entrySet()) {
+ String attrName = e.getKey();
+ AttributePolicy globalAttrPolicy = globalAttrPolicies.get(attrName);
+ if (globalAttrPolicy != null) {
+ AttributePolicy attrPolicy = e.getValue();
+ AttributePolicy joined = AttributePolicy.Util.join(
+ attrPolicy, globalAttrPolicy);
+ if (!joined.equals(attrPolicy)) {
+ if (anded == null) {
+ anded = Maps.newLinkedHashMap();
+ anded.putAll(this.attrPolicies);
+ }
+ anded.put(attrName, joined);
+ }
+ }
+ }
+ for (Map.Entry<String, AttributePolicy> e : globalAttrPolicies.entrySet()) {
+ String attrName = e.getKey();
+ if (!this.attrPolicies.containsKey(attrName)) {
+ if (anded == null) {
+ anded = Maps.newLinkedHashMap();
+ anded.putAll(this.attrPolicies);
+ }
+ anded.put(attrName, e.getValue());
+ }
+ }
+ if (anded == null) { return this; }
+ return new ElementAndAttributePolicies(
+ elementName, elPolicy, anded, skipIfEmpty);
+ }
+
}
diff --git a/src/main/org/owasp/html/FilterUrlByProtocolAttributePolicy.java b/src/main/org/owasp/html/FilterUrlByProtocolAttributePolicy.java
index 48c9723..6435d61 100644
--- a/src/main/org/owasp/html/FilterUrlByProtocolAttributePolicy.java
+++ b/src/main/org/owasp/html/FilterUrlByProtocolAttributePolicy.java
@@ -148,4 +148,15 @@
return s;
}
-}
\ No newline at end of file
+ @Override
+ public boolean equals(Object o) {
+ return o != null && this.getClass() == o.getClass()
+ && protocols.equals(((FilterUrlByProtocolAttributePolicy) o).protocols);
+ }
+
+ @Override
+ public int hashCode() {
+ return protocols.hashCode();
+ }
+
+}
diff --git a/src/main/org/owasp/html/HtmlPolicyBuilder.java b/src/main/org/owasp/html/HtmlPolicyBuilder.java
index fc47c64..a113cdf 100644
--- a/src/main/org/owasp/html/HtmlPolicyBuilder.java
+++ b/src/main/org/owasp/html/HtmlPolicyBuilder.java
@@ -486,9 +486,10 @@
: this.textContainers.entrySet()) {
if (Boolean.TRUE.equals(textContainer.getValue())) {
textContainers.add(textContainer.getKey());
- }
+ }
}
- return new PolicyFactory(compilePolicies(), textContainers.build());
+ return new PolicyFactory(compilePolicies(), textContainers.build(),
+ ImmutableMap.copyOf(globalAttrPolicies));
}
// Speed up subsequent builds by caching the compiled policies.
@@ -597,6 +598,8 @@
= ImmutableMap.builder();
for (Map.Entry<String, AttributePolicy> ape : elAttrPolicies.entrySet()) {
String attributeName = ape.getKey();
+ // Handle below so we don't end up putting the same key into the map
+ // twice. ImmutableMap.Builder hates that.
if (globalAttrPolicies.containsKey(attributeName)) { continue; }
AttributePolicy policy = ape.getValue();
if (!AttributePolicy.REJECT_ALL_ATTRIBUTE_POLICY.equals(policy)) {
diff --git a/src/main/org/owasp/html/PolicyFactory.java b/src/main/org/owasp/html/PolicyFactory.java
index c4bb86e..ace5ede 100644
--- a/src/main/org/owasp/html/PolicyFactory.java
+++ b/src/main/org/owasp/html/PolicyFactory.java
@@ -54,12 +54,16 @@
implements Function<HtmlStreamEventReceiver, HtmlSanitizer.Policy> {
private final ImmutableMap<String, ElementAndAttributePolicies> policies;
+ private final ImmutableMap<String, AttributePolicy> globalAttrPolicies;
private final ImmutableSet<String> textContainers;
- PolicyFactory(ImmutableMap<String, ElementAndAttributePolicies> policies,
- ImmutableSet<String> textContainers) {
+ PolicyFactory(
+ ImmutableMap<String, ElementAndAttributePolicies> policies,
+ ImmutableSet<String> textContainers,
+ ImmutableMap<String, AttributePolicy> globalAttrPolicies) {
this.policies = policies;
this.textContainers = textContainers;
+ this.globalAttrPolicies = globalAttrPolicies;
}
/** Produces a sanitizer that emits tokens to {@code out}. */
@@ -129,6 +133,7 @@
public PolicyFactory and(PolicyFactory f) {
ImmutableMap.Builder<String, ElementAndAttributePolicies> b
= ImmutableMap.builder();
+ // Merge this and f into a map of element names to attribute policies.
for (Map.Entry<String, ElementAndAttributePolicies> e
: policies.entrySet()) {
String elName = e.getKey();
@@ -136,14 +141,21 @@
ElementAndAttributePolicies q = f.policies.get(elName);
if (q != null) {
p = p.and(q);
+ } else {
+ // Mix in any globals that are not already taken into account in this.
+ p = p.andGlobals(f.globalAttrPolicies);
}
b.put(elName, p);
}
+ // Handle keys that are in f but not in this.
for (Map.Entry<String, ElementAndAttributePolicies> e
: f.policies.entrySet()) {
String elName = e.getKey();
if (!policies.containsKey(elName)) {
- b.put(elName, e.getValue());
+ ElementAndAttributePolicies p = e.getValue();
+ // Mix in any globals that are not already taken into account in this.
+ p = p.andGlobals(f.globalAttrPolicies);
+ b.put(elName, p);
}
}
ImmutableSet<String> textContainers;
@@ -157,6 +169,30 @@
.addAll(f.textContainers)
.build();
}
- return new PolicyFactory(b.build(), textContainers);
+ ImmutableMap<String, AttributePolicy> allGlobalAttrPolicies;
+ if (f.globalAttrPolicies.isEmpty()) {
+ allGlobalAttrPolicies = this.globalAttrPolicies;
+ } else if (this.globalAttrPolicies.isEmpty()) {
+ allGlobalAttrPolicies = f.globalAttrPolicies;
+ } else {
+ ImmutableMap.Builder<String, AttributePolicy> ab = ImmutableMap.builder();
+ for (Map.Entry<String, AttributePolicy> e
+ : this.globalAttrPolicies.entrySet()) {
+ String attrName = e.getKey();
+ ab.put(
+ attrName,
+ AttributePolicy.Util.join(
+ e.getValue(), f.globalAttrPolicies.get(attrName)));
+ }
+ for (Map.Entry<String, AttributePolicy> e
+ : f.globalAttrPolicies.entrySet()) {
+ String attrName = e.getKey();
+ if (!this.globalAttrPolicies.containsKey(attrName)) {
+ ab.put(attrName, e.getValue());
+ }
+ }
+ allGlobalAttrPolicies = ab.build();
+ }
+ return new PolicyFactory(b.build(), textContainers, allGlobalAttrPolicies);
}
}
diff --git a/src/main/org/owasp/html/StylingPolicy.java b/src/main/org/owasp/html/StylingPolicy.java
index 1c448e7..95f9eda 100644
--- a/src/main/org/owasp/html/StylingPolicy.java
+++ b/src/main/org/owasp/html/StylingPolicy.java
@@ -216,4 +216,16 @@
}
return true;
}
+
+ @Override
+ public boolean equals(Object o) {
+ return o != null && getClass() == o.getClass()
+ && cssSchema.equals(((StylingPolicy) o).cssSchema);
+ }
+
+ @Override
+ public int hashCode() {
+ return cssSchema.hashCode();
+ }
+
}
diff --git a/src/tests/org/owasp/html/SanitizersTest.java b/src/tests/org/owasp/html/SanitizersTest.java
index 68f621c..f16e047 100644
--- a/src/tests/org/owasp/html/SanitizersTest.java
+++ b/src/tests/org/owasp/html/SanitizersTest.java
@@ -83,6 +83,18 @@
}
@Test
+ public static final void testStylesAndFormatting() {
+ PolicyFactory sanitizer = Sanitizers.FORMATTING
+ .and(Sanitizers.BLOCKS).and(Sanitizers.STYLES).and(Sanitizers.LINKS);
+ String input = "<span style=\"font-weight:bold;"
+ + "text-decoration:underline;background-color:yellow\""
+ + ">aaaaaaaaaaaaaaaaaaaaaaa</span>";
+ String got = sanitizer.sanitize(input);
+ String want = input;
+ assertEquals(want, got);
+ }
+
+ @Test
public static final void testAndIntersects() {
PolicyFactory restrictedLink = new HtmlPolicyBuilder()
.allowElements("a")
