This is an automated email from the ASF dual-hosted git repository.
kusal pushed a commit to branch WW-5528-multipart-illegal-char-errors-67
in repository https://gitbox.apache.org/repos/asf/struts.git
commit e16d24b146b30629710a78946834baa02cb15c9a
Author: Kusal Kithul-Godage <g...@kusal.io>
AuthorDate: Thu Feb 6 18:49:34 2025 +1100
WW-5528 Ensure multipart upload illegal characters reported as error
---
.../multipart/AbstractMultiPartRequest.java | 61 +++++++++++++++-------
.../multipart/JakartaMultiPartRequest.java | 28 +++-------
.../multipart/JakartaStreamMultiPartRequest.java | 17 ++----
.../org/apache/struts2/struts-messages.properties | 2 +
.../ActionFileUploadInterceptorTest.java | 9 ++--
.../interceptor/FileUploadInterceptorTest.java | 7 +--
6 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 1511096b0..b522a4532 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
@@ -22,7 +22,7 @@ import com.opensymphony.xwork2.LocaleProviderFactory;
import com.opensymphony.xwork2.inject.Inject;
import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
import com.opensymphony.xwork2.security.ExcludedPatternsChecker;
-import org.apache.commons.lang3.BooleanUtils;
+import org.apache.commons.io.FilenameUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
@@ -33,12 +33,17 @@ import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
+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_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);
private static final String EXCLUDED_FILE_PATTERN =
"^(.*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*)$";
@@ -88,13 +93,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) {
+ DefaultExcludedPatternsChecker patternsChecker = new
DefaultExcludedPatternsChecker();
+ patternsChecker.setAdditionalExcludePatterns(dmiValue ?
EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT : EXCLUDED_FILE_PATTERN);
+ this.patternsChecker = patternsChecker;
}
/**
@@ -174,16 +180,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);
}
/**
@@ -194,4 +191,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.trim().isEmpty()) {
+ LOG.debug(() -> "No file has been uploaded for the field: " +
normalizeSpace(fieldName));
+ return true;
+ }
+
+ if (isExcluded(fileName)) {
+ String 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)) {
+ String 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 8ed908ff9..6bbfffec8 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
@@ -27,6 +27,7 @@ import org.apache.commons.fileupload.RequestContext;
import org.apache.commons.fileupload.disk.DiskFileItem;
import org.apache.commons.fileupload.disk.DiskFileItemFactory;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
+import org.apache.commons.lang3.BooleanUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
@@ -60,13 +61,14 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
// maps parameter name -> List of param values
protected Map<String, List<String>> params = new HashMap<>();
+
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));
}
/**
@@ -123,21 +125,7 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
}
protected void processFileField(FileItem item) {
- LOG.debug("Item is a file upload");
-
- if (isExcluded(item.getName())) {
- LOG.warn("File name [{}] is not accepted",
normalizeSpace(item.getName()));
- return;
- }
-
- if (isExcluded(item.getFieldName())) {
- LOG.warn("Field name [{}] is not accepted",
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;
}
@@ -154,10 +142,10 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
protected void processNormalFormField(FileItem item, String charset)
throws UnsupportedEncodingException {
try {
- LOG.debug("Item is a normal form field");
+ String fieldName = item.getFieldName();
+ LOG.debug("Item: {} is a normal form field", fieldName);
- if (isExcluded(item.getFieldName())) {
- LOG.warn("Form field name [{}] is not accepted",
normalizeSpace(item.getFieldName()));
+ if (isInvalidInput(fieldName)) {
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 cb7475f40..655112010 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
@@ -26,6 +26,7 @@ import
org.apache.commons.fileupload.FileUploadBase.FileSizeLimitExceededExcepti
import org.apache.commons.fileupload.FileUploadException;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.commons.fileupload.util.Streams;
+import org.apache.commons.lang3.BooleanUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
@@ -209,12 +210,12 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
}
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));
}
/**
@@ -325,8 +326,7 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
*/
protected void processFileItemStreamAsFormField(FileItemStream itemStream)
{
String fieldName = itemStream.getFieldName();
- if (isExcluded(fieldName)) {
- LOG.warn("Form field [{}] rejected!", normalizeSpace(fieldName));
+ if (isInvalidInput(fieldName)) {
return;
}
try {
@@ -351,14 +351,7 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
* @param location location
*/
protected void processFileItemStreamAsFileField(FileItemStream itemStream,
String location) {
- // Skip file uploads that don't have a file name - meaning that no
file was selected.
- if (itemStream.getName() == null ||
itemStream.getName().trim().isEmpty()) {
- LOG.debug("No file has been uploaded for the field: {}",
normalizeSpace(itemStream.getFieldName()));
- return;
- }
-
- if (isExcluded(itemStream.getName())) {
- LOG.warn("File field [{}] rejected",
normalizeSpace(itemStream.getName()));
+ if (isInvalidInput(itemStream.getFieldName(), itemStream.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 57b7e2b54..b75a4144e 100644
--- a/core/src/main/resources/org/apache/struts2/struts-messages.properties
+++ b/core/src/main/resources/org/apache/struts2/struts-messages.properties
@@ -27,6 +27,8 @@ struts.messages.removing.file=Removing file {0} {1}
struts.messages.error.uploading=Error uploading: {0}
struts.messages.error.file.too.large=File {0} is too large to be uploaded.
Maximum allowed size is {4} bytes!
struts.messages.upload.error.parameter.too.long=The request parameter "{0}"
was too long. Max length allowed is {1}, but found {2}!
+struts.messages.upload.error.illegal.characters.field=The multipart upload
field name "{0}" contains illegal characters!
+struts.messages.upload.error.illegal.characters.name=The multipart upload
filename "{0}" contains illegal characters!
struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0}
"{1}" "{2}" {3}
struts.messages.error.file.extension.not.allowed=File extension not allowed:
{0} "{1}" "{2}" {3}
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 b9e64df8f..85bf5ab4b 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
@@ -24,9 +24,6 @@ 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.security.DefaultAcceptedPatternsChecker;
-import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
-import
com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPatternsChecker;
import com.opensymphony.xwork2.util.ClassLoaderUtil;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.struts2.ServletActionContext;
@@ -693,7 +690,8 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
interceptor.intercept(mai);
- assertFalse(action.hasActionErrors());
+ assertThat(action.getActionErrors())
+ .containsExactly("The multipart upload field name \"top.file\"
contains illegal characters!");
assertNull(action.getUploadFiles());
}
@@ -724,7 +722,8 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
interceptor.intercept(mai);
- assertFalse(action.hasActionErrors());
+ assertThat(action.getActionErrors())
+ .containsExactly("The multipart upload filename
\"../deleteme.txt\" contains illegal characters!");
assertNull(action.getUploadFiles());
}
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 0df273321..fe4c2ef21 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
@@ -23,7 +23,6 @@ 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.security.DefaultNotExcludedAcceptedPatternsChecker;
import com.opensymphony.xwork2.util.ClassLoaderUtil;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.struts2.ServletActionContext;
@@ -755,7 +754,8 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
interceptor.intercept(mai);
- assertFalse(action.hasActionErrors());
+ assertThat(action.getActionErrors())
+ .containsExactly("The multipart upload field name \"top.file\"
contains illegal characters!");
assertNull(action.getUploadFiles());
}
@@ -786,7 +786,8 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
interceptor.intercept(mai);
- assertFalse(action.hasActionErrors());
+ assertThat(action.getActionErrors())
+ .containsExactly("The multipart upload filename
\"../deleteme.txt\" contains illegal characters!");
assertNull(action.getUploadFiles());
}
