Rails Brakeman warning: Dynamic Render Path false alarm?

Question

I'm just getting started with Rails, so I'm using Brakeman to learn about potential vulnerabilities in my newbie code. It's throwing a high-confidence "Dynamic Render Path" warning about the following code in my show.js.erb file:

$('#media-fragment').html('<%= escape_javascript(render(params[:partial])) %>');

I actually expected this was a problem, so no surprise there. So I changed it to the following:

  # controller:
  def show
    if legal_partial?
      @allowed_partial = params[:partial]
    else
      raise StandardError, "unexpected partial request: #{params[:partial]}"
    end
  end

  private

  def legal_partial?
    %w(screenshots video updates).include? params[:partial]
  end

  # ...
  # show.js.erb
  $('#media-fragment').html('<%= escape_javascript(render(@allowed_partial)) %>');

Although I believe the code is now safe, Brakeman is still unhappy with this. Is there a more idiomatic way to control rendering of a partial based on user input?

Answer 1

Update (2/5/2016):

This has been fixed as of Brakeman 3.0.3.

If the legal_partial? method is inlined like this:

def show
  if %w(screenshots video updates).include? params[:partial]
    @allowed_partial = params[:partial]
  else
    raise StandardError, "unexpected partial request: #{params[:partial]}"
  end
end

Brakeman will be able to detect the guard condition and will no longer warn about the later render call.

---

Original answer:

Unfortunately, Brakeman does not know that if legal_partial? is a proper guard. All it knows is that params[:partial] is assigned to @allowed_partial, and that is then passed to render.

You may be able to tell that @allowed_partial will always be a safe value. At that point, you have to consider whether or not it makes sense to add complexity in order to make a tool happy.

Just as an example, you could do this:

def show
  render_allowed_partial params[:partial]
end

def render_allowed_partial name
  if %w(screenshots video updates).include? name
    @allowed_partial = name
  else
    raise StandardError, "unexpected partial request: #{params[:partial]}"
  end
end

It's basically the same thing, except now you are hiding the assignment of @allowed_partial from Brakeman.

(Warning: Not necessarily "best" way of doing this.)