CXF
  1. CXF
  2. CXF-3879

Add the ability to enforce a maximum attachment size

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.12, 2.3.7, 2.4.3
    • Fix Version/s: 2.4.4, 2.3.8
    • Component/s: JAX-RS
    • Labels:
      None
    • Estimated Complexity:
      Unknown

      Description

      Safe handling of multipart-* HTTP request requires the ability to cap the size of the uploaded attachments before they get cached. CXF does currently not provide an option for this (other frameworks such as the commons fileupload and the 3.0 servlet spec do provide this). I've attached a quick patch that allows one to set a option for enforcing a size limit while doing the attachment parsing (similar to the threshold and temp dir options). The biggest question imo is how to best bubble up a appropriate error. I chose to subclass IOException and then later on transform it into a 413 (request size too large) HTTP response, but would welcome input on other approaches.

      I will attach a patch against CXF 2.2, but believe that it should also apply to newer versions.

        Activity

        Hide
        Sam Meder added a comment -

        Enforce a optional attachment size limit - Patch against 2.2

        Show
        Sam Meder added a comment - Enforce a optional attachment size limit - Patch against 2.2
        Hide
        Sam Meder added a comment -

        Updated patch which factors out some repeated code.

        Show
        Sam Meder added a comment - Updated patch which factors out some repeated code.
        Hide
        Sam Meder added a comment -

        By the way, are there existing tests for any of the code I touched? Quick look didn't turn up any, so any pointers would be welcome.

        Show
        Sam Meder added a comment - By the way, are there existing tests for any of the code I touched? Quick look didn't turn up any, so any pointers would be welcome.
        Hide
        Daniel Kulp added a comment -

        This patch adds a dependency on javax.ws.rs.WebApplicationException into the core which wouldn't be allowed. It would have the throw the IOException that the JAX-RS runtime would need to map appropriately.

        Show
        Daniel Kulp added a comment - This patch adds a dependency on javax.ws.rs.WebApplicationException into the core which wouldn't be allowed. It would have the throw the IOException that the JAX-RS runtime would need to map appropriately.
        Hide
        Sergey Beryozkin added a comment -

        for JAXRS these classes are tested indirectly in systest/jaxrs/JAXRSMultipartProvider test, for JAXWS - in MTOM related tests in systest/jaxws.

        The patch looks good - can you please recreate it against the trunk ?
        And move throwing WebApplicationException to org.apache.cxf.jaxrs.provider.MultipartProvider.readFrom,
        try

        { List<Attachment> infos = AttachmentUtils.getAttachments(mc, attachmentDir, attachmentThreshold); }

        catch (CacheSizeExceededException ex)

        { throw new WebApplicationException(413); }

        as in SOAP case it will have to be 500...

        Unfortunately I won;t have time today to recreate the patch against the trunk - so if you can do it then shortly then I might be able to apply later this evening or may be Dan can do it later if he is OK with the changes, thanks

        Show
        Sergey Beryozkin added a comment - for JAXRS these classes are tested indirectly in systest/jaxrs/JAXRSMultipartProvider test, for JAXWS - in MTOM related tests in systest/jaxws. The patch looks good - can you please recreate it against the trunk ? And move throwing WebApplicationException to org.apache.cxf.jaxrs.provider.MultipartProvider.readFrom, try { List<Attachment> infos = AttachmentUtils.getAttachments(mc, attachmentDir, attachmentThreshold); } catch (CacheSizeExceededException ex) { throw new WebApplicationException(413); } as in SOAP case it will have to be 500... Unfortunately I won;t have time today to recreate the patch against the trunk - so if you can do it then shortly then I might be able to apply later this evening or may be Dan can do it later if he is OK with the changes, thanks
        Hide
        Sergey Beryozkin added a comment -

        "This patch adds a dependency on javax.ws.rs.WebApplicationException into the core which wouldn't be allowed. It would have the throw the IOException that the JAX-RS runtime would need to map appropriately."

        Yep...

        Show
        Sergey Beryozkin added a comment - "This patch adds a dependency on javax.ws.rs.WebApplicationException into the core which wouldn't be allowed. It would have the throw the IOException that the JAX-RS runtime would need to map appropriately." Yep...
        Hide
        Sam Meder added a comment -

        I'll add a updated patch later today. Thanks for the feedback.

        Show
        Sam Meder added a comment - I'll add a updated patch later today. Thanks for the feedback.
        Hide
        Sam Meder added a comment -

        Latest version moves the exception handling as suggested and adds a test for the new behavior.

        Show
        Sam Meder added a comment - Latest version moves the exception handling as suggested and adds a test for the new behavior.
        Hide
        Sam Meder added a comment -

        One more version. Moved handlind of the cache exception to MessageContextImpl since that seems to cover more code paths.

        Show
        Sam Meder added a comment - One more version. Moved handlind of the cache exception to MessageContextImpl since that seems to cover more code paths.
        Hide
        Sergey Beryozkin added a comment -

        thanks - looks good.
        If you can wait then I will apply it early next week as I'm off till Monday - I'd like to see if I can move away a property from FormEncodingProvider - may be it should stay there.

        Show
        Sergey Beryozkin added a comment - thanks - looks good. If you can wait then I will apply it early next week as I'm off till Monday - I'd like to see if I can move away a property from FormEncodingProvider - may be it should stay there.
        Hide
        Daniel Kulp added a comment -

        Patch applied. Major thanks!

        Show
        Daniel Kulp added a comment - Patch applied. Major thanks!

          People

          • Assignee:
            Daniel Kulp
            Reporter:
            Sam Meder
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development