Fix/safelist deep copy constructor (#1763)

Copies nested data structures instead of using a shallow copy to avoid unexpected state mutations after copy constructor usage.
jhy · May 15, 2022 · eaf5028 · eaf5028
1 parent 7261248
commit eaf5028
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 8 deletions.
diff --git a/src/main/java/org/jsoup/safety/Safelist.java b/src/main/java/org/jsoup/safety/Safelist.java
@@ -63,10 +63,10 @@ XSS attack examples (that jsoup will safegaurd against the default Cleaner and S
  </p>
  */
 public class Safelist {
-    private Set<TagName> tagNames; // tags allowed, lower case. e.g. [p, br, span]
-    private Map<TagName, Set<AttributeKey>> attributes; // tag -> attribute[]. allowed attributes [href] for a tag.
-    private Map<TagName, Map<AttributeKey, AttributeValue>> enforcedAttributes; // always set these attribute values
-    private Map<TagName, Map<AttributeKey, Set<Protocol>>> protocols; // allowed URL protocols for attributes
+    private final Set<TagName> tagNames; // tags allowed, lower case. e.g. [p, br, span]
+    private final Map<TagName, Set<AttributeKey>> attributes; // tag -> attribute[]. allowed attributes [href] for a tag.
+    private final Map<TagName, Map<AttributeKey, AttributeValue>> enforcedAttributes; // always set these attribute values
+    private final Map<TagName, Map<AttributeKey, Set<Protocol>>> protocols; // allowed URL protocols for attributes
     private boolean preserveRelativeLinks; // option to preserve relative links
 
     /**
@@ -203,9 +203,19 @@ public Safelist() {
     public Safelist(Safelist copy) {
         this();
         tagNames.addAll(copy.tagNames);
-        attributes.putAll(copy.attributes);
-        enforcedAttributes.putAll(copy.enforcedAttributes);
-        protocols.putAll(copy.protocols);
+        for (Map.Entry<TagName, Set<AttributeKey>> copyTagAttributes : copy.attributes.entrySet()) {
+            attributes.put(copyTagAttributes.getKey(), new HashSet<>(copyTagAttributes.getValue()));
+        }
+        for (Map.Entry<TagName, Map<AttributeKey, AttributeValue>> enforcedEntry : copy.enforcedAttributes.entrySet()) {
+            enforcedAttributes.put(enforcedEntry.getKey(), new HashMap<>(enforcedEntry.getValue()));
+        }
+        for (Map.Entry<TagName, Map<AttributeKey, Set<Protocol>>> protocolsEntry : copy.protocols.entrySet()) {
+            Map<AttributeKey, Set<Protocol>> attributeProtocolsCopy = new HashMap<>();
+            for (Map.Entry<AttributeKey, Set<Protocol>> attributeProtocols : protocolsEntry.getValue().entrySet()) {
+                attributeProtocolsCopy.put(attributeProtocols.getKey(), new HashSet<>(attributeProtocols.getValue()));
+            }
+            protocols.put(protocolsEntry.getKey(), attributeProtocolsCopy);
+        }
         preserveRelativeLinks = copy.preserveRelativeLinks;
     }
 
@@ -620,7 +630,7 @@ static Protocol valueOf(String value) {
     }
 
     abstract static class TypedValue {
-        private String value;
+        private final String value;
 
         TypedValue(String value) {
             Validate.notNull(value);

diff --git a/src/test/java/org/jsoup/safety/SafelistTest.java b/src/test/java/org/jsoup/safety/SafelistTest.java
@@ -0,0 +1,65 @@
+package org.jsoup.safety;
+
+import org.jsoup.nodes.Attribute;
+import org.jsoup.nodes.Attributes;
+import org.jsoup.nodes.Element;
+import org.jsoup.parser.Tag;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+public class SafelistTest {
+    private static final String TEST_TAG = "testTag";
+    private static final String TEST_ATTRIBUTE = "testAttribute";
+    private static final String TEST_SCHEME = "valid-scheme";
+    private static final String TEST_VALUE = TEST_SCHEME + "://testValue";
+
+    @Test
+    public void testCopyConstructor_noSideEffectOnTags() {
+        Safelist safelist1 = Safelist.none().addTags(TEST_TAG);
+        Safelist safelist2 = new Safelist(safelist1);
+        safelist1.addTags("invalidTag");
+
+        assertFalse(safelist2.isSafeTag("invalidTag"));
+    }
+
+    @Test
+    public void testCopyConstructor_noSideEffectOnAttributes() {
+        Safelist safelist1 = Safelist.none().addAttributes(TEST_TAG, TEST_ATTRIBUTE);
+        Safelist safelist2 = new Safelist(safelist1);
+        safelist1.addAttributes(TEST_TAG, "invalidAttribute");
+
+        assertFalse(safelist2.isSafeAttribute(TEST_TAG, null, new Attribute("invalidAttribute", TEST_VALUE)));
+    }
+
+    @Test
+    public void testCopyConstructor_noSideEffectOnEnforcedAttributes() {
+        Safelist safelist1 = Safelist.none().addEnforcedAttribute(TEST_TAG, TEST_ATTRIBUTE, TEST_VALUE);
+        Safelist safelist2 = new Safelist(safelist1);
+        safelist1.addEnforcedAttribute(TEST_TAG, TEST_ATTRIBUTE, "invalidValue");
+
+        for (Attribute enforcedAttribute : safelist2.getEnforcedAttributes(TEST_TAG)) {
+            assertNotEquals("invalidValue", enforcedAttribute.getValue());
+        }
+    }
+
+    @Test
+    public void testCopyConstructor_noSideEffectOnProtocols() {
+        final String invalidScheme = "invalid-scheme";
+        Safelist safelist1 = Safelist.none()
+                .addAttributes(TEST_TAG, TEST_ATTRIBUTE)
+                .addProtocols(TEST_TAG, TEST_ATTRIBUTE, TEST_SCHEME);
+        Safelist safelist2 = new Safelist(safelist1);
+        safelist1.addProtocols(TEST_TAG, TEST_ATTRIBUTE, invalidScheme);
+
+        Attributes attributes = new Attributes();
+        Attribute invalidAttribute = new Attribute(TEST_ATTRIBUTE, invalidScheme + "://someValue");
+        attributes.put(invalidAttribute);
+        Element invalidElement = new Element(Tag.valueOf(TEST_TAG), "", attributes);
+
+        assertFalse(safelist2.isSafeAttribute(TEST_TAG, invalidElement, invalidAttribute));
+    }
+
+
+}