Merge pull request #5469 from Sage-Bionetworks/release-507

Merge Release 507 into develop
Sage-Bionetworks · Jul 26, 2024 · 7f6ceed · 7f6ceed
2 parents 1a67bc8 + 2d5f46b
commit 7f6ceed
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 4 deletions.
diff --git a/src/main/java/org/sagebionetworks/web/server/servlet/filter/HtmlInjectionFilter.java b/src/main/java/org/sagebionetworks/web/server/servlet/filter/HtmlInjectionFilter.java
@@ -1,5 +1,6 @@
 package org.sagebionetworks.web.server.servlet.filter;
 
+import com.google.gwt.safehtml.shared.SimpleHtmlSanitizer;
 import freemarker.template.Configuration;
 import freemarker.template.Template;
 import freemarker.template.TemplateException;
@@ -117,6 +118,24 @@ private Map<String, String> addDefaultsToDataModel(
     return dataModel;
   }
 
+  private Map<String, String> preprocess(Map<String, String> dataModel) {
+    String[] keysToClean = new String[] {
+      PAGE_TITLE_KEY,
+      PAGE_DESCRIPTION_KEY,
+      OG_PAGE_TITLE_KEY,
+      OG_PAGE_DESCRIPTION_KEY,
+    };
+    Map<String, String> sanitizedModel = new HashMap<>();
+    sanitizedModel.putAll(dataModel);
+    for (String key : keysToClean) {
+      String sanitizedValue = SimpleHtmlSanitizer
+        .sanitizeHtml(dataModel.get(key))
+        .asString();
+      sanitizedModel.put(key, sanitizedValue);
+    }
+    return sanitizedModel;
+  }
+
   public static boolean isGWTPlace(String targetPath) {
     //  if it contains a ':', then assume it is a GWT place with token
     return targetPath.contains(":");
@@ -355,7 +374,8 @@ protected void doFilterInternal(
         StringWriter stringWriter = new StringWriter();
         try {
           addDefaultsToDataModel(dataModel);
-          portalHtmlTemplate.process(dataModel, stringWriter);
+          Map<String, String> sanitizedModel = preprocess(dataModel);
+          portalHtmlTemplate.process(sanitizedModel, stringWriter);
         } catch (TemplateException e) {
           e.printStackTrace();
         }

diff --git a/src/test/java/org/sagebionetworks/web/unitserver/filter/HtmlInjectionFilterTest.java b/src/test/java/org/sagebionetworks/web/unitserver/filter/HtmlInjectionFilterTest.java
@@ -120,7 +120,8 @@ public class HtmlInjectionFilterTest {
   public static final String TEAM_NAME = "the greatest team name";
   public static final String TEAM_DESCRIPTION = "an average team description";
 
-  public static final String DISCUSSION_THREAD_TITLE = "my test thread title";
+  public static final String DISCUSSION_THREAD_TITLE =
+    "</head><script>window.alert('pwned')</script>my test thread title";
 
   public static final String WIKI_PAGE_MARKDOWN =
     "This is a description of the object";
@@ -278,7 +279,7 @@ public void testSynapseEntityPageWithoutWikiDescriptionAsHuman()
     assertFalse(outputString.contains(WIKI_PAGE_MARKDOWN));
     assertTrue(
       outputString.contains(
-        "'my mock entity' (Synapse ID: syn12345) is a file on Synapse."
+        "&#39;my mock entity&#39; (Synapse ID: syn12345) is a file on Synapse."
       )
     );
     assertTrue(outputString.contains(SYNAPSE_PLATFORM_DESCRIPTION));
@@ -304,7 +305,12 @@ public void testDiscussionThreadPage()
 
     verify(mockPrintWriter).print(stringCaptor.capture());
     String outputString = stringCaptor.getValue();
-    assertTrue(outputString.contains(DISCUSSION_THREAD_TITLE));
+    // verify dangerous value has been html sanitized
+    assertTrue(
+      outputString.contains(
+        "&lt;/head&gt;&lt;script&gt;window.alert(&#39;pwned&#39;)&lt;/script&gt;my test thread title"
+      )
+    );
   }
 
   @Test