fix(file upload) #27047 : Binary File upload to a content type fails …

…(Multi-part upload)
dotCMS · Dec 19, 2023 · 812619d · 812619d
1 parent 7f10b93
commit 812619d
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 17 deletions.
diff --git a/dotCMS/src/curl-test/TempAPI.postman_collection.json b/dotCMS/src/curl-test/TempAPI.postman_collection.json
@@ -1,9 +1,9 @@
 {
 	"info": {
-		"_postman_id": "cf0ae976-98a7-4e35-a637-699ebbb6198a",
+		"_postman_id": "93b8fdd2-a647-40ad-9fb3-a27309267ba3",
 		"name": "TempAPI",
 		"schema": "https://schema.getpostman.com/json/collection/v2.1.0/collection.json",
-		"_exporter_id": "781456"
+		"_exporter_id": "5403727"
 	},
 	"item": [
 		{
@@ -104,6 +104,77 @@
 			},
 			"response": []
 		},
+		{
+			"name": "Upload file greater than 50MB",
+			"event": [
+				{
+					"listen": "test",
+					"script": {
+						"exec": [
+							"pm.test(\"HTTP Status code must be 200\", function () {",
+							"    pm.response.to.have.status(200);",
+							"});",
+							"",
+							"pm.test(\"Check that the uploaded file has the expected properties\", function () {",
+							"    var jsonData = pm.response.json().tempFiles[0];",
+							"    pm.expect(jsonData.fileName).to.eql('test-video-71megabytes.mov', 'The uploaded file name does not match the file name in the request');",
+							"    pm.expect(jsonData.length).to.be.gte(50000000, 'The uploaded file is NOT greater than the expected 50MB');",
+							"});",
+							""
+						],
+						"type": "text/javascript"
+					}
+				}
+			],
+			"request": {
+				"auth": {
+					"type": "basic",
+					"basic": [
+						{
+							"key": "password",
+							"value": "admin",
+							"type": "string"
+						},
+						{
+							"key": "username",
+							"value": "admin@dotcms.com",
+							"type": "string"
+						}
+					]
+				},
+				"method": "POST",
+				"header": [
+					{
+						"key": "Origin",
+						"value": "{{serverURL}}",
+						"type": "text"
+					}
+				],
+				"body": {
+					"mode": "formdata",
+					"formdata": [
+						{
+							"key": "file",
+							"type": "file",
+							"src": "/Users/jcastro-2022/dotCMS/git/dev/third-branch/core/dotCMS/src/curl-test/resources/test-video-71megabytes.mov"
+						}
+					]
+				},
+				"url": {
+					"raw": "{{serverURL}}/api/v1/temp",
+					"host": [
+						"{{serverURL}}"
+					],
+					"path": [
+						"api",
+						"v1",
+						"temp"
+					]
+				},
+				"description": "The `MultiPartSecurityRequestWrapper` must be able to handle the upload of safe harmless files of any size. However, by default, if such a file is greater than 50MB, it must be cached to disk. This feature must always work."
+			},
+			"response": []
+		},
 		{
 			"name": "Given_AnonUser_Unable_To_Upload",
 			"event": [

diff --git a/dotCMS/src/curl-test/resources/test-video-71megabytes.mov b/dotCMS/src/curl-test/resources/test-video-71megabytes.mov
diff --git a/dotCMS/src/main/java/com/dotcms/security/multipart/MultiPartSecurityRequestWrapper.java b/dotCMS/src/main/java/com/dotcms/security/multipart/MultiPartSecurityRequestWrapper.java
@@ -1,10 +1,17 @@
 package com.dotcms.security.multipart;
 
+import com.dotcms.exception.ExceptionUtil;
 import com.dotcms.util.SecurityUtils;
 import com.dotmarketing.util.Config;
 import com.dotmarketing.util.ConfigUtils;
 import com.dotmarketing.util.Logger;
-import com.google.common.collect.ImmutableList;
+import io.vavr.Lazy;
+import org.apache.commons.io.IOUtils;
+
+import javax.servlet.ReadListener;
+import javax.servlet.ServletInputStream;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
 import java.io.BufferedInputStream;
 import java.io.BufferedReader;
 import java.io.ByteArrayInputStream;
@@ -16,12 +23,6 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.List;
-import javax.servlet.ReadListener;
-import javax.servlet.ServletInputStream;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletRequestWrapper;
-import org.apache.commons.io.IOUtils;
 
 /**
  * This request wrapper prevents DoS attacks on multipart requests
@@ -36,28 +37,28 @@ public class MultiPartSecurityRequestWrapper extends HttpServletRequestWrapper {
 
     private static final SecurityUtils securityUtils = new SecurityUtils();
 
-    // if greater than 50MB, cache to disk
-    private static final long CACHE_IF_LARGER = Config.getIntProperty("MULTI_PART_CACHE_IF_LARGER",1024*1000*50);
+    /** By default, we'll always cache files greater than 50MB to disk */
+    private static final Lazy<Long> CACHE_IF_LARGER = Lazy.of(() -> Config.getLongProperty("MULTI_PART_CACHE_IF_LARGER",1024*1000*50));
 
     boolean shouldCacheToDisk(final HttpServletRequest request) {
 
         final int contentLength = request.getContentLength();
-        return contentLength <= 0 || contentLength > CACHE_IF_LARGER;
+        return contentLength <= 0 || contentLength > CACHE_IF_LARGER.get();
     }
 
     public MultiPartSecurityRequestWrapper(final HttpServletRequest request) throws IOException {
 
         super(request);
 
-        Logger.debug(this, ()-> "Creating a MultiPartSecurityRequestWrapper");
+        Logger.debug(this, "Creating a MultiPartSecurityRequestWrapper");
         if(this.shouldCacheToDisk(request)) {
 
-            Logger.debug(this, ()-> "Should Cache To Disk...");
+            Logger.debug(this, String.format("File being uploaded is greater than %d bytes. It must be cached to disk...", CACHE_IF_LARGER.get()));
             this.body = null;
-            final Path tempFilePath = Files.createTempFile(Path.of(ConfigUtils.getAssetTempPath()),"multipartSec", ".tmp");
+            final Path tempFilePath = Files.createTempFile(Path.of(ConfigUtils.getAssetTempPath(),File.separator),"multipartSec", ".tmp");
             this.tmpFile = tempFilePath.toFile();
             // security demands we add this check here
-            if (tmpFile.getCanonicalPath().startsWith(tmpdir.getCanonicalPath())) {
+            if (tmpFile.getCanonicalPath().startsWith(new File(ConfigUtils.getAssetTempPath()).getCanonicalPath())) {
                 try (final OutputStream outputStream = Files.newOutputStream(tempFilePath)) {
                     IOUtils.copy(request.getInputStream(), outputStream);
                     this.checkFile(tmpFile);
@@ -165,8 +166,12 @@ private void checkMemory(final byte[] bodyBytes) throws IOException {
     private void checkFile(final File tmpFile) throws IOException {
         //Even though this is might seem redundant. Security demands we check paths everytime we manipulate file paths
         if (tmpFile.getCanonicalPath().startsWith(tmpdir.getCanonicalPath())) {
-            try (InputStream in = new BufferedInputStream(Files.newInputStream(tmpFile.toPath()))) {
+            try (final InputStream in = new BufferedInputStream(Files.newInputStream(tmpFile.toPath()))) {
                 checkSecurityInputStream(in);
+            } catch (final Exception e) {
+                Logger.warn(this.getClass(), String.format("Could not check the path for file " +
+                        "'%s': %s", tmpFile.getCanonicalPath(), ExceptionUtil.getErrorMessage(e)), e);
+                throw e;
             }
         }
     }