From 4e68bcc9b222a2e1e7914158381b8720bdb6b2d5 Mon Sep 17 00:00:00 2001 From: cschwier Date: Thu, 5 May 2022 18:16:02 +0200 Subject: [PATCH 1/3] Add test case for shared state of Safelist's copy constructor --- .../java/org/jsoup/safety/SafelistTest.java | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 src/test/java/org/jsoup/safety/SafelistTest.java 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 0000000000..8b1c1ffd09 --- /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)); + } + + +} From aebe4a2110008a903f64eabb412ec8104ce880e1 Mon Sep 17 00:00:00 2001 From: cschwier Date: Thu, 5 May 2022 15:29:46 +0200 Subject: [PATCH 2/3] Fix copy constructor of Safelist Copies nested data structures instead of using a shallow copy to avoid unexpected state mutations after copy constructor usage. --- src/main/java/org/jsoup/safety/Safelist.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jsoup/safety/Safelist.java b/src/main/java/org/jsoup/safety/Safelist.java index 76d56d2be2..e27a6bec6a 100644 --- a/src/main/java/org/jsoup/safety/Safelist.java +++ b/src/main/java/org/jsoup/safety/Safelist.java @@ -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> copyTagAttributes : copy.attributes.entrySet()) { + attributes.put(copyTagAttributes.getKey(), new HashSet<>(copyTagAttributes.getValue())); + } + for (Map.Entry> enforcedEntry : copy.enforcedAttributes.entrySet()) { + enforcedAttributes.put(enforcedEntry.getKey(), new HashMap<>(enforcedEntry.getValue())); + } + for (Map.Entry>> protocolsEntry : copy.protocols.entrySet()) { + Map> attributeProtocolsCopy = new HashMap<>(); + for (Map.Entry> attributeProtocols : protocolsEntry.getValue().entrySet()) { + attributeProtocolsCopy.put(attributeProtocols.getKey(), new HashSet<>(attributeProtocols.getValue())); + } + protocols.put(protocolsEntry.getKey(), attributeProtocolsCopy); + } preserveRelativeLinks = copy.preserveRelativeLinks; } From 8f1820b8d98bf213c2b68f8a5d5a33d0824436e8 Mon Sep 17 00:00:00 2001 From: cschwier Date: Thu, 5 May 2022 18:23:27 +0200 Subject: [PATCH 3/3] Remove unnecessary mutability of Safelist interna Makes a couple of properties final which shouldn't be replaced to further reduce the risk of unexpected mutation. --- src/main/java/org/jsoup/safety/Safelist.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/jsoup/safety/Safelist.java b/src/main/java/org/jsoup/safety/Safelist.java index e27a6bec6a..cc5604e5e3 100644 --- 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

*/ public class Safelist { - private Set tagNames; // tags allowed, lower case. e.g. [p, br, span] - private Map> attributes; // tag -> attribute[]. allowed attributes [href] for a tag. - private Map> enforcedAttributes; // always set these attribute values - private Map>> protocols; // allowed URL protocols for attributes + private final Set tagNames; // tags allowed, lower case. e.g. [p, br, span] + private final Map> attributes; // tag -> attribute[]. allowed attributes [href] for a tag. + private final Map> enforcedAttributes; // always set these attribute values + private final Map>> protocols; // allowed URL protocols for attributes private boolean preserveRelativeLinks; // option to preserve relative links /** @@ -630,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);