5

I've just started using the brakeman gem to explore my rails app for security vulnerabilities.

I've managed to get everything tidy except for several cross site scripting warnings.

These all share the following in common:

  • They're all link_to tags
  • They all have instance variables in the class, alt or title attributes
  • The instance variables all represent an active record query that includes associated models
  • The instance variables are all "commentable". This describes a polymorphic association for user generated comments, similar in approach to the revised version of this Railscast.

e.g

<%= link_to "Click" , :class=> @model.association.attribute, :alt=> @model.association.attribute, :title=> @model.association.attribute, @model.association %>

where 

@model = @commentable = Model.includes(:association1, association2: {:nested-association1, :nested-association2}).find(params[:id])

Is this something I need to be concerned about/ take action for? I thought Rails 3.2 escapes these by default.

I'd welcome advice to help me understand this issue better, and identify what steps I should take, if any.

Andy Harvey
  • 12,333
  • 17
  • 93
  • 185

1 Answers1

4

I was unable to reproduce any warnings from the code you provided. What version of Brakeman are you using? What was the actual warning (redacted as necessary)?

I suspect you are getting warnings because user input is being detected in the href value of the link. See this pull request for more information about why this can be dangerous.

Unfortunately, without more information, I cannot tell if this is a false positive that needs to be fixed or a legitimate warning.

Edit:

Okay, now I am seeing the warning when testing with @model = @commentable = ... This is a problem with how Brakeman is handling the assignment.

If you are linking to an instance of a model, there should be no warning. If you are linking to a model attribute then this is counted as user input.

Yes, Rails will escape HTML, but it does not deal with links beginning with javascript: or data: which can be used for XSS.

Justin
  • 1,561
  • 10
  • 12
  • thanks for your response Justin. Having looked again at the error I've also noticed that they are all complex `includes` statements (i.e. several models, some of which are nested). Also (and perhaps this is the cause) they are all "commentable". I'll update my question with more detail. – Andy Harvey Jul 04 '12 at 08:33
  • perhaps refactoring my `@model = @commentable = ARquery` to two lines might resolve this? But I'm still not sure I understand why this is an issue. Won't Rails escape this by default? Appreciate any advice to help me improve my understanding. – Andy Harvey Jul 04 '12 at 08:46
  • Updated answer in light of this new info. It looks like splitting over two lines does resolve the issue, which is a bug in Brakeman. – Justin Jul 05 '12 at 18:12
  • thanks for looking into this Justin. Just to clarify, does this mean I can safely ignore these warnings, or does it indicate action needed to secure `@commentable`? – Andy Harvey Jul 06 '12 at 06:24
  • Yes, if splitting the assignment into two makes the warnings disappear, then you can ignore them. `link_to` with a model is safe. – Justin Jul 06 '12 at 15:11
  • I have the same problem (not sure if it's different enough to make a new post) but this solution isn't working. Should I make another post? – Mary Jun 15 '15 at 21:02