From 2d6f1bc0a6f5ac575a56784ac6461816b67c4f21 Mon Sep 17 00:00:00 2001 From: Yasser Zamani Date: Mon, 5 Jun 2023 13:37:59 +0430 Subject: [PATCH] add some improvements --- .../accessor/XWorkListPropertyAccessor.java | 5 ++ .../org/apache/struts2/StrutsConstants.java | 3 + .../config/entities/ConstantConfig.java | 10 +++ .../multipart/AbstractMultiPartRequest.java | 10 +++ .../multipart/JakartaMultiPartRequest.java | 13 +++- .../org/apache/struts2/default.properties | 1 + .../apache/struts2/struts-messages.properties | 1 + .../XWorkListPropertyAccessorTest.java | 16 +++-- .../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 ece1a92488..47a6404381 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 Object getProperty(Map context, Object target, Object name) throws OgnlEx 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 15eb43d870..1f71646fd0 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 f728d91154..9210d767fb 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 Map getAllAsStringsMap() { 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 void setMultipartMaxFiles(Long multipartMaxFiles) { 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 e46cecc001..1c90c40124 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 void setMaxFiles(String maxFiles) { 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 00d9224013..5030ab7d4b 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 @@ protected void processNormalFormField(FileItem item, String charset) throws Unsu 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 8c4144de86..250c7d4f83 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 bf75f05b4a..aee519afff 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 66c716c2c0..4a89d89354 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.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 void testContains() { 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 void testCanAccessListSizeProperty() { 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 void testAutoGrowthCollectionLimit() { 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 842a7db3ac..8996c593ec 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 void testInvalidContentTypeMultipartRequest() throws Exception { 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 void testNoContentMultipartRequest() throws Exception { 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 void testSuccessUploadOfATextFileMultipartRequest() throws Exception { mai.setInvocationContext(ActionContext.getContext()); Map 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 void testMultipleAccept() throws Exception { mai.setInvocationContext(ActionContext.getContext()); Map 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); @@ -402,7 +402,7 @@ public void testUnacceptedNumberOfFiles() throws Exception { mai.setInvocationContext(ActionContext.getContext()); Map 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 void testUnacceptedNumberOfFiles() throws Exception { 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 param = new HashMap<>(); + ActionContext.getContext() + .withParameters(HttpParameters.create(param).build()) + .withServletRequest(createMultipartRequest(req, -1, 20)); + + interceptor.intercept(mai); + + assertTrue(action.hasActionErrors()); + + Collection 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 void testMultipartRequestLocalizedError() throws Exception { 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 @@ private String encodeTextFile(String bondary, String endline, String name, Strin 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()); }