From f913c407e947db0812d3e3ea8116675de0d1b82b Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 11 Feb 2024 10:38:22 +0100 Subject: [PATCH] WW-5388 Fixes a few issues in Servlet 6 file upload usage - removes duplicated validation messages where both interceptors have been used - defines a proper error message on a general file upload exception - removes generic definition in tests --- .../multipart/AbstractMultiPartRequest.java | 46 +++-- .../JakartaStreamMultiPartRequest.java | 22 ++- .../AbstractFileUploadInterceptor.java | 3 +- .../interceptor/FileUploadInterceptor.java | 19 +- .../apache/struts2/struts-messages.properties | 3 +- .../AbstractMultiPartRequestTest.java | 14 ++ .../ActionFileUploadInterceptorTest.java | 139 +++++++------- .../FileUploadInterceptorTest.java | 179 +++++++++++------- 8 files changed, 238 insertions(+), 187 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java index 67ece0514..8840ad3d7 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java @@ -58,7 +58,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { public static final int BUFFER_SIZE = 10240; /** - * Internal list of raised errors to be passed to the the Struts2 framework. + * Internal list of raised errors to be passed to the Struts2 framework. */ protected List errors = new ArrayList<>(); @@ -83,7 +83,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { protected Long maxStringLength; /** - * Specifies the maximum size per file in the request. + * Specifies the maximum size per a file in the request. */ protected Long maxFileSize; @@ -233,34 +233,31 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { try { processUpload(request, saveDir); } catch (FileUploadException e) { - LOG.debug("Request exceeded size limit!", e); - LocalizedMessage errorMessage; + LOG.debug("Error parsing the multi-part request!", e); + Class exClass = FileUploadException.class; + Object[] args = new Object[]{}; + if (e instanceof FileUploadByteCountLimitException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getFieldName(), ex.getFileName(), ex.getPermitted(), ex.getActualSize() - }); + exClass = ex.getClass(); + args = new Object[]{ex.getFieldName(), ex.getFileName(), ex.getPermitted(), ex.getActualSize()}; } else if (e instanceof FileUploadFileCountLimitException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getPermitted(), ex.getActualSize() - }); + exClass = ex.getClass(); + args = new Object[]{ex.getPermitted(), ex.getActualSize()}; } else if (e instanceof FileUploadSizeException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getPermitted(), ex.getActualSize() - }); + exClass = ex.getClass(); + args = new Object[]{ex.getPermitted(), ex.getActualSize()}; } else if (e instanceof FileUploadContentTypeException ex) { - errorMessage = buildErrorMessage(e, new Object[]{ - ex.getContentType() - }); - } else { - errorMessage = buildErrorMessage(e, new Object[]{}); + exClass = ex.getClass(); + args = new Object[]{ex.getContentType()}; } + LocalizedMessage errorMessage = buildErrorMessage(exClass, e.getMessage(), args); if (!errors.contains(errorMessage)) { errors.add(errorMessage); } } catch (IOException e) { LOG.debug("Unable to parse request", e); - LocalizedMessage errorMessage = buildErrorMessage(e, new Object[]{}); + LocalizedMessage errorMessage = buildErrorMessage(e.getClass(), e.getMessage(), new Object[]{}); if (!errors.contains(errorMessage)) { errors.add(errorMessage); } @@ -270,15 +267,16 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { /** * Build error message. * - * @param e the Throwable/Exception - * @param args arguments + * @param exceptionClass a class of the exception + * @param defaultMessage a default message to use + * @param args arguments * @return error message */ - protected LocalizedMessage buildErrorMessage(Throwable e, Object[] args) { - String errorKey = "struts.messages.upload.error." + e.getClass().getSimpleName(); + protected LocalizedMessage buildErrorMessage(Class exceptionClass, String defaultMessage, Object[] args) { + String errorKey = "struts.messages.upload.error." + exceptionClass.getSimpleName(); LOG.debug("Preparing error message for key: [{}]", errorKey); - return new LocalizedMessage(this.getClass(), errorKey, e.getMessage(), args); + return new LocalizedMessage(this.getClass(), errorKey, defaultMessage, args); } /** diff --git a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java index 53381935b..c6457c605 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java @@ -143,11 +143,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest file, String originalFilename, String contentType, String inputName) { Set errorMessages = new HashSet<>(); ValidationAware validation = null; diff --git a/core/src/main/java/org/apache/struts2/interceptor/FileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/FileUploadInterceptor.java index 5fe9a2003..7065fed1d 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/FileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/FileUploadInterceptor.java @@ -22,14 +22,14 @@ import com.opensymphony.xwork2.ActionContext; import com.opensymphony.xwork2.ActionInvocation; import com.opensymphony.xwork2.ActionProxy; import com.opensymphony.xwork2.interceptor.ValidationAware; +import jakarta.servlet.http.HttpServletRequest; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.action.UploadedFilesAware; import org.apache.struts2.dispatcher.Parameter; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; import org.apache.struts2.dispatcher.multipart.UploadedFile; -import jakarta.servlet.http.HttpServletRequest; -import java.text.NumberFormat; import java.util.ArrayList; import java.util.Enumeration; import java.util.HashMap; @@ -192,17 +192,26 @@ public class FileUploadInterceptor extends AbstractFileUploadInterceptor { HttpServletRequest request = ac.getServletRequest(); - if (!(request instanceof MultiPartRequestWrapper)) { + if (!(request instanceof MultiPartRequestWrapper multiWrapper)) { if (LOG.isDebugEnabled()) { ActionProxy proxy = invocation.getProxy(); - LOG.debug(getTextMessage(STRUTS_MESSAGES_BYPASS_REQUEST_KEY, new String[]{proxy.getNamespace(), proxy.getActionName()})); + LOG.debug(getTextMessage(STRUTS_MESSAGES_BYPASS_REQUEST_KEY, + new String[]{proxy.getNamespace(), proxy.getActionName()}) + ); } return invocation.invoke(); } Object action = invocation.getAction(); - MultiPartRequestWrapper multiWrapper = (MultiPartRequestWrapper) request; + if (action instanceof UploadedFilesAware) { + LOG.debug("Ignoring action: {} implementing: {} as it will be handled by: {}", + invocation.getProxy().getActionName(), + UploadedFilesAware.class.getSimpleName(), + ActionFileUploadInterceptor.class.getSimpleName() + ); + return invocation.invoke(); + } applyValidation(action, multiWrapper); diff --git a/core/src/main/resources/org/apache/struts2/struts-messages.properties b/core/src/main/resources/org/apache/struts2/struts-messages.properties index beaa0927d..7c418f75c 100644 --- a/core/src/main/resources/org/apache/struts2/struts-messages.properties +++ b/core/src/main/resources/org/apache/struts2/struts-messages.properties @@ -72,7 +72,8 @@ struts.messages.upload.error.FileUploadSizeException=Request exceeded allowed si # 0 - content type struts.messages.upload.error.FileUploadContentTypeException=Request has wrong content type: {0}! -struts.messages.upload.error.FileUploadException=Error uploading: {0}! +# Default error message when handling multi-part request +struts.messages.upload.error.FileUploadException=Error parsing the multi-part request. devmode.notification=Developer Notification (set struts.devMode to false to disable this message):\n{0} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java index 02424279d..eaf7049ae 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java @@ -460,6 +460,20 @@ abstract class AbstractMultiPartRequestTest { .containsOnly("multi1", "multi2"); } + @Test + public void unableParseRequest() throws IOException { + String content = formFile("file1", "test1.csv", "1,2,3,4"); + mockRequest.setContent(content.getBytes(StandardCharsets.UTF_8)); + + assertThat(JakartaServletDiskFileUpload.isMultipartContent(mockRequest)).isTrue(); + + multiPart.parse(mockRequest, tempDir); + + assertThat(multiPart.getErrors()) + .map(LocalizedMessage::getTextKey) + .containsExactly("struts.messages.upload.error.FileUploadException"); + } + protected String formFile(String fieldName, String filename, String content) { return endline + "--" + boundary + endline + diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index 40c4103f4..0c383e024 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -25,16 +25,15 @@ import com.opensymphony.xwork2.ValidationAwareSupport; import com.opensymphony.xwork2.mock.MockActionInvocation; import com.opensymphony.xwork2.mock.MockActionProxy; import com.opensymphony.xwork2.util.ClassLoaderUtil; -import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload; -import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.action.UploadedFilesAware; import org.apache.struts2.dispatcher.multipart.JakartaMultiPartRequest; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; import org.apache.struts2.dispatcher.multipart.StrutsUploadedFile; import org.apache.struts2.dispatcher.multipart.UploadedFile; +import org.assertj.core.util.Files; import org.springframework.mock.web.MockHttpServletRequest; import java.io.File; @@ -52,7 +51,7 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { - private static final UploadedFile EMPTY_FILE = new UploadedFile<>() { + private static final UploadedFile EMPTY_FILE = new UploadedFile<>() { @Override public Long length() { return 0L; @@ -79,8 +78,8 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } @Override - public String getContent() { - return ""; + public File getContent() { + return Files.newTemporaryFile(); } @Override @@ -94,6 +93,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } }; + private MockHttpServletRequest request; private ActionFileUploadInterceptor interceptor; private File tempDir; @@ -229,7 +229,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testNoMultipartRequest() throws Exception { - MyFileUploadAction action = new MyFileUploadAction<>(); + MyFileUploadAction action = new MyFileUploadAction(); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); @@ -245,18 +245,16 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testInvalidContentTypeMultipartRequest() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); + request.setContentType("multipart/form-data"); // not a multipart contentype + request.setMethod("post"); - req.setContentType("multipart/form-data"); // not a multipart contentype - req.setMethod("post"); - - MyFileUploadAction action = container.inject(MyFileUploadAction.class); + MyFileUploadAction action = container.inject(MyFileUploadAction.class); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.intercept(mai); @@ -271,13 +269,13 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { req.addHeader("Content-type", "multipart/form-data"); req.setContent(null); // there is no content - MyFileUploadAction action = container.inject(MyFileUploadAction.class); + MyFileUploadAction action = container.inject(MyFileUploadAction.class); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.intercept(mai); @@ -285,10 +283,9 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testSuccessUploadOfATextFileMultipartRequest() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = (""" @@ -299,15 +296,15 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { Unit test of ActionFileUploadInterceptor\r -----1234--\r """); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); - MyFileUploadAction action = new MyFileUploadAction<>(); + MyFileUploadAction action = new MyFileUploadAction(); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.intercept(mai); @@ -322,29 +319,28 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } /** - * tests whether with multiple files sent with the same name, the ones with forbiddenTypes (see + * Tests whether with multiple files sent with the same name, the ones with forbiddenTypes (see * ActionFileUploadInterceptor.setAllowedTypes(...) ) are sorted out. */ public void testMultipleAccept() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("POST"); - req.addHeader("Content-type", "multipart/form-data; boundary=\"" + boundary + "\""); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("POST"); + request.addHeader("Content-type", "multipart/form-data; boundary=\"" + boundary + "\""); String content = encodeTextFile("test.html", "text/plain", plainContent) + encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + endline + "--" + boundary + "--"; - req.setContent(content.getBytes()); + request.setContent(content.getBytes()); - assertTrue(JakartaServletDiskFileUpload.isMultipartContent(req)); + assertTrue(JakartaServletDiskFileUpload.isMultipartContent(request)); - MyFileUploadAction action = new MyFileUploadAction<>(); + MyFileUploadAction action = new MyFileUploadAction(); container.inject(action); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.setAllowedTypes("text/html"); interceptor.intercept(mai); @@ -358,10 +354,9 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testUnacceptedNumberOfFiles() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("POST"); - req.addHeader("Content-type", "multipart/form-data; boundary=" + boundary); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("POST"); + request.addHeader("Content-type", "multipart/form-data; boundary=" + boundary); String content = encodeTextFile("test.html", "text/plain", plainContent) + encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + @@ -371,17 +366,17 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { boundary + "--" + endline; - req.setContent(content.getBytes()); + request.setContent(content.getBytes()); - assertTrue(JakartaServletFileUpload.isMultipartContent(req)); + assertTrue(JakartaServletFileUpload.isMultipartContent(request)); - MyFileUploadAction action = new MyFileUploadAction<>(); + MyFileUploadAction action = new MyFileUploadAction(); container.inject(action); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles(req)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); interceptor.setAllowedTypes("text/html"); interceptor.intercept(mai); @@ -395,10 +390,9 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testMultipartRequestMaxFileSize() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = (""" @@ -409,16 +403,16 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { Unit test of ActionFileUploadInterceptor\r -----1234--\r """); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); - MyFileUploadAction action = container.inject(MyFileUploadAction.class); + MyFileUploadAction action = container.inject(MyFileUploadAction.class); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); ActionContext.getContext() - .withServletRequest(createMultipartRequestMaxFileSize(req)); + .withServletRequest(createMultipartRequestMaxFileSize()); interceptor.intercept(mai); @@ -434,10 +428,9 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testMultipartRequestMaxStringLength() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = (""" @@ -456,16 +449,16 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { long string should not work\r -----1234--\r """); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); - MyFileUploadAction action = container.inject(MyFileUploadAction.class); + MyFileUploadAction action = container.inject(MyFileUploadAction.class); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); ActionContext.getContext() - .withServletRequest(createMultipartRequestMaxStringLength(req)); + .withServletRequest(createMultipartRequestMaxStringLength()); interceptor.intercept(mai); @@ -480,10 +473,9 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } public void testMultipartRequestLocalizedError() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = (""" @@ -494,9 +486,9 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { Unit test of ActionFileUploadInterceptor\r -----1234--\r """); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); - MyFileUploadAction action = container.inject(MyFileUploadAction.class); + MyFileUploadAction action = container.inject(MyFileUploadAction.class); MockActionInvocation mai = new MockActionInvocation(); mai.setAction(action); @@ -504,7 +496,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { mai.setInvocationContext(ActionContext.getContext()); ActionContext.getContext() .withLocale(Locale.GERMAN) - .withServletRequest(createMultipartRequestMaxSize(req, 10)); + .withServletRequest(createMultipartRequestMaxSize(10)); interceptor.intercept(mai); @@ -529,49 +521,48 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { content; } - private MultiPartRequestWrapper createMultipartRequestMaxFileSize(HttpServletRequest req) { - return createMultipartRequest(req, -1, 10, -1, -1); + private MultiPartRequestWrapper createMultipartRequestMaxFileSize() { + return createMultipartRequest(-1, 10, -1, -1); } - private MultiPartRequestWrapper createMultipartRequestMaxFiles(HttpServletRequest req) { - return createMultipartRequest(req, -1, -1, 3, -1); + private MultiPartRequestWrapper createMultipartRequestMaxFiles() { + return createMultipartRequest(-1, -1, 3, -1); } - private MultiPartRequestWrapper createMultipartRequestMaxSize(HttpServletRequest req, int maxsize) { - return createMultipartRequest(req, maxsize, -1, -1, -1); + private MultiPartRequestWrapper createMultipartRequestMaxSize(int maxsize) { + return createMultipartRequest(maxsize, -1, -1, -1); } - private MultiPartRequestWrapper createMultipartRequestMaxStringLength(HttpServletRequest req) { - return createMultipartRequest(req, -1, -1, -1, 20); + private MultiPartRequestWrapper createMultipartRequestMaxStringLength() { + return createMultipartRequest(-1, -1, -1, 20); } - private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); jak.setMaxSize(String.valueOf(maxsize)); jak.setMaxFileSize(String.valueOf(maxfilesize)); jak.setMaxFiles(String.valueOf(maxfiles)); jak.setMaxStringLength(String.valueOf(maxStringLength)); jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); - return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + return new MultiPartRequestWrapper(jak, request, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); } protected void setUp() throws Exception { super.setUp(); - + request = new MockHttpServletRequest(); interceptor = new ActionFileUploadInterceptor(); container.inject(interceptor); tempDir = File.createTempFile("struts", "fileupload"); - tempDir.delete(); - tempDir.mkdirs(); + assertThat(tempDir.delete()).isTrue(); + assertThat(tempDir.mkdirs()).isTrue(); } protected void tearDown() throws Exception { - tempDir.delete(); interceptor.destroy(); super.tearDown(); } - public static class MyFileUploadAction extends ActionSupport implements UploadedFilesAware { + public static class MyFileUploadAction extends ActionSupport implements UploadedFilesAware { private List> uploadedFiles; @Override diff --git a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java index 2f3145979..57f4650cc 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java @@ -23,16 +23,17 @@ import com.opensymphony.xwork2.ActionSupport; import com.opensymphony.xwork2.DefaultLocaleProvider; import com.opensymphony.xwork2.ValidationAwareSupport; import com.opensymphony.xwork2.mock.MockActionInvocation; +import com.opensymphony.xwork2.mock.MockActionProxy; import com.opensymphony.xwork2.util.ClassLoaderUtil; -import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload; -import org.apache.struts2.ServletActionContext; import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.action.UploadedFilesAware; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.multipart.JakartaMultiPartRequest; import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper; import org.apache.struts2.dispatcher.multipart.StrutsUploadedFile; import org.apache.struts2.dispatcher.multipart.UploadedFile; +import org.assertj.core.util.Files; import org.springframework.mock.web.MockHttpServletRequest; import java.io.File; @@ -52,7 +53,7 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class FileUploadInterceptorTest extends StrutsInternalTestCase { - private static final UploadedFile EMPTY_FILE = new UploadedFile<>() { + private static final UploadedFile EMPTY_FILE = new UploadedFile<>() { @Override public Long length() { return 0L; @@ -79,8 +80,8 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } @Override - public String getContent() { - return ""; + public File getContent() { + return Files.newTemporaryFile(); } @Override @@ -96,6 +97,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { private FileUploadInterceptor interceptor; private File tempDir; + private MockHttpServletRequest request; public void testAcceptFileWithEmptyAllowedTypesAndExtensions() { // when allowed type is empty @@ -177,9 +179,6 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testAcceptFileWithNoFile() { - FileUploadInterceptor interceptor = new FileUploadInterceptor(); - interceptor.setContainer(container); - interceptor.setAllowedTypes("text/plain"); // when file is not of allowed types @@ -240,10 +239,8 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testInvalidContentTypeMultipartRequest() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - - req.setContentType("multipart/form-data"); // not a multipart contentype - req.setMethod("post"); + request.setContentType("multipart/form-data"); // not a multipart contentype + request.setMethod("post"); MyFileupAction action = container.inject(MyFileupAction.class); MockActionInvocation mai = new MockActionInvocation(); @@ -252,7 +249,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { mai.setInvocationContext(ActionContext.getContext()); ActionContext.getContext().withParameters(HttpParameters.create().build()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.intercept(mai); @@ -260,12 +257,10 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testNoContentMultipartRequest() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data"); - req.setContent(null); // there is no content + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data"); + request.setContent(null); // there is no content MyFileupAction action = container.inject(MyFileupAction.class); MockActionInvocation mai = new MockActionInvocation(); @@ -274,7 +269,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { mai.setInvocationContext(ActionContext.getContext()); ActionContext.getContext().withParameters(HttpParameters.create().build()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.intercept(mai); @@ -282,10 +277,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testSuccessUploadOfATextFileMultipartRequest() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = ("-----1234\r\n" + @@ -295,7 +289,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { "Unit test of FileUploadInterceptor" + "\r\n" + "-----1234--\r\n"); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); MyFileupAction action = new MyFileupAction(); @@ -305,7 +299,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { mai.setInvocationContext(ActionContext.getContext()); Map param = new HashMap<>(); ActionContext.getContext().withParameters(HttpParameters.create(param).build()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.intercept(mai); @@ -337,10 +331,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { final String bondary = "simple boundary"; final String endline = "\r\n"; - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("POST"); - req.addHeader("Content-type", "multipart/form-data; boundary=" + bondary); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("POST"); + request.addHeader("Content-type", "multipart/form-data; boundary=" + bondary); String content = encodeTextFile("test.html", "text/plain", plainContent) + encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + @@ -351,9 +344,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { bondary + "--" + endline; - req.setContent(content.getBytes()); + request.setContent(content.getBytes()); - assertTrue(JakartaServletFileUpload.isMultipartContent(req)); + assertTrue(JakartaServletFileUpload.isMultipartContent(request)); MyFileupAction action = new MyFileupAction(); container.inject(action); @@ -363,7 +356,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { mai.setInvocationContext(ActionContext.getContext()); Map param = new HashMap<>(); ActionContext.getContext().withParameters(HttpParameters.create(param).build()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(req, 2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); interceptor.setAllowedTypes("text/html"); interceptor.intercept(mai); @@ -390,10 +383,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { final String boundary = "simple boundary"; final String endline = "\r\n"; - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("POST"); - req.addHeader("Content-type", "multipart/form-data; boundary=" + boundary); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("POST"); + request.addHeader("Content-type", "multipart/form-data; boundary=" + boundary); String content = encodeTextFile("test.html", "text/plain", plainContent) + encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + @@ -403,9 +395,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { boundary + "--" + endline; - req.setContent(content.getBytes()); + request.setContent(content.getBytes()); - assertTrue(JakartaServletFileUpload.isMultipartContent(req)); + assertTrue(JakartaServletFileUpload.isMultipartContent(request)); MyFileupAction action = new MyFileupAction(); container.inject(action); @@ -414,8 +406,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); Map param = new HashMap<>(); - ActionContext.getContext().withParameters(HttpParameters.create(param).build()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles(req)); + ActionContext.getContext() + .withParameters(HttpParameters.create(param).build()) + .withServletRequest(createMultipartRequestMaxFiles()); interceptor.setAllowedTypes("text/html"); interceptor.intercept(mai); @@ -430,10 +423,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testMultipartRequestMaxFileSize() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = ("-----1234\r\n" + @@ -443,7 +435,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { "Unit test of FileUploadInterceptor" + "\r\n" + "-----1234--\r\n"); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); MyFileupAction action = container.inject(MyFileupAction.class); @@ -454,7 +446,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { Map param = new HashMap<>(); ActionContext.getContext() .withParameters(HttpParameters.create(param).build()) - .withServletRequest(createMultipartRequestMaxFileSize(req)); + .withServletRequest(createMultipartRequestMaxFileSize()); interceptor.intercept(mai); @@ -470,10 +462,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testMultipartRequestMaxStringLength() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = ("-----1234\r\n" + @@ -493,7 +484,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { "long string should not work" + "\r\n" + "-----1234--\r\n"); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); MyFileupAction action = container.inject(MyFileupAction.class); @@ -504,7 +495,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { Map param = new HashMap<>(); ActionContext.getContext() .withParameters(HttpParameters.create(param).build()) - .withServletRequest(createMultipartRequestMaxStringLength(req)); + .withServletRequest(createMultipartRequestMaxStringLength()); interceptor.intercept(mai); @@ -519,10 +510,9 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } public void testMultipartRequestLocalizedError() throws Exception { - MockHttpServletRequest req = new MockHttpServletRequest(); - req.setCharacterEncoding(StandardCharsets.UTF_8.name()); - req.setMethod("post"); - req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); // inspired by the unit tests for jakarta commons fileupload String content = ("-----1234\r\n" + @@ -532,7 +522,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { "Unit test of FileUploadInterceptor" + "\r\n" + "-----1234--\r\n"); - req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); MyFileupAction action = container.inject(MyFileupAction.class); @@ -544,7 +534,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { ActionContext.getContext() .withParameters(HttpParameters.create(param).build()) .withLocale(Locale.GERMAN) - .withServletRequest(createMultipartRequestMaxSize(req, 10)); + .withServletRequest(createMultipartRequestMaxSize(10)); interceptor.intercept(mai); @@ -557,6 +547,38 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte Größe")); } + public void testSkippingUploadedFileAware() throws Exception { + request.setCharacterEncoding(StandardCharsets.UTF_8.name()); + request.setMethod("post"); + request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + + // inspired by the unit tests for jakarta commons fileupload + String content = ("-----1234\r\n" + + "Content-Disposition: form-data; name=\"file\"; filename=\"deleteme.txt\"\r\n" + + "Content-Type: text/html\r\n" + + "\r\n" + + "Unit test of FileUploadInterceptor" + + "\r\n" + + "-----1234--\r\n"); + request.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + MyFileUploadAction action = container.inject(MyFileUploadAction.class); + + MockActionInvocation mai = new MockActionInvocation(); + mai.setInvocationContext(ActionContext.getContext()); + mai.setAction(action); + MockActionProxy map = new MockActionProxy(); + map.setActionName("uploadedFiles"); + mai.setProxy(map); + ActionContext.getContext() + .withParameters(HttpParameters.create(new HashMap()).build()) + .withServletRequest(createMultipartRequestMaxSize(10)); + + interceptor.intercept(mai); + + assertFalse(action.hasActionErrors()); + } + private String encodeTextFile(String filename, String contentType, String content) { return "\r\n" + "--" + @@ -574,44 +596,44 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { content; } - private MultiPartRequestWrapper createMultipartRequestMaxFileSize(HttpServletRequest req) { - return createMultipartRequest(req, -1, 10, -1, -1); + private MultiPartRequestWrapper createMultipartRequestMaxFileSize() { + return createMultipartRequest(-1, 10, -1, -1); } - private MultiPartRequestWrapper createMultipartRequestMaxFiles(HttpServletRequest req) { - return createMultipartRequest(req, -1, -1, 3, -1); + private MultiPartRequestWrapper createMultipartRequestMaxFiles() { + return createMultipartRequest(-1, -1, 3, -1); } - private MultiPartRequestWrapper createMultipartRequestMaxSize(HttpServletRequest req, int maxsize) { - return createMultipartRequest(req, maxsize, -1, -1, -1); + private MultiPartRequestWrapper createMultipartRequestMaxSize(int maxsize) { + return createMultipartRequest(maxsize, -1, -1, -1); } - private MultiPartRequestWrapper createMultipartRequestMaxStringLength(HttpServletRequest req) { - return createMultipartRequest(req, -1, -1, -1, 20); + private MultiPartRequestWrapper createMultipartRequestMaxStringLength() { + return createMultipartRequest(-1, -1, -1, 20); } - private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { + private MultiPartRequestWrapper createMultipartRequest(int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); jak.setMaxSize(String.valueOf(maxsize)); jak.setMaxFileSize(String.valueOf(maxfilesize)); jak.setMaxFiles(String.valueOf(maxfiles)); jak.setMaxStringLength(String.valueOf(maxStringLength)); jak.setDefaultEncoding(StandardCharsets.UTF_8.name()); - return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); + return new MultiPartRequestWrapper(jak, request, tempDir.getAbsolutePath(), new DefaultLocaleProvider()); } protected void setUp() throws Exception { super.setUp(); + request = new MockHttpServletRequest(); interceptor = new FileUploadInterceptor(); container.inject(interceptor); tempDir = File.createTempFile("struts", "fileupload"); - tempDir.delete(); - tempDir.mkdirs(); + assertThat(tempDir.delete()).isTrue(); + assertThat(tempDir.mkdirs()).isTrue(); } protected void tearDown() throws Exception { - tempDir.delete(); interceptor.destroy(); super.tearDown(); } @@ -619,4 +641,17 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { public static class MyFileupAction extends ActionSupport { } + public static class MyFileUploadAction extends ActionSupport implements UploadedFilesAware { + private List> uploadedFiles; + + @Override + public void withUploadedFiles(List> uploadedFiles) { + this.uploadedFiles = uploadedFiles; + } + + public List> getUploadFiles() { + return this.uploadedFiles; + } + } + }