Skip to content

Commit fe3dd6b

Browse files
kusalkkusalk
committed
WW-5528 Ensure multipart upload illegal characters reported as error
1 parent 7a77c7a commit fe3dd6b

File tree

7 files changed

+68
-59
lines changed

7 files changed

+68
-59
lines changed

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

Lines changed: 43 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
import com.opensymphony.xwork2.inject.Inject;
2323
import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
2424
import com.opensymphony.xwork2.security.ExcludedPatternsChecker;
25-
import org.apache.commons.lang3.BooleanUtils;
25+
import org.apache.commons.io.FilenameUtils;
2626
import org.apache.logging.log4j.LogManager;
2727
import org.apache.logging.log4j.Logger;
2828
import org.apache.struts2.StrutsConstants;
@@ -33,12 +33,17 @@
3333
import java.util.List;
3434
import java.util.Locale;
3535

36+
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
37+
3638
/**
3739
* Abstract class with some helper methods, it should be used
3840
* when starting development of another implementation of {@link MultiPartRequest}
3941
*/
4042
public abstract class AbstractMultiPartRequest implements MultiPartRequest {
4143

44+
protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD = "struts.messages.upload.error.illegal.characters.field";
45+
protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME = "struts.messages.upload.error.illegal.characters.name";
46+
4247
private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class);
4348

4449
private static final String EXCLUDED_FILE_PATTERN = "^(.*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*)$";
@@ -88,13 +93,14 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
8893

8994
private final ExcludedPatternsChecker patternsChecker;
9095

