Struts 2
  1. Struts 2
  2. WW-3025

Parameters get lost when file upload over max size allowed

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1.6
    • Fix Version/s: 2.3.20
    • 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
          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
          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
          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
          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
          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
          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
          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
          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
          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?
          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
          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.
          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
          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
          Hide
          pbenedict added a comment -

          Has anyone thought about that losing all parameters should be the expected behavior? Unless the W3C has some sort of guarantee on the order of parameters, I think it is possible for some parameters to also be after the file upload, which would lead to a really unexpected case of having some parameters but not all.

          Show
          pbenedict added a comment - Has anyone thought about that losing all parameters should be the expected behavior? Unless the W3C has some sort of guarantee on the order of parameters, I think it is possible for some parameters to also be after the file upload, which would lead to a really unexpected case of having some parameters but not all.
          Hide
          Chris Cranford added a comment -

          I believe most application/web servers have some configuration at the server level which dictates the maximum request and/or file upload request sizes. When these values are checked and exceeded, the server typically reports some 4xx/5xx error response to the client; therefore no handler code ever fires.

          In the cases where the handler code does fire, the server has already said and determined (since it didn't error) that space requirements were sufficient - then the upload parser should simply take the request and parse it. In the case where individual file sizes exceed some application setting, simply don't stream that part of the request - ignore it and move to the next chunk. That's precisely what my class I attached does.

          The end result is the action's state is maintained with all form parameters, excluding any file data which was considered outside the boundaries of the application's configuration. Now the action can query whatever it may need from a backing bean/service, copy whatever files were OK to their new location and report those files which failed with their conditions.

          The key here is that the application server itself determines the outcome. Either it's accepted or not; no gray area. When it reaches the application itself, now you can be free on how you want to handle it but at least you're guaranteed that the request's state complete rather than partial or non-existent.

          Show
          Chris Cranford added a comment - I believe most application/web servers have some configuration at the server level which dictates the maximum request and/or file upload request sizes. When these values are checked and exceeded, the server typically reports some 4xx/5xx error response to the client; therefore no handler code ever fires. In the cases where the handler code does fire, the server has already said and determined (since it didn't error) that space requirements were sufficient - then the upload parser should simply take the request and parse it. In the case where individual file sizes exceed some application setting, simply don't stream that part of the request - ignore it and move to the next chunk. That's precisely what my class I attached does. The end result is the action's state is maintained with all form parameters, excluding any file data which was considered outside the boundaries of the application's configuration. Now the action can query whatever it may need from a backing bean/service, copy whatever files were OK to their new location and report those files which failed with their conditions. The key here is that the application server itself determines the outcome. Either it's accepted or not; no gray area. When it reaches the application itself, now you can be free on how you want to handle it but at least you're guaranteed that the request's state complete rather than partial or non-existent.
          Hide
          ASF subversion and git services added a comment -

          Commit e7a414fea354ccf6694f3d101d53f5ff311c69d2 in struts's branch refs/heads/develop from Lukasz Lenart
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=e7a414f ]

          WW-3025 Adds new JakartaStream multipart request handler
          It solves problem with loosing parameters during large files which
          exceeds upload file limit

          Show
          ASF subversion and git services added a comment - Commit e7a414fea354ccf6694f3d101d53f5ff311c69d2 in struts's branch refs/heads/develop from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=e7a414f ] WW-3025 Adds new JakartaStream multipart request handler It solves problem with loosing parameters during large files which exceeds upload file limit
          Hide
          Lukasz Lenart added a comment -

          I added your MultiPart parser to Struts core libs, docs updated as well
          https://cwiki.apache.org/confluence/display/WW/File+Upload#FileUpload-AlternateLibraries

          Thanks Chris Cranford!

          Show
          Lukasz Lenart added a comment - I added your MultiPart parser to Struts core libs, docs updated as well https://cwiki.apache.org/confluence/display/WW/File+Upload#FileUpload-AlternateLibraries Thanks Chris Cranford !
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK6-develop #65 (See https://builds.apache.org/job/Struts-JDK6-develop/65/)
          WW-3025 Adds new JakartaStream multipart request handler (lukaszlenart: rev e7a414fea354ccf6694f3d101d53f5ff311c69d2)

          • core/src/main/resources/org/apache/struts2/default.properties
          • core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java
          • core/src/main/resources/struts-default.xml
          • core/src/main/java/org/apache/struts2/StrutsConstants.java
          Show
          Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-develop #65 (See https://builds.apache.org/job/Struts-JDK6-develop/65/ ) WW-3025 Adds new JakartaStream multipart request handler (lukaszlenart: rev e7a414fea354ccf6694f3d101d53f5ff311c69d2) core/src/main/resources/org/apache/struts2/default.properties core/src/main/java/org/apache/struts2/dispatcher/multipart/JakartaStreamMultiPartRequest.java core/src/main/resources/struts-default.xml core/src/main/java/org/apache/struts2/StrutsConstants.java

            People

            • Assignee:
              Lukasz Lenart
              Reporter:
              Tom Nguyen
            • Votes:
              6 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development