Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Slash usage in to_param results in escaped character and no route match with 4.1.2 upgrade #16058

Closed
tirdadc opened this Issue Jul 4, 2014 · 25 comments

Comments

Projects
None yet
9 participants

tirdadc commented Jul 4, 2014

I've been using this as a way to generate slugs without relying on a gem, based on existing examples:

# item.rb
def to_param
  "#{id}/" + "#{title}".parameterize
end

It was working fine up until I upgraded from 4.1.1 to 4.1.2, and where I once had this in links:
http://app.dev/items/811/some-title

I now get:
http://app.dev/items/811%2Fsome-title

which results in a 'no route matched' error.

Here is the route setup:

resources :items, format: false, id: /[0-9]+\/.+/

Is there anyway to prevent the slash from getting escaped in > 4.1.2?

Member

chancancode commented Jul 5, 2014

@chancancode chancancode added this to the 4.1.5 milestone Jul 5, 2014

Owner

matthewd commented Jul 5, 2014

Ugly URL notwithstanding, that sounds to me like it should work... the regexp applies to a single path component, so it should be matching against the post-unescape value.. right?

Then again, I don't see why it would've worked before, so maybe I'm just very wrong.

Owner

pixeltrix commented Jul 5, 2014

This was changed in 5460591 to make the behaviour more consistent - if you want to use '/' in a path parameter it needs to be a glob param (i.e. *id instead of :id). Unfortunately you can't change the param in a resources to a glob since it would mess with nested resources so you'll need to explicitly set the route, e.g:

scope format: false do
  get '/items', to: 'items#index'
  get '/items/*id', to: 'items#show'
end

@matthewd the route is matched against PATH_INFO which is unescaped.

Member

chancancode commented Jul 5, 2014

(4-1-stable backport: 8a06764)

tirdadc commented Jul 6, 2014

@pixeltrix Thanks, that worked. Unfortunately not being able to use resources in this case makes this a bit too verbose as I have to explicitly state all the routes that involve the id to insure the existing paths are working, so now I have this:

resources :items, id: /[0-9]+\/.+/, only: [:index, :new, :create], format: false
scope format: false do
  get '/items/*id/edit', to: "items#edit"
  patch '/items/*id', to: "items#update"
  put '/items/*id', to: "items#update"
  delete '/items/*id', to: "items#destroy"
  get '/items/*id', to: "items#show"
end

There's realistically no reason for me to have that to_param variant of the id anywhere but for the #show method, but I'm not sure how that would be feasible.

Owner

pixeltrix commented Jul 6, 2014

@tirdadc just a thought what happens when an item has title of edit - that will give you a items#show url of /items/1/edit. This will be confusing and not something I'd recommend. It'll also prevent you from using member actions on resources, e.g. /items/1/archive => items#archive.

Owner

pixeltrix commented Jul 6, 2014

Closing this because it's the expected behaviour.

@pixeltrix pixeltrix closed this Jul 6, 2014

Contributor

joevandyk commented Jul 9, 2014

Is it possible to have to_param return a string that has a slash in it without the slash getting escaped? Our URLs changed as a result of upgrading to 4.1.2.

We do something like

class Product
  def to_param
    "#{uuid}/#{name}"
  end
end

to make URLs that look like /deals/8d8a8s/a-product-name

But in 4.1.2, the URLs look like /deals/8d8a8s%2Fa-product-name

https://www.tanga.com/deals/b502f4ce9f5b%2Fincipio-wireless-charge-pad-for-qi-enabled-devices is an example of what it looks like on our site now. The URL used to be https://www.tanga.com/deals/b502f4ce9f5b/incipio-wireless-charge-pad-for-qi-enabled-devices

Owner

matthewd commented Jul 9, 2014

@joevandyk I think my first suggestion would be that you define a custom product_path helper instead; if you're producing multiple path components, it seems more like you're using to_param to build a custom URL, rather than supplying a replacement for :id.

@pixeltrix maybe we need to skip escaping on slashes for now, and instead produce a deprecation warning... at least for 4.1? 😐

