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?

Justin · Answer 1 · 2016-02-05T18:07:06.400

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.)

I can't seem to get the guard condition to work for model attributes. I've got a method on my model: `def sanitized_partial_path; if ["foo", "bar"].include?(attribute); "path/to/#{attribute}"; end;` and it still gives the false positive. — Nick, May 05 '16 at 13:50
@Nick Brakeman isn't going to look at the contents of `sanitized_partial_path` (although in the future Brakeman Pro will). The example in my answer works because of the assignment to an instance variable in the controller action which is used later in the view. — Justin, May 08 '16 at 20:13

score 1 · Answer 2 · answered Mar 14 '18 at 13:47

Using brakeman 4.2.0

I had a similar issue trying to render a specific hand-positioned-and-named template. Every product of my app required that specific named template. The template name came from the controller params as params[:a_particular_slug].underscore.

I solved with something like this:

  def show
    if @products = Product.where(a_slug: params[:a_particular_slug])
      render template: lookup_context.find(params[:a_particular_slug].underscore, ["featured_products"])
    else
      render_404
    end
  end

Here I'm looking for a template. If you need to use a partial, be aware that lookup_context.find third params set to true allows to search for partials.

You can find more about lookup_context.find here

Hope this helps.

Rails Brakeman warning: Dynamic Render Path false alarm?

2 Answers2

Linked