add some improvements

apache · Jun 4, 2023 · 5a67d58 · 5a67d58
1 parent 3ef7747
commit 5a67d58
Show file tree

Hide file tree

Showing 9 changed files with 111 additions and 12 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
@@ -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
@@ -145,6 +145,9 @@ public final class StrutsConstants {
     /** The maximum number of files allowed in a multipart request */
     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 maximum size per file in a multipart request */
     public static final String STRUTS_MULTIPART_MAXFILESIZE = "struts.multipart.maxFileSize";
     /** The directory to use for storing uploaded files */

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
@@ -66,6 +66,7 @@ public class ConstantConfig {
     private Long multipartMaxSize;
     private Long multipartMaxFiles;
     private Long multipartMaxFileSize;
+    private Long multipartMaxStringLength;
     private String multipartSaveDir;
     private Integer multipartBufferSize;
     private BeanConfig multipartParser;
@@ -201,6 +202,7 @@ public Map<String, String> getAllAsStringsMap() {
         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_MAXFILESIZE, Objects.toString(multipartMaxFileSize, 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));
@@ -602,6 +604,14 @@ public void setMultipartMaxFileSize(Long multipartMaxFileSize) {
         this.multipartMaxFileSize = multipartMaxFileSize;
     }
 
+    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
@@ -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 maximum size per file in the request.
      */
@@ -106,6 +111,11 @@ public void setMaxFileSize(String maxFileSize) {
         this.maxFileSize = Long.parseLong(maxFileSize);
     }
 
+    @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
@@ -140,8 +140,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
@@ -69,6 +69,7 @@ struts.multipart.parser=jakarta
 struts.multipart.saveDir=
 struts.multipart.maxSize=2097152
 struts.multipart.maxFiles=256
+struts.multipart.maxStringLength=4096
 # struts.multipart.maxFileSize=
 
 ### Load custom property files (does not override struts.properties!)

diff --git 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
@@ -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
@@ -452,6 +452,55 @@ public void testMultipartRequestMaxFileSize() throws Exception {
                 msg);
     }
 
+    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(createMultipartRequestMaxStringLength(req, 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());
@@ -512,22 +561,29 @@ private String encodeTextFile(String bondary, String endline, String name, Strin
     }
 
     private MultiPartRequestWrapper createMultipartRequestMaxFileSize(HttpServletRequest req, int maxfilesize) throws IOException {
-        return createMultipartRequest(req, -1, maxfilesize, -1);
+        return createMultipartRequest(req, -1, maxfilesize, -1, -1);
     }
 
     private MultiPartRequestWrapper createMultipartRequestMaxFiles(HttpServletRequest req, int maxfiles) throws IOException {
-        return createMultipartRequest(req, -1, -1, maxfiles);
+        return createMultipartRequest(req, -1, -1, maxfiles, -1);
     }
 
     private MultiPartRequestWrapper createMultipartRequestMaxSize(HttpServletRequest req, int maxsize) throws IOException {
-        return createMultipartRequest(req, maxsize, -1, -1);
+        return createMultipartRequest(req, maxsize, -1, -1, -1);
     }
 
-    private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxfilesize, int maxfiles) throws IOException {
+    private MultiPartRequestWrapper createMultipartRequestMaxStringLength(HttpServletRequest req, int maxStringLength) throws IOException {
+        return createMultipartRequest(req, -1, -1, -1, maxStringLength);
+    }
+
+    private MultiPartRequestWrapper createMultipartRequest(HttpServletRequest req, int maxsize, int maxfilesize,
+                                                           int maxfiles, int maxStringLength) throws IOException {
+
         JakartaMultiPartRequest jak = new JakartaMultiPartRequest();
         jak.setMaxSize(String.valueOf(maxsize));
         jak.setMaxFileSize(String.valueOf(maxfilesize));
         jak.setMaxFiles(String.valueOf(maxfiles));
+        jak.setMaxStringLength(String.valueOf(maxStringLength));
         return new MultiPartRequestWrapper(jak, req, tempDir.getAbsolutePath(), new DefaultLocaleProvider());
     }