Merge pull request #178 from presidentbeef/fix_yaml_load_false_positive

Fix YAML.load false positive

lib/brakeman/checks/check_file_access.rb

@@ -9,7 +9,9 @@ class Brakeman::CheckFileAccess < Brakeman::BaseCheck
 
   def run_check
     Brakeman.debug "Finding possible file access"
-    methods = tracker.find_call :targets => [:Dir, :File, :IO, :Kernel, :"Net::FTP", :"Net::HTTP", :PStore, :Pathname, :Shell, :YAML], :methods => [:[], :chdir, :chroot, :delete, :entries, :foreach, :glob, :install, :lchmod, :lchown, :link, :load, :load_file, :makedirs, :move, :new, :open, :read, :readlines, :rename, :rmdir, :safe_unlink, :symlink, :syscopy, :sysopen, :truncate, :unlink]
+    methods = tracker.find_call :targets => [:Dir, :File, :IO, :Kernel, :"Net::FTP", :"Net::HTTP", :PStore, :Pathname, :Shell], :methods => [:[], :chdir, :chroot, :delete, :entries, :foreach, :glob, :install, :lchmod, :lchown, :link, :load, :load_file, :makedirs, :move, :new, :open, :read, :readlines, :rename, :rmdir, :safe_unlink, :symlink, :syscopy, :sysopen, :truncate, :unlink]
+
+    methods.concat tracker.find_call :target => :YAML, :methods => [:load_file, :parse_file]
 
     Brakeman.debug "Finding calls to load()"
     methods.concat tracker.find_call :target => false, :method => :load

test/apps/rails3/app/controllers/home_controller.rb

@@ -101,6 +101,14 @@ class HomeController < ApplicationController
     @user = User.find(current_user)
   end
 
+  def test_yaml_file_access
+    #Should not warn
+    YAML.load "some/path/#{params[:user][:file]}"
+
+    #Should warn
+    YAML.parse_file("whatever/" + params[:file_name])
+  end
+
   private
 
   def filter_it

test/tests/test_rails3.rb

@@ -15,7 +15,7 @@ class Rails3Tests < Test::Unit::TestCase
       :controller => 1,
       :model => 5,
       :template => 30,
-      :warning => 30
+      :warning => 31
     }
   end
 
@@ -81,6 +81,24 @@ class Rails3Tests < Test::Unit::TestCase
       :file => /home_controller\.rb/
   end
 
+  def test_file_access_yaml_load
+    assert_no_warning :type => :warning,
+      :warning_type => "File Access",
+      :line => 106,
+      :message => /^Parameter\ value\ used\ in\ file\ name/,
+      :confidence => 0,
+      :file => /home_controller\.rb/
+  end
+
+  def test_file_access_yaml_parse_file
+    assert_warning :type => :warning,
+      :warning_type => "File Access",
+      :line => 109,
+      :message => /^Parameter\ value\ used\ in\ file\ name/,
+      :confidence => 0,
+      :file => /home_controller\.rb/
+  end
+
   def test_mass_assignment
     assert_warning :type => :warning,
       :warning_type => "Mass Assignment",