WW-5528 Ensure multipart upload illegal characters reported as error

core/src/main/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequest.java

@@ -18,14 +18,13 @@
  */
 package org.apache.struts2.dispatcher.multipart;
 
-import jakarta.servlet.http.HttpServletRequest;
 import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
 import org.apache.commons.fileupload2.core.FileUploadContentTypeException;
 import org.apache.commons.fileupload2.core.FileUploadException;
 import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
 import org.apache.commons.fileupload2.core.FileUploadSizeException;
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
-import org.apache.commons.lang3.BooleanUtils;
+import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -35,6 +34,7 @@
 import org.apache.struts2.security.DefaultExcludedPatternsChecker;
 import org.apache.struts2.security.ExcludedPatternsChecker;
 
+import jakarta.servlet.http.HttpServletRequest;
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.nio.file.Path;
@@ -45,13 +45,17 @@
 import java.util.List;
 import java.util.Map;
 
+import static org.apache.commons.lang3.StringUtils.normalizeSpace;
+
 /**
  * Abstract class with some helper methods, it should be used
  * when starting development of another implementation of {@link MultiPartRequest}
  */
 public abstract class AbstractMultiPartRequest implements MultiPartRequest {
 
     protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY = "struts.messages.upload.error.parameter.too.long";
+    protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD = "struts.messages.upload.error.illegal.characters.field";
+    protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME = "struts.messages.upload.error.illegal.characters.name";
 
     private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class);
 
@@ -116,13 +120,14 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
 
     private final ExcludedPatternsChecker patternsChecker;
 
-    protected AbstractMultiPartRequest(String dmiValue) {
-        patternsChecker = new DefaultExcludedPatternsChecker();
-        if (BooleanUtils.toBoolean(dmiValue)) {
-            ((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT);
-        } else {
-            ((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN);
-        }
+    protected AbstractMultiPartRequest() {
+        this(false);
+    }
+
+    protected AbstractMultiPartRequest(boolean dmiValue) {
+        var patternsChecker = new DefaultExcludedPatternsChecker();
+        patternsChecker.setAdditionalExcludePatterns(dmiValue ? EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT : EXCLUDED_FILE_PATTERN);
+        this.patternsChecker = patternsChecker;
     }
 
     /**
@@ -302,16 +307,7 @@ protected LocalizedMessage buildErrorMessage(Class<? extends Throwable> exceptio
      * @return the canonical name based on the supplied filename
      */
     protected String getCanonicalName(final String originalFileName) {
-        String fileName = originalFileName;
-
-        int forwardSlash = fileName.lastIndexOf('/');
-        int backwardSlash = fileName.lastIndexOf('\\');
-        if (forwardSlash != -1 && forwardSlash > backwardSlash) {
-            fileName = fileName.substring(forwardSlash + 1);
-        } else {
-            fileName = fileName.substring(backwardSlash + 1);
-        }
-        return fileName;
+        return FilenameUtils.getName(originalFileName);
     }
 
     /**
@@ -443,4 +439,32 @@ protected boolean isExcluded(String fileName) {
         return patternsChecker.isExcluded(fileName).isExcluded();
     }
 
+    protected boolean isInvalidInput(String fieldName, String fileName) {
+        // Skip file uploads that don't have a file name - meaning that no file was selected.
+        if (fileName == null || fileName.isBlank()) {
+            LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fieldName));
+            return true;
+        }
+
+        if (isExcluded(fileName)) {
+            var normalizedFileName = normalizeSpace(fileName);
+            LOG.debug("File name [{}] is not accepted", normalizedFileName);
+            errors.add(new LocalizedMessage(getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME, null,
+                    new String[]{normalizedFileName}));
+            return true;
+        }
+
+        return isInvalidInput(fieldName);
+    }
+
+    protected boolean isInvalidInput(String fieldName) {
+        if (isExcluded(fieldName)) {
+            var normalizedFieldName = normalizeSpace(fieldName);
+            LOG.debug("Form field [{}}] is rejected!", normalizedFieldName);
+            errors.add(new LocalizedMessage(getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD, null,
+                    new String[]{normalizedFieldName}));
+            return true;
+        }
+        return false;
+    }
 }

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java

@@ -18,16 +18,17 @@
  */
 package org.apache.struts2.dispatcher.multipart;
 
-import jakarta.servlet.http.HttpServletRequest;
 import org.apache.commons.fileupload2.core.DiskFileItem;
 import org.apache.commons.fileupload2.core.DiskFileItemFactory;
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
+import org.apache.commons.lang3.BooleanUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.StrutsConstants;
 import org.apache.struts2.inject.Inject;
 
+import jakarta.servlet.http.HttpServletRequest;
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.nio.file.Path;
@@ -44,12 +45,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
     private static final Logger LOG = LogManager.getLogger(JakartaMultiPartRequest.class);
 
     public JakartaMultiPartRequest() {
-        super(Boolean.FALSE.toString());
+        super();
     }
 
     @Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
     public JakartaMultiPartRequest(String dmiValue) {
-        super(dmiValue);
+        super(BooleanUtils.toBoolean(dmiValue));
     }
 
     @Override
@@ -88,15 +89,14 @@ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset,
     }
 
     protected void processNormalFormField(DiskFileItem item, Charset charset) throws IOException {
-        LOG.debug("Item: {} is a normal form field", item.getName());
+        var fieldName = item.getFieldName();
+        LOG.debug("Item: {} is a normal form field", fieldName);
 
-        if (isExcluded(item.getFieldName())) {
-            LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(item.getFieldName())));
+        if (isInvalidInput(fieldName)) {
             return;
         }
 
         List<String> values;
-        String fieldName = item.getFieldName();
         if (parameters.get(fieldName) != null) {
             values = parameters.get(fieldName);
         } else {
@@ -116,19 +116,7 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws
     }
 
     protected void processFileField(DiskFileItem item) {
-        if (isExcluded(item.getName())) {
-            LOG.warn(() -> "File name [%s] is not accepted".formatted(normalizeSpace(item.getName())));
-            return;
-        }
-
-        if (isExcluded(item.getFieldName())) {
-            LOG.warn(() -> "Field name [%s] is not accepted".formatted(normalizeSpace(item.getFieldName())));
-            return;
-        }
-
-        // Skip file uploads that don't have a file name - meaning that no file was selected.
-        if (item.getName() == null || item.getName().trim().isEmpty()) {
-            LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(item.getFieldName()));
+        if (isInvalidInput(item.getFieldName(), item.getName())) {
             return;
         }
 

core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java

@@ -18,18 +18,19 @@
  */
 package org.apache.struts2.dispatcher.multipart;
 
-import jakarta.servlet.http.HttpServletRequest;
 import org.apache.commons.fileupload2.core.DiskFileItemFactory;
 import org.apache.commons.fileupload2.core.FileItemInput;
 import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
 import org.apache.commons.fileupload2.core.FileUploadSizeException;
 import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
+import org.apache.commons.lang3.BooleanUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.struts2.StrutsConstants;
 import org.apache.struts2.dispatcher.LocalizedMessage;
 import org.apache.struts2.inject.Inject;
 
+import jakarta.servlet.http.HttpServletRequest;
 import java.io.BufferedOutputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
@@ -59,12 +60,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
     private static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
 
     public JakartaStreamMultiPartRequest() {
-        super(Boolean.FALSE.toString());
+        super();
     }
 
     @Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
     public JakartaStreamMultiPartRequest(String dmiValue) {
-        super(dmiValue);
+        super(BooleanUtils.toBoolean(dmiValue));
     }
 
     /**
@@ -125,13 +126,11 @@ private String readStream(InputStream inputStream) throws IOException {
      */
     protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
         String fieldName = fileItemInput.getFieldName();
-        String fieldValue = readStream(fileItemInput.getInputStream());
-
-        if (isExcluded(fieldName)) {
-            LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(fieldName)));
+        if (isInvalidInput(fieldName)) {
             return;
         }
 
+        String fieldValue = readStream(fileItemInput.getInputStream());
         if (exceedsMaxStringLength(fieldName, fieldValue)) {
             return;
         }
@@ -203,14 +202,7 @@ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long
      * @param location      location
      */
     protected void processFileItemAsFileField(FileItemInput fileItemInput, Path location) throws IOException {
-        // Skip file uploads that don't have a file name - meaning that no file was selected.
-        if (fileItemInput.getName() == null || fileItemInput.getName().trim().isEmpty()) {
-            LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fileItemInput.getFieldName()));
-            return;
-        }
-
-        if (isExcluded(fileItemInput.getName())) {
-            LOG.warn(() -> "File field [%s] rejected".formatted(normalizeSpace(fileItemInput.getName())));
+        if (isInvalidInput(fileItemInput.getFieldName(), fileItemInput.getName())) {
             return;
         }
 

core/src/main/resources/org/apache/struts2/struts-messages.properties

@@ -40,6 +40,10 @@ struts.messages.error.file.too.large=The file is too large to be uploaded: {0} "
 # 1 - max string length
 # 2 - actual size
 struts.messages.upload.error.parameter.too.long=The request parameter "{0}" was too long. Max length allowed is {1}, but found {2}!
+# 0 - field name
+struts.messages.upload.error.illegal.characters.field=The multipart upload field name "{0}" contains illegal characters!
+# 0 - file name
+struts.messages.upload.error.illegal.characters.name=The multipart upload filename "{0}" contains illegal characters!
 # 0 - input name
 # 1 - original filename
 # 2 - file name after uploading the file

core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java

@@ -518,8 +518,8 @@ public void maliciousFields() throws IOException {
 
         multiPart.parse(mockRequest, tempDir);
 
-        assertThat(multiPart.getErrors())
-                .isEmpty();
+        assertThat(multiPart.getErrors()).extracting(LocalizedMessage::getTextKey)
+                .containsExactly(AbstractMultiPartRequest.STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD);
 
         assertThat(multiPart.getParameterNames().asIterator()).toIterable()
                 .isEmpty();
@@ -537,8 +537,8 @@ public void maliciousFilename() throws IOException {
 
         multiPart.parse(mockRequest, tempDir);
 
-        assertThat(multiPart.getErrors())
-                .isEmpty();
+        assertThat(multiPart.getErrors()).extracting(LocalizedMessage::getTextKey)
+                .containsExactly(AbstractMultiPartRequest.STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME);
 
         assertThat(multiPart.getParameterNames().asIterator()).toIterable()
                 .hasSize(1);

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;
 
@@ -540,7 +540,8 @@ public void testUnacceptedFieldName() throws Exception {
 
         interceptor.intercept(mai);
 
-        assertFalse(action.hasActionErrors());
+        assertThat(action.getActionErrors())
+                .containsExactly("The multipart upload field name \"top.file\" contains illegal characters!");
         assertNull(action.getUploadFiles());
     }
 
@@ -570,7 +571,8 @@ public void testUnacceptedFileName() throws Exception {
 
         interceptor.intercept(mai);
 
-        assertFalse(action.hasActionErrors());
+        assertThat(action.getActionErrors())
+                .containsExactly("The multipart upload filename \"../deleteme.txt\" contains illegal characters!");
         assertNull(action.getUploadFiles());
     }