Skip to content

Commit ff249c6

Browse files
kusalkkusalk
committed
WW-5528 Ensure multipart upload illegal characters reported as error
1 parent a1de1cf commit ff249c6

File tree

6 files changed

+77
-67
lines changed

6 files changed

+77
-67
lines changed

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

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,13 @@
1818
*/
1919
package org.apache.struts2.dispatcher.multipart;
2020

21-
import jakarta.servlet.http.HttpServletRequest;
2221
import org.apache.commons.fileupload2.core.FileUploadByteCountLimitException;
2322
import org.apache.commons.fileupload2.core.FileUploadContentTypeException;
2423
import org.apache.commons.fileupload2.core.FileUploadException;
2524
import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
2625
import org.apache.commons.fileupload2.core.FileUploadSizeException;
2726
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
28-
import org.apache.commons.lang3.BooleanUtils;
27+
import org.apache.commons.io.FilenameUtils;
2928
import org.apache.commons.lang3.StringUtils;
3029
import org.apache.logging.log4j.LogManager;
3130
import org.apache.logging.log4j.Logger;
@@ -35,6 +34,7 @@
3534
import org.apache.struts2.security.DefaultExcludedPatternsChecker;
3635
import org.apache.struts2.security.ExcludedPatternsChecker;
3736

37+
import jakarta.servlet.http.HttpServletRequest;
3838
import java.io.IOException;
3939
import java.nio.charset.Charset;
4040
import java.nio.file.Path;
@@ -45,13 +45,17 @@
4545
import java.util.List;
4646
import java.util.Map;
4747

48+
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
49+
4850
/**
4951
* Abstract class with some helper methods, it should be used
5052
* when starting development of another implementation of {@link MultiPartRequest}
5153
*/
5254
public abstract class AbstractMultiPartRequest implements MultiPartRequest {
5355

5456
protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_PARAMETER_TOO_LONG_KEY = "struts.messages.upload.error.parameter.too.long";
57+
protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD = "struts.messages.upload.error.illegal.characters.field";
58+
protected static final String STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME = "struts.messages.upload.error.illegal.characters.name";
5559

5660
private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class);
5761

@@ -116,13 +120,14 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
116120

117121
private final ExcludedPatternsChecker patternsChecker;
118122

119-
protected AbstractMultiPartRequest(String dmiValue) {
120-
patternsChecker = new DefaultExcludedPatternsChecker();
121-
if (BooleanUtils.toBoolean(dmiValue)) {
122-
((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT);
123-
} else {
124-
((DefaultExcludedPatternsChecker) patternsChecker).setAdditionalExcludePatterns(EXCLUDED_FILE_PATTERN);
125-
}
123+
protected AbstractMultiPartRequest() {
124+
this(false);
125+
}
126+
127+
protected AbstractMultiPartRequest(boolean dmiValue) {
128+
var patternsChecker = new DefaultExcludedPatternsChecker();
129+
patternsChecker.setAdditionalExcludePatterns(dmiValue ? EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT : EXCLUDED_FILE_PATTERN);
130+
this.patternsChecker = patternsChecker;
126131
}
127132

128133
/**
@@ -302,16 +307,7 @@ protected LocalizedMessage buildErrorMessage(Class<? extends Throwable> exceptio
302307
* @return the canonical name based on the supplied filename
303308
*/
304309
protected String getCanonicalName(final String originalFileName) {
305-
String fileName = originalFileName;
306-
307-
int forwardSlash = fileName.lastIndexOf('/');
308-
int backwardSlash = fileName.lastIndexOf('\\');
309-
if (forwardSlash != -1 && forwardSlash > backwardSlash) {
310-
fileName = fileName.substring(forwardSlash + 1);
311-
} else {
312-
fileName = fileName.substring(backwardSlash + 1);
313-
}
314-
return fileName;
310+
return FilenameUtils.getName(originalFileName);
315311
}
316312

317313
/**
@@ -443,4 +439,32 @@ protected boolean isExcluded(String fileName) {
443439
return patternsChecker.isExcluded(fileName).isExcluded();
444440
}
445441

442+
protected boolean isInvalidInput(String fieldName, String fileName) {
443+
// Skip file uploads that don't have a file name - meaning that no file was selected.
444+
if (fileName == null || fileName.isBlank()) {
445+
LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fieldName));
446+
return true;
447+
}
448+
449+
if (isExcluded(fileName)) {
450+
var normalizedFileName = normalizeSpace(fileName);
451+
LOG.debug("File name [{}] is not accepted", normalizedFileName);
452+
errors.add(new LocalizedMessage(getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME, null,
453+
new String[]{normalizedFileName}));
454+
return true;
455+
}
456+
457+
return isInvalidInput(fieldName);
458+
}
459+
460+
protected boolean isInvalidInput(String fieldName) {
461+
if (isExcluded(fieldName)) {
462+
var normalizedFieldName = normalizeSpace(fieldName);
463+
LOG.debug("Form field [{}}] is rejected!", normalizedFieldName);
464+
errors.add(new LocalizedMessage(getClass(), STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD, null,
465+
new String[]{normalizedFieldName}));
466+
return true;
467+
}
468+
return false;
469+
}
446470
}

‎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
@@ -18,16 +18,17 @@
1818
*/
1919
package org.apache.struts2.dispatcher.multipart;
2020

21-
import jakarta.servlet.http.HttpServletRequest;
2221
import org.apache.commons.fileupload2.core.DiskFileItem;
2322
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
2423
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
24+
import org.apache.commons.lang3.BooleanUtils;
2525
import org.apache.commons.lang3.StringUtils;
2626
import org.apache.logging.log4j.LogManager;
2727
import org.apache.logging.log4j.Logger;
2828
import org.apache.struts2.StrutsConstants;
2929
import org.apache.struts2.inject.Inject;
3030

31+
import jakarta.servlet.http.HttpServletRequest;
3132
import java.io.IOException;
3233
import java.nio.charset.Charset;
3334
import java.nio.file.Path;
@@ -44,12 +45,12 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
4445
private static final Logger LOG = LogManager.getLogger(JakartaMultiPartRequest.class);
4546

4647
public JakartaMultiPartRequest() {
47-
super(Boolean.FALSE.toString());
48+
super();
4849
}
4950

5051
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
5152
public JakartaMultiPartRequest(String dmiValue) {
52-
super(dmiValue);
53+
super(BooleanUtils.toBoolean(dmiValue));
5354
}
5455

5556
@Override
@@ -88,15 +89,14 @@ protected JakartaServletDiskFileUpload createJakartaFileUpload(Charset charset,
8889
}
8990

9091
protected void processNormalFormField(DiskFileItem item, Charset charset) throws IOException {
91-
LOG.debug("Item: {} is a normal form field", item.getName());
92+
var fieldName = item.getFieldName();
93+
LOG.debug("Item: {} is a normal form field", fieldName);
9294

93-
if (isExcluded(item.getFieldName())) {
94-
LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(item.getFieldName())));
95+
if (isInvalidInput(fieldName)) {
9596
return;
9697
}
9798

9899
List<String> values;
99-
String fieldName = item.getFieldName();
100100
if (parameters.get(fieldName) != null) {
101101
values = parameters.get(fieldName);
102102
} else {
@@ -116,19 +116,7 @@ protected void processNormalFormField(DiskFileItem item, Charset charset) throws
116116
}
117117

118118
protected void processFileField(DiskFileItem item) {
119-
if (isExcluded(item.getName())) {
120-
LOG.warn(() -> "File name [%s] is not accepted".formatted(normalizeSpace(item.getName())));
121-
return;
122-
}
123-
124-
if (isExcluded(item.getFieldName())) {
125-
LOG.warn(() -> "Field name [%s] is not accepted".formatted(normalizeSpace(item.getFieldName())));
126-
return;
127-
}
128-
129-
// Skip file uploads that don't have a file name - meaning that no file was selected.
130-
if (item.getName() == null || item.getName().trim().isEmpty()) {
131-
LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(item.getFieldName()));
119+
if (isInvalidInput(item.getFieldName(), item.getName())) {
132120
return;
133121
}
134122

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

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,19 @@
1818
*/
1919
package org.apache.struts2.dispatcher.multipart;
2020

21-
import jakarta.servlet.http.HttpServletRequest;
2221
import org.apache.commons.fileupload2.core.DiskFileItemFactory;
2322
import org.apache.commons.fileupload2.core.FileItemInput;
2423
import org.apache.commons.fileupload2.core.FileUploadFileCountLimitException;
2524
import org.apache.commons.fileupload2.core.FileUploadSizeException;
2625
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
26+
import org.apache.commons.lang3.BooleanUtils;
2727
import org.apache.logging.log4j.LogManager;
2828
import org.apache.logging.log4j.Logger;
2929
import org.apache.struts2.StrutsConstants;
3030
import org.apache.struts2.dispatcher.LocalizedMessage;
3131
import org.apache.struts2.inject.Inject;
3232

33+
import jakarta.servlet.http.HttpServletRequest;
3334
import java.io.BufferedOutputStream;
3435
import java.io.ByteArrayOutputStream;
3536
import java.io.File;
@@ -59,12 +60,12 @@ public class JakartaStreamMultiPartRequest extends AbstractMultiPartRequest {
5960
private static final Logger LOG = LogManager.getLogger(JakartaStreamMultiPartRequest.class);
6061

6162
public JakartaStreamMultiPartRequest() {
62-
super(Boolean.FALSE.toString());
63+
super();
6364
}
6465

6566
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
6667
public JakartaStreamMultiPartRequest(String dmiValue) {
67-
super(dmiValue);
68+
super(BooleanUtils.toBoolean(dmiValue));
6869
}
6970

7071
/**
@@ -125,13 +126,11 @@ private String readStream(InputStream inputStream) throws IOException {
125126
*/
126127
protected void processFileItemAsFormField(FileItemInput fileItemInput) throws IOException {
127128
String fieldName = fileItemInput.getFieldName();
128-
String fieldValue = readStream(fileItemInput.getInputStream());
129-
130-
if (isExcluded(fieldName)) {
131-
LOG.warn(() -> "Form field [%s] is rejected!".formatted(normalizeSpace(fieldName)));
129+
if (isInvalidInput(fieldName)) {
132130
return;
133131
}
134132

133+
String fieldValue = readStream(fileItemInput.getInputStream());
135134
if (exceedsMaxStringLength(fieldName, fieldValue)) {
136135
return;
137136
}
@@ -203,14 +202,7 @@ private void exceedsMaxSizeOfFiles(FileItemInput fileItemInput, File file, Long
203202
* @param location location
204203
*/
205204
protected void processFileItemAsFileField(FileItemInput fileItemInput, Path location) throws IOException {
206-
// Skip file uploads that don't have a file name - meaning that no file was selected.
207-
if (fileItemInput.getName() == null || fileItemInput.getName().trim().isEmpty()) {
208-
LOG.debug(() -> "No file has been uploaded for the field: " + normalizeSpace(fileItemInput.getFieldName()));
209-
return;
210-
}
211-
212-
if (isExcluded(fileItemInput.getName())) {
213-
LOG.warn(() -> "File field [%s] rejected".formatted(normalizeSpace(fileItemInput.getName())));
205+
if (isInvalidInput(fileItemInput.getFieldName(), fileItemInput.getName())) {
214206
return;
215207
}
216208

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,10 @@ struts.messages.error.file.too.large=The file is too large to be uploaded: {0} "
4040
# 1 - max string length
4141
# 2 - actual size
4242
struts.messages.upload.error.parameter.too.long=The request parameter "{0}" was too long. Max length allowed is {1}, but found {2}!
43+
# 0 - field name
44+
struts.messages.upload.error.illegal.characters.field=The multipart upload field name "{0}" contains illegal characters!
45+
# 0 - file name
46+
struts.messages.upload.error.illegal.characters.name=The multipart upload filename "{0}" contains illegal characters!
4347
# 0 - input name
4448
# 1 - original filename
4549
# 2 - file name after uploading the file

‎core/src/test/java/org/apache/struts2/dispatcher/multipart/AbstractMultiPartRequestTest.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -518,8 +518,8 @@ public void maliciousFields() throws IOException {
518518

519519
multiPart.parse(mockRequest, tempDir);
520520

521-
assertThat(multiPart.getErrors())
522-
.isEmpty();
521+
assertThat(multiPart.getErrors()).extracting(LocalizedMessage::getTextKey)
522+
.containsExactly(AbstractMultiPartRequest.STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_FIELD);
523523

524524
assertThat(multiPart.getParameterNames().asIterator()).toIterable()
525525
.isEmpty();
@@ -537,8 +537,8 @@ public void maliciousFilename() throws IOException {
537537

538538
multiPart.parse(mockRequest, tempDir);
539539

540-
assertThat(multiPart.getErrors())
541-
.isEmpty();
540+
assertThat(multiPart.getErrors()).extracting(LocalizedMessage::getTextKey)
541+
.containsExactly(AbstractMultiPartRequest.STRUTS_MESSAGES_UPLOAD_ERROR_ILLEGAL_CHARACTERS_NAME);
542542

543543
assertThat(multiPart.getParameterNames().asIterator()).toIterable()
544544
.hasSize(1);

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

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,21 +18,21 @@
1818
*/
1919
package org.apache.struts2.interceptor;
2020

21-
import org.apache.struts2.ActionContext;
22-
import org.apache.struts2.ActionSupport;
23-
import org.apache.struts2.locale.DefaultLocaleProvider;
24-
import org.apache.struts2.ValidationAwareSupport;
25-
import org.apache.struts2.mock.MockActionInvocation;
26-
import org.apache.struts2.mock.MockActionProxy;
27-
import org.apache.struts2.util.ClassLoaderUtil;
2821
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletDiskFileUpload;
2922
import org.apache.commons.fileupload2.jakarta.servlet6.JakartaServletFileUpload;
23+
import org.apache.struts2.ActionContext;
24+
import org.apache.struts2.ActionSupport;
3025
import org.apache.struts2.StrutsInternalTestCase;
26+
import org.apache.struts2.ValidationAwareSupport;
3127
import org.apache.struts2.action.UploadedFilesAware;
3228
import org.apache.struts2.dispatcher.multipart.JakartaMultiPartRequest;
3329
import org.apache.struts2.dispatcher.multipart.MultiPartRequestWrapper;
3430
import org.apache.struts2.dispatcher.multipart.StrutsUploadedFile;
3531
import org.apache.struts2.dispatcher.multipart.UploadedFile;
32+
import org.apache.struts2.locale.DefaultLocaleProvider;
33+
import org.apache.struts2.mock.MockActionInvocation;
34+
import org.apache.struts2.mock.MockActionProxy;
35+
import org.apache.struts2.util.ClassLoaderUtil;
3636
import org.assertj.core.util.Files;
3737
import org.springframework.mock.web.MockHttpServletRequest;
3838

@@ -540,7 +540,8 @@ public void testUnacceptedFieldName() throws Exception {
540540

541541
interceptor.intercept(mai);
542542

543-
assertFalse(action.hasActionErrors());
543+
assertThat(action.getActionErrors())
544+
.containsExactly("The multipart upload field name \"top.file\" contains illegal characters!");
544545
assertNull(action.getUploadFiles());
545546
}
546547

@@ -570,7 +571,8 @@ public void testUnacceptedFileName() throws Exception {
570571

571572
interceptor.intercept(mai);
572573

573-
assertFalse(action.hasActionErrors());
574+
assertThat(action.getActionErrors())
575+
.containsExactly("The multipart upload filename \"../deleteme.txt\" contains illegal characters!");
574576
assertNull(action.getUploadFiles());
575577
}
576578

0 commit comments

Comments
 (0)