Struts 2
  1. Struts 2
  2. WW-2073

File upload - maximum size validation does not work; (JakartaMultiPartRequest with struts.multipart.maxSize overrides fileInterceptor.maxsize)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 2.0.9
    • Fix Version/s: Future
    • Component/s: Core Interceptors
    • Labels:
      None
    • Flags:
      Patch

      Description

      By adding upload.setSizeMax to JakartaMultiPartRequest (@see https://issues.apache.org/struts/browse/WW-1549), the file upload interceptor max size validation has been disabled. Furthermore, whenever there is a single file above the allowed size none of the request parameters are set on the action This happens due to upload.parseRequest(createRequestContext(servletRequest)) which throws exception (o.a.c.fileupload.FileItemIteratorImpl checks max size).
      If the application requires the parameters it will fail.

      There is also slight confusion and disconnect between:
      struts.multipart.maxSize and fileInterceptor.maxsize

      If you upload file that exceeds allowed struts.multipart.maxSize fileInterceptor will never kick in.

      IMHO file upload interceptor should be responsible for max size validation (esp. that it reports errors as field errors and not as action errors).
      Furthermore, we should drop 'struts.multipart.maxSize' since is confusing along with fileInterceptor.maxsize, also fileInterceptor.maxsize should get a new default max size. The other option is to use struts.multipart.maxSize to set fileInterceptor.maxsize.

      This line should be removed so file interceptor can validate files:
      ------
      Index: src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java
      ===================================================================
      — src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java (revision 560614)
      +++ src/main/java/org/apache/struts2/dispatcher/multipart/JakartaMultiPartRequest.java (working copy)
      @@ -86,7 +86,6 @@
      // Parse the request
      try {
      ServletFileUpload upload = new ServletFileUpload(fac);

      • upload.setSizeMax(maxSize);
        List items = upload.parseRequest(createRequestContext(servletRequest));

      --------------

        Activity

        Lukasz Racon created issue -
        Jeff Turner made changes -
        Field Original Value New Value
        Workflow Struts [ 41615 ] Struts - editable closed status [ 42269 ]
        Ted Husted made changes -
        Fix Version/s 2.0.10 [ 21850 ]
        Hide
        Dale Newfield added a comment -

        I must admit, I am confused about this myself.

        I currently set both struts.multipart.maxSize and the "fileUpload" interceptor's "maximumSize" (different from maxsize ?)

        <interceptor-ref name="fileUpload">
        <param name="maximumSize">2097152</param><!-- 2MB -->
        <param name="allowedTypes">image/jpeg,image/pjpeg,application/octet-stream</param><!-- When a file upload field is left empty, firefox sends an empty file of this type - must allow it through the interceptor, but explicitly fail/ignore it (especially if the accompanying file size is non-zero) -->
        </interceptor-ref>

        Can I request that the first release including this fix include specific instructions as to what should/should not be set?

        Show
        Dale Newfield added a comment - I must admit, I am confused about this myself. I currently set both struts.multipart.maxSize and the "fileUpload" interceptor's "maximumSize" (different from maxsize ?) <interceptor-ref name="fileUpload"> <param name="maximumSize">2097152</param><!-- 2MB --> <param name="allowedTypes">image/jpeg,image/pjpeg,application/octet-stream</param><!-- When a file upload field is left empty, firefox sends an empty file of this type - must allow it through the interceptor, but explicitly fail/ignore it (especially if the accompanying file size is non-zero) --> </interceptor-ref> Can I request that the first release including this fix include specific instructions as to what should/should not be set?
        Hide
        Peter Motyka added a comment -

        To work around this, I'm setting struts.multipart.maxSize to a value larger than fileUpload.maximumSize. I agree that the upload.setSizeMax should be removed from JakartaMultiPartRequest.java.

        Show
        Peter Motyka added a comment - To work around this, I'm setting struts.multipart.maxSize to a value larger than fileUpload.maximumSize. I agree that the upload.setSizeMax should be removed from JakartaMultiPartRequest.java.
        Hide
        Don Brown added a comment -

        Hmm...I'm not sure we would want to do that. By checking the max size in the parser, we ensure that we don't parse and download a huge (probably malicious) file. This is important because it prevents a malicious user from submitting a 100 GB file that fills up our disk and brings the server down. The FileUploadIntereptor max size validator, on the other hand, only kicks in once the file is parsed and stored on disk.

        I do agree we need to erase this confusion. As I said the parser max size check is a security feature, while the other is more of an application validation. Any suggestions?

        Show
        Don Brown added a comment - Hmm...I'm not sure we would want to do that. By checking the max size in the parser, we ensure that we don't parse and download a huge (probably malicious) file. This is important because it prevents a malicious user from submitting a 100 GB file that fills up our disk and brings the server down. The FileUploadIntereptor max size validator, on the other hand, only kicks in once the file is parsed and stored on disk. I do agree we need to erase this confusion. As I said the parser max size check is a security feature, while the other is more of an application validation. Any suggestions?
        Hide
        Don Brown added a comment -

        Moving to 2.1.0 as more discussion is needed on this improvement.

        Show
        Don Brown added a comment - Moving to 2.1.0 as more discussion is needed on this improvement.
        Don Brown made changes -
        Issue Type Bug [ 1 ] Improvement [ 4 ]
        Fix Version/s 2.1.0 [ 21794 ]
        Fix Version/s 2.0.10 [ 21850 ]
        Ted Husted made changes -
        Fix Version/s 2.1.1 [ 21863 ]
        Fix Version/s 2.1.0 [ 21794 ]
        Hide
        Tom Schneider added a comment -

        Don, I think you're correct, I think we need both. Perhaps we just need to make it clearer in the documentation.

        Show
        Tom Schneider added a comment - Don, I think you're correct, I think we need both. Perhaps we just need to make it clearer in the documentation.
        Hide
        Ratna Sekhar added a comment -

        Hi can you tell me whether this bug is fixed or not,
        I am using struts -2 .0.6 jar file.
        I am uploading file more than 2mb and we are not using validation framework.
        so can you tell me a solution for this,
        so that i can upload more than 2mb file.

        Thanks
        Ratna

        Show
        Ratna Sekhar added a comment - Hi can you tell me whether this bug is fixed or not, I am using struts -2 .0.6 jar file. I am uploading file more than 2mb and we are not using validation framework. so can you tell me a solution for this, so that i can upload more than 2mb file. Thanks Ratna
        Hide
        Lukasz Racon added a comment -

        Please use user group to ask this kind of questions.
        To upload more then 2MB - check struts.multipart.maxSize in your default.properties/struts xml configuration and read java docs for FileUploadInterceptor. To get more details how both work please read this ticket again.

        Show
        Lukasz Racon added a comment - Please use user group to ask this kind of questions. To upload more then 2MB - check struts.multipart.maxSize in your default.properties/struts xml configuration and read java docs for FileUploadInterceptor. To get more details how both work please read this ticket again.
        Ted Husted made changes -
        Fix Version/s Future [ 21530 ]
        Fix Version/s 2.1.1 [ 21863 ]
        Hide
        James Mitchell added a comment -

        As stated by Don. We will not be removing this. In fact, I added this to PellMultiPartRequest as it was not using that setting.

        Show
        James Mitchell added a comment - As stated by Don. We will not be removing this. In fact, I added this to PellMultiPartRequest as it was not using that setting.
        James Mitchell made changes -
        Resolution Won't Fix [ 2 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Antonio Petrelli made changes -
        Workflow Struts - editable closed status [ 42269 ] Struts - editable closed status (temporary) [ 48713 ]
        Antonio Petrelli made changes -
        Workflow Struts - editable closed status (temporary) [ 48713 ] Struts - editable closed status [ 52109 ]
        Jeff Turner made changes -
        Project Import Mon Feb 01 01:17:42 UTC 2010 [ 1264987062082 ]
        Lukasz Lenart made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Lukasz Racon
          • Votes:
            3 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development