This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch fix/WW-5501-exclude
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 4b25e174df7b57eaaf1e24dda8627e781b62b33f
Author: Lukasz Lenart <lukaszlen...@apache.org>
AuthorDate: Fri Jan 10 07:45:57 2025 +0100
WW-5501 Only exclude malicious file names
---
.../multipart/AbstractMultiPartRequest.java | 41 +++++++++++++++-------
.../multipart/JakartaMultiPartRequest.java | 6 ++--
.../multipart/JakartaStreamMultiPartRequest.java | 5 ++-
.../ActionFileUploadInterceptorTest.java | 4 ---
.../interceptor/FileUploadInterceptorTest.java | 4 ---
5 files changed, 33 insertions(+), 27 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 ed013b724..8d04f2980 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
@@ -20,7 +20,8 @@ package org.apache.struts2.dispatcher.multipart;
import com.opensymphony.xwork2.LocaleProviderFactory;
import com.opensymphony.xwork2.inject.Inject;
-import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker;
+import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
+import com.opensymphony.xwork2.security.ExcludedPatternsChecker;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.StrutsConstants;
@@ -28,8 +29,10 @@ import org.apache.struts2.dispatcher.LocalizedMessage;
import javax.servlet.http.HttpServletRequest;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.List;
import java.util.Locale;
+import java.util.stream.Stream;
/**
* Abstract class with some helper methods, it should be used
@@ -39,13 +42,15 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
private static final Logger LOG =
LogManager.getLogger(AbstractMultiPartRequest.class);
+ private static final String EXCLUDED_FILE_PATTERN =
".*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*";
+
/**
* Defines the internal buffer size used during streaming operations.
*/
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<LocalizedMessage> errors = new ArrayList<>();
@@ -80,7 +85,18 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
* Localization to be used regarding errors.
*/
protected Locale defaultLocale = Locale.ENGLISH;
- private NotExcludedAcceptedPatternsChecker patternsChecker;
+
+ private final ExcludedPatternsChecker patternsChecker;
+
+ public AbstractMultiPartRequest() {
+ patternsChecker = new DefaultExcludedPatternsChecker();
+ String[] patterns = Stream.concat(
+ Arrays.stream(new String[]{EXCLUDED_FILE_PATTERN}),
+
Arrays.stream(DefaultExcludedPatternsChecker.EXCLUDED_PATTERNS)
+ )
+ .toArray(String[]::new);
+ patternsChecker.setExcludedPatterns(patterns);
+ }
/**
* @param bufferSize Sets the buffer size to be used.
@@ -123,14 +139,9 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
defaultLocale =
localeProviderFactory.createLocaleProvider().getLocale();
}
- @Inject
- public void
setNotExcludedAllowedPatternsChecker(NotExcludedAcceptedPatternsChecker
patternsChecker) {
- this.patternsChecker = patternsChecker;
- }
-
/**
* @param request Inspect the servlet request and set the locale if one
wasn't provided by
- * the Struts2 framework.
+ * the Struts2 framework.
*/
protected void setLocale(HttpServletRequest request) {
if (defaultLocale == null) {
@@ -141,7 +152,7 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
/**
* Build error message.
*
- * @param e the Throwable/Exception
+ * @param e the Throwable/Exception
* @param args arguments
* @return error message
*/
@@ -154,7 +165,7 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
/* (non-Javadoc)
* @see
org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors()
- */
+ */
public List<LocalizedMessage> getErrors() {
return errors;
}
@@ -176,8 +187,12 @@ public abstract class AbstractMultiPartRequest implements
MultiPartRequest {
return fileName;
}
- protected boolean isAccepted(String fileName) {
- return patternsChecker.isAllowed(fileName).isAllowed();
+ /**
+ * @param fileName file name to check
+ * @return true if the file name is excluded
+ */
+ protected boolean isExcluded(String fileName) {
+ return patternsChecker.isExcluded(fileName).isExcluded();
}
}
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 0367ac5e6..e78dea6eb 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
@@ -115,12 +115,12 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
protected void processFileField(FileItem item) {
LOG.debug("Item is a file upload");
- if (!isAccepted(item.getName())) {
+ if (isExcluded(item.getName())) {
LOG.warn("File name [{}] is not accepted",
normalizeSpace(item.getName()));
return;
}
- if (!isAccepted(item.getFieldName())) {
+ if (isExcluded(item.getFieldName())) {
LOG.warn("Field name [{}] is not accepted",
normalizeSpace(item.getFieldName()));
return;
}
@@ -146,7 +146,7 @@ public class JakartaMultiPartRequest extends
AbstractMultiPartRequest {
try {
LOG.debug("Item is a normal form field");
- if (!isAccepted(item.getFieldName())) {
+ if (isExcluded(item.getFieldName())) {
LOG.warn("Form field name [{}] is not accepted",
normalizeSpace(item.getFieldName()));
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 9abe7a011..757f56b49 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
@@ -25,7 +25,6 @@ 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.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.struts2.dispatcher.LocalizedMessage;
@@ -315,7 +314,7 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
*/
protected void processFileItemStreamAsFormField(FileItemStream itemStream)
{
String fieldName = itemStream.getFieldName();
- if (!isAccepted(fieldName)) {
+ if (isExcluded(fieldName)) {
LOG.warn("Form field [{}] rejected!", normalizeSpace(fieldName));
return;
}
@@ -347,7 +346,7 @@ public class JakartaStreamMultiPartRequest extends
AbstractMultiPartRequest {
return;
}
- if (!isAccepted(itemStream.getName())) {
+ if (isExcluded(itemStream.getName())) {
LOG.warn("File field [{}] rejected",
normalizeSpace(itemStream.getName()));
return;
}
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 819ec89e0..b9e64df8f 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
@@ -767,15 +767,11 @@ public class ActionFileUploadInterceptorTest extends
StrutsInternalTestCase {
jak.setMaxFileSize(String.valueOf(maxfilesize));
jak.setMaxFiles(String.valueOf(maxfiles));
jak.setMaxStringLength(String.valueOf(maxStringLength));
- DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
- jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
private MultiPartRequestWrapper
createMultipartRequestNoMaxParamsSet(HttpServletRequest req) {
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
- DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
- jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
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 4fd91ecd2..0df273321 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
@@ -829,16 +829,12 @@ public class FileUploadInterceptorTest extends
StrutsInternalTestCase {
jak.setMaxFileSize(String.valueOf(maxfilesize));
jak.setMaxFiles(String.valueOf(maxfiles));
jak.setMaxStringLength(String.valueOf(maxStringLength));
- DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
- jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
private MultiPartRequestWrapper
createMultipartRequestNoMaxParamsSet(HttpServletRequest req) {
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
- DefaultNotExcludedAcceptedPatternsChecker patternsChecker =
container.inject(DefaultNotExcludedAcceptedPatternsChecker.class);
- jak.setNotExcludedAllowedPatternsChecker(patternsChecker);
return new MultiPartRequestWrapper(jak, req,
tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}
