Don't allow illegal characters in XmlAttrFilter

jasmith-hs · jasmith-hs · commit c1403a0f8433 · 2024-05-07T16:18:48.000-04:00
Fixes GHSA-h75v-3vvj-5mfj
diff --git a/src/main/java/com/hubspot/jinjava/lib/filter/XmlAttrFilter.java b/src/main/java/com/hubspot/jinjava/lib/filter/XmlAttrFilter.java
@@ -8,6 +8,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.regex.Pattern;
 import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringEscapeUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -37,6 +38,11 @@
 )
 public class XmlAttrFilter implements Filter {
 
+  // See https://html.spec.whatwg.org/#attribute-name-state Don't allow characters that would change the attribute name/value state
+  private static final Pattern ILLEGAL_ATTRIBUTE_KEY_PATTERN = Pattern.compile(
+    "[\\s/>=]"
+  );
+
   @Override
   public String getName() {
     return "xmlattr";
@@ -53,6 +59,11 @@ public Object filter(Object var, JinjavaInterpreter interpreter, String... args)
     List<String> attrs = new ArrayList<>();
 
     for (Map.Entry<String, Object> entry : dict.entrySet()) {
+      if (ILLEGAL_ATTRIBUTE_KEY_PATTERN.matcher(entry.getKey()).find()) {
+        throw new IllegalArgumentException(
+          String.format("Invalid character in attribute name: %s", entry.getKey())
+        );
+      }
       attrs.add(
         new StringBuilder(entry.getKey())
           .append("=\"")
diff --git a/src/test/java/com/hubspot/jinjava/lib/filter/XmlAttrFilterTest.java b/src/test/java/com/hubspot/jinjava/lib/filter/XmlAttrFilterTest.java
@@ -2,8 +2,11 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 
+import com.google.common.collect.ImmutableList;
 import com.hubspot.jinjava.BaseJinjavaTest;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import org.jsoup.Jsoup;
 import org.jsoup.nodes.Document;
@@ -27,4 +30,27 @@ public void testXmlAttr() {
     assertThat(dom.select("ul").attr("id")).isEqualTo("list-42");
     assertThat(dom.select("ul").attr("missing")).isEmpty();
   }
+
+  @Test
+  public void itDoesNotAllowInvalidKeys() {
+    List<String> invalidStrings = ImmutableList.of("\t", "\n", "\f", " ", "/", ">", "=");
+    invalidStrings.forEach(invalidString ->
+      assertThat(
+        jinjava
+          .renderForResult(
+            String.format("{{ {'%s': 'foo'}|xmlattr }}", invalidString),
+            Collections.emptyMap()
+          )
+          .getErrors()
+      )
+        .matches(templateErrors ->
+          templateErrors.size() == 1 &&
+          templateErrors
+            .get(0)
+            .getException()
+            .getCause()
+            .getCause() instanceof IllegalArgumentException
+        )
+    );
+  }
 }