Contributor

joevandyk commented Jul 9, 2014

Hm, what's the best way to do that?

I have many places where I do include Rails.application.routes.url_helpers, would I also have to include the module that contains this helper method in all those places?

Owner

pixeltrix commented Jul 10, 2014

@matthewd I don't think we could add a deprecation warning without it affecting runtime performance because it would have to check every time a url is generated. However what I might be able to do is check at route definition time what the constraint is and if it allows '/' then don't escape it.

However I still think people are 'Doing it Wrong' to use resources in this manner since it will have unintended side effects on member actions. If someone want's to use a '/' in a param then they should use a glob - I'm pretty sure that used to be the case with Rails 2.3.

@pbc pbc referenced this issue in gitlabhq/gitlabhq Jul 11, 2014

Closed

Encoded slashes in links result in 404 error #7265

stevegrossi added a commit to stevegrossi/stevegrossi that referenced this issue Jul 17, 2014

Contributor

joevandyk commented Jul 19, 2014

@pixeltrix I'm using a glob in the route, the problem is that I can't use to_param to make a value with a slash in it anymore.

Contributor

joevandyk commented Jul 19, 2014

@pixeltrix I'm not using resources at all.

My route looks like:
get '/deals/:id' => 'products#show', :as => :product, :constraints => {:id=>/.*/}

And when I upgraded to Rails 4.1.2, these URLs started to be generated differently. They still work, but the slashes are encoded differently.

Owner

pixeltrix commented Jul 20, 2014

@joevandyk you need to use *id in the path, e.g:

get '/deals/*id' => 'products#show', :as => :product

You probably don't need the constraint once you use *id

Contributor

joevandyk commented Jul 21, 2014

@pixeltrix I also have routes like '/deals/:id/edit', so I use the constraint to match those types of routes as well.

But in any event, removing the constraint doesn't solve the problem of not being able to use / in URL parameters.

Owner

pixeltrix commented Jul 21, 2014

@joevandyk when I say a glob param I'm talking about *id and not the glob in the constraint - using :id will always escape the / by design. If you use *id param, you can still use a constraint if that's what you need and then the router will not escape the / as verified by the tests here and here.

I've been stung by this change as well.

thomas-mcdonald added a commit to thomas-mcdonald/qa that referenced this issue Jul 24, 2014

Fix for breaking change in Rails point release.
rails/rails#16058

At least we haven't really deployed QA anywhere so changing URLs at whim is
currently painless.
Contributor

Loremaster commented Jul 30, 2014

@joevandyk, how did you solve you problem? I have same problem, only in my case I has such routes:

Rails.application.routes.draw do
  scope "(:locale)", locale: /#{I18n.available_locales.join("|")}/ do
      resources :machines, except: :destroy do
          collection do
            get  :search
            get  'search/:ad_type(/:machine_type(/:machine_subtype(/:brand)))', action: 'search', as: :pretty_search

            get  ':subcategory/:brand(/:model)/:id', action: 'show', as: :pretty
            patch ':subcategory/:brand(/:model)/:id', action: 'update'                                  # To be able to update machines with new rich paths.
          end
      end
  end
end

I tried for show route such code:

resources :machines, except: :destroy do
 #...
end

scope format: false do
 get '/machines/*id', to: "machines#show"
end

And that:

resources :machines, except: :destroy do
 #...
end

scope format: false do
 get '/machines/*slug/:id', to: "machines#show"
end

Without any luck. I still have encoded slashes. I can't make upgrade because of that problem. Can anyone may recommend what I should do?

Owner

pixeltrix commented Jul 30, 2014

@Loremaster if the examples you've given are correct then it may be the routes with * in aren't named - check the output of rake routes.

Contributor

Loremaster commented Aug 1, 2014

@pixeltrix Yes, you are right. My route:

resources :machines, except: :destroy do
 # ...
 get  ':subcategory/:brand(/:model)/:id', action: 'show', as: :pretty
end

scope format: false do
 get '/machines/*id', to: "machines#show"
end

Is not named, as rake routes show:

