Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.33, 2.5.12
    • Fix Version/s: 2.3.34, 2.5.13
    • Component/s: Core, XML Validators
    • Labels:
      None

      Issue Links

        Activity

        Hide
        lukaszlenart Lukasz Lenart added a comment -

        PRs got merged, thanks a lot

        Show
        lukaszlenart Lukasz Lenart added a comment - PRs got merged, thanks a lot
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/159
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3fddfb6eb562d597c935084e9e81d43ed6bcd02c in struts's branch refs/heads/support-2-3 from Stefaan Dutry
        [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=3fddfb6 ]

        WW-4834 Improve RegEx used to validate URLs

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3fddfb6eb562d597c935084e9e81d43ed6bcd02c in struts's branch refs/heads/support-2-3 from Stefaan Dutry [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=3fddfb6 ] WW-4834 Improve RegEx used to validate URLs
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sdutry commented on the issue:

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

        @lukaszlenart
        i copied the regex after the changes from #157 .
        So it should be the same. Feel free to check.

        Show
        githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/159 @lukaszlenart i copied the regex after the changes from #157 . So it should be the same. Feel free to check.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user lukaszlenart commented on the issue:

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

        Is it ready to merge? Does it contain the latest changes from #157?

        Show
        githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/159 Is it ready to merge? Does it contain the latest changes from #157?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/157
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        WW-4834 fixes faulty regex

        Show
        jira-bot ASF subversion and git services added a comment - Commit a05259ed69a5a48379aa91650e4cd1cb4bd6e5ad in struts's branch refs/heads/master from Lukasz Lenart [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=a05259e ] WW-4834 fixes faulty regex
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        WW-4834 fixed faulty regex

        Show
        jira-bot ASF subversion and git services added a comment - Commit 8a04e80f01350c90f053d71366d5e0c2186fded5 in struts's branch refs/heads/master from Stefaan Dutry [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=8a04e80 ] WW-4834 fixed faulty regex
        Hide
        sdutry Stefaan Dutry added a comment -

        Lukasz Lenart
        Feel free to merge when you think it's ok.
        I've also made a pull request for the support-2-3 branch.

        Alternatively if you want me to merge, please let me know, also if pull request 159 should be merged or not.

        Show
        sdutry Stefaan Dutry added a comment - Lukasz Lenart Feel free to merge when you think it's ok. I've also made a pull request for the support-2-3 branch. Alternatively if you want me to merge, please let me know, also if pull request 159 should be merged or not.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user atcazzual commented on the issue:

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

        Looks good to me. All matching capabilities from prior to the optimizations seem to be there.

        Show
        githubbot ASF GitHub Bot added a comment - Github user atcazzual commented on the issue: https://github.com/apache/struts/pull/157 Looks good to me. All matching capabilities from prior to the optimizations seem to be there.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sdutry commented on the issue:

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

        see #157 and #156

        Show
        githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/159 see #157 and #156
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user sdutry opened a pull request:

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

        WW-4834 Improve RegEx used to validate URLs

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

        $ git pull https://github.com/sdutry/struts WW-4834-support-2-3

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

        https://github.com/apache/struts/pull/159.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 #159


        commit 3fddfb6eb562d597c935084e9e81d43ed6bcd02c
        Author: Stefaan Dutry <stefaan.dutry@gmail.com>
        Date: 2017-08-04T11:58:31Z

        WW-4834 Improve RegEx used to validate URLs


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user sdutry opened a pull request: https://github.com/apache/struts/pull/159 WW-4834 Improve RegEx used to validate URLs You can merge this pull request into a Git repository by running: $ git pull https://github.com/sdutry/struts WW-4834 -support-2-3 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/159.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 #159 commit 3fddfb6eb562d597c935084e9e81d43ed6bcd02c Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-08-04T11:58:31Z WW-4834 Improve RegEx used to validate URLs
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user lukaszlenart commented on the issue:

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

        I would wait a bit and give Adam a chance to post a comment (if he wants to). No rush

        Show
        githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/157 I would wait a bit and give Adam a chance to post a comment (if he wants to). No rush
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sdutry commented on the issue:

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

        @lukaszlenart
        Sorry for breaking it in the first place. That wasn't my intention.

        Do you want me to merge this now, or am i still overlooking stuff?

        Show
        githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/157 @lukaszlenart Sorry for breaking it in the first place. That wasn't my intention. Do you want me to merge this now, or am i still overlooking stuff?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user lukaszlenart commented on the issue:

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

        I think we are good here, thanks a lot for yours work

        Show
        githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/157 I think we are good here, thanks a lot for yours work
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sdutry commented on the issue:

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

        > After the commit above, any IP with 3 digits in the last octet will not pass validation

        You are right, i forgot the grouping (meaning the or statements mean something completely different), i'll fix this ASAP

        > The expression did not seem to work at all until I escaped the slashes, changing / to \/
        I don't see why this would be. I am not aware that a "/" is a special sign inside a java string or java RegEx. Or is it because this same regex is used in javascript regex validation (where the '/' sign does mean something). I'll change it back ASAP too

        Show
        githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/157 > After the commit above, any IP with 3 digits in the last octet will not pass validation You are right, i forgot the grouping (meaning the or statements mean something completely different), i'll fix this ASAP > The expression did not seem to work at all until I escaped the slashes, changing / to \/ I don't see why this would be. I am not aware that a "/" is a special sign inside a java string or java RegEx. Or is it because this same regex is used in javascript regex validation (where the '/' sign does mean something). I'll change it back ASAP too
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user atcazzual commented on the issue:

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

        The expression did not seem to work at all until I escaped the slashes, changing `/` to `\/`

        Once I got it working, there then seems to be a bug in the new expression when matching on URLs that use IP addresses. The grouping has changed causing two problems with matching IP addresses.

        1. The dot `.` character that delimits the octets in an IP address only applies to the last condition, `25[0-5]\.` on line 57, instead of all conditions for an IP octet. This makes matching most IP address fail. The only IPs that will match would need to have 3-digit octets for the first three where the first two-digits are `25`. NOTE: This seems to have been resolved by the commit above.
        2. The conditions for the last octet are no longer grouped (line 58) making the OR `|` operator act on what was a higher level group. Because of this, the fourth octet would have to be only one or two digits.

        For example, only IPs like the following will pass validation:
        http://**25**3.**25**4.**25**5.1 (mostly resolved by the commit above)
        http://**25**3.**25**4.**25**5.12 (mostly resolved by the commit above)

        After the commit above, any IP with 3 digits in the last octet will *not* pass validation:
        http<nolink>://1.2.3.*100*
        http<nolink>://1.2.3.*255*

        Show
        githubbot ASF GitHub Bot added a comment - Github user atcazzual commented on the issue: https://github.com/apache/struts/pull/157 The expression did not seem to work at all until I escaped the slashes, changing `/` to `\/` Once I got it working, there then seems to be a bug in the new expression when matching on URLs that use IP addresses. The grouping has changed causing two problems with matching IP addresses. 1. The dot `.` character that delimits the octets in an IP address only applies to the last condition, `25 [0-5] \.` on line 57, instead of all conditions for an IP octet. This makes matching most IP address fail. The only IPs that will match would need to have 3-digit octets for the first three where the first two-digits are `25`. NOTE: This seems to have been resolved by the commit above. 2. The conditions for the last octet are no longer grouped (line 58) making the OR `|` operator act on what was a higher level group. Because of this, the fourth octet would have to be only one or two digits. For example, only IPs like the following will pass validation: http://**25**3.**25**4.**25**5.1 (mostly resolved by the commit above) http://**25**3.**25**4.**25**5.12 (mostly resolved by the commit above) After the commit above, any IP with 3 digits in the last octet will * not * pass validation: http<nolink>://1.2.3.* 100 * http<nolink>://1.2.3.* 255 *
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user sdutry commented on the issue:

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

        Ok,

        I will wait.

        Show
        githubbot ASF GitHub Bot added a comment - Github user sdutry commented on the issue: https://github.com/apache/struts/pull/157 Ok, I will wait.
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user lukaszlenart commented on the issue:

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

        Could you wait a bit with merging this PR? I have asked somebody to take a look on this.

        Show
        githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/157 Could you wait a bit with merging this PR? I have asked somebody to take a look on this.
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user sdutry opened a pull request:

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

        WW-4834 fixed faulty regex

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

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

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

        https://github.com/apache/struts/pull/157.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 #157


        commit 8a04e80f01350c90f053d71366d5e0c2186fded5
        Author: Stefaan Dutry <stefaan.dutry@gmail.com>
        Date: 2017-08-02T07:31:11Z

        WW-4834 fixed faulty regex


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user sdutry opened a pull request: https://github.com/apache/struts/pull/157 WW-4834 fixed faulty regex You can merge this pull request into a Git repository by running: $ git pull https://github.com/sdutry/struts WW-4834 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/157.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 #157 commit 8a04e80f01350c90f053d71366d5e0c2186fded5 Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-08-02T07:31:11Z WW-4834 fixed faulty regex
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Jenkins build Struts-master-JDK7 #13 (See https://builds.apache.org/job/Struts-master-JDK7/13/)
        WW-4834 Improve RegEx used to validate URLs (stefaan.dutry: rev 9d47af6ffa355977b5acc713e6d1f25fac260a28)

        • (edit) core/src/main/java/com/opensymphony/xwork2/validator/validators/URLValidator.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-master-JDK7 #13 (See https://builds.apache.org/job/Struts-master-JDK7/13/ ) WW-4834 Improve RegEx used to validate URLs (stefaan.dutry: rev 9d47af6ffa355977b5acc713e6d1f25fac260a28) (edit) core/src/main/java/com/opensymphony/xwork2/validator/validators/URLValidator.java
        Hide
        sdutry Stefaan Dutry added a comment -

        problem located, fix incoming

        Show
        sdutry Stefaan Dutry added a comment - problem located, fix incoming
        Hide
        sdutry Stefaan Dutry added a comment -

        You're right.
        Sorry about this.
        Let me quickly try and fix it.

        Show
        sdutry Stefaan Dutry added a comment - You're right. Sorry about this. Let me quickly try and fix it.
        Hide
        lukaszlenart Lukasz Lenart added a comment - - edited

        This won't pass

        assertTrue(pattern.matcher("http://127.0.0.1").matches());
        
        Show
        lukaszlenart Lukasz Lenart added a comment - - edited This won't pass assertTrue(pattern.matcher( "http: //127.0.0.1" ).matches());
        Hide
        sdutry Stefaan Dutry added a comment -

        RegEx:

        ^(?:https?|ftp)://(?:(?:[a-z0-9$_.+!*'(),;?&=\-]|%[0-9a-f]{2})+(?::(?:[a-z0-9$_.+!*'(),;?&=\-]|%[0-9a-f]{2})+)?@)?#?(?:(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)*[a-z][a-z0-9-]*[a-z0-9]|(?:[1-9]?\d|1\d{2}|2[0-4]\d|25[0-5]\.){3}[1-9]?\d|1\d{2}|2[0-4]\d|25[0-5])(?::\d+)?)(?:(?:/(?:[a-z0-9$_.+!*'(),;:@&=\-]|%[0-9a-f]{2})*)*(?:\?(?:[a-z0-9$_.+!*'(),;:@&=\-/:]|%[0-9a-f]{2})*)?)?(?:#(?:[a-z0-9$_.+!*'(),;:@&=\-]|%[0-9a-f]{2})*)?$
        

        as java string

        "^(?:https?|ftp)://(?:(?:[a-z0-9$_.+!*'(),;?&=\\-]|%[0-9a-f]{2})+(?::(?:[a-z0-9$_.+!*'(),;?&=\\-]|%[0-9a-f]{2})+)?@)?#?(?:(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)*[a-z][a-z0-9-]*[a-z0-9]|(?:[1-9]?\\d|1\\d{2}|2[0-4]\\d|25[0-5]\\.){3}[1-9]?\\d|1\\d{2}|2[0-4]\\d|25[0-5])(?::\\d+)?)(?:(?:/(?:[a-z0-9$_.+!*'(),;:@&=\\-]|%[0-9a-f]{2})*)*(?:\\?(?:[a-z0-9$_.+!*'(),;:@&=\\-/:]|%[0-9a-f]{2})*)?)?(?:#(?:[a-z0-9$_.+!*'(),;:@&=\\-]|%[0-9a-f]{2})*)?$"
        
        Show
        sdutry Stefaan Dutry added a comment - RegEx: ^(?:https?|ftp)://(?:(?:[a-z0-9$_.+!*'(),;?&=\-]|%[0-9a-f]{2})+(?::(?:[a-z0-9$_.+!*'(),;?&=\-]|%[0-9a-f]{2})+)?@)?#?(?:(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\.)*[a-z][a-z0-9-]*[a-z0-9]|(?:[1-9]?\d|1\d{2}|2[0-4]\d|25[0-5]\.){3}[1-9]?\d|1\d{2}|2[0-4]\d|25[0-5])(?::\d+)?)(?:(?:/(?:[a-z0-9$_.+!*'(),;:@&=\-]|%[0-9a-f]{2})*)*(?:\?(?:[a-z0-9$_.+!*'(),;:@&=\-/:]|%[0-9a-f]{2})*)?)?(?:#(?:[a-z0-9$_.+!*'(),;:@&=\-]|%[0-9a-f]{2})*)?$ as java string "^(?:https?|ftp)://(?:(?:[a-z0-9$_.+!*'(),;?&=\\-]|%[0-9a-f]{2})+(?::(?:[a-z0-9$_.+!*'(),;?&=\\-]|%[0-9a-f]{2})+)?@)?#?(?:(?:(?:[a-z0-9](?:[a-z0-9-]*[a-z0-9])?\\.)*[a-z][a-z0-9-]*[a-z0-9]|(?:[1-9]?\\d|1\\d{2}|2[0-4]\\d|25[0-5]\\.){3}[1-9]?\\d|1\\d{2}|2[0-4]\\d|25[0-5])(?::\\d+)?)(?:(?:/(?:[a-z0-9$_.+!*'(),;:@&=\\-]|%[0-9a-f]{2})*)*(?:\\?(?:[a-z0-9$_.+!*'(),;:@&=\\-/:]|%[0-9a-f]{2})*)?)?(?:#(?:[a-z0-9$_.+!*'(),;:@&=\\-]|%[0-9a-f]{2})*)?$"
        Hide
        sdutry Stefaan Dutry added a comment - - edited

        Lukasz Lenart
        what's wrong with it?

        Show
        sdutry Stefaan Dutry added a comment - - edited Lukasz Lenart what's wrong with it?
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user lukaszlenart commented on the issue:

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

        Ach... you did merge it :\ But this isn't a valid RegEx

        Show
        githubbot ASF GitHub Bot added a comment - Github user lukaszlenart commented on the issue: https://github.com/apache/struts/pull/156 Ach... you did merge it :\ But this isn't a valid RegEx
        Hide
        githubbot ASF GitHub Bot added a comment -

        Github user asfgit closed the pull request at:

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

        Show
        githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/struts/pull/156
        Hide
        jira-bot ASF subversion and git services added a comment -

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

        WW-4834 Improve RegEx used to validate URLs

        Show
        jira-bot ASF subversion and git services added a comment - Commit 9d47af6ffa355977b5acc713e6d1f25fac260a28 in struts's branch refs/heads/master from Stefaan Dutry [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9d47af6 ] WW-4834 Improve RegEx used to validate URLs
        Hide
        lukaszlenart Lukasz Lenart added a comment -

        Yes, but I can do it once the change got merged into master

        Show
        lukaszlenart Lukasz Lenart added a comment - Yes, but I can do it once the change got merged into master
        Hide
        sdutry Stefaan Dutry added a comment -

        Lukasz Lenart
        Seeing the fix versions for this issue, does this mean you want these changes applied in both branches:

        • master
        • support-2-3
        Show
        sdutry Stefaan Dutry added a comment - Lukasz Lenart Seeing the fix versions for this issue, does this mean you want these changes applied in both branches: master support-2-3
        Hide
        sdutry Stefaan Dutry added a comment -

        Can anyone tell me if the optional # on line 54 is needed?

        Show
        sdutry Stefaan Dutry added a comment - Can anyone tell me if the optional # on line 54 is needed?
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user sdutry opened a pull request:

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

        WW-4834 Improve RegEx used to validate URLs

        changed the regex according to https://github.com/apache/struts/commit/8df5a897f61f3ef45c36fdd9275e66669ae4516c#commitcomment-23414493

        Can anyone tell me if the optional `#` on line 54 is needed?

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

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

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

        https://github.com/apache/struts/pull/156.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 #156


        commit 9d47af6ffa355977b5acc713e6d1f25fac260a28
        Author: Stefaan Dutry <stefaan.dutry@gmail.com>
        Date: 2017-08-01T15:33:53Z

        WW-4834 Improve RegEx used to validate URLs


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user sdutry opened a pull request: https://github.com/apache/struts/pull/156 WW-4834 Improve RegEx used to validate URLs changed the regex according to https://github.com/apache/struts/commit/8df5a897f61f3ef45c36fdd9275e66669ae4516c#commitcomment-23414493 Can anyone tell me if the optional `#` on line 54 is needed? You can merge this pull request into a Git repository by running: $ git pull https://github.com/sdutry/struts WW-4834 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/struts/pull/156.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 #156 commit 9d47af6ffa355977b5acc713e6d1f25fac260a28 Author: Stefaan Dutry <stefaan.dutry@gmail.com> Date: 2017-08-01T15:33:53Z WW-4834 Improve RegEx used to validate URLs

          People

          • Assignee:
            Unassigned
            Reporter:
            lukaszlenart Lukasz Lenart
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development