Skip to content

Commit

Permalink
Merge pull request #1068 from JCgH4164838Gh792C124B5/localS2_66_Jakar…
Browse files Browse the repository at this point in the history 
…taMulipartRequestFix1

Potential mitigation for WW-5466
  • Loading branch information
@lukaszlenart
lukaszlenart authored Oct 20, 2024
2 parents 9f2770b + 09eb286 commit b359da5
Show file tree
Hide file tree
Showing 3 changed files with 332 additions and 2 deletions.
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu
}

long size = item.getSize();
if (size > maxStringLength) {
if (maxStringLength != null && 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,
Expand Down
151 changes: 151 additions & 0 deletions core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,148 @@ public void testSuccessUploadOfATextFileMultipartRequest() throws Exception {
assertNotNull("deleteme.txt", files.get(0).getOriginalName());
}

public void testSuccessUploadOfATextFileMultipartRequestNoMaxParamsSet() 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 = new MyFileUploadAction();

MockActionInvocation mai = new MockActionInvocation();
mai.setAction(action);
mai.setResultCode("success");
mai.setInvocationContext(ActionContext.getContext());
ActionContext.getContext().withServletRequest(createMultipartRequestNoMaxParamsSet(req));

interceptor.intercept(mai);

assertFalse(action.hasErrors());

List<UploadedFile> files = action.getUploadFiles();

assertNotNull(files);
assertEquals(1, files.size());
assertEquals("text/html", files.get(0).getContentType());
assertNotNull("deleteme.txt", files.get(0).getOriginalName());
}

public void testSuccessUploadOfATextFileMultipartRequestWithNormalFieldsMaxParamsSet() 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" +
"Content-Disposition: form-data; name=\"normalFormField1\"\r\n" +
"\r\n" +
"normal field 1" +
"\r\n" +
"-----1234\r\n" +
"Content-Disposition: form-data; name=\"normalFormField2\"\r\n" +
"\r\n" +
"normal field 2" +
"\r\n" +
"-----1234--\r\n");
req.setContent(content.getBytes(StandardCharsets.US_ASCII));

MyFileUploadAction action = new MyFileUploadAction();

MockActionInvocation mai = new MockActionInvocation();
mai.setAction(action);
mai.setResultCode("success");
mai.setInvocationContext(ActionContext.getContext());
ActionContext.getContext().withServletRequest(createMultipartRequest(req, 2000, 2000, 5, 100));

interceptor.intercept(mai);

assertFalse(action.hasErrors());

List<UploadedFile> files = action.getUploadFiles();

assertNotNull(files);
assertEquals(1, files.size());
assertEquals("text/html", files.get(0).getContentType());
assertNotNull("deleteme.txt", files.get(0).getOriginalName());

// Confirm normalFormField1, normalFormField2 were processed by the MultiPartRequestWrapper.
HttpServletRequest invocationServletRequest = mai.getInvocationContext().getServletRequest();
assertTrue("invocation servelt request is not a MultiPartRequestWrapper ?", invocationServletRequest instanceof MultiPartRequestWrapper);
MultiPartRequestWrapper multipartRequestWrapper = (MultiPartRequestWrapper) invocationServletRequest;
assertNotNull("normalFormField1 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField1"));
assertNotNull("normalFormField2 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField2"));
}

public void testSuccessUploadOfATextFileMultipartRequestWithNormalFieldsNoMaxParamsSet() 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" +
"Content-Disposition: form-data; name=\"normalFormField1\"\r\n" +
"\r\n" +
"normal field 1" +
"\r\n" +
"-----1234\r\n" +
"Content-Disposition: form-data; name=\"normalFormField2\"\r\n" +
"\r\n" +
"normal field 2" +
"\r\n" +
"-----1234--\r\n");
req.setContent(content.getBytes(StandardCharsets.US_ASCII));

MyFileUploadAction action = new MyFileUploadAction();

MockActionInvocation mai = new MockActionInvocation();
mai.setAction(action);
mai.setResultCode("success");
mai.setInvocationContext(ActionContext.getContext());
ActionContext.getContext().withServletRequest(createMultipartRequestNoMaxParamsSet(req));

interceptor.intercept(mai);

assertFalse(action.hasErrors());

List<UploadedFile> files = action.getUploadFiles();

assertNotNull(files);
assertEquals(1, files.size());
assertEquals("text/html", files.get(0).getContentType());
assertNotNull("deleteme.txt", files.get(0).getOriginalName());

// Confirm normalFormField1, normalFormField2 were processed by the MultiPartRequestWrapper.
HttpServletRequest invocationServletRequest = mai.getInvocationContext().getServletRequest();
assertTrue("invocation servelt request is not a MultiPartRequestWrapper ?", invocationServletRequest instanceof MultiPartRequestWrapper);
MultiPartRequestWrapper multipartRequestWrapper = (MultiPartRequestWrapper) invocationServletRequest;
assertNotNull("normalFormField1 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField1"));
assertNotNull("normalFormField2 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField2"));
}

/**
* tests whether with multiple files sent with the same name, the ones with forbiddenTypes (see
* ActionFileUploadInterceptor.setAllowedTypes(...) ) are sorted out.
Expand Down Expand Up @@ -564,6 +706,12 @@ private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, i
return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}

private MultiPartRequestWrapper createMultipartRequestNoMaxParamsSet(HttpServletRequest req) {

JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}

protected void setUp() throws Exception {
super.setUp();

Expand All @@ -583,6 +731,9 @@ protected void tearDown() throws Exception {
public static class MyFileUploadAction extends ActionSupport implements UploadedFilesAware {
private List<UploadedFile> uploadedFiles;

// Note: We do not currently need fields/getters/setters for normalFormField1, normalFormField2 since
// the upload interceptor only prepares the normal field parameters.

@Override
public void withUploadedFiles(List<UploadedFile> uploadedFiles) {
this.uploadedFiles = uploadedFiles;
Expand Down
181 changes: 180 additions & 1 deletion core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,6 +329,178 @@ public void testSuccessUploadOfATextFileMultipartRequest() throws Exception {
assertNotNull("deleteme.txt", fileRealFilenames[0]);
}

public void testSuccessUploadOfATextFileMultipartRequestNoMaxParamsSet() 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 FileUploadInterceptor" +
"\r\n" +
"-----1234--\r\n");
req.setContent(content.getBytes(StandardCharsets.US_ASCII));

MyFileupAction action = new MyFileupAction();

MockActionInvocation mai = new MockActionInvocation();
mai.setAction(action);
mai.setResultCode("success");
mai.setInvocationContext(ActionContext.getContext());
Map<String, Object> param = new HashMap<>();
ActionContext.getContext().withParameters(HttpParameters.create(param).build());
ActionContext.getContext().withServletRequest(createMultipartRequestNoMaxParamsSet(req));

interceptor.intercept(mai);

assertFalse(action.hasErrors());

HttpParameters parameters = mai.getInvocationContext().getParameters();
assertEquals(3, parameters.keySet().size());
UploadedFile[] files = (UploadedFile[]) parameters.get("file").getObject();
String[] fileContentTypes = parameters.get("fileContentType").getMultipleValues();
String[] fileRealFilenames = parameters.get("fileFileName").getMultipleValues();

assertNotNull(files);
assertNotNull(fileContentTypes);
assertNotNull(fileRealFilenames);
assertEquals(1, files.length);
assertEquals(1, fileContentTypes.length);
assertEquals(1, fileRealFilenames.length);
assertEquals("text/html", fileContentTypes[0]);
assertNotNull("deleteme.txt", fileRealFilenames[0]);
}

public void testSuccessUploadOfATextFileMultipartRequestWithNormalFieldsMaxParamsSet() 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 FileUploadInterceptor" +
"\r\n" +
"-----1234\r\n" +
"Content-Disposition: form-data; name=\"normalFormField1\"\r\n" +
"\r\n" +
"normal field 1" +
"\r\n" +
"-----1234\r\n" +
"Content-Disposition: form-data; name=\"normalFormField2\"\r\n" +
"\r\n" +
"normal field 2" +
"\r\n" +
"-----1234--\r\n");
req.setContent(content.getBytes(StandardCharsets.US_ASCII));

MyFileupAction action = new MyFileupAction();

MockActionInvocation mai = new MockActionInvocation();
mai.setAction(action);
mai.setResultCode("success");
mai.setInvocationContext(ActionContext.getContext());
Map<String, Object> param = new HashMap<>();
ActionContext.getContext().withParameters(HttpParameters.create(param).build());
ActionContext.getContext().withServletRequest(createMultipartRequest(req, 2000, 2000, 5, 100));

interceptor.intercept(mai);

assertFalse(action.hasErrors());

HttpParameters parameters = mai.getInvocationContext().getParameters();
assertEquals(3, parameters.keySet().size());
UploadedFile[] files = (UploadedFile[]) parameters.get("file").getObject();
String[] fileContentTypes = parameters.get("fileContentType").getMultipleValues();
String[] fileRealFilenames = parameters.get("fileFileName").getMultipleValues();

assertNotNull(files);
assertNotNull(fileContentTypes);
assertNotNull(fileRealFilenames);
assertEquals(1, files.length);
assertEquals(1, fileContentTypes.length);
assertEquals(1, fileRealFilenames.length);
assertEquals("text/html", fileContentTypes[0]);
assertNotNull("deleteme.txt", fileRealFilenames[0]);

// Confirm normalFormField1, normalFormField2 were processed by the MultiPartRequestWrapper.
HttpServletRequest invocationServletRequest = mai.getInvocationContext().getServletRequest();
assertTrue("invocation servelt request is not a MultiPartRequestWrapper ?", invocationServletRequest instanceof MultiPartRequestWrapper);
MultiPartRequestWrapper multipartRequestWrapper = (MultiPartRequestWrapper) invocationServletRequest;
assertNotNull("normalFormField1 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField1"));
assertNotNull("normalFormField2 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField2"));
}

public void testSuccessUploadOfATextFileMultipartRequestWithNormalFieldsNoMaxParamsSet() 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 FileUploadInterceptor" +
"\r\n" +
"-----1234\r\n" +
"Content-Disposition: form-data; name=\"normalFormField1\"\r\n" +
"\r\n" +
"normal field 1 with no max parameters set" +
"\r\n" +
"-----1234\r\n" +
"Content-Disposition: form-data; name=\"normalFormField2\"\r\n" +
"\r\n" +
"normal field 2 with no max parameters set" +
"\r\n" +
"-----1234--\r\n");
req.setContent(content.getBytes(StandardCharsets.US_ASCII));

MyFileupAction action = new MyFileupAction();

MockActionInvocation mai = new MockActionInvocation();
mai.setAction(action);
mai.setResultCode("success");
mai.setInvocationContext(ActionContext.getContext());
Map<String, Object> param = new HashMap<>();
ActionContext.getContext().withParameters(HttpParameters.create(param).build());
ActionContext.getContext().withServletRequest(createMultipartRequestNoMaxParamsSet(req));

interceptor.intercept(mai);

assertFalse(action.hasErrors());

HttpParameters parameters = mai.getInvocationContext().getParameters();
assertEquals(3, parameters.keySet().size());
UploadedFile[] files = (UploadedFile[]) parameters.get("file").getObject();
String[] fileContentTypes = parameters.get("fileContentType").getMultipleValues();
String[] fileRealFilenames = parameters.get("fileFileName").getMultipleValues();

assertNotNull(files);
assertNotNull(fileContentTypes);
assertNotNull(fileRealFilenames);
assertEquals(1, files.length);
assertEquals(1, fileContentTypes.length);
assertEquals(1, fileRealFilenames.length);
assertEquals("text/html", fileContentTypes[0]);
assertNotNull("deleteme.txt", fileRealFilenames[0]);

// Confirm normalFormField1, normalFormField2 were processed by the MultiPartRequestWrapper.
HttpServletRequest invocationServletRequest = mai.getInvocationContext().getServletRequest();
assertTrue("invocation servelt request is not a MultiPartRequestWrapper ?", invocationServletRequest instanceof MultiPartRequestWrapper);
MultiPartRequestWrapper multipartRequestWrapper = (MultiPartRequestWrapper) invocationServletRequest;
assertNotNull("normalFormField1 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField1"));
assertNotNull("normalFormField2 missing from MultiPartRequestWrapper parameters ?", multipartRequestWrapper.getParameter("normalFormField2"));
}

/**
* tests whether with multiple files sent with the same name, the ones with forbiddenTypes (see
* FileUploadInterceptor.setAllowedTypes(...) ) are sorted out.
Expand Down Expand Up @@ -598,6 +770,12 @@ private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, i
return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}

private MultiPartRequestWrapper createMultipartRequestNoMaxParamsSet(HttpServletRequest req) {

JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}

@Override
protected void setUp() throws Exception {
super.setUp();
Expand All @@ -621,7 +799,8 @@ public static class MyFileupAction extends ActionSupport {
private static final long serialVersionUID = 6255238895447968889L;

// no methods
// Note: We do not currently need fields/getters/setters for normalFormField1, normalFormField2 since
// the upload interceptor only prepares the normal field parameters.
}


}

0 comments on commit b359da5

Please sign in to comment.