Grails
  1. Grails
  2. GRAILS-3879

UrlMappingsFilter bug with action ending in '.'

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.0.4
    • Fix Version/s: 1.0.5, 1.1-beta3
    • Component/s: Controllers
    • Labels:
      None
    • Patch Submitted:
      Yes

      Description

      If someone requests a URL that ends in '.' but is otherwise a valid controller action (e.g. "/admin/user/list.") the 'list' action is displayed when it should have been a 404. This is caused by the call to WebUtils.getFormatFromURI(uri) in UrlMappingsFilter (line 88 in 1.0.4, line 101 in the 1.1 branch).

      It checks for a null format and since a blank string is returned in this case, it's tricked into thinking it's a file-extension request and trims off the dot. If you change "if(format!=null)" to "if(StringUtils.hasLength(format))" then it works as it should.

      This is slightly more urgent than it appears since this creates a back door if using the Acegi plugin; "/admin/user/list." doesn't match the typical security mapping of "/admin/user/list/*=ROLE_WHATEVER". The workaround is to add an additional mapping for "/admin/user/list=ROLE_WHATEVER" but it's not very DRY.

        Activity

        Hide
        Burt Beckwith added a comment -

        Jira ate my asterisks - the two mappings should be

        /admin/user/list/@@=ROLE_WHATEVER

        and

        /admin/user/list@=ROLE_WHATEVER

        replace @ with an asterisk.

        Show
        Burt Beckwith added a comment - Jira ate my asterisks - the two mappings should be /admin/user/list/@@=ROLE_WHATEVER and /admin/user/list@=ROLE_WHATEVER replace @ with an asterisk.
        Hide
        Peter Ledbrook added a comment -

        Up-rating this issue - not good to have easy ways of bypassing security checks!

        Show
        Peter Ledbrook added a comment - Up-rating this issue - not good to have easy ways of bypassing security checks!
        Hide
        Peter Ledbrook added a comment -

        Hi Burt,

        Not quite sure why "/admin/user/list.xml" isn't also a problem. What's the difference?

        Thanks,

        Peter

        Show
        Peter Ledbrook added a comment - Hi Burt, Not quite sure why "/admin/user/list.xml" isn't also a problem. What's the difference? Thanks, Peter
        Hide
        Burt Beckwith added a comment -

        I think this is less of an issue than I thought - it's more of a documentation problem. You wouldn't guard UserController.list() with /user/list/**, you'd naively guard it with /user/list and properly with /user/list*. I was thinking about a bigger problem that I fixed in the 0.5.1 release where URLs containing a dot weren't checked at all, so mapping /user/** wouldn't block access to "/user/list." and this bug would result in "/user/list" being rendered.

        It behaves differently in 1.1. It only strips off the extension if it's a supported mime type, so "list.xml" goes through to the "list" action and shows the page, while "list." generates the same 404 that you'd get in 1.0.4 with the proposed fix since getArtefactForFeature() returns null.

        Show
        Burt Beckwith added a comment - I think this is less of an issue than I thought - it's more of a documentation problem. You wouldn't guard UserController.list() with /user/list/**, you'd naively guard it with /user/list and properly with /user/list*. I was thinking about a bigger problem that I fixed in the 0.5.1 release where URLs containing a dot weren't checked at all, so mapping /user/** wouldn't block access to "/user/list." and this bug would result in "/user/list" being rendered. It behaves differently in 1.1. It only strips off the extension if it's a supported mime type, so "list.xml" goes through to the "list" action and shows the page, while "list." generates the same 404 that you'd get in 1.0.4 with the proposed fix since getArtefactForFeature() returns null.
        Hide
        Peter Ledbrook added a comment -

        OK, my plan is to modify WebUtils.getFormatFromURI(uri) so that it returns null if there is no extension after the ".". If that sounds fine to you, I'll commit and resolve the issue.

        Show
        Peter Ledbrook added a comment - OK, my plan is to modify WebUtils.getFormatFromURI(uri) so that it returns null if there is no extension after the ".". If that sounds fine to you, I'll commit and resolve the issue.
        Hide
        Burt Beckwith added a comment -

        Makes sense to me - thanks.

        Show
        Burt Beckwith added a comment - Makes sense to me - thanks.
        Hide
        Graeme Rocher added a comment -

        Bulk closing bunch of resolved issues

        Show
        Graeme Rocher added a comment - Bulk closing bunch of resolved issues

          People

          • Assignee:
            Peter Ledbrook
            Reporter:
            Burt Beckwith
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development