False positive on Unsafe parameter value in link_to href #897

Closed
bugzpodder opened this Issue Jun 28, 2016 · 1 comment

Projects

None yet

2 participants

@bugzpodder
bugzpodder commented Jun 28, 2016 edited

First refer to rails source:
link_to essentially calls url_for: https://github.com/rails/rails/blob/c24659bf9963578f82173b00ee9dca087692ed25/actionview/lib/action_view/helpers/url_helper.rb#L181

I have this test case, where I marked xss on issues that would potentially cause xss:

<%= link_to("good", params.merge(:page => 2)) %>
<%= link_to("good", url_for(params.merge(:page => 2))) %>
<%= link_to("xss", url_for(params[:bad])) %>
<%= link_to("xss", params[:bad]) %>

The result is one false-positive, one false-negative:

| High       | test                                      | Cross Site Scripting | Unsafe parameter value in link_to href near line 1: link_to("good", +params.merge(:page => 2)+)                                                                             >>
| High       | test                                      | Cross Site Scripting | Unsafe parameter value in link_to href near line 4: link_to("xss", +params[:xss]+)                                                                                          >>

I think line 3's usecase is very rare so its fine not to catch it. However line 1 is an extremely common pattern when attaching some kind of pagination or additional params.

I did some research on this and did not find an issue that is identical although there has been several link_to issues that are similar.

@presidentbeef
Owner

Hi Jack,

I agree. I think I can fix both of those cases. Thanks for reporting!

@presidentbeef presidentbeef locked and limited conversation to collaborators Nov 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.