15

I'm using Brakeman to identify security issues. It's flagging up any links which use params.merge as a cross site scripting vulnerability. How can I sanitize something like the following? 

  - @archives.each do |archive|
    =  link_to "FTP", params.merge(:action => :ftp, :archive => archive, :recipient => "company")
snowangel
  • 3,452
  • 3
  • 29
  • 72

1 Answers1

17

You should create a new hash based on only the elements of params which you expect and wish to allow to be a part of the FTP link and use that to merge your additional parameters.

What you have allows me to add whatever I want to that FTP link by modifying the querystring, opening up the door to security vulnerabilities. By building a hash for use in place of the params in the params.merge(... you're effectively whitelisting expected querystring components for use in the template you're rendering.

As a GET example, if you expect a URL like

/some/path?opt1=val1&opt2=val2

your controller action you might do

@cleaned_params = { opt1: params[:opt1], opt2: params[:opt2] }
@cleaned_params.merge! action: :ftp, archive: archive, recipient: :company

And then pass @cleaned_params to the link_to

=  link_to "FTP", @cleaned_params

This way if I manually enter a URL like

/some/path?opt1=val1&opt2=val2&maliciousopt=somexss

The params[:maliciousopt] will never make it into your FTP link_to in your view.

The same behaviour applies to POST requests, only to be malicious I might add a couple fields to the form before submitting it

<input type="hidden" name="maliciousopt" value="somexss" />
deefour
  • 34,974
  • 7
  • 97
  • 90
  • Or, in fact, in my case here I can simply remove the params.merge and the required params get passed in without any interference from a user. – snowangel Sep 05 '12 at 11:20
  • 8
    Wouldn't an attacker just have to replace some of the acceptable params with their malicious script and get it stuck in the page either way? – Vardarac Jan 21 '16 at 22:06