91-
protected AbstractMultiPartRequest(String dmiValue) {
92-
patternsChecker = new DefaultExcludedPatternsChecker();
93-
if (BooleanUtils.toBoolean(dmiValue)) {
94-
((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT);
95-
} else {
96-
((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN);
97-
}
96+
protected AbstractMultiPartRequest() {
97+
this(false);
98+
}
99+
100+
protected AbstractMultiPartRequest(boolean dmiValue) {
101+
DefaultExcludedPatternsChecker patternsChecker = new DefaultExcludedPatternsChecker();
102+
patternsChecker.setAdditionalExcludePatterns(dmiValue ? EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT : EXCLUDED_FILE_PATTERN);
103+
this.patternsChecker = patternsChecker;
98104
}
99105

100106
/**
@@ -174,16 +180,7 @@ public List<LocalizedMessage> getErrors() {
174180
* @return the canonical name based on the supplied filename
175181
*/
176182
protected String getCanonicalName(final String originalFileName) {
177-
String fileName = originalFileName;
178-
179-
int forwardSlash = fileName.lastIndexOf('/');
180-
int backwardSlash = fileName.lastIndexOf('\\');
181-
if (forwardSlash != -1 && forwardSlash > backwardSlash) {
182-
fileName = fileName.substring(forwardSlash + 1);
183-
} else {
184-
fileName = fileName.substring(backwardSlash + 1);
185-
}
186-
return fileName;
183+
return FilenameUtils.getName(originalFileName);
187184
}
188185

189186
/**
@@ -194,4 +191,32 @@ protected boolean isExcluded(String fileName) {
194191
return patternsChecker.isExcluded(fileName).isExcluded();
195192
}
196193

194+
protected boolean isInvalidInput(String fieldName, String fileName) {
195+
// Skip file uploads that don't have a file name - meaning that no file was selected.
196+
if (fileName == null || fileName.trim().isEmpty()) {
197+
LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fieldName));
198+
return true;
199+
}
200+
201+
if (isExcluded(fileName)) {
202+
String normalizedFileName = normalizeSpace(fileName);
203+
LOG.debug("File name [{}] is not accepted", normalizedFileName);
204+
errors.add(new LocalizedMessage(getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME, null,
205+
new String[]{normalizedFileName}));
206+
return true;
207+
}
208+
209+
return isInvalidInput(fieldName);
210+
}
211+
212+
protected boolean isInvalidInput(String fieldName) {
213+
if (isExcluded(fieldName)) {
214+
String normalizedFieldName = normalizeSpace(fieldName);
215+
LOG.debug("Form field [{}] is rejected!", normalizedFieldName);
216+
errors.add(new LocalizedMessage(getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD, null,
217+
new String[]{normalizedFieldName}));
218+
return true;
219+
}
220+
return false;
221+
}
197222
}

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

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.apache.commons.fileupload.disk.DiskFileItem;
2828
import org.apache.commons.fileupload.disk.DiskFileItemFactory;
2929
import org.apache.commons.fileupload.servlet.ServletFileUpload;
30+
import org.apache.commons.lang3.BooleanUtils;
3031
import org.apache.commons.lang3.StringUtils;
3132
import org.apache.logging.log4j.LogManager;
3233
import org.apache.logging.log4j.Logger;
@@ -60,13 +61,14 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
6061

6162
// maps parameter name -> List of param values
6263
protected Map<String, List<String>> params = new HashMap<>();
64+
6365
public JakartaMultiPartRequest() {
64-
super(Boolean.FALSE.toString());
66+
super();
6567
}
6668

6769
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
6870
public JakartaMultiPartRequest(String dmiValue) {
69-
super(dmiValue);
71+
super(BooleanUtils.toBoolean(dmiValue));
7072
}
7173

7274
/**
@@ -123,21 +125,7 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws
123125
}
124126

125127
protected void processFileField(FileItem item) {
126-
LOG.debug("Item is a file upload");
127-
128-
if (isExcluded(item.getName())) {
129-
LOG.warn("File name [{}] is not accepted", normalizeSpace(item.getName()));
130-
return;
131-
}
132-
133-
if (isExcluded(item.getFieldName())) {
134-
LOG.warn("Field name [{}] is not accepted", normalizeSpace(item.getFieldName()));
135-
return;
136-
}
137-
138-
// Skip file uploads that don't have a file name - meaning that no file was selected.
139-
if (item.getName() == null || item.getName().trim().isEmpty()) {
140-
LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(item.getFieldName()));
128+
if (isInvalidInput(item.getFieldName(), item.getName())) {
141129
return;
142130
}
143131

@@ -154,10 +142,10 @@ protected void processFileField(FileItem item) {
154142

155143
protected void processNormalFormField(FileItem item, String charset) throws UnsupportedEncodingException {
156144
try {
157-
LOG.debug("Item is a normal form field");
145+
String fieldName = item.getFieldName();
146+
LOG.debug("Item: {} is a normal form field", normalizeSpace(fieldName));
158147

159-
if (isExcluded(item.getFieldName())) {
160-
LOG.warn("Form field name [{}] is not accepted", normalizeSpace(item.getFieldName()));
148+
if (isInvalidInput(fieldName)) {
161149
return;
162150
}
163151

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

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.apache.commons.fileupload.FileUploadException;
2727
import org.apache.commons.fileupload.servlet.ServletFileUpload;
2828
import org.apache.commons.fileupload.util.Streams;
29+
import org.apache.commons.lang3.BooleanUtils;
2930
import org.apache.logging.log4j.LogManager;
3031
import org.apache.logging.log4j.Logger;
3132
import org.apache.struts2.StrutsConstants;
@@ -209,12 +210,12 @@ public void parse(HttpServletRequest request, String saveDir) throws IOException
209210
}
210211

211212
public JakartaStreamMultiPartRequest() {
212-
super(Boolean.FALSE.toString());
213+
super();
213214
}
214215

215216
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
216217
public JakartaStreamMultiPartRequest(String dmiValue) {
217-
super(dmiValue);
218+
super(BooleanUtils.toBoolean(dmiValue));
218219
}
219220

220221
/**
@@ -325,8 +326,7 @@ protected void addFileSkippedError(String fileName, HttpServletRequest request)
325326
*/
326327
protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
327328
String fieldName = itemStream.getFieldName();
328-
if (isExcluded(fieldName)) {
329-
LOG.warn("Form field [{}] rejected!", normalizeSpace(fieldName));
329+
if (isInvalidInput(fieldName)) {
330330
return;
331331
}
332332
try {
@@ -351,14 +351,7 @@ protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
351351
* @param location location
352352
*/
353353
protected void processFileItemStreamAsFileField(FileItemStream itemStream, String location) {
354-
// Skip file uploads that don't have a file name - meaning that no file was selected.
355-
if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
356-
LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(itemStream.getFieldName()));
357-
return;
358-
}
359-
360-
if (isExcluded(itemStream.getName())) {
361-
LOG.warn("File field [{}] rejected", normalizeSpace(itemStream.getName()));
354+
if (isInvalidInput(itemStream.getFieldName(), itemStream.getName())) {
362355
return;
363356
}
364357

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ struts.messages.removing.file=Removing file {0} {1}
2727
struts.messages.error.uploading=Error uploading: {0}
2828
struts.messages.error.file.too.large=File {0} is too large to be uploaded. Maximum allowed size is {4} bytes!
2929
struts.messages.upload.error.parameter.too.long=The request parameter "{0}" was too long. Max length allowed is {1}, but found {2}!
30+
struts.messages.upload.error.illegal.characters.field=The multipart upload field name "{0}" contains illegal characters!
31+
struts.messages.upload.error.illegal.characters.name=The multipart upload filename "{0}" contains illegal characters!
3032
struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0} "{1}" "{2}" {3}
3133
struts.messages.error.file.extension.not.allowed=File extension not allowed: {0} "{1}" "{2}" {3}
3234

‎core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,6 @@
2424
import com.opensymphony.xwork2.ValidationAwareSupport;
2525
import com.opensymphony.xwork2.mock.MockActionInvocation;
2626
import com.opensymphony.xwork2.mock.MockActionProxy;
27-
import com.opensymphony.xwork2.security.DefaultAcceptedPatternsChecker;
28-
import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
29-
import com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPatternsChecker;
3027
import com.opensymphony.xwork2.util.ClassLoaderUtil;
3128
import org.apache.commons.fileupload.servlet.ServletFileUpload;
3229
import org.apache.struts2.ServletActionContext;
@@ -693,7 +690,8 @@ public void testUnacceptedFieldName() throws Exception {
693690

694691
interceptor.intercept(mai);
695692

696-
assertFalse(action.hasActionErrors());
693+
assertThat(action.getActionErrors())
694+
.containsExactly("The multipart upload field name \"top.file\" contains illegal characters!");
697695
assertNull(action.getUploadFiles());
698696
}
699697

@@ -724,7 +722,8 @@ public void testUnacceptedFileName() throws Exception {
724722

725723
interceptor.intercept(mai);
726724

727-
assertFalse(action.hasActionErrors());
725+
assertThat(action.getActionErrors())
726+
.containsExactly("The multipart upload filename \"../deleteme.txt\" contains illegal characters!");
728727
assertNull(action.getUploadFiles());
729728
}
730729

‎core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import com.opensymphony.xwork2.DefaultLocaleProvider;
2424
import com.opensymphony.xwork2.ValidationAwareSupport;
2525
import com.opensymphony.xwork2.mock.MockActionInvocation;
26-
import com.opensymphony.xwork2.security.DefaultNotExcludedAcceptedPatternsChecker;
2726
import com.opensymphony.xwork2.util.ClassLoaderUtil;
2827
import org.apache.commons.fileupload.servlet.ServletFileUpload;
2928
import org.apache.struts2.ServletActionContext;
@@ -755,7 +754,8 @@ public void testUnacceptedFieldName() throws Exception {
755754

756755
interceptor.intercept(mai);
757756

758-
assertFalse(action.hasActionErrors());
757+
assertThat(action.getActionErrors())
758+
.containsExactly("The multipart upload field name \"top.file\" contains illegal characters!");
759759
assertNull(action.getUploadFiles());
760760
}
761761

@@ -786,7 +786,8 @@ public void testUnacceptedFileName() throws Exception {
786786

787787
interceptor.intercept(mai);
788788

789-
assertFalse(action.hasActionErrors());
789+
assertThat(action.getActionErrors())
790+
.containsExactly("The multipart upload filename \"../deleteme.txt\" contains illegal characters!");
790791
assertNull(action.getUploadFiles());
791792
}
792793

‎plugins/pell-multipart/src/main/java/org/apache/struts2/dispatcher/multipart/PellMultiPartRequest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import com.opensymphony.xwork2.inject.Inject;
2222
import http.utils.multipartrequest.ServletMultipartRequest;
23+
import org.apache.commons.lang3.BooleanUtils;
2324
import org.apache.logging.log4j.LogManager;
2425
import org.apache.logging.log4j.Logger;
2526
import org.apache.struts2.StrutsConstants;
@@ -42,7 +43,7 @@ public class PellMultiPartRequest extends AbstractMultiPartRequest {
4243

4344
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
4445
public PellMultiPartRequest(String dmiValue) {
45-
super(dmiValue);
46+
super(BooleanUtils.toBoolean(dmiValue));
4647
}
4748

4849
/**

0 commit comments

Comments
 (0)