Skip to content

Commit 9b04437

Browse files
lukaszlenartlukaszlenart
authored
WW-5501 Reverts all changes related to WW-5501 (#1218)
* Reverts all changes related to WW-5501 * Fixes CodeQL scan by using proper versions of actions * WW-5501 Uses FilenameUtils instead of a custom code
1 parent 58f37ba commit 9b04437

File tree

8 files changed

+34
-251
lines changed

8 files changed

+34
-251
lines changed

‎.github/workflows/codeql.yml

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ name: "CodeQL"
1717

1818
on:
1919
push:
20-
branches: [ "master" ]
20+
branches:
21+
- 'release/*'
2122
pull_request:
2223

2324
permissions:
@@ -41,15 +42,21 @@ jobs:
4142
matrix:
4243
language: [ 'java' ]
4344
steps:
44-
- name: Checkout repository
45-
uses: actions/checkout@v4
46-
- name: Initialize CodeQL
47-
uses: github/codeql-action/[email protected]
48-
with:
49-
languages: ${{ matrix.language }}
50-
- name: Autobuild
51-
uses: github/codeql-action/[email protected]
52-
- name: Perform CodeQL Analysis
53-
uses: github/codeql-action/[email protected]
54-
with:
55-
category: "/language:${{matrix.language}}"
45+
- name: Checkout repository
46+
uses: actions/checkout@v4
47+
- name: Setup Java JDK
48+
uses: actions/setup-java@v4
49+
with:
50+
distribution: temurin
51+
java-version: 17
52+
cache: 'maven'
53+
- name: Initialize CodeQL
54+
uses: github/codeql-action/[email protected]
55+
with:
56+
languages: ${{ matrix.language }}
57+
- name: Autobuild
58+
uses: github/codeql-action/[email protected]
59+
- name: Perform CodeQL Analysis
60+
uses: github/codeql-action/[email protected]
61+
with:
62+
category: "/language:${{matrix.language}}"

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

Lines changed: 4 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,6 @@
2020

2121
import com.opensymphony.xwork2.LocaleProviderFactory;
2222
import com.opensymphony.xwork2.inject.Inject;
23-
import com.opensymphony.xwork2.security.DefaultExcludedPatternsChecker;
24-
import com.opensymphony.xwork2.security.ExcludedPatternsChecker;
2523
import org.apache.commons.io.FilenameUtils;
2624
import org.apache.logging.log4j.LogManager;
2725
import org.apache.logging.log4j.Logger;
@@ -33,29 +31,21 @@
3331
import java.util.List;
3432
import java.util.Locale;
3533

36-
import static org.apache.commons.lang3.StringUtils.normalizeSpace;
37-
3834
/**
3935
* Abstract class with some helper methods, it should be used
4036
* when starting development of another implementation of {@link MultiPartRequest}
4137
*/
4238
public abstract class AbstractMultiPartRequest implements MultiPartRequest {
4339

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-
4740
private static final Logger LOG = LogManager.getLogger(AbstractMultiPartRequest.class);
4841

49-
private static final String EXCLUDED_FILE_PATTERN = "^(.*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*)$";
50-
private static final String EXCLUDED_FILE_PATTERN_WITH_DMI_SUPPORT = "^(?!action:[^<>&\"'|;\\\\/?*:]+(![^<>&\"'|;\\\\/?*:]+)?$)(.*[<>&\"'|;\\\\/?*:]+.*|.*\\.\\..*)$\n";
51-
5242
/**
5343
* Defines the internal buffer size used during streaming operations.
5444
*/
5545
public static final int BUFFER_SIZE = 10240;
5646

5747
/**
58-
* Internal list of raised errors to be passed to the Struts2 framework.
48+
* Internal list of raised errors to be passed to the the Struts2 framework.
5949
*/
6050
protected List<LocalizedMessage> errors = new ArrayList<>();
6151

@@ -91,18 +81,6 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
9181
*/
9282
protected Locale defaultLocale = Locale.ENGLISH;
9383

94-
private final ExcludedPatternsChecker patternsChecker;
95-
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;
104-
}
105-
10684
/**
10785
* @param bufferSize Sets the buffer size to be used.
10886
*/
@@ -146,7 +124,7 @@ public void setLocaleProviderFactory(LocaleProviderFactory localeProviderFactory
146124

147125
/**
148126
* @param request Inspect the servlet request and set the locale if one wasn't provided by
149-
* the Struts2 framework.
127+
* the Struts2 framework.
150128
*/
151129
protected void setLocale(HttpServletRequest request) {
152130
if (defaultLocale == null) {
@@ -157,7 +135,7 @@ protected void setLocale(HttpServletRequest request) {
157135
/**
158136
* Build error message.
159137
*
160-
* @param e the Throwable/Exception
138+
* @param e the Throwable/Exception
161139
* @param args arguments
162140
* @return error message
163141
*/
@@ -170,7 +148,7 @@ protected LocalizedMessage buildErrorMessage(Throwable e, Object[] args) {
170148

171149
/* (non-Javadoc)
172150
* @see org.apache.struts2.dispatcher.multipart.MultiPartRequest#getErrors()
173-
*/
151+
*/
174152
public List<LocalizedMessage> getErrors() {
175153
return errors;
176154
}
@@ -183,40 +161,4 @@ protected String getCanonicalName(final String originalFileName) {
183161
return FilenameUtils.getName(originalFileName);
184162
}
185163

186-
/**
187-
* @param fileName file name to check
188-
* @return true if the file name is excluded
189-
*/
190-
protected boolean isExcluded(String fileName) {
191-
return patternsChecker.isExcluded(fileName).isExcluded();
192-
}
193-
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-
}
222164
}

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

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

21-
import com.opensymphony.xwork2.inject.Inject;
2221
import org.apache.commons.fileupload.FileCountLimitExceededException;
2322
import org.apache.commons.fileupload.FileItem;
2423
import org.apache.commons.fileupload.FileUploadBase;
@@ -27,11 +26,9 @@
2726
import org.apache.commons.fileupload.disk.DiskFileItem;
2827
import org.apache.commons.fileupload.disk.DiskFileItemFactory;
2928
import org.apache.commons.fileupload.servlet.ServletFileUpload;
30-
import org.apache.commons.lang3.BooleanUtils;
3129
import org.apache.commons.lang3.StringUtils;
3230
import org.apache.logging.log4j.LogManager;
3331
import org.apache.logging.log4j.Logger;
34-
import org.apache.struts2.StrutsConstants;
3532
import org.apache.struts2.dispatcher.LocalizedMessage;
3633

3734
import javax.servlet.http.HttpServletRequest;
@@ -62,15 +59,6 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
6259
// maps parameter name -> List of param values
6360
protected Map<String, List<String>> params = new HashMap<>();
6461

65-
public JakartaMultiPartRequest() {
66-
super();
67-
}
68-
69-
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
70-
public JakartaMultiPartRequest(String dmiValue) {
71-
super(BooleanUtils.toBoolean(dmiValue));
72-
}
73-
7462
/**
7563
* Creates a new request wrapper to handle multi-part data using methods adapted from Jason Pell's
7664
* multipart classes (see class description).
@@ -125,7 +113,11 @@ protected void processUpload(HttpServletRequest request, String saveDir) throws
125113
}
126114

127115
protected void processFileField(FileItem item) {
128-
if (isInvalidInput(item.getFieldName(), item.getName())) {
116+
LOG.debug("Item is a file upload");
117+
118+
// Skip file uploads that don't have a file name - meaning that no file was selected.
119+
if (item.getName() == null || item.getName().trim().isEmpty()) {
120+
LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(item.getFieldName()));
129121
return;
130122
}
131123

@@ -142,12 +134,7 @@ protected void processFileField(FileItem item) {
142134

143135
protected void processNormalFormField(FileItem item, String charset) throws UnsupportedEncodingException {
144136
try {
145-
String fieldName = item.getFieldName();
146-
LOG.debug("Item: {} is a normal form field", normalizeSpace(fieldName));
147-
148-
if (isInvalidInput(fieldName)) {
149-
return;
150-
}
137+
LOG.debug("Item is a normal form field");
151138

152139
List<String> values;
153140
if (params.get(item.getFieldName()) != null) {

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

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

21-
import com.opensymphony.xwork2.inject.Inject;
2221
import org.apache.commons.fileupload.FileItemIterator;
2322
import org.apache.commons.fileupload.FileItemStream;
2423
import org.apache.commons.fileupload.FileUploadBase;
2524
import org.apache.commons.fileupload.FileUploadBase.FileSizeLimitExceededException;
2625
import org.apache.commons.fileupload.FileUploadException;
2726
import org.apache.commons.fileupload.servlet.ServletFileUpload;
2827
import org.apache.commons.fileupload.util.Streams;
29-
import org.apache.commons.lang3.BooleanUtils;
3028
import org.apache.logging.log4j.LogManager;
3129
import org.apache.logging.log4j.Logger;
32-
import org.apache.struts2.StrutsConstants;
3330
import org.apache.struts2.dispatcher.LocalizedMessage;
3431

3532
import javax.servlet.http.HttpServletRequest;
@@ -209,15 +206,6 @@ public void parse(HttpServletRequest request, String saveDir) throws IOException
209206
}
210207
}
211208

212-
public JakartaStreamMultiPartRequest() {
213-
super();
214-
}
215-
216-
@Inject(value = StrutsConstants.STRUTS_ENABLE_DYNAMIC_METHOD_INVOCATION, required = false)
217-
public JakartaStreamMultiPartRequest(String dmiValue) {
218-
super(BooleanUtils.toBoolean(dmiValue));
219-
}
220-
221209
/**
222210
* Processes the upload.
223211
*
@@ -326,9 +314,6 @@ protected void addFileSkippedError(String fileName, HttpServletRequest request)
326314
*/
327315
protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
328316
String fieldName = itemStream.getFieldName();
329-
if (isInvalidInput(fieldName)) {
330-
return;
331-
}
332317
try {
333318
List<String> values;
334319
String fieldValue = Streams.asString(itemStream.openStream());
@@ -351,7 +336,9 @@ protected void processFileItemStreamAsFormField(FileItemStream itemStream) {
351336
* @param location location
352337
*/
353338
protected void processFileItemStreamAsFileField(FileItemStream itemStream, String location) {
354-
if (isInvalidInput(itemStream.getFieldName(), itemStream.getName())) {
339+
// Skip file uploads that don't have a file name - meaning that no file was selected.
340+
if (itemStream.getName() == null || itemStream.getName().trim().isEmpty()) {
341+
LOG.debug("No file has been uploaded for the field: {}", normalizeSpace(itemStream.getFieldName()));
355342
return;
356343
}
357344

@@ -400,9 +387,7 @@ protected File createTemporaryFile(String fileName, String location) throws IOEx
400387
}
401388

402389
File file = File.createTempFile(prefix + "_", suffix, new File(location));
403-
if (LOG.isDebugEnabled()) {
404-
LOG.debug("Creating temporary file [{}] (originally [{}]).", file.getName(), normalizeSpace(fileName));
405-
}
390+
LOG.debug("Creating temporary file [{}] (originally [{}]).", file.getName(), normalizeSpace(fileName));
406391
return file;
407392
}
408393

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,6 @@ 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!
3230
struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0} "{1}" "{2}" {3}
3331
struts.messages.error.file.extension.not.allowed=File extension not allowed: {0} "{1}" "{2}" {3}
3432

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

Lines changed: 0 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -663,70 +663,6 @@ public void testMultipartRequestLocalizedError() throws Exception {
663663
assertTrue(msg.startsWith("Der Request übertraf die maximal erlaubte Größe"));
664664
}
665665

666-
public void testUnacceptedFieldName() throws Exception {
667-
MockHttpServletRequest req = new MockHttpServletRequest();
668-
req.setCharacterEncoding(StandardCharsets.UTF_8.name());
669-
req.setMethod("post");
670-
req.addHeader("Content-type", "multipart/form-data; boundary=---1234");
671-
672-
// inspired by the unit tests for jakarta commons fileupload
673-
String content = ("-----1234\r\n" +
674-
"Content-Disposition: form-data; name=\"top.file\"; filename=\"deleteme.txt\"\r\n" +
675-
"Content-Type: text/html\r\n" +
676-
"\r\n" +
677-
"Unit test of ActionFileUploadInterceptor" +
678-
"\r\n" +
679-
"-----1234--\r\n");
680-
req.setContent(content.getBytes(StandardCharsets.US_ASCII));
681-
682-
MyFileUploadAction action = container.inject(MyFileUploadAction.class);
683-
684-
MockActionInvocation mai = new MockActionInvocation();
685-
mai.setAction(action);
686-
mai.setResultCode("success");
687-
mai.setInvocationContext(ActionContext.getContext());
688-
ActionContext.getContext()
689-
.withServletRequest(createMultipartRequestMaxSize(req, 2000));
690-
691-
interceptor.intercept(mai);
692-
693-
assertThat(action.getActionErrors())
694-
.containsExactly("The multipart upload field name \"top.file\" contains illegal characters!");
695-
assertNull(action.getUploadFiles());
696-
}
697-
698-
public void testUnacceptedFileName() throws Exception {
699-
MockHttpServletRequest req = new MockHttpServletRequest();
700-
req.setCharacterEncoding(StandardCharsets.UTF_8.name());
701-
req.setMethod("post");
702-
req.addHeader("Content-type", "multipart/form-data; boundary=---1234");
703-
704-
// inspired by the unit tests for jakarta commons fileupload
705-
String content = ("-----1234\r\n" +
706-
"Content-Disposition: form-data; name=\"file\"; filename=\"../deleteme.txt\"\r\n" +
707-
"Content-Type: text/html\r\n" +
708-
"\r\n" +
709-
"Unit test of ActionFileUploadInterceptor" +
710-
"\r\n" +
711-
"-----1234--\r\n");
712-
req.setContent(content.getBytes(StandardCharsets.US_ASCII));
713-
714-
MyFileUploadAction action = container.inject(MyFileUploadAction.class);
715-
716-
MockActionInvocation mai = new MockActionInvocation();
717-
mai.setAction(action);
718-
mai.setResultCode("success");
719-
mai.setInvocationContext(ActionContext.getContext());
720-
ActionContext.getContext()
721-
.withServletRequest(createMultipartRequestMaxSize(req, 2000));
722-
723-
interceptor.intercept(mai);
724-
725-
assertThat(action.getActionErrors())
726-
.containsExactly("The multipart upload filename \"../deleteme.txt\" contains illegal characters!");
727-
assertNull(action.getUploadFiles());
728-
}
729-
730666
private String encodeTextFile(String filename, String contentType, String content) {
731667
return "\r\n" +
732668
"--" +

0 commit comments

Comments
 (0)