You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by ya...@apache.org on 2023/06/05 09:08:47 UTC
[struts] 01/01: add some improvements
This is an automated email from the ASF dual-hosted git repository.
yasserzamani pushed a commit to branch struts-6-1-2-1
in repository https://gitbox.apache.org/repos/asf/struts.git
commit 2d6f1bc0a6f5ac575a56784ac6461816b67c4f21
Author: Yasser Zamani <ya...@apache.org>
AuthorDate: Mon Jun 5 13:37:59 2023 +0430
add some improvements
---
.../ognl/accessor/XWorkListPropertyAccessor.java | 5 ++
.../java/org/apache/struts2/StrutsConstants.java | 3 +
.../struts2/config/entities/ConstantConfig.java | 10 ++++
.../multipart/AbstractMultiPartRequest.java | 10 ++++
.../multipart/JakartaMultiPartRequest.java | 13 ++++-
.../org/apache/struts2/default.properties | 1 +
.../org/apache/struts2/struts-messages.properties | 1 +
.../accessor/XWorkListPropertyAccessorTest.java | 16 +++---
.../interceptor/FileUploadInterceptorTest.java | 64 +++++++++++++++++++---
9 files changed, 108 insertions(+), 15 deletions(-)
diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java
index ece1a9248..47a640438 100644
--- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java
+++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessor.java
@@ -109,6 +109,11 @@ public class XWorkListPropertyAccessor extends ListPropertyAccessor {
if (listSize <= index) {
Object result;
+ if (index > autoGrowCollectionLimit) {
+ throw new OgnlException("Error auto growing collection size to " + index + " which limited to "
+ + autoGrowCollectionLimit);
+ }
+
for (int i = listSize; i < index; i++) {
list.add(null);
}
diff --git a/core/src/main/java/org/apache/struts2/StrutsConstants.java b/core/src/main/java/org/apache/struts2/StrutsConstants.java
index 15eb43d87..1f71646fd 100644
--- a/core/src/main/java/org/apache/struts2/StrutsConstants.java
+++ b/core/src/main/java/org/apache/struts2/StrutsConstants.java
@@ -145,6 +145,9 @@ public final class StrutsConstants {
/** The maximized number of files allowed to upload */
public static final String STRUTS_MULTIPART_MAXFILES = "struts.multipart.maxFiles";
+ /** The maximum length of a string parameter in a multipart request. */
+ public static final String STRUTS_MULTIPART_MAX_STRING_LENGTH = "struts.multipart.maxStringLength";
+
/** The directory to use for storing uploaded files */
public static final String STRUTS_MULTIPART_SAVEDIR = "struts.multipart.saveDir";
diff --git a/core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java b/core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java
index f728d9115..9210d767f 100644
--- a/core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java
+++ b/core/src/main/java/org/apache/struts2/config/entities/ConstantConfig.java
@@ -65,6 +65,7 @@ public class ConstantConfig {
private String uiThemeExpansionToken;
private Long multipartMaxSize;
private Long multipartMaxFiles;
+ private Long multipartMaxStringLength;
private String multipartSaveDir;
private Integer multipartBufferSize;
private BeanConfig multipartParser;
@@ -197,6 +198,7 @@ public class ConstantConfig {
map.put(StrutsConstants.STRUTS_UI_THEME_EXPANSION_TOKEN, uiThemeExpansionToken);
map.put(StrutsConstants.STRUTS_MULTIPART_MAXSIZE, Objects.toString(multipartMaxSize, null));
map.put(StrutsConstants.STRUTS_MULTIPART_MAXFILES, Objects.toString(multipartMaxFiles, null));
+ map.put(StrutsConstants.STRUTS_MULTIPART_MAX_STRING_LENGTH, Objects.toString(multipartMaxStringLength, null));
map.put(StrutsConstants.STRUTS_MULTIPART_SAVEDIR, multipartSaveDir);
map.put(StrutsConstants.STRUTS_MULTIPART_BUFFERSIZE, Objects.toString(multipartBufferSize, null));
map.put(StrutsConstants.STRUTS_MULTIPART_PARSER, beanConfToString(multipartParser));
@@ -590,6 +592,14 @@ public class ConstantConfig {
this.multipartMaxFiles = multipartMaxFiles;
}
+ public Long getMultipartMaxStringLength() {
+ return multipartMaxStringLength;
+ }
+
+ public void setMultipartMaxStringLength(Long multipartMaxStringLength) {
+ this.multipartMaxStringLength = multipartMaxStringLength;
+ }
+
public String getMultipartSaveDir() {
return multipartSaveDir;
}
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 e46cecc00..1c90c4012 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
@@ -58,6 +58,11 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
*/
protected Long maxFiles;
+ /**
+ * Specifies the maximum length of a string parameter in a multipart request.
+ */
+ protected Long maxStringLength;
+
/**
* Specifies the buffer size to use during streaming.
*/
@@ -96,6 +101,11 @@ public abstract class AbstractMultiPartRequest implements MultiPartRequest {
this.maxFiles = Long.parseLong(maxFiles);
}
+ @Inject(StrutsConstants.STRUTS_MULTIPART_MAX_STRING_LENGTH)
+ public void setMaxStringLength(String maxStringLength) {
+ this.maxStringLength = Long.parseLong(maxStringLength);
+ }
+
@Inject
public void setLocaleProviderFactory(LocaleProviderFactory localeProviderFactory) {
defaultLocale = localeProviderFactory.createLocaleProvider().getLocale();
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 00d922401..5030ab7d4 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
@@ -137,8 +137,19 @@ public class JakartaMultiPartRequest extends AbstractMultiPartRequest {
values = new ArrayList<>();
}
- if (item.getSize() == 0) {
+ long size = item.getSize();
+ if (size == 0) {
values.add(StringUtils.EMPTY);
+ } else if (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 });
+
+ if (!errors.contains(localizedMessage)) {
+ errors.add(localizedMessage);
+ }
+ return;
+
} else if (charset != null) {
values.add(item.getString(charset));
} else {
diff --git a/core/src/main/resources/org/apache/struts2/default.properties b/core/src/main/resources/org/apache/struts2/default.properties
index 8c4144de8..250c7d4f8 100644
--- a/core/src/main/resources/org/apache/struts2/default.properties
+++ b/core/src/main/resources/org/apache/struts2/default.properties
@@ -69,6 +69,7 @@ struts.multipart.parser=jakarta
struts.multipart.saveDir=
struts.multipart.maxSize=2097152
struts.multipart.maxFiles=256
+struts.multipart.maxStringLength=4096
### Load custom property files (does not override struts.properties!)
# struts.custom.properties=application,org/apache/struts2/extension/custom
diff --git a/core/src/main/resources/org/apache/struts2/struts-messages.properties b/core/src/main/resources/org/apache/struts2/struts-messages.properties
index bf75f05b4..aee519aff 100644
--- a/core/src/main/resources/org/apache/struts2/struts-messages.properties
+++ b/core/src/main/resources/org/apache/struts2/struts-messages.properties
@@ -26,6 +26,7 @@ struts.messages.invalid.content.type=Could not find a Content-Type for {0}. Veri
struts.messages.removing.file=Removing file {0} {1}
struts.messages.error.uploading=Error uploading: {0}
struts.messages.error.file.too.large=File {0} is too large to be uploaded. Maximum allowed size is {4} bytes!
+struts.messages.upload.error.parameter.too.long=The request parameter "{0}" was too long. Max length allowed is {1}, but found {2}!
struts.messages.error.content.type.not.allowed=Content-Type not allowed: {0} "{1}" "{2}" {3}
struts.messages.error.file.extension.not.allowed=File extension not allowed: {0} "{1}" "{2}" {3}
diff --git a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java
index 66c716c2c..4a89d8935 100644
--- a/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java
+++ b/core/src/test/java/com/opensymphony/xwork2/ognl/accessor/XWorkListPropertyAccessorTest.java
@@ -22,7 +22,7 @@ import com.opensymphony.xwork2.ActionContext;
import com.opensymphony.xwork2.XWorkTestCase;
import com.opensymphony.xwork2.util.ListHolder;
import com.opensymphony.xwork2.util.ValueStack;
-import ognl.ListPropertyAccessor;
+import com.opensymphony.xwork2.util.reflection.ReflectionContextState;
import ognl.PropertyAccessor;
import java.util.ArrayList;
@@ -42,11 +42,11 @@ public class XWorkListPropertyAccessorTest extends XWorkTestCase {
assertNotNull(listHolder.getLongs());
assertEquals(3, listHolder.getLongs().size());
- assertEquals(new Long(1), (Long) listHolder.getLongs().get(0));
- assertEquals(new Long(2), (Long) listHolder.getLongs().get(1));
- assertEquals(new Long(3), (Long) listHolder.getLongs().get(2));
+ assertEquals(new Long(1), listHolder.getLongs().get(0));
+ assertEquals(new Long(2), listHolder.getLongs().get(1));
+ assertEquals(new Long(3), listHolder.getLongs().get(2));
- assertTrue(((Boolean) vs.findValue("longs.contains(1)")).booleanValue());
+ assertTrue((Boolean) vs.findValue("longs.contains(1)"));
}
public void testCanAccessListSizeProperty() {
@@ -60,8 +60,8 @@ public class XWorkListPropertyAccessorTest extends XWorkTestCase {
vs.push(listHolder);
- assertEquals(new Integer(myList.size()), vs.findValue("strings.size()"));
- assertEquals(new Integer(myList.size()), vs.findValue("strings.size"));
+ assertEquals(myList.size(), vs.findValue("strings.size()"));
+ assertEquals(myList.size(), vs.findValue("strings.size"));
}
public void testAutoGrowthCollectionLimit() {
@@ -73,12 +73,14 @@ public class XWorkListPropertyAccessorTest extends XWorkTestCase {
listHolder.setStrings(myList);
ValueStack vs = ActionContext.getContext().getValueStack();
+ ReflectionContextState.setCreatingNullObjects(vs.getContext(), true);
vs.push(listHolder);
vs.setValue("strings[0]", "a");
vs.setValue("strings[1]", "b");
vs.setValue("strings[2]", "c");
vs.setValue("strings[3]", "d");
+ vs.findValue("strings[3]");
assertEquals(3, vs.findValue("strings.size()"));
}
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 842a7db3a..8996c593e 100644
--- a/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
+++ b/core/src/test/java/org/apache/struts2/interceptor/FileUploadInterceptorTest.java
@@ -235,7 +235,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
mai.setInvocationContext(ActionContext.getContext());
ActionContext.getContext().setParameters(HttpParameters.create().build());
- ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
+ ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
interceptor.intercept(mai);
@@ -257,7 +257,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
mai.setInvocationContext(ActionContext.getContext());
ActionContext.getContext().setParameters(HttpParameters.create().build());
- ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
+ ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
interceptor.intercept(mai);
@@ -288,7 +288,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
mai.setInvocationContext(ActionContext.getContext());
Map<String, Object> param = new HashMap<>();
ActionContext.getContext().setParameters(HttpParameters.create(param).build());
- ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
+ ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
interceptor.intercept(mai);
@@ -349,7 +349,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
mai.setInvocationContext(ActionContext.getContext());
Map<String, Object> param = new HashMap<String, Object>();
ActionContext.getContext().setParameters(HttpParameters.create(param).build());
- ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
+ ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
interceptor.setAllowedTypes("text/html");
interceptor.intercept(mai);
@@ -402,7 +402,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
mai.setInvocationContext(ActionContext.getContext());
Map<String, Object> param = new HashMap<>();
ActionContext.getContext().setParameters(HttpParameters.create(param).build());
- ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000));
+ ActionContext.getContext().put(ServletActionContext.HTTP_REQUEST, createMultipartRequest(req, 2000, -1));
interceptor.setAllowedTypes("text/html");
interceptor.intercept(mai);
@@ -413,6 +413,55 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
assertEquals("Request exceeded allowed number of files! Max allowed files number is: 3!", action.getActionErrors().iterator().next());
}
+ public void testMultipartRequestMaxStringLength() 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" +
+ "it works" +
+ "\r\n" +
+ "-----1234\r\n" +
+ "Content-Disposition: form-data; name=\"normalFormField2\"\r\n" +
+ "\r\n" +
+ "long string should not work" +
+ "\r\n" +
+ "-----1234--\r\n");
+ req.setContent(content.getBytes(StandardCharsets.US_ASCII));
+
+ MyFileupAction action = container.inject(MyFileupAction.class);
+
+ 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())
+ .withServletRequest(createMultipartRequest(req, -1, 20));
+
+ interceptor.intercept(mai);
+
+ assertTrue(action.hasActionErrors());
+
+ Collection<String> errors = action.getActionErrors();
+ assertEquals(1, errors.size());
+ String msg = errors.iterator().next();
+ assertEquals(
+ "The request parameter \"normalFormField2\" was too long. Max length allowed is 20, but found 27!",
+ msg);
+ }
+
public void testMultipartRequestLocalizedError() throws Exception {
MockHttpServletRequest req = new MockHttpServletRequest();
req.setCharacterEncoding(StandardCharsets.UTF_8.name());
@@ -439,7 +488,7 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
ActionContext.getContext()
.withParameters(HttpParameters.create(param).build())
.withLocale(Locale.GERMAN)
- .withServletRequest(createMultipartRequest(req, 10));
+ .withServletRequest(createMultipartRequest(req, 10, -1));
interceptor.intercept(mai);
@@ -472,10 +521,11 @@ public class FileUploadInterceptorTest extends StrutsInternalTestCase {
return sb.toString();
}
- private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize) throws IOException {
+ private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxStringLength) throws IOException {
JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
jak.setMaxSize(String.valueOf(maxsize));
jak.setMaxFiles("3");
+ jak.setMaxStringLength(String.valueOf(maxStringLength));
return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
}