This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push: new d9c9d2b03 WW-5546 Fixes NPE when uploaded file is empty (#1263) d9c9d2b03 is described below commit d9c9d2b0307fe7a4126c6f00df29951c54517802 Author: Lukasz Lenart <lukaszlen...@apache.org> AuthorDate: Wed May 7 07:18:15 2025 +0200 WW-5546 Fixes NPE when uploaded file is empty (#1263) --- .../interceptor/AbstractFileUploadInterceptor.java | 2 +- .../ActionFileUploadInterceptorTest.java | 344 +++++++++++---------- 2 files changed, 174 insertions(+), 172 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index c0a1bd9d8..8d7ef49f1 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -115,7 +115,7 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor } // If it's null the upload failed - if (file == null) { + if (file == null || file.getContent() == null) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_UPLOADING_KEY, new String[]{inputName}); if (validation != null) { validation.addFieldError(inputName, errMsg); 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 b8761a8a4..26cf7e180 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -18,21 +18,21 @@ */ package org.apache.struts2.interceptor; -import org.apache.struts2.ActionContext; -import org.apache.struts2.ActionSupport; -import org.apache.struts2.locale.DefaultLocaleProvider; -import org.apache.struts2.ValidationAwareSupport; -import org.apache.struts2.mock.MockActionInvocation; -import org.apache.struts2.mock.MockActionProxy; -import org.apache.struts2.util.ClassLoaderUtil; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload; import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload; +import org.apache.struts2.ActionContext; +import org.apache.struts2.ActionSupport; import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.ValidationAwareSupport; 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.apache.struts2.locale.DefaultLocaleProvider; +import org.apache.struts2.mock.MockActionInvocation; +import org.apache.struts2.mock.MockActionProxy; +import org.apache.struts2.util.ClassLoaderUtil; import org.assertj.core.util.Files; import org.springframework.mock.web.MockHttpServletRequest; @@ -40,7 +40,6 @@ import java.io.File; import java.net.URI; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.util.Collection; import java.util.List; import java.util.Locale; @@ -51,53 +50,6 @@ import static org.assertj.core.api.Assertions.assertThat; */ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { - private static final UploadedFile EMPTY_FILE = new UploadedFile() { - @Override - public Long length() { - return 0L; - } - - @Override - public String getName() { - return ""; - } - - @Override - public boolean isFile() { - return false; - } - - @Override - public boolean delete() { - return false; - } - - @Override - public String getAbsolutePath() { - return null; - } - - @Override - public File getContent() { - return Files.newTemporaryFile(); - } - - @Override - public String getOriginalName() { - return null; - } - - @Override - public String getContentType() { - return null; - } - - @Override - public String getInputName() { - return null; - } - }; - private MockHttpServletRequest request; private ActionFileUploadInterceptor interceptor; private File tempDir; @@ -105,16 +57,16 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { private final String htmlContent = "<html><head></head><body>html content</body></html>"; private final String plainContent = "plain content"; private final String boundary = "simple boundary"; - private final String endline = "\r\n"; + private final String endLine = "\r\n"; public void testAcceptFileWithEmptyAllowedTypesAndExtensions() { // when allowed type is empty ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, EMPTY_FILE, "filename", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); - assertTrue(ok); - assertTrue(validation.getFieldErrors().isEmpty()); - assertFalse(validation.hasErrors()); + assertThat(ok).isTrue(); + assertThat(validation.getFieldErrors()).isEmpty(); + assertThat(validation.hasErrors()).isFalse(); } public void testAcceptFileWithoutEmptyTypes() { @@ -122,39 +74,38 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, EMPTY_FILE, "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); - assertTrue(ok); - assertTrue(validation.getFieldErrors().isEmpty()); - assertFalse(validation.hasErrors()); + assertThat(ok).isTrue(); + assertThat(validation.getFieldErrors()).isEmpty(); + assertThat(validation.hasErrors()).isFalse(); // when file is not of allowed types validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, EMPTY_FILE, "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); - assertFalse(notOk); - assertFalse(validation.getFieldErrors().isEmpty()); - assertTrue(validation.hasErrors()); + assertThat(notOk).isFalse(); + assertThat(validation.getFieldErrors()).isNotEmpty(); + assertThat(validation.hasErrors()).isTrue(); } - public void testAcceptFileWithWildcardContent() { interceptor.setAllowedTypes("text/*"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, EMPTY_FILE, "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); - assertTrue(ok); - assertTrue(validation.getFieldErrors().isEmpty()); - assertFalse(validation.hasErrors()); + assertThat(ok).isTrue(); + assertThat(validation.getFieldErrors()).isEmpty(); + assertThat(validation.hasErrors()).isFalse(); interceptor.setAllowedTypes("text/h*"); validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, EMPTY_FILE, "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); - assertFalse(notOk); - assertFalse(validation.getFieldErrors().isEmpty()); - assertTrue(validation.hasErrors()); + assertThat(notOk).isFalse(); + assertThat(validation.getFieldErrors()).isNotEmpty(); + assertThat(validation.hasErrors()).isTrue(); } public void testAcceptFileWithoutEmptyExtensions() { @@ -162,48 +113,62 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is of allowed extensions ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, EMPTY_FILE, "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); - assertTrue(ok); - assertTrue(validation.getFieldErrors().isEmpty()); - assertFalse(validation.hasErrors()); + assertThat(ok).isTrue(); + assertThat(validation.getFieldErrors()).isEmpty(); + assertThat(validation.hasErrors()).isFalse(); // when file is not of allowed extensions validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, EMPTY_FILE, "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); - assertFalse(notOk); - assertFalse(validation.getFieldErrors().isEmpty()); - assertTrue(validation.hasErrors()); + assertThat(notOk).isFalse(); + assertThat(validation.getFieldErrors()).isNotEmpty(); + assertThat(validation.hasErrors()).isTrue(); - //test with multiple extensions interceptor.setAllowedExtensions(".txt,.lol"); validation = new ValidationAwareSupport(); - ok = interceptor.acceptFile(validation, EMPTY_FILE, "filename.lol", "text/plain", "inputName"); + ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); - assertTrue(ok); - assertTrue(validation.getFieldErrors().isEmpty()); - assertFalse(validation.hasErrors()); + assertThat(ok).isTrue(); + assertThat(validation.getFieldErrors()).isEmpty(); + assertThat(validation.hasErrors()).isFalse(); } public void testAcceptFileWithNoFile() { - ActionFileUploadInterceptor interceptor = new ActionFileUploadInterceptor(); - interceptor.setContainer(container); - interceptor.setAllowedTypes("text/plain"); // when file is not of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); boolean notOk = interceptor.acceptFile(validation, null, "filename.html", "text/html", "inputName"); - assertFalse(notOk); - assertFalse(validation.getFieldErrors().isEmpty()); - assertTrue(validation.hasErrors()); - List<String> errors = validation.getFieldErrors().get("inputName"); - assertEquals(1, errors.size()); - String msg = errors.get(0); - assertTrue(msg.startsWith("Error uploading:")); - assertTrue(msg.indexOf("inputName") > 0); + assertThat(notOk).isFalse(); + assertThat(validation.getFieldErrors()).isNotEmpty(); + assertThat(validation.hasErrors()).isTrue(); + assertThat(validation.getFieldErrors().get("inputName")) + .hasSize(1) + .first() + .asString() + .startsWith("Error uploading:") + .contains("inputName"); + } + + public void testAcceptFileWithNoContent() { + interceptor.setAllowedTypes("text/plain"); + + ValidationAwareSupport validation = new ValidationAwareSupport(); + boolean notOk = interceptor.acceptFile(validation, createTestFile(null), "filename.html", "text/plain", "inputName"); + + assertThat(notOk).isFalse(); + assertThat(validation.getFieldErrors()).isNotEmpty(); + assertThat(validation.hasErrors()).isTrue(); + assertThat(validation.getFieldErrors().get("inputName")) + .hasSize(1) + .first() + .asString() + .startsWith("Error uploading:") + .contains("inputName"); } public void testAcceptFileWithMaxSize() throws Exception { @@ -214,23 +179,18 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { URL url = ClassLoaderUtil.getResource("log4j2.xml", ActionFileUploadInterceptorTest.class); File file = new File(new URI(url.toString())); - assertTrue("log4j2.xml should be in src/test folder", file.exists()); + assertThat(file).exists(); UploadedFile uploadedFile = StrutsUploadedFile.Builder.create(file).withContentType("text/html").withOriginalName("filename").build(); boolean notOk = interceptor.acceptFile(validation, uploadedFile, "filename", "text/html", "inputName"); - assertFalse(notOk); - assertFalse(validation.getFieldErrors().isEmpty()); - assertTrue(validation.hasErrors()); - List<String> errors = validation.getFieldErrors().get("inputName"); - assertEquals(1, errors.size()); - String msg = errors.get(0); - // the error message should contain at least this test - assertThat(msg).contains( - "The file is too large to be uploaded", - "inputName", - "log4j2.xml", - "allowed mx size is 10" - ); + assertThat(notOk).isFalse(); + assertThat(validation.getFieldErrors()).isNotEmpty(); + assertThat(validation.hasErrors()).isTrue(); + assertThat(validation.getFieldErrors().get("inputName")) + .hasSize(1) + .first() + .asString() + .contains("The file is too large to be uploaded", "inputName", "log4j2.xml", "allowed mx size is 10"); } public void testNoMultipartRequest() throws Exception { @@ -246,11 +206,11 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { mai.setInvocationContext(ActionContext.getContext()); // if no multipart request it will bypass and execute it - assertEquals("NoMultipart", interceptor.intercept(mai)); + assertThat(interceptor.intercept(mai)).isEqualTo("NoMultipart"); } public void testInvalidContentTypeMultipartRequest() throws Exception { - request.setContentType("multipart/form-data"); // not a multipart contentype + request.setContentType("multipart/form-data"); // not a multipart Content-Type request.setMethod("post"); MyFileUploadAction action = container.inject(MyFileUploadAction.class); @@ -263,7 +223,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertTrue(action.hasErrors()); + assertThat(action.hasErrors()).isTrue(); } public void testNoContentMultipartRequest() throws Exception { @@ -284,7 +244,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertTrue(action.hasErrors()); + assertThat(action.hasErrors()).isTrue(); } public void testSuccessUploadOfATextFileMultipartRequest() throws Exception { @@ -292,7 +252,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { request.setMethod("post"); request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); - // inspired by the unit tests for jakarta commons fileupload + // inspired by the unit tests for Jakarta Commons FileUpload String content = (""" -----1234\r Content-Disposition: form-data; name="file"; filename="deleteme.txt"\r @@ -313,14 +273,13 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertFalse(action.hasErrors()); + assertThat(action.hasErrors()).isFalse(); List<UploadedFile> files = action.getUploadFiles(); - assertNotNull(files); - assertEquals(1, files.size()); - assertEquals("text/html", files.get(0).getContentType()); - assertNotNull("deleteme.txt", files.get(0).getOriginalName()); + assertThat(files).isNotNull().hasSize(1); + assertThat(files.get(0).getContentType()).isEqualTo("text/html"); + assertThat(files.get(0).getOriginalName()).isEqualTo("deleteme.txt"); } /** @@ -334,10 +293,10 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { String content = encodeTextFile("test.html", "text/plain", plainContent) + encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + - endline + "--" + boundary + "--"; + endLine + "--" + boundary + "--"; request.setContent(content.getBytes()); - assertTrue(JakartaServletDiskFileUpload.isMultipartContent(request)); + assertThat(JakartaServletDiskFileUpload.isMultipartContent(request)).isTrue(); MyFileUploadAction action = new MyFileUploadAction(); container.inject(action); @@ -345,17 +304,18 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { mai.setAction(action); mai.setResultCode("success"); mai.setInvocationContext(ActionContext.getContext()); - ActionContext.getContext().withServletRequest(createMultipartRequestMaxSize(2000)); + ActionContext.getContext().withServletRequest(createMultipartRequestMaxFiles()); interceptor.setAllowedTypes("text/html"); interceptor.intercept(mai); List<UploadedFile> files = action.getUploadFiles(); - assertNotNull(files); - assertEquals("files accepted ", 2, files.size()); - assertEquals("text/html", files.get(0).getContentType()); - assertNotNull("test1.html", files.get(0).getOriginalName()); + assertThat(files).isNotNull().hasSize(2); + assertThat(files.get(0).getContentType()).isEqualTo("text/html"); + assertThat(files.get(0).getOriginalName()).isEqualTo("test1.html"); + assertThat(files.get(1).getContentType()).isEqualTo("text/html"); + assertThat(files.get(1).getOriginalName()).isEqualTo("test2.html"); } public void testUnacceptedNumberOfFiles() throws Exception { @@ -366,14 +326,14 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { encodeTextFile("test1.html", "text/html", htmlContent) + encodeTextFile("test2.html", "text/html", htmlContent) + encodeTextFile("test3.html", "text/html", htmlContent) + - endline + + endLine + "--" + boundary + "--" + - endline; + endLine; request.setContent(content.getBytes()); - assertTrue(JakartaServletFileUpload.isMultipartContent(request)); + assertThat(JakartaServletFileUpload.isMultipartContent(request)).isTrue(); MyFileUploadAction action = new MyFileUploadAction(); container.inject(action); @@ -386,12 +346,11 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.setAllowedTypes("text/html"); interceptor.intercept(mai); - assertNull(action.getUploadFiles()); - assertEquals(1, action.getActionErrors().size()); - assertEquals( - "Request exceeded allowed number of files! Permitted number of files is: 3!", - action.getActionErrors().iterator().next() - ); + assertThat(action.getUploadFiles()).isNull(); + assertThat(action.getActionErrors()) + .hasSize(1) + .first() + .isEqualTo("Request exceeded allowed number of files! Permitted number of files is: 3!"); } public void testMultipartRequestMaxFileSize() throws Exception { @@ -399,7 +358,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { request.setMethod("post"); request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); - // inspired by the unit tests for jakarta commons fileupload + // inspired by the unit tests for Jakarta Commons FileUpload String content = (""" -----1234\r Content-Disposition: form-data; name="file"; filename="deleteme.txt"\r @@ -421,15 +380,12 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertTrue(action.hasActionErrors()); + assertThat(action.hasActionErrors()).isTrue(); - Collection<String> errors = action.getActionErrors(); - assertEquals(1, errors.size()); - String msg = errors.iterator().next(); - // FIXME: the expected size is 40 - length of the string - assertEquals( - "File deleteme.txt assigned to file exceeded allowed size limit! Max size allowed is: 10 but file was: 11!", - msg); + assertThat(action.getActionErrors()) + .hasSize(1) + .first() + .isEqualTo("File deleteme.txt assigned to file exceeded allowed size limit! Max size allowed is: 10 but file was: 11!"); } public void testMultipartRequestMaxStringLength() throws Exception { @@ -437,7 +393,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { request.setMethod("post"); request.addHeader("Content-type", "multipart/form-data; boundary=---1234"); - // inspired by the unit tests for jakarta commons fileupload + // inspired by the unit tests for Jakarta Commons FileUpload String content = (""" -----1234\r Content-Disposition: form-data; name="file"; filename="deleteme.txt"\r @@ -467,14 +423,12 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertTrue(action.hasActionErrors()); + assertThat(action.hasActionErrors()).isTrue(); - Collection<String> errors = action.getActionErrors(); - assertEquals(1, errors.size()); - String msg = errors.iterator().next(); - assertEquals( - "The request parameter \"normalFormField2\" was too long. Max length allowed is 20, but found 27!", - msg); + assertThat(action.getActionErrors()) + .hasSize(1) + .first() + .isEqualTo("The request parameter \"normalFormField2\" was too long. Max length allowed is 20, but found 27!"); } public void testMultipartRequestLocalizedError() throws Exception { @@ -482,7 +436,6 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { 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 Content-Disposition: form-data; name="file"; filename="deleteme.txt"\r @@ -505,24 +458,24 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.intercept(mai); - assertTrue(action.hasActionErrors()); + assertThat(action.hasActionErrors()).isTrue(); - Collection<String> errors = action.getActionErrors(); - assertEquals(1, errors.size()); - String msg = errors.iterator().next(); - // the error message should contain at least this test - assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte Größe")); + assertThat(action.getActionErrors()) + .hasSize(1) + .first() + .asString() + .startsWith("Der Request übertraf die maximal erlaubte Größe"); } private String encodeTextFile(String filename, String contentType, String content) { - return endline + + return endLine + "--" + boundary + - endline + - "Content-Disposition: form-data; name=\"" + "file" + "\"; filename=\"" + filename + - endline + + endLine + + "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"" + + endLine + "Content-Type: " + contentType + - endline + - endline + + endLine + + endLine + content; } @@ -568,6 +521,55 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { super.tearDown(); } + private UploadedFile createTestFile(File content) { + return new UploadedFile() { + @Override + public Long length() { + return 0L; + } + + @Override + public String getName() { + return ""; + } + + @Override + public boolean isFile() { + return false; + } + + @Override + public boolean delete() { + return false; + } + + @Override + public String getAbsolutePath() { + return null; + } + + @Override + public File getContent() { + return content; + } + + @Override + public String getOriginalName() { + return null; + } + + @Override + public String getContentType() { + return null; + } + + @Override + public String getInputName() { + return null; + } + }; + } + public static class MyFileUploadAction extends ActionSupport implements UploadedFilesAware { private List<UploadedFile> uploadedFiles;