Skip to content

Commit

Permalink
Use more specific check for :format in route path
Browse files Browse the repository at this point in the history
The current check for whether to add an optional format to the path
is very lax and will match things like `:format_id` where there are
nested resources, e.g:

    resources :formats do
      resources :items
    end

Fix this by using a more restrictive regex pattern that looks for
the patterns `(.:format)`, `.:format` or `/` at the end of the path.
Note that we need to allow for multiple closing parenthesis since
the route may be of this form:

    get "/books(/:action(.:format))", controller: "books"

This probably isn't what's intended since it means that the default
index action route doesn't support a format but we have a test for
it so we need to allow it.

Fixes #28517.
  • Loading branch information
pixeltrix committed Apr 18, 2017
1 parent 5f84634 commit ea11acd
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 1 deletion.
27 changes: 27 additions & 0 deletions actionpack/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,30 @@
* Use more specific check for :format in route path

The current check for whether to add an optional format to the path is very lax
and will match things like `:format_id` where there are nested resources, e.g:

``` ruby
resources :formats do
resources :items
end
```

Fix this by using a more restrictive regex pattern that looks for the patterns
`(.:format)`, `.:format` or `/` at the end of the path. Note that we need to
allow for multiple closing parenthesis since the route may be of this form:

``` ruby
get "/books(/:action(.:format))", controller: "books"
```

This probably isn't what's intended since it means that the default index action
route doesn't support a format but we have a test for it so we need to allow it.

Fixes #28517.

*Andrew White*


## Rails 4.2.8 (February 21, 2017) ##

* No changes.
Expand Down
3 changes: 2 additions & 1 deletion actionpack/lib/action_dispatch/routing/mapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ def constraint_args(constraint, request)

class Mapping #:nodoc:
ANCHOR_CHARACTERS_REGEX = %r{\A(\\A|\^)|(\\Z|\\z|\$)\Z}
OPTIONAL_FORMAT_REGEX = %r{(?:\(\.:format\)+|\.:format|/)\Z}

attr_reader :requirements, :conditions, :defaults
attr_reader :to, :default_controller, :default_action, :as, :anchor
Expand Down Expand Up @@ -144,7 +145,7 @@ def normalize_path!(path, format)
end

def optional_format?(path, format)
format != false && !path.include?(':format') && !path.end_with?('/')
format != false && path !~ OPTIONAL_FORMAT_REGEX
end

def normalize_options!(options, formatted, path_params, path_ast, modyoule)
Expand Down
18 changes: 18 additions & 0 deletions actionpack/test/dispatch/routing_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3490,6 +3490,24 @@ def test_head_fetch_with_mount_on_root
assert_equal 'HEAD', @response.body
end

def test_nested_routes_under_format_resource
draw do
resources :formats do
resources :items
end
end

get "/formats/1/items.json"
assert_equal 200, @response.status
assert_equal "items#index", @response.body
assert_equal "/formats/1/items.json", format_items_path(1, :json)

get "/formats/1/items/2.json"
assert_equal 200, @response.status
assert_equal "items#show", @response.body
assert_equal "/formats/1/items/2.json", format_item_path(1, 2, :json)
end

private

def draw(&block)
Expand Down

0 comments on commit ea11acd

Please sign in to comment.