Preserve order of rel attributes (#336)

OWASP · Jan 3, 2025 · 5ffada8 · 5ffada8
1 parent f729a08
commit 5ffada8
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 5 deletions.
diff --git a/java10-shim/src/main/java/org/owasp/shim/ForJava9AndLater.java b/java10-shim/src/main/java/org/owasp/shim/ForJava9AndLater.java
@@ -62,6 +62,6 @@ final class ForJava9AndLater extends Java8Shim {
     }
 
     @Override public <T> Set<T> setCopyOf(Collection<? extends T> c) {
-        return Set.copyOf(c);
+        return Collections.unmodifiableSet(new LinkedHashSet<>(c));
     }
 }
diff --git a/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java b/owasp-java-html-sanitizer/src/main/java/org/owasp/html/HtmlPolicyBuilder.java
@@ -33,6 +33,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -428,7 +429,7 @@ public HtmlPolicyBuilder requireRelNofollowOnLinks() {
   public HtmlPolicyBuilder requireRelsOnLinks(String... linkValues) {
     this.invalidateCompiledState();
     if (this.extraRelsForLinks == null) {
-      this.extraRelsForLinks = new HashSet<>();
+      this.extraRelsForLinks = new LinkedHashSet<>();
     }
     for (String linkValue : linkValues) {
       linkValue = HtmlLexer.canonicalKeywordAttributeValue(linkValue);
@@ -1112,8 +1113,8 @@ static final class JoinRelsOnLinksPolicies
 
     public JoinableElementPolicy join(
         Iterable<? extends JoinableElementPolicy> toJoin) {
-      Set<String> extra = new HashSet<>();
-      Set<String> skip = new HashSet<>();
+      Set<String> extra = new LinkedHashSet<>();
+      Set<String> skip = new LinkedHashSet<>();
       for (JoinableElementPolicy ep : toJoin) {
         RelsOnLinksPolicy p = (RelsOnLinksPolicy) ep;
         extra.addAll(p.extra);

diff --git a/owasp-java-html-sanitizer/src/test/java/org/owasp/html/SanitizersTest.java b/owasp-java-html-sanitizer/src/test/java/org/owasp/html/SanitizersTest.java
@@ -252,6 +252,30 @@ public static final void testLinks() {
         s.sanitize("<a name=\"header\" id=\"header\">Header text</a>"));
   }
 
+  @Test
+  public static final void testLinksRelAttributeAdditionsOrder() {
+    // Issue 336.
+    PolicyFactory pf = Sanitizers.LINKS.and(
+            new HtmlPolicyBuilder()
+            .allowElements("a")
+            .requireRelsOnLinks("noopener", "noreferrer")
+            .toFactory());
+
+    assertEquals(
+            "<a href=\"foo.html\" rel=\"nofollow noopener noreferrer\">Link text</a>",
+            pf.sanitize("<a href=\"foo.html\">Link text</a>"));
+
+    pf = Sanitizers.LINKS.and(
+            new HtmlPolicyBuilder()
+                    .allowElements("a")
+                    .requireRelsOnLinks("noreferrer", "noopener")
+                    .toFactory());
+
+    assertEquals(
+            "<a href=\"foo.html\" rel=\"nofollow noreferrer noopener\">Link text</a>",
+            pf.sanitize("<a href=\"foo.html\">Link text</a>"));
+  }
+
   @Test
   public static final void testExplicitlyAllowedProtocolsAreCaseInsensitive() {
     // Issue 24.
@@ -552,7 +576,7 @@ public static final void testStyleGlobally() {
     String want = "<h1 style=\"color:green\">This is some green text</h1>";
     assertEquals(want, policyBuilder.sanitize(input));
   }
-  
+
   static int fac(int n) {
     int ifac = 1;
     for (int i = 1; i <= n; ++i) {