MediaFileServlet: Force Content-Disposition=attachment header for SVG…

… files served by MediaFileServlet. Also ensure URLs to SVG images are always handled by MediaFileServlet.
wcm-io · Aug 17, 2022 · d72c596 · d72c596
1 parent a875253
commit d72c596
Show file tree

Hide file tree

Showing 5 changed files with 47 additions and 11 deletions.
diff --git a/changes.xml b/changes.xml
@@ -23,6 +23,12 @@
     xsi:schemaLocation="http://maven.apache.org/changes/1.0.0 http://maven.apache.org/plugins/maven-changes-plugin/xsd/changes-1.0.0.xsd">
   <body>
 
+    <release version="1.14.6" date="not released">
+      <action type="update" dev="sseifert">
+        MediaFileServlet: Force Content-Disposition=attachment header for SVG files served by MediaFileServlet. Also ensure URLs to SVG images are always handled by MediaFileServlet.
+      </action>
+    </release>
+
     <release version="1.14.4" date="2022-06-16">
       <action type="update" dev="sseifert">
         Switch to AEM 6.5.7 as minimum version.

diff --git a/src/main/java/io/wcm/handler/media/impl/AbstractMediaFileServlet.java b/src/main/java/io/wcm/handler/media/impl/AbstractMediaFileServlet.java
@@ -158,22 +158,31 @@ protected void sendBinaryData(byte[] binaryData, String contentType,
       // Overwrite MIME type with one suited for downloads
       response.setContentType(ContentType.DOWNLOAD);
 
-      // Construct disposition header
-      StringBuilder dispositionHeader = new StringBuilder("attachment;");
-      String suffix = request.getRequestPathInfo().getSuffix();
-      suffix = StringUtils.stripStart(suffix, "/");
-      if (StringUtils.isNotEmpty(suffix)) {
-        dispositionHeader.append("filename=\"").append(suffix).append('\"');
-      }
-
-      response.setHeader(HEADER_CONTENT_DISPOSITION, dispositionHeader.toString());
+      // set content disposition header to file name from suffix
+      setContentDispositionAttachmentHeader(request, response);
+    }
+
+    // special handling for SVG images which are not treated as download:
+    // force content disposition header to prevent stored XSS attack with malicious JavaScript in SVG file
+    else if (StringUtils.equals(contentType, ContentType.SVG)) {
+      setContentDispositionAttachmentHeader(request, response);
     }
 
     // write binary data
     OutputStream out = response.getOutputStream();
     out.write(binaryData);
     out.flush();
+  }
 
+  private void setContentDispositionAttachmentHeader(SlingHttpServletRequest request, SlingHttpServletResponse response) {
+    // Construct disposition header
+    StringBuilder dispositionHeader = new StringBuilder("attachment;");
+    String suffix = request.getRequestPathInfo().getSuffix();
+    suffix = StringUtils.stripStart(suffix, "/");
+    if (StringUtils.isNotEmpty(suffix)) {
+      dispositionHeader.append("filename=\"").append(suffix).append('\"');
+    }
+    response.setHeader(HEADER_CONTENT_DISPOSITION, dispositionHeader.toString());
   }
 
 }
diff --git a/src/main/java/io/wcm/handler/mediasource/dam/impl/RenditionMetadata.java b/src/main/java/io/wcm/handler/mediasource/dam/impl/RenditionMetadata.java
@@ -177,6 +177,10 @@ public void setMediaFormat(MediaFormat mediaFormat) {
           + "." + MediaFileServlet.SELECTOR_DOWNLOAD
           + "." + MediaFileServlet.EXTENSION, getFileName(contentDispositionAttachment));
     }
+    else if (MediaFileType.isVectorImage(getFileExtension())) {
+      return RenditionMetadata.buildMediaPath(getRendition().getPath() + "." + MediaFileServlet.SELECTOR
+          + "." + MediaFileServlet.EXTENSION, getFileName(contentDispositionAttachment));
+    }
     else if (MediaFileType.isBrowserImage(getFileExtension()) || !MediaFileType.isImage(getFileExtension())) {
       // use "deep URL" to reference rendition directly
       // do not use Asset URL for original rendition because it creates conflicts for dispatcher cache (filename vs. directory for asset resource name)

diff --git a/src/test/java/io/wcm/handler/media/impl/MediaFileServletTest.java b/src/test/java/io/wcm/handler/media/impl/MediaFileServletTest.java
@@ -20,6 +20,7 @@
 package io.wcm.handler.media.impl;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 
 import javax.servlet.http.HttpServletResponse;
 
@@ -37,6 +38,7 @@
 class MediaFileServletTest {
 
   private static final long EXPECTED_CONTENT_LENGTH = 15471;
+  private static final long EXPECTED_CONTENT_LENGTH_SVG = 718;
 
   private final AemContext context = AppAemContext.newAemContext();
 
@@ -56,6 +58,21 @@ void testGet() throws Exception {
     assertEquals(ContentType.JPEG, context.response().getContentType());
     assertEquals(EXPECTED_CONTENT_LENGTH, context.response().getOutput().length);
     assertEquals(EXPECTED_CONTENT_LENGTH, context.response().getContentLength());
+    assertNull(context.response().getHeader(AbstractMediaFileServlet.HEADER_CONTENT_DISPOSITION));
+  }
+
+  @Test
+  void testGet_SVG() throws Exception {
+    context.currentResource(context.load().binaryFile("/filetype/sample.svg", "/content/sample.svg"));
+
+    underTest.service(context.request(), context.response());
+
+    assertEquals(HttpServletResponse.SC_OK, context.response().getStatus());
+    assertEquals(ContentType.SVG, context.response().getContentType());
+    assertEquals(EXPECTED_CONTENT_LENGTH_SVG, context.response().getOutput().length);
+    assertEquals(EXPECTED_CONTENT_LENGTH_SVG, context.response().getContentLength());
+    // forced content disposition header for SVG to prevent stored XSS
+    assertEquals("attachment;", context.response().getHeader(AbstractMediaFileServlet.HEADER_CONTENT_DISPOSITION));
   }
 
   @Test

diff --git a/src/test/java/io/wcm/handler/media/impl/MediaHandlerImplImageFileTypesEnd2EndTest.java b/src/test/java/io/wcm/handler/media/impl/MediaHandlerImplImageFileTypesEnd2EndTest.java
@@ -318,7 +318,7 @@ void testFileUpload_TIFF_AutoCrop() {
   void testAsset_SVG_Original() {
     Asset asset = createSampleAsset("/filetype/sample.svg", ContentType.SVG);
     buildAssertMedia(asset, 100, 50,
-        "/content/dam/sample.svg/_jcr_content/renditions/original./sample.svg",
+        "/content/dam/sample.svg/_jcr_content/renditions/original.media_file.file/sample.svg",
         ContentType.SVG);
   }
 
@@ -334,7 +334,7 @@ void testAsset_SVG_Original_ContentDisposition() {
   void testAsset_SVG_Rescale() {
     Asset asset = createSampleAsset("/filetype/sample.svg", ContentType.SVG);
     buildAssertMedia_Rescale(asset, 80, 40,
-        "/content/dam/sample.svg/_jcr_content/renditions/original./sample.svg",
+        "/content/dam/sample.svg/_jcr_content/renditions/original.media_file.file/sample.svg",
         ContentType.SVG);
   }