From 09993e3476feca1c6700b55270bf4636aea8c440 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 23 Dec 2024 11:00:29 +0100 Subject: [PATCH 1/2] WW-5501 Exclude malicious names --- .../DefaultExcludedPatternsChecker.java | 1 + .../multipart/AbstractMultiPartRequest.java | 15 ++++ .../multipart/JakartaMultiPartRequest.java | 22 ++++-- .../JakartaStreamMultiPartRequest.java | 25 +++++-- .../DefaultExcludedPatternsCheckerTest.java | 2 +- .../ActionFileUploadInterceptorTest.java | 73 ++++++++++++++++++- .../FileUploadInterceptorTest.java | 73 ++++++++++++++++++- 7 files changed, 192 insertions(+), 19 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java b/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java index cf425d67c..fc96bb0f9 100644 --- a/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java +++ b/core/src/main/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsChecker.java @@ -36,6 +36,7 @@ public class DefaultExcludedPatternsChecker implements ExcludedPatternsChecker { private static final Logger LOG = LogManager.getLogger(DefaultExcludedPatternsChecker.class); public static final String[] EXCLUDED_PATTERNS = { + "(^|\\%\\{)(#?top\\.)[^\\s]*", "(^|\\%\\{)((#?)(top(\\.|\\['|\\[\")|\\[\\d\\]\\.)?)(dojo|struts|session|request|response|application|servlet(Request|Response|Context)|parameters|context|_memberAccess)(\\.|\\[).*", ".*(^|\\.|\\[|\\'|\"|get)class(\\(\\.|\\[|\\'|\").*", "actionErrors|actionMessages|fieldErrors" 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 23d879ba4..f19b9ebd2 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,6 +20,7 @@ package org.apache.struts2.dispatcher.multipart; import com.opensymphony.xwork2.LocaleProviderFactory; import com.opensymphony.xwork2.inject.Inject; +import com.opensymphony.xwork2.security.NotExcludedAcceptedPatternsChecker; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; @@ -79,6 +80,7 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { * Localization to be used regarding errors. */ protected Locale defaultLocale = Locale.ENGLISH; + private NotExcludedAcceptedPatternsChecker patternsChecker; /** * @param bufferSize Sets the buffer size to be used. @@ -121,6 +123,11 @@ 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. @@ -169,4 +176,12 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return fileName; } + protected boolean isAccepted(String fileName) { + return patternsChecker.isAllowed(fileName).isAllowed(); + } + + protected String sanitizeNewlines(String before) { + return before.replaceAll("[\n\r]", "_"); + } + } 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 c2cc07dbb..e6acf8a6f 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 @@ -113,6 +113,16 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { protected void processFileField(FileItem item) { LOG.debug("Item is a file upload"); + if (!isAccepted(item.getName())) { + LOG.warn("File name [{}] is not accepted", sanitizeNewlines(item.getName())); + return; + } + + if (!isAccepted(item.getFieldName())) { + LOG.warn("Field name [{}] is not accepted", sanitizeNewlines(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: {}", sanitizeNewlines(item.getFieldName())); @@ -134,6 +144,11 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { try { LOG.debug("Item is a normal form field"); + if (!isAccepted(item.getFieldName())) { + LOG.warn("Form field name [{}] is not accepted", sanitizeNewlines(item.getFieldName())); + return; + } + List values; if (params.get(item.getFieldName()) != null) { values = params.get(item.getFieldName()); @@ -143,7 +158,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { long size = item.getSize(); if (maxStringLength != null && size > maxStringLength) { - LOG.debug("Form field {} of size {} bytes exceeds limit of {}.", sanitizeNewlines(item.getFieldName()), size, maxStringLength); + LOG.debug("Form field [{}] of size [{}] bytes exceeds limit of [{}].", sanitizeNewlines(item.getFieldName()), size, maxStringLength); String errorKey = "struts.messages.upload.error.parameter.too.long"; LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), errorKey, null, new Object[]{item.getFieldName(), maxStringLength, size}); @@ -359,7 +374,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { for (String name : names) { List items = files.get(name); for (FileItem item : items) { - LOG.debug("Removing file {} {}", name, item); + LOG.debug("Removing file [{}]", sanitizeNewlines(name)); if (!item.isInMemory()) { item.delete(); } @@ -367,7 +382,4 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { } } - private String sanitizeNewlines(String before) { - return before.replaceAll("[\n\r]", "_"); - } } 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 3985e0f52..539fd6cbb 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 @@ -77,7 +77,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { File file = fileInfo.getFile(); LOG.debug("Deleting file '{}'.", file.getName()); if (!file.delete()) { - LOG.warn("There was a problem attempting to delete file '{}'.", file.getName()); + LOG.warn("There was a problem attempting to delete file [{}].", file.getName()); } } } @@ -252,7 +252,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { // prevent processing file field item if request size not allowed. if (!requestSizePermitted) { addFileSkippedError(itemStream.getName(), request); - LOG.debug("Skipped stream '{}', request maximum size ({}) exceeded.", itemStream.getName(), maxSize); + LOG.debug("Skipped stream [{}], request maximum size ({}) exceeded.", sanitizeNewlines(itemStream.getName()), maxSize); continue; } @@ -296,7 +296,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { * @param request the servlet request */ protected void addFileSkippedError(String fileName, HttpServletRequest request) { - String exceptionMessage = "Skipped file " + fileName + "; request size limit exceeded."; + String exceptionMessage = "Skipped file " + sanitizeNewlines(fileName) + "; request size limit exceeded."; long allowedMaxSize = maxSize != null ? maxSize : -1; FileSizeLimitExceededException exception = new FileUploadBase.FileSizeLimitExceededException(exceptionMessage, getRequestSize(request), allowedMaxSize); LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize}); @@ -312,6 +312,10 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { */ protected void processFileItemStreamAsFormField(FileItemStream itemStream) { String fieldName = itemStream.getFieldName(); + if (!isAccepted(fieldName)) { + LOG.warn("Form field [{}] rejected!", sanitizeNewlines(fieldName)); + return; + } try { List values; String fieldValue = Streams.asString(itemStream.openStream()); @@ -323,7 +327,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } values.add(fieldValue); } catch (IOException e) { - LOG.warn("Failed to handle form field '{}'.", fieldName, e); + LOG.warn("Failed to handle form field [{}]", sanitizeNewlines(fieldName), e); } } @@ -336,7 +340,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { 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: {}", itemStream.getFieldName()); + LOG.debug("No file has been uploaded for the field: {}", sanitizeNewlines(itemStream.getFieldName())); + return; + } + + if (!isAccepted(itemStream.getName())) { + LOG.warn("File field [{}] rejected", sanitizeNewlines(itemStream.getName())); return; } @@ -353,7 +362,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { try { file.delete(); } catch (SecurityException se) { - LOG.warn("Failed to delete '{}' due to security exception above.", file.getName(), se); + LOG.warn("Failed to delete [{}] due to security exception above.", sanitizeNewlines(file.getName()), se); } } } @@ -385,7 +394,9 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } File file = File.createTempFile(prefix + "_", suffix, new File(location)); - LOG.debug("Creating temporary file '{}' (originally '{}').", file.getName(), fileName); + if (LOG.isDebugEnabled()) { + LOG.debug("Creating temporary file [{}] (originally [{}]).", file.getName(), sanitizeNewlines(fileName)); + } return file; } diff --git a/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java b/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java index 738def5d6..114354dd5 100644 --- a/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java +++ b/core/src/test/java/com/opensymphony/xwork2/security/DefaultExcludedPatternsCheckerTest.java @@ -93,7 +93,7 @@ public class DefaultExcludedPatternsCheckerTest extends XWorkTestCase { public void testDefaultExcludePatterns() throws Exception { // given - List prefixes = Arrays.asList("#[0].%s", "[0].%s", "top.%s", "%{[0].%s}", "%{#[0].%s}", "%{top.%s}", "%{#top.%s}", "%{#%s}", "%{%s}", "#%s"); + List prefixes = Arrays.asList("#[0].%s", "[0].%s", "top.%s", "%{[0].%s}", "%{#[0].%s}", "%{top.%s}", "%{#top.%s}", "%{#%s}", "%{%s}", "#%s", "top.param", "top.request"); List inners = Arrays.asList("servletRequest", "servletResponse", "servletContext", "application", "session", "struts", "request", "response", "dojo", "parameters"); List suffixes = Arrays.asList("['test']", "[\"test\"]", ".test"); 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 81aa122ed..819ec89e0 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -24,6 +24,9 @@ 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; @@ -663,6 +666,68 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte Größe")); } + public void testUnacceptedFieldName() throws Exception { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setCharacterEncoding(StandardCharsets.UTF_8.name()); + req.setMethod("post"); + req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + + // inspired by the unit tests for jakarta commons fileupload + String content = ("-----1234\r\n" + + "Content-Disposition: form-data; name=\"top.file\"; filename=\"deleteme.txt\"\r\n" + + "Content-Type: text/html\r\n" + + "\r\n" + + "Unit test of ActionFileUploadInterceptor" + + "\r\n" + + "-----1234--\r\n"); + req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + MyFileUploadAction action = container.inject(MyFileUploadAction.class); + + MockActionInvocation mai = new MockActionInvocation(); + mai.setAction(action); + mai.setResultCode("success"); + mai.setInvocationContext(ActionContext.getContext()); + ActionContext.getContext() + .withServletRequest(createMultipartRequestMaxSize(req, 2000)); + + interceptor.intercept(mai); + + assertFalse(action.hasActionErrors()); + assertNull(action.getUploadFiles()); + } + + public void testUnacceptedFileName() throws Exception { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setCharacterEncoding(StandardCharsets.UTF_8.name()); + req.setMethod("post"); + req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + + // inspired by the unit tests for jakarta commons fileupload + String content = ("-----1234\r\n" + + "Content-Disposition: form-data; name=\"file\"; filename=\"../deleteme.txt\"\r\n" + + "Content-Type: text/html\r\n" + + "\r\n" + + "Unit test of ActionFileUploadInterceptor" + + "\r\n" + + "-----1234--\r\n"); + req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + MyFileUploadAction action = container.inject(MyFileUploadAction.class); + + MockActionInvocation mai = new MockActionInvocation(); + mai.setAction(action); + mai.setResultCode("success"); + mai.setInvocationContext(ActionContext.getContext()); + ActionContext.getContext() + .withServletRequest(createMultipartRequestMaxSize(req, 2000)); + + interceptor.intercept(mai); + + assertFalse(action.hasActionErrors()); + assertNull(action.getUploadFiles()); + } + private String encodeTextFile(String filename, String contentType, String content) { return "\r\n" + "--" + @@ -672,7 +737,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { "file" + "\"; filename=\"" + filename + - "\r\n" + + "\"\r\n" + "Content-Type: " + contentType + "\r\n" + @@ -697,18 +762,20 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { - JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); jak.setMaxSize(String.valueOf(maxsize)); 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 36fad5847..4fd91ecd2 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java @@ -23,6 +23,7 @@ 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; @@ -727,6 +728,68 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte Größe")); } + public void testUnacceptedFieldName() throws Exception { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setCharacterEncoding(StandardCharsets.UTF_8.name()); + req.setMethod("post"); + req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + + // inspired by the unit tests for jakarta commons fileupload + String content = ("-----1234\r\n" + + "Content-Disposition: form-data; name=\"top.file\"; filename=\"deleteme.txt\"\r\n" + + "Content-Type: text/html\r\n" + + "\r\n" + + "Unit test of ActionFileUploadInterceptor" + + "\r\n" + + "-----1234--\r\n"); + req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + ActionFileUploadInterceptorTest.MyFileUploadAction action = container.inject(ActionFileUploadInterceptorTest.MyFileUploadAction.class); + + MockActionInvocation mai = new MockActionInvocation(); + mai.setAction(action); + mai.setResultCode("success"); + mai.setInvocationContext(ActionContext.getContext()); + ActionContext.getContext() + .withServletRequest(createMultipartRequestMaxSize(req, 2000)); + + interceptor.intercept(mai); + + assertFalse(action.hasActionErrors()); + assertNull(action.getUploadFiles()); + } + + public void testUnacceptedFileName() throws Exception { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setCharacterEncoding(StandardCharsets.UTF_8.name()); + req.setMethod("post"); + req.addHeader("Content-type", "multipart/form-data; boundary=---1234"); + + // inspired by the unit tests for jakarta commons fileupload + String content = ("-----1234\r\n" + + "Content-Disposition: form-data; name=\"file\"; filename=\"../deleteme.txt\"\r\n" + + "Content-Type: text/html\r\n" + + "\r\n" + + "Unit test of ActionFileUploadInterceptor" + + "\r\n" + + "-----1234--\r\n"); + req.setContent(content.getBytes(StandardCharsets.US_ASCII)); + + ActionFileUploadInterceptorTest.MyFileUploadAction action = container.inject(ActionFileUploadInterceptorTest.MyFileUploadAction.class); + + MockActionInvocation mai = new MockActionInvocation(); + mai.setAction(action); + mai.setResultCode("success"); + mai.setInvocationContext(ActionContext.getContext()); + ActionContext.getContext() + .withServletRequest(createMultipartRequestMaxSize(req, 2000)); + + interceptor.intercept(mai); + + assertFalse(action.hasActionErrors()); + assertNull(action.getUploadFiles()); + } + private String encodeTextFile(String filename, String contentType, String content) { return "\r\n" + "--" + @@ -736,7 +799,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { "file" + "\"; filename=\"" + filename + - "\r\n" + + "\"\r\n" + "Content-Type: " + contentType + "\r\n" + @@ -761,18 +824,22 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase { } private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxfilesize, int maxfiles, int maxStringLength) { - JakartaMultiPartRequest jak = new JakartaMultiPartRequest(); jak.setMaxSize(String.valueOf(maxsize)); 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()); } From 3ea126388c9d90a825cf2ef782093584e34d2277 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 23 Dec 2024 14:13:26 +0100 Subject: [PATCH 2/2] WW-5501 Uses StringUtils.normalizeSpace instead of sanitizeNewlines --- .../multipart/AbstractMultiPartRequest.java | 4 ---- .../multipart/JakartaMultiPartRequest.java | 16 +++++++++------- .../JakartaStreamMultiPartRequest.java | 19 +++++++++++-------- 3 files changed, 20 insertions(+), 19 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 f19b9ebd2..ed013b724 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 @@ -180,8 +180,4 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest { return patternsChecker.isAllowed(fileName).isAllowed(); } - protected String sanitizeNewlines(String before) { - return before.replaceAll("[\n\r]", "_"); - } - } 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 e6acf8a6f..0367ac5e6 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 @@ -44,6 +44,8 @@ import java.util.List; import java.util.Map; import java.util.Set; +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + /** * Multipart form data request adapter for Jakarta Commons Fileupload package. */ @@ -100,7 +102,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { protected void processUpload(HttpServletRequest request, String saveDir) throws FileUploadException, UnsupportedEncodingException { if (ServletFileUpload.isMultipartContent(request)) { for (FileItem item : parseRequest(request, saveDir)) { - LOG.debug("Found file item: [{}]", sanitizeNewlines(item.getFieldName())); + LOG.debug("Found file item: [{}]", normalizeSpace(item.getFieldName())); if (item.isFormField()) { processNormalFormField(item, request.getCharacterEncoding()); } else { @@ -114,18 +116,18 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { LOG.debug("Item is a file upload"); if (!isAccepted(item.getName())) { - LOG.warn("File name [{}] is not accepted", sanitizeNewlines(item.getName())); + LOG.warn("File name [{}] is not accepted", normalizeSpace(item.getName())); return; } if (!isAccepted(item.getFieldName())) { - LOG.warn("Field name [{}] is not accepted", sanitizeNewlines(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: {}", sanitizeNewlines(item.getFieldName())); + LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(item.getFieldName())); return; } @@ -145,7 +147,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { LOG.debug("Item is a normal form field"); if (!isAccepted(item.getFieldName())) { - LOG.warn("Form field name [{}] is not accepted", sanitizeNewlines(item.getFieldName())); + LOG.warn("Form field name [{}] is not accepted", normalizeSpace(item.getFieldName())); return; } @@ -158,7 +160,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { long size = item.getSize(); if (maxStringLength != null && size > maxStringLength) { - LOG.debug("Form field [{}] of size [{}] bytes exceeds limit of [{}].", sanitizeNewlines(item.getFieldName()), size, maxStringLength); + LOG.debug("Form field [{}] of size [{}] bytes exceeds limit of [{}].", normalizeSpace(item.getFieldName()), size, maxStringLength); String errorKey = "struts.messages.upload.error.parameter.too.long"; LocalizedMessage localizedMessage = new LocalizedMessage(this.getClass(), errorKey, null, new Object[]{item.getFieldName(), maxStringLength, size}); @@ -374,7 +376,7 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest { for (String name : names) { List items = files.get(name); for (FileItem item : items) { - LOG.debug("Removing file [{}]", sanitizeNewlines(name)); + LOG.debug("Removing file [{}]", normalizeSpace(name)); if (!item.isInMemory()) { item.delete(); } 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 539fd6cbb..9abe7a011 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,6 +25,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.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.dispatcher.LocalizedMessage; @@ -45,6 +46,8 @@ import java.util.List; import java.util.Map; import java.util.UUID; +import static org.apache.commons.lang3.StringUtils.normalizeSpace; + /** * Multi-part form data request adapter for Jakarta Commons FileUpload package that * leverages the streaming API rather than the traditional non-streaming API. @@ -252,7 +255,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { // prevent processing file field item if request size not allowed. if (!requestSizePermitted) { addFileSkippedError(itemStream.getName(), request); - LOG.debug("Skipped stream [{}], request maximum size ({}) exceeded.", sanitizeNewlines(itemStream.getName()), maxSize); + LOG.debug("Skipped stream [{}], request maximum size ({}) exceeded.", normalizeSpace(itemStream.getName()), maxSize); continue; } @@ -296,7 +299,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { * @param request the servlet request */ protected void addFileSkippedError(String fileName, HttpServletRequest request) { - String exceptionMessage = "Skipped file " + sanitizeNewlines(fileName) + "; request size limit exceeded."; + String exceptionMessage = "Skipped file " + normalizeSpace(fileName) + "; request size limit exceeded."; long allowedMaxSize = maxSize != null ? maxSize : -1; FileSizeLimitExceededException exception = new FileUploadBase.FileSizeLimitExceededException(exceptionMessage, getRequestSize(request), allowedMaxSize); LocalizedMessage message = buildErrorMessage(exception, new Object[]{fileName, getRequestSize(request), allowedMaxSize}); @@ -313,7 +316,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { protected void processFileItemStreamAsFormField(FileItemStream itemStream) { String fieldName = itemStream.getFieldName(); if (!isAccepted(fieldName)) { - LOG.warn("Form field [{}] rejected!", sanitizeNewlines(fieldName)); + LOG.warn("Form field [{}] rejected!", normalizeSpace(fieldName)); return; } try { @@ -327,7 +330,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { } values.add(fieldValue); } catch (IOException e) { - LOG.warn("Failed to handle form field [{}]", sanitizeNewlines(fieldName), e); + LOG.warn("Failed to handle form field [{}]", normalizeSpace(fieldName), e); } } @@ -340,12 +343,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { 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: {}", sanitizeNewlines(itemStream.getFieldName())); + LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(itemStream.getFieldName())); return; } if (!isAccepted(itemStream.getName())) { - LOG.warn("File field [{}] rejected", sanitizeNewlines(itemStream.getName())); + LOG.warn("File field [{}] rejected", normalizeSpace(itemStream.getName())); return; } @@ -362,7 +365,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { try { file.delete(); } catch (SecurityException se) { - LOG.warn("Failed to delete [{}] due to security exception above.", sanitizeNewlines(file.getName()), se); + LOG.warn("Failed to delete [{}] due to security exception above.", normalizeSpace(file.getName()), se); } } } @@ -395,7 +398,7 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest { File file = File.createTempFile(prefix + "_", suffix, new File(location)); if (LOG.isDebugEnabled()) { - LOG.debug("Creating temporary file [{}] (originally [{}]).", file.getName(), sanitizeNewlines(fileName)); + LOG.debug("Creating temporary file [{}] (originally [{}]).", file.getName(), normalizeSpace(fileName)); } return file; }