Repository: jhy/jsoup (java)

Issue:
Copy constructor of Safelist shares state
The copy constructor of Safelist `public Safelist(Safelist copy)` does not deep copy the data structures as advertised in the JavaDoc but shares the nested data structures, e.g. `Map<TagName, Set<AttributeKey>>`. This causes unexpected mutation.

An example would be:
```java
Safelist safelist1 = Safelist.none().addAttributes("foo", "bar");
Safelist safelist2 = new Safelist(safelist1);
safelist1.addAttributes("foo", "baz");

final String html = "<foo bar=\"val\" baz=\"val\" />";
System.out.println(Jsoup.clean(html, safelist1));
System.out.println(Jsoup.clean(html, safelist2));
```
The second output should not contain the `baz` attribute but does.

Pull request:
Fix/safelist deep copy constructor
Fixes a bug where the copy constructor of Safelist would share state with its source Safelist which can unexpectedly mutate both safelists if the modifying operation is invoked on either.
In particular, this affected the allowed attributes, enforced attribute values and protocols.

fixes #1764

Patch:
diff --git a/pom.xml b/pom.xml
index 4328b127..22a9cecd 100644
--- a/pom.xml
+++ b/pom.xml
@@ -40,7 +40,7 @@
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-compiler-plugin</artifactId>
-        <version>3.8.1</version>
+        <version>3.10.1</version>
         <configuration>
           <source>1.8</source>
           <target>1.8</target>
@@ -57,7 +57,7 @@
         <!-- Ensure Java 8 and Android 10 API compatibility -->
         <groupId>org.codehaus.mojo</groupId>
         <artifactId>animal-sniffer-maven-plugin</artifactId>
-        <version>1.20</version>
+        <version>1.21</version>
         <executions>
           <execution>
             <id>animal-sniffer</id>
@@ -83,7 +83,7 @@
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-javadoc-plugin</artifactId>
-        <version>3.3.1</version>
+        <version>3.4.0</version>
         <configuration>
             <doclint>none</doclint>
             <source>8</source>
@@ -120,7 +120,7 @@
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-jar-plugin</artifactId>
-        <version>3.2.0</version>
+        <version>3.2.2</version>
         <configuration>
           <archive>
             <manifestEntries>
@@ -136,7 +136,7 @@
       <plugin>
         <groupId>org.apache.felix</groupId>
         <artifactId>maven-bundle-plugin</artifactId>
-        <version>5.1.3</version>
+        <version>5.1.6</version>
         <executions>
           <execution>
             <id>bundle-manifest</id>
@@ -166,7 +166,7 @@
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-surefire-plugin</artifactId>
-        <version>3.0.0-M5</version>
+        <version>3.0.0-M6</version>
         <configuration>
           <!-- smaller stack to find stack overflows -->
           <argLine>-Xss256k</argLine>
@@ -174,7 +174,7 @@
       </plugin>
       <plugin>
         <artifactId>maven-failsafe-plugin</artifactId>
-        <version>3.0.0-M5</version>
+        <version>3.0.0-M6</version>
         <executions>
           <execution>
             <goals>
@@ -192,7 +192,7 @@
         <!-- API version compat check - https://siom79.github.io/japicmp/ -->
         <groupId>com.github.siom79.japicmp</groupId>
         <artifactId>japicmp-maven-plugin</artifactId>
-        <version>0.15.4</version>
+        <version>0.15.7</version>
         <configuration>
           <!-- hard code previous version; can't detect when running stateless on build server -->
           <oldVersion>
@@ -296,7 +296,7 @@
         <plugins>
           <plugin>
             <artifactId>maven-failsafe-plugin</artifactId>
-            <version>3.0.0-M5</version>
+            <version>3.0.0-M6</version>
             <executions>
               <execution>
                 <goals>
@@ -325,7 +325,7 @@
       <!-- gson, to fetch entities from w3.org -->
       <groupId>com.google.code.gson</groupId>
       <artifactId>gson</artifactId>
-      <version>2.8.9</version>
+      <version>2.9.0</version>
       <scope>test</scope>
     </dependency>
 
@@ -333,7 +333,7 @@
       <!-- jetty for webserver integration tests. 9.x is last with Java7 support -->
       <groupId>org.eclipse.jetty</groupId>
       <artifactId>jetty-server</artifactId>
-      <version>9.4.44.v20210927</version>
+      <version>9.4.46.v20220331</version>
       <scope>test</scope>
     </dependency>
 
diff --git a/src/main/java/org/jsoup/safety/Safelist.java b/src/main/java/org/jsoup/safety/Safelist.java
index 76d56d2b..cc5604e5 100644
--- a/src/main/java/org/jsoup/safety/Safelist.java
+++ b/src/main/java/org/jsoup/safety/Safelist.java
@@ -63,10 +63,10 @@ import static org.jsoup.internal.Normalizer.lowerCase;
  </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 class 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 @@ public class Safelist {
     }
 
     abstract static class TypedValue {
-        private String value;
+        private final String value;
 
         TypedValue(String value) {
             Validate.notNull(value);


Test patch:
diff --git a/src/test/java/org/jsoup/safety/SafelistTest.java b/src/test/java/org/jsoup/safety/SafelistTest.java
new file mode 100644
index 00000000..8b1c1ffd
--- /dev/null
+++ 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));
+    }
+
+
+}