Uploaded image for project: 'Struts 2'
  1. Struts 2
  2. WW-4818

Default Multipart validation regex is invalid

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.12
    • Fix Version/s: 2.5.13
    • Component/s: None
    • Labels:
      None

      Description

      2.5.12 introduced a regex matches for multipart requests. The default regex used, however is significantly too strict based on the RFC, as well as common practice. Specifically, at minimum, it needs to include the hyphen and more likely needs to support all of the fields defined by the RFC (https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html).

      bcharsnospace := DIGIT / ALPHA / "'" / "(" / ")" / "+" / "_" / "," / "-" / "." / "/" / ":" / "=" / "?"

      In basic testing, we've seen:

       Content-Type: multipart/form-data; boundary=BRKIypZ3Stvuclu7C-CTbP2fNljGAOVk[\r][\n]

      (generated by the Apache HttpClient)
      and

      multipart/form-data; boundary=----WebKitFormBoundaryZGDtABnGWGozLAjh

      (generated by Safari)

        Issue Links

          Activity

          Hide
          abrin adam brin added a comment -

          HtmlUnit (which also uses the Apache HTTP client includes "_" commonly in the boundary.

          Content-Type: multipart/form-data; boundary=uhUF9k2Dei7tZu4UQYPyqpL8Upg_y5W

          Show
          abrin adam brin added a comment - HtmlUnit (which also uses the Apache HTTP client includes "_" commonly in the boundary. Content-Type: multipart/form-data; boundary=uhUF9k2Dei7tZu4UQYPyqpL8Upg_y5W
          Hide
          abrin adam brin added a comment -

          This is what we're currently using to test:

          <constant name="struts.multipart.validationRegex" value="^multipart\/form-data(; boundary=[a-zA-Z0-9\-\(\)\+\_\,\-\.\/\:\=\?]{1,70})?"/>
          
          Show
          abrin adam brin added a comment - This is what we're currently using to test: <constant name= "struts.multipart.validationRegex" value= "^multipart\/form-data(; boundary=[a-zA-Z0-9\-\(\)\+\_\,\-\.\/\:\=\?]{1,70})?" />
          Hide
          sdutry Stefaan Dutry added a comment - - edited

          Would the following regex be sufficient? (Keeping the characters in the order of the RFC spec and removing all unnecessary character escaping)

          regex
          ^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\-./:=?]{1,70})?
          

          or in the java code:

          org.apache.struts2.dispatcher.Dispatcher (line 91)
          public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70})?";
          

          edit: added _

          Show
          sdutry Stefaan Dutry added a comment - - edited Would the following regex be sufficient? (Keeping the characters in the order of the RFC spec and removing all unnecessary character escaping) regex ^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\-./:=?]{1,70})? or in the java code: org.apache.struts2.dispatcher.Dispatcher (line 91) public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70})?" ; edit: added _
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user sdutry opened a pull request:

          https://github.com/apache/struts/pull/151

          WW-4818 change default Multipart validation regex to comply with RFC1341

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/sdutry/struts WW-4818

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/struts/pull/151.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #151


          commit 68d52dbe42aebc8e24379ebfaf4f306dd261b91c
          Author: Stefaan Dutry <stefaan.dutry@gmail.com>
          Date: 2017-07-25T11:05:07Z

          WW-4818 change default Multipart validation regex to comply with RFC1341


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user sdutry opened a pull request: https://github.com/apache/struts/pull/151 WW-4818 change default Multipart validation regex to comply with RFC1341 You can merge this pull request into a Git repository by running: $ git pull https://github.com/sdutry/struts WW-4818 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/151.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #151 commit 68d52dbe42aebc8e24379ebfaf4f306dd261b91c Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-07-25T11:05:07Z WW-4818 change default Multipart validation regex to comply with RFC1341
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129280692

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          You have to escape `.` as it now means `Any single character`

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129280692 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – You have to escape `.` as it now means `Any single character`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129281026

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          Also `-` has to be escaped as it is treated as a group separator

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129281026 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – Also `-` has to be escaped as it is treated as a group separator
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129281083

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          not within the square brackets according to my knowledge

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129281083 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – not within the square brackets according to my knowledge
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129281192

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          o! didn't know that

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129281192 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – o! didn't know that
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129282110

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          you are correct about the `` needing to be escaped , but that's the only one i escaped. (they're in the order they're mentioned in RFC1341 (so i moved the `` backwards in the pattern)

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129282110 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – you are correct about the ` ` needing to be escaped , but that's the only one i escaped. (they're in the order they're mentioned in RFC1341 (so i moved the ` ` backwards in the pattern)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129282870

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          There are two test cases that you can extend or another one - see `DispatcherTest`

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129282870 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – There are two test cases that you can extend or another one - see `DispatcherTest`
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on a diff in the pull request:

          https://github.com/apache/struts/pull/151#discussion_r129283402

          — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java —
          @@ -88,7 +88,7 @@
          */
          public static final String REQUEST_POST_METHOD = "POST";

          • public static final String MULTIPART_FORM_DATA_REGEX = "^multipart
            /form-data(; boundary=\\-a-zA-Z0-9 {1,70})?";
            + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary=[0-9a-zA-Z'()+_,\\-./:=?]{1,70}

            )?";

              • End diff –

          Ok, i'll add some tests to confirm the pattern is correct.
          (Or if needed to correct it)

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on a diff in the pull request: https://github.com/apache/struts/pull/151#discussion_r129283402 — Diff: core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java — @@ -88,7 +88,7 @@ */ public static final String REQUEST_POST_METHOD = "POST"; public static final String MULTIPART_FORM_DATA_REGEX = "^multipart /form-data(; boundary= \\-a-zA-Z0-9 {1,70})?"; + public static final String MULTIPART_FORM_DATA_REGEX = "^multipart/form-data(; boundary= [0-9a-zA-Z'()+_,\\-./:=?] {1,70} )?"; End diff – Ok, i'll add some tests to confirm the pattern is correct. (Or if needed to correct it)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on the issue:

          https://github.com/apache/struts/pull/151

          @lukaszlenart
          I added 2 simple tests.

          • one containing all the special allowed characters
          • another one containing a single not-allowed character

          Please feel free to tell me what other test-cases you want added.
          (for example, any specific characters you want tested?)

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/151 @lukaszlenart I added 2 simple tests. one containing all the special allowed characters another one containing a single not-allowed character Please feel free to tell me what other test-cases you want added. (for example, any specific characters you want tested?)
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lukaszlenart commented on the issue:

          https://github.com/apache/struts/pull/151

          Looks good 👍 LGTM

          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/151 Looks good 👍 LGTM
          Hide
          abrin adam brin added a comment -

          I think that would work, here's what we put into our struts.xml (with escapes)

               <constant name="struts.multipart.validationRegex" value="^multipart\/form-data(; boundary=[a-zA-Z0-9\-\(\)\+\_\,\-\.\/\:\=\?]{1,70})?"/>
          
          Show
          abrin adam brin added a comment - I think that would work, here's what we put into our struts.xml (with escapes) <constant name= "struts.multipart.validationRegex" value= "^multipart\/form-data(; boundary=[a-zA-Z0-9\-\(\)\+\_\,\-\.\/\:\=\?]{1,70})?" />
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sdutry commented on the issue:

          https://github.com/apache/struts/pull/151

          @lukaszlenart
          Am i allowed to merge this or is there more work/checks that needs to happen first?

          Show
          githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/151 @lukaszlenart Am i allowed to merge this or is there more work/checks that needs to happen first?
          Hide
          githubbot ASF GitHub Bot added a comment -
          Show
          githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/151 @sdutry yes, you are http://struts.apache.org/submitting-patches.html#how-to-merge-pull-requests
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 68d52dbe42aebc8e24379ebfaf4f306dd261b91c in struts's branch refs/heads/master from Stefaan Dutry
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=68d52db ]

          WW-4818 change default Multipart validation regex to comply with RFC1341

          Show
          jira-bot ASF subversion and git services added a comment - Commit 68d52dbe42aebc8e24379ebfaf4f306dd261b91c in struts's branch refs/heads/master from Stefaan Dutry [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=68d52db ] WW-4818 change default Multipart validation regex to comply with RFC1341
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit bbbe2a80356811ff4dbaa99da2417a067eb614cc in struts's branch refs/heads/master from Stefaan Dutry
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=bbbe2a8 ]

          WW-4818 added a couple of simple tests for MULTIPART_FORM_DATA_REGEX

          Show
          jira-bot ASF subversion and git services added a comment - Commit bbbe2a80356811ff4dbaa99da2417a067eb614cc in struts's branch refs/heads/master from Stefaan Dutry [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=bbbe2a8 ] WW-4818 added a couple of simple tests for MULTIPART_FORM_DATA_REGEX
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/struts/pull/151

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/151
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Struts-master-JDK7 #7 (See https://builds.apache.org/job/Struts-master-JDK7/7/)
          WW-4818 change default Multipart validation regex to comply with RFC1341 (stefaan.dutry: rev 68d52dbe42aebc8e24379ebfaf4f306dd261b91c)

          • (edit) core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java
            WW-4818 added a couple of simple tests for MULTIPART_FORM_DATA_REGEX (stefaan.dutry: rev bbbe2a80356811ff4dbaa99da2417a067eb614cc)
          • (edit) core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-master-JDK7 #7 (See https://builds.apache.org/job/Struts-master-JDK7/7/ ) WW-4818 change default Multipart validation regex to comply with RFC1341 (stefaan.dutry: rev 68d52dbe42aebc8e24379ebfaf4f306dd261b91c) (edit) core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java WW-4818 added a couple of simple tests for MULTIPART_FORM_DATA_REGEX (stefaan.dutry: rev bbbe2a80356811ff4dbaa99da2417a067eb614cc) (edit) core/src/test/java/org/apache/struts2/dispatcher/DispatcherTest.java
          Hide
          thrawnca Mitth'raw'nuruodo added a comment -

          As a general FYI: You don't absolutely have to escape hyphens within character classes; you just need to put them at the start of the class.

          Show
          thrawnca Mitth'raw'nuruodo added a comment - As a general FYI: You don't absolutely have to escape hyphens within character classes; you just need to put them at the start of the class.

            People

            • Assignee:
              Unassigned
              Reporter:
              abrin adam brin
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development