This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5528-multipart-illegal-char-errors
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 96ee51fb1297d84f02a73c488c49c6bded9987db
Author: Kusal Kithul-Godage <g...@kusal.io>
AuthorDate: Thu Feb 6 11:39:09 2025 +1100
WW-5528 Ensure multipart upload illegal characters reported as error
---
.../multipart/AbstractMultiPartRequest.java | 62 +++++++++++++++-------
.../multipart/JakartaMultiPartRequest.java | 28 +++-------
.../multipart/JakartaStreamMultiPartRequest.java | 22 +++-----
.../org/apache/struts2/struts-messages.properties | 4 ++
.../multipart/AbstractMultiPartRequestTest.java | 8 +--
5 files changed, 66 insertions(+), 58 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 7f848a56d..00e774a72 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
@@ -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.inject.Inject;
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,6 +45,8 @@ import java.util.HashMap;
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}
@@ -52,6 +54,8 @@ import java.util.Map;
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 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
* @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 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
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;
+ }
}
diff --git
a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
b/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
index ad2544b52..a61ced6da 100644
---
a/core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
+++
b/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 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
}
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 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
}
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;
}
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 abe7ad3a6..76c0e3654 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
@@ -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 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
*/
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 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
* @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;
}
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 7c418f75c..e802b5cc4 100644
--- a/core/src/main/resources/org/apache/struts2/struts-messages.properties
+++ b/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
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 29143f7ef..699d0b66c 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
@@ -518,8 +518,8 @@ abstract class AbstractMultiPartRequestTest {
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 @@ abstract class AbstractMultiPartRequestTest {
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);