pretty_machines GET    (/:locale)/machines/:subcategory/:brand(/:model)/:id(.:format)                            machines#show {:locale=>/ru|en|de|es|it|tr|fr/}
machine GET    (/:locale)/machines/:id(.:format)                                                         machines#show {:locale=>/ru|en|de|es|it|tr|fr/}
GET    (/:locale)/machines/*id                                                                   machines#show {:locale=>/ru|en|de|es|it|tr|fr/}

But I can't change name to machine:

 scope format: false do
    get '/machines/*id', to: "machines#show", as: :machine
 end

because it generates the error:

Invalid route name, already in use: 'machine' 
You may have defined two routes with the same name using the `:as` option, or you may be overriding a route already defined by a resource with the same naming. For the latter, you can restrict the routes created with `resources` as explained here: 
http://guides.rubyonrails.org/routing.html#restricting-the-routes-created

And if I try another name:

 scope format: false do
    get '/machines/*id', to: "machines#show", as: :my_machine
 end

Then slashes are still escaped. Any ideas what to do next?

Owner

pixeltrix commented Aug 2, 2014

And if I try another name … Then slashes are still escaped. Any ideas what to do next?

@Loremaster are you using the url helper my_machine_path to generate the route or url_for with a hash? The latter will match the first defined route so the resources :machines defined route will be generated before your custom route.

Contributor

Loremaster commented Aug 4, 2014

No, I don't. I still use link_to with old machine_path. It looks like this:

view

link_to "machine", machine_path(@machine)

I build link in to_param method using link_to helper too, which looks like this

machine.rb

  def to_param
    MachinePrettyPath.show_path(self, cut_model_text: true)
  end

Here is the actual method, using to build custom path with slashes:

machine_pretty_path.rb

    # Return path to the maсhine#show path, using new text (for seo).
    # Example:
    #   show_path(first_machine)
    #   => '/machines/my subcategory/my brand/1'
    #
    # Options:
    #  * cut_model_text - remove text of the model's name in the beginning of new path.
    #    Example:
    #      show_path(first_machine, cut_model_text: true)
    #      => '/my subcategory/my brand/1'
    def show_path(machine, cut_model_text: false)
      brand_name = machine.brand_name || 'brand'
      inner_options = {subcategory: machine.machine_type.name, brand: brand_name, id: machine.id}

      inner_options.merge!(model: machine.model_name) if machine.model_name.present?

      inner_options = parametrize_keys(inner_options)
      path = Rails.application.routes.url_helpers.pretty_machines_path(inner_options)

      if cut_model_text == true
        path = path.sub("/machines/", "")
      end
      path
    end

routes.rb

Rails.application.routes.draw do
  scope "(:locale)", locale: /#{I18n.available_locales.join("|")}/ do
      resources :machines, except: :destroy do
          collection do
            get  :search
            get  'search/:ad_type(/:machine_type(/:machine_subtype(/:brand)))', action: 'search', as: :pretty_search

            get  ':subcategory/:brand(/:model)/:id', action: 'show', as: :pretty
            patch ':subcategory/:brand(/:model)/:id', action: 'update'                                  # To be able to update machines with new rich paths.
          end
      end

     scope format: false do
       get '/machines/*id', to: "machines#show", as: :my_machine
    end
  end
end

rake routes:

                       search_machines GET    (/:locale)/machines/search(.:format)                                                      machines#search {:locale=>/ru|en|de|es|it|tr|fr/}
                       pretty_search_machines GET    (/:locale)/machines/search/:ad_type(/:machine_type(/:machine_subtype(/:brand)))(.:format) machines#search {:locale=>/ru|en|de|es|it|tr|fr/}
                              pretty_machines GET    (/:locale)/machines/:subcategory/:brand(/:model)/:id(.:format)                            machines#show {:locale=>/ru|en|de|es|it|tr|fr/}
                                              PATCH  (/:locale)/machines/:subcategory/:brand(/:model)/:id(.:format)                            machines#update {:locale=>/ru|en|de|es|it|tr|fr/}
                                     machines GET    (/:locale)/machines(.:format)                                                             machines#index {:locale=>/ru|en|de|es|it|tr|fr/}
                                              POST   (/:locale)/machines(.:format)                                                             machines#create {:locale=>/ru|en|de|es|it|tr|fr/}
                                  new_machine GET    (/:locale)/machines/new(.:format)                                                         machines#new {:locale=>/ru|en|de|es|it|tr|fr/}
                                 edit_machine GET    (/:locale)/machines/:id/edit(.:format)                                                    machines#edit {:locale=>/ru|en|de|es|it|tr|fr/}
                                      machine GET    (/:locale)/machines/:id(.:format)                                                         machines#show {:locale=>/ru|en|de|es|it|tr|fr/}
                                              PATCH  (/:locale)/machines/:id(.:format)                                                         machines#update {:locale=>/ru|en|de|es|it|tr|fr/}
                                              PUT    (/:locale)/machines/:id(.:format)                                                         machines#update {:locale=>/ru|en|de|es|it|tr|fr/}
                                   my_machine GET    (/:locale)/machines/*id                                                                   machines#show {:locale=>/ru|en|de|es|it|tr|fr/}
Owner

pixeltrix commented Aug 4, 2014

machine GET (/:locale)/machines/:id(.:format)

When you use machine_path you're still using the route with : not * - use my_machine_path instead.

stevegrossi added a commit to stevegrossi/stevegrossi that referenced this issue Aug 8, 2014

stevegrossi added a commit to stevegrossi/stevegrossi that referenced this issue Aug 9, 2014

koraktor added a commit to Homebrew/formulae.brew.sh that referenced this issue Aug 13, 2014

Monkey patch escaped repository URLs
Rails 4.1.2+ won't allow slashes in resourceful routes.

See rails/rails#16058

koraktor added a commit to Homebrew/formulae.brew.sh that referenced this issue Aug 17, 2014

Monkey patch escaped repository URLs
Rails 4.1.2+ won't allow slashes in resourceful routes.

See rails/rails#16058

koraktor added a commit to Homebrew/formulae.brew.sh that referenced this issue Aug 18, 2014

Monkey patch escaped repository URLs
Rails 4.1.2+ won't allow slashes in resourceful routes.

See rails/rails#16058

koraktor added a commit to Homebrew/formulae.brew.sh that referenced this issue Aug 19, 2014

Monkey patch escaped repository URLs
Rails 4.1.2+ won't allow slashes in resourceful routes.

See rails/rails#16058

leoasis commented Aug 21, 2014

I believe a valid use case for slashes to identify a resource is when you want to use a date, for example to identify blog posts:

/blog/posts/2014/08/21

This is particularly helpful to use with slashes since you can have routes with incomplete dates (ie with only the year and/or month parts) that would search for posts in that year or year-month, while when adding the day you can have the particular post:

/blog/posts/2014
/blog/posts/2014/08

In cases when you can have more than one post, you can add a small description to identify posts in the same day:

/blog/posts/2014/08/21-post1
/blog/posts/2014/08/21-post2

It would still be really helpful to keep these routes with resources, and not fall back to the glob param.

How would be the best way to tackle this anyway with what we have now?

koraktor added a commit to Homebrew/formulae.brew.sh that referenced this issue Aug 22, 2014

Monkey patch escaped repository URLs
Rails 4.1.2+ won't allow slashes in resourceful routes.

See rails/rails#16058

@byroot byroot referenced this issue in Shopify/shipit-engine Aug 27, 2014

Merged

Update Rails to 4.1.5 #220

byroot added a commit to Shopify/shipit-engine that referenced this issue Aug 27, 2014

Update Rails to 4.1.5
All the routing mess is due to rails/rails#16058

potomak added a commit to potomak/repo-follow that referenced this issue Sep 6, 2014

stevegrossi added a commit to stevegrossi/stevegrossi that referenced this issue Nov 23, 2014

this is still broken :/

@pixeltrix pixeltrix locked and limited conversation to collaborators Aug 18, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.