Struts 2
  1. Struts 2
  2. WW-3025

Parameters get lost when file upload over max size allowed

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.1.6
    • Fix Version/s: 2.3.18
    • Component/s: Core Interceptors
    • Labels:
      None
    • Environment:

      All

    • Flags:
      Important

      Description

      When the uploaded file gets rejected because it's content, size, or because of a general problem an Exception is thrown by the MultiPartRequest class. Exceptions are: InvalidContentTypeException, UnknownSizeException, SizeLimitExceededException, and FileUploadException. This can lead to serious problems within the application because the other parameters from the upload form get lost. Happening in a profile page for example means that the user data is lost this can lead to a security Exception. In other case this usually just involves a OGNL-Exception. Meaning your field data like personal file name is lost. Workaround found in http://henning.kropponline.de/index.php/2009/01/18/struts2-fileuploadbase-exception/, but the the still keep uploading to server, not secured.

        Issue Links

          Activity

          Hide
          Lukasz Lenart added a comment -

          Postponed as FILEUPLAOD issue is still unsolved

          Show
          Lukasz Lenart added a comment - Postponed as FILEUPLAOD issue is still unsolved
          Lukasz Lenart made changes -
          Fix Version/s 2.3.x [ 12319176 ]
          Fix Version/s 2.3.17 [ 12324780 ]
          Lukasz Lenart made changes -
          Fix Version/s 2.3.17 [ 12324780 ]
          Fix Version/s Future [ 12314681 ]
          Chris Cranford made changes -
          Attachment JakartaStreamMultiPartRequest.java [ 12613168 ]
          Hide
          Chris Cranford added a comment -

          This class uses the FileUpload Streaming API to minimize the memory footprint and avoid memory consumption on large uploads.

          It allows for the maximum request size check that generally was suggested to be set to -1 and be skipped. Instead, this class checks the request size itself rather than delegate to the FileUpload API, avoiding the exception that causes parameters to be lost.

          Parameter information is still passed to the action layer, excluding files should the request exceed the maximum size, populates an error messages list that is passed to the action.

          I configured this in my struts.xml as follows:

          	<bean type="org.apache.struts2.dispatcher.multipart.MultiPartRequest" name="jakartaStream" class="com.setech.mrovelocityhub.web.struts2.dispatcher.multipart.JakartaStreamMultiPartRequest" scope="default" />	
          	<constant name="struts.multipart.parser"				   value="jakartaStream" />
          	<constant name="struts.multipart.bufferSize"		       value="10240"				/>
          

          I added an additional constant called bufferSize to allow users to customize this as needed which would need to be added to StrutsConstants and removed from the attached class should this get bundled.

          Please review, tweak as needed.

          Show
          Chris Cranford added a comment - This class uses the FileUpload Streaming API to minimize the memory footprint and avoid memory consumption on large uploads. It allows for the maximum request size check that generally was suggested to be set to -1 and be skipped. Instead, this class checks the request size itself rather than delegate to the FileUpload API, avoiding the exception that causes parameters to be lost. Parameter information is still passed to the action layer, excluding files should the request exceed the maximum size, populates an error messages list that is passed to the action. I configured this in my struts.xml as follows: <bean type= "org.apache.struts2.dispatcher.multipart.MultiPartRequest" name= "jakartaStream" class= "com.setech.mrovelocityhub.web.struts2.dispatcher.multipart.JakartaStreamMultiPartRequest" scope= "default" /> <constant name= "struts.multipart.parser" value= "jakartaStream" /> <constant name= "struts.multipart.bufferSize" value= "10240" /> I added an additional constant called bufferSize to allow users to customize this as needed which would need to be added to StrutsConstants and removed from the attached class should this get bundled. Please review, tweak as needed.
          Hide
          Chris Cranford added a comment -

          I am working on a new multipart parser for Struts2 I am calling JakartaStreamMultiPartRequest.

          This multi-part parser behaves identical to the existing Jakarta multi-part parser except that it uses the Commons FileUpload Streaming API and rather than delegating maximum request size check to the File Upload API, it's done internally to avoid the existing problem of the Upload API breaking the loop iteration and parameters being lost.

          I should have it polished and posted as an attachment within the next 24-48 hours.

          Show
          Chris Cranford added a comment - I am working on a new multipart parser for Struts2 I am calling JakartaStreamMultiPartRequest. This multi-part parser behaves identical to the existing Jakarta multi-part parser except that it uses the Commons FileUpload Streaming API and rather than delegating maximum request size check to the File Upload API, it's done internally to avoid the existing problem of the Upload API breaking the loop iteration and parameters being lost. I should have it polished and posted as an attachment within the next 24-48 hours.
          Gavin made changes -
          Link This issue depends upon FILEUPLOAD-194 [ FILEUPLOAD-194 ]
          Gavin made changes -
          Link This issue depends on FILEUPLOAD-194 [ FILEUPLOAD-194 ]
          Lukasz Lenart made changes -
          Fix Version/s Future [ 12314681 ]
          Fix Version/s 2.3.1 [ 12315916 ]
          Hide
          Lukasz Lenart added a comment -

          I'm not sure if I understand it correctly and that's way I move it to the future

          Show
          Lukasz Lenart added a comment - I'm not sure if I understand it correctly and that's way I move it to the future
          Lukasz Lenart made changes -
          Fix Version/s 2.2.x [ 12314679 ]
          Hide
          Fabio Fucci added a comment - - edited

          Setting the maximum size property in the FileUploadInterceptor as Maurizio suggested

          <interceptor-ref name="fileUpload">
          <param name="maximumSize">100</param>
          </interceptor-ref>
          

          will solve only the simple case of checking the size of a single uploaded file (even if you have multiple uploaded files the interceptor checks for the size of every single file).
          The only way to limit the total size of uploaded files (the sum of all the files) is to act on struts.multipart.maxSize property (that is affected from the reported problem).

          Am I right?

          Show
          Fabio Fucci added a comment - - edited Setting the maximum size property in the FileUploadInterceptor as Maurizio suggested <interceptor-ref name="fileUpload"> <param name="maximumSize">100</param> </interceptor-ref> will solve only the simple case of checking the size of a single uploaded file (even if you have multiple uploaded files the interceptor checks for the size of every single file). The only way to limit the total size of uploaded files (the sum of all the files) is to act on struts.multipart.maxSize property (that is affected from the reported problem). Am I right?
          Maurizio Cucchiara made changes -
          Link This issue is cloned as WW-3665 [ WW-3665 ]
          Hide
          jagub zhang added a comment -

          I think there is 2 problems in this question.

          1.commons-fileupload can not control error.
          It can only throw FileUploadException. I suggest it supply an error handler.
          2.interface MultiPartRequest without method getFileSize()
          When FileUploadInterceptor check the file size, must use File#length() method.

          about the first one,
          we can create a new DiskFileItemFactory, create file only the file size under maximumSize
          If file size is above the maximumSize, only register size of the DiskFileItem.
          (just control fileupload error ourself, before commons team fix the problem)

          about the second,
          If MultiPartRequest supply getFileSize() method, FileUploadInterceptor can check file size without the real file object
          then we can check file size even it is not be stored.

          Show
          jagub zhang added a comment - I think there is 2 problems in this question. 1.commons-fileupload can not control error. It can only throw FileUploadException. I suggest it supply an error handler. 2.interface MultiPartRequest without method getFileSize() When FileUploadInterceptor check the file size, must use File#length() method. about the first one, we can create a new DiskFileItemFactory, create file only the file size under maximumSize If file size is above the maximumSize, only register size of the DiskFileItem. (just control fileupload error ourself, before commons team fix the problem) about the second, If MultiPartRequest supply getFileSize() method, FileUploadInterceptor can check file size without the real file object then we can check file size even it is not be stored.
          Hide
          Maurizio Cucchiara added a comment -

          Here it is the culprit FILEUPLOAD-194

          Show
          Maurizio Cucchiara added a comment - Here it is the culprit FILEUPLOAD-194
          Maurizio Cucchiara made changes -
          Link This issue depends on FILEUPLOAD-194 [ FILEUPLOAD-194 ]
          Hide
          Maurizio Cucchiara added a comment - - edited

          Hi Jagob,
          you're right but what you suggested it's a workaround.
          I'm going to follow Lukasz's advice, posting first a question to the commons ML.
          Before that I would like to write a simple struts, non-dependant, test.

          Show
          Maurizio Cucchiara added a comment - - edited Hi Jagob, you're right but what you suggested it's a workaround. I'm going to follow Lukasz's advice, posting first a question to the commons ML. Before that I would like to write a simple struts, non-dependant, test.
          Hide
          jagub zhang added a comment - - edited

          I don't think it is a problem.
          Struts has another setting for fileupload, "struts.multipart.maxSize" in struts.xml
          If don't set it , the default value is 2097152 (2M).
          It means size of any file over 2M, SizeLimitExceededException will happen.

          add this line into struts.xml
          <constant name="struts.multipart.maxSize" value="-1" />
          commons-fileupload will not check the size of the file.

          then add maximumSize parameter of the fileupload interceptor, just like Mr.Maurizio Cucchiara 's recommend.
          It will continue parse next parameters.

          Show
          jagub zhang added a comment - - edited I don't think it is a problem. Struts has another setting for fileupload, "struts.multipart.maxSize" in struts.xml If don't set it , the default value is 2097152 (2M). It means size of any file over 2M, SizeLimitExceededException will happen. add this line into struts.xml <constant name="struts.multipart.maxSize" value="-1" /> commons-fileupload will not check the size of the file. then add maximumSize parameter of the fileupload interceptor, just like Mr.Maurizio Cucchiara 's recommend. It will continue parse next parameters.
          Hide
          Lukasz Lenart added a comment -

          Maybe requesting change to commons-fileupload could help ?

          Show
          Lukasz Lenart added a comment - Maybe requesting change to commons-fileupload could help ?
          Hide
          Maurizio Cucchiara added a comment -

          Unfortunately commons-fileupload throws away every request that exceeds the size limit, as a result, struts loses any chance to recovery the request parameters.
          I strongly recommend to use the maximumSize parameter of the fileupload interceptor [1]:

          <action name="doUpload" class="com.example.UploadAction">
          <interceptor-ref name="fileUpload">
          <param name="maximumSize">100</param>
          </interceptor-ref>
          <interceptor-ref name="basicStack"/>
          <interceptor-ref name="workflow"/>

          <result name="input">upload.jsp</result>
          <result name="success">good_result.jsp</result>
          </action>

          I also suggest to include workflow interceptor to the interceptor stack, such that it yield the input result.

          [1] http://struts.apache.org/2.2.3/docs/file-upload-interceptor.html

          Show
          Maurizio Cucchiara added a comment - Unfortunately commons-fileupload throws away every request that exceeds the size limit, as a result, struts loses any chance to recovery the request parameters. I strongly recommend to use the maximumSize parameter of the fileupload interceptor [1] : <action name="doUpload" class="com.example.UploadAction"> <interceptor-ref name="fileUpload"> <param name="maximumSize">100</param> </interceptor-ref> <interceptor-ref name="basicStack"/> <interceptor-ref name="workflow"/> <result name="input">upload.jsp</result> <result name="success">good_result.jsp</result> </action> I also suggest to include workflow interceptor to the interceptor stack, such that it yield the input result. [1] http://struts.apache.org/2.2.3/docs/file-upload-interceptor.html
          Lukasz Lenart made changes -
          Fix Version/s 2.3 [ 12315916 ]
          Hide
          Scott Stanlick added a comment -

          Any movement on this one? I worked about six hours yesterday on this one before determining the problem was here. I think the file upload docs should point out this nasty, nasty side effect. As far as refactoring goes, a more immediate resolution might be to modify the processUpload in JakartaMultiPartRequest to deal with the FileUploadException and populate the processNormalFormField(...) unconditionally.

          Also, the workaround link above is dead – as am I after chasing this nasty pig.

          Peace,
          Scott

          Show
          Scott Stanlick added a comment - Any movement on this one? I worked about six hours yesterday on this one before determining the problem was here. I think the file upload docs should point out this nasty, nasty side effect. As far as refactoring goes, a more immediate resolution might be to modify the processUpload in JakartaMultiPartRequest to deal with the FileUploadException and populate the processNormalFormField(...) unconditionally. Also, the workaround link above is dead – as am I after chasing this nasty pig. Peace, Scott
          Jeff Turner made changes -
          Project Import Mon Feb 01 01:17:42 UTC 2010 [ 1264987062082 ]
          Wes Wannemacher made changes -
          Field Original Value New Value
          Fix Version/s 2.2.x [ 21892 ]
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          Hide
          Wes Wannemacher added a comment -

          This sounds like a bit of refactoring would be required. Pushing off until 2.2.x

          Show
          Wes Wannemacher added a comment - This sounds like a bit of refactoring would be required. Pushing off until 2.2.x
          Tom Nguyen created issue -

            People

            • Assignee:
              Unassigned
              Reporter:
              Tom Nguyen
            • Votes:
              6 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:

                Development