fix: Do not sanitize plain strings and do not strip table tags (#633)

* Do not sanitize plain strings and preserve table tags in sanitation * Test that plain string are not touched * Add unit test for testing that table is preserved in sanitation
vaadin · Mar 21, 2022 · 141d83d · 141d83d
1 parent f21d9c3
commit 141d83d
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 10 deletions.
diff --git a/addon/src/main/java/com/vaadin/addon/charts/model/serializers/StringSerializer.java b/addon/src/main/java/com/vaadin/addon/charts/model/serializers/StringSerializer.java
@@ -18,9 +18,11 @@
  */
 
 import java.io.IOException;
+import java.util.regex.Pattern;
 
 import org.jsoup.Jsoup;
-import org.jsoup.safety.Whitelist;
+import org.jsoup.nodes.Document;
+import org.jsoup.safety.Safelist;
 
 import com.fasterxml.jackson.core.JsonGenerator;
 import com.fasterxml.jackson.core.JsonProcessingException;
@@ -56,7 +58,13 @@ public void serialize(String value, JsonGenerator gen,
      */
     private boolean shouldSanitize(String value, JsonGenerator gen) {
         return value != null && gen.getOutputContext().inObject()
-                && !gen.getOutputContext().getCurrentName().startsWith("_fn_");
+                && !gen.getOutputContext().getCurrentName().startsWith("_fn_")
+                && isHtml(value);
+    }
+
+    private boolean isHtml(String html) {
+        Pattern htmlPattern = Pattern.compile(".*\\<[^>]+>.*", Pattern.DOTALL);
+        return htmlPattern.matcher(html).matches();        
     }
 
     /*
@@ -66,11 +74,10 @@ private boolean shouldSanitize(String value, JsonGenerator gen) {
      * SNYK-JS-HIGHCHARTS-571995Ï
      */
     private String sanitize(String html) {
-        return Jsoup.clean(html,
-                Whitelist.basic().addTags("img", "h1", "h2", "h3", "s")
-                        .addAttributes("img", "align", "alt", "height", "src",
-                                "title", "width")
-                        .addAttributes(":all", "style")
-                        .addProtocols("img", "src", "data"));
+        Safelist safelist = Safelist.relaxed()
+                .addAttributes(":all", "style")
+                .addEnforcedAttribute("a", "rel", "nofollow");
+        String sanitized = Jsoup.clean(html, "", safelist, new Document.OutputSettings().prettyPrint(false));
+        return sanitized;
     }
 }
diff --git a/...c/test/java/com/vaadin/addon/charts/model/junittests/UnsafeHTMLJSONSerializationTest.java b/...c/test/java/com/vaadin/addon/charts/model/junittests/UnsafeHTMLJSONSerializationTest.java
@@ -6,6 +6,7 @@
 import org.junit.Test;
 
 import com.vaadin.addon.charts.model.Configuration;
+import com.vaadin.addon.charts.model.Tooltip;
 
 /**
  * Tests for the sanitization of unsafe HTML in JSON serialization in
@@ -14,14 +15,36 @@
  */
 public class UnsafeHTMLJSONSerializationTest {
 
+    @Test
+    public void configurationJSONSerialization_tablePreserved() {
+        Configuration conf = new Configuration();
+        Tooltip tooltip = new Tooltip();
+        tooltip.setUseHTML(true);
+        tooltip.setPointFormat("<table><tbody><tr><td>{series.name}</td></tr><tr><td>{point.y}</td></tr></tbody></table>");
+        conf.setTooltip(tooltip);
+            assertEquals(
+                "{\"tooltip\":{\"pointFormat\":\"<table><tbody><tr><td>{series.name}</td></tr><tr><td>{point.y}</td></tr></tbody></table>\",\"useHTML\":true},\"plotOptions\":{},\"series\":[],\"exporting\":{\"enabled\":false}}",
+                toJSON(conf));
+    }
+
+    @Test
+    public void configurationJSONSerialization_plainString_notTouched() {
+        Configuration conf = new Configuration();
+        conf.setTitle(
+                "This & that is this & that");
+            assertEquals(
+                "{\"title\":{\"text\":\"This & that is this & that\"},\"plotOptions\":{},\"series\":[],\"exporting\":{\"enabled\":false}}",
+                toJSON(conf));
+    }
+
     @Test
     public void configurationJSONSerialization_unsafeTitle_unsafeHTMLNotSerialized() {
         Configuration conf = new Configuration();
         conf.setTitle(
                 "1. JavaScript injected in plain href: <a href=\'#\";alert(\"XSS 1\");var fake=\"\'>Click here</a><br> 2. JavaScript injected as href=\"javascript:...\": <a href=\"javascript:alert(\'XSS 2\')\">Click here</a><br> Source: <a href=\"http://thebulletin.metapress.com/content/c4120650912x74k7/fulltext.pdf\">thebulletin.metapress.com</a>");
 
         assertEquals(
-                "{\"title\":{\"text\":\"1. JavaScript injected in plain href: <a rel=\\\"nofollow\\\">Click here</a>\\n<br> 2. JavaScript injected as href=\\\"javascript:...\\\": <a rel=\\\"nofollow\\\">Click here</a>\\n<br> Source: <a href=\\\"http://thebulletin.metapress.com/content/c4120650912x74k7/fulltext.pdf\\\" rel=\\\"nofollow\\\">thebulletin.metapress.com</a>\"},\"plotOptions\":{},\"series\":[],\"exporting\":{\"enabled\":false}}",
+                "{\"title\":{\"text\":\"1. JavaScript injected in plain href: <a rel=\\\"nofollow\\\">Click here</a><br> 2. JavaScript injected as href=\\\"javascript:...\\\": <a rel=\\\"nofollow\\\">Click here</a><br> Source: <a href=\\\"http://thebulletin.metapress.com/content/c4120650912x74k7/fulltext.pdf\\\" rel=\\\"nofollow\\\">thebulletin.metapress.com</a>\"},\"plotOptions\":{},\"series\":[],\"exporting\":{\"enabled\":false}}",
                 toJSON(conf));
     }
 
@@ -31,7 +54,7 @@ public void configurationJSONSerialization_unsafeSubTitle_unsafeHTMLNotSerialize
         conf.setSubTitle(
                 "1. JavaScript injected in plain href: <a href=\'#\";alert(\"XSS 1\");var fake=\"\'>Click here</a><br> 2. JavaScript injected as href=\"javascript:...\": <a href=\"javascript:alert(\'XSS 2\')\">Click here</a><br> Source: <a href=\"http://thebulletin.metapress.com/content/c4120650912x74k7/fulltext.pdf\">thebulletin.metapress.com</a>");
         assertEquals(
-                "{\"subtitle\":{\"text\":\"1. JavaScript injected in plain href: <a rel=\\\"nofollow\\\">Click here</a>\\n<br> 2. JavaScript injected as href=\\\"javascript:...\\\": <a rel=\\\"nofollow\\\">Click here</a>\\n<br> Source: <a href=\\\"http://thebulletin.metapress.com/content/c4120650912x74k7/fulltext.pdf\\\" rel=\\\"nofollow\\\">thebulletin.metapress.com</a>\"},\"plotOptions\":{},\"series\":[],\"exporting\":{\"enabled\":false}}",
+                "{\"subtitle\":{\"text\":\"1. JavaScript injected in plain href: <a rel=\\\"nofollow\\\">Click here</a><br> 2. JavaScript injected as href=\\\"javascript:...\\\": <a rel=\\\"nofollow\\\">Click here</a><br> Source: <a href=\\\"http://thebulletin.metapress.com/content/c4120650912x74k7/fulltext.pdf\\\" rel=\\\"nofollow\\\">thebulletin.metapress.com</a>\"},\"plotOptions\":{},\"series\":[],\"exporting\":{\"enabled\":false}}",
                 toJSON(conf));
     }
 }