Merge branch 'master' into update_ruby_parser

Conflicts:
	lib/brakeman/processors/controller_processor.rb
	lib/brakeman/scanner.rb

lib/brakeman.rb

@@ -68,15 +68,6 @@ module Brakeman
     options = get_defaults.merge! options
     options[:output_formats] = get_output_formats options
 
-    app_path = options[:app_path]
-
-    abort("Please supply the path to a Rails application.") unless app_path and File.exist? app_path + "/app"
-
-    if File.exist? app_path + "/script/rails"
-      options[:rails3] = true
-      notify "[Notice] Detected Rails 3 application" unless options[:quiet]
-    end
-
     options
   end
 

lib/brakeman/app_tree.rb

+module Brakeman
+  class AppTree
+    VIEW_EXTENSIONS = %w[html.erb html.haml rhtml js.erb].join(",")
+
+    attr_reader :root
+
+    def self.from_options(options)
+      root = options[:app_path]
+
+      # Convert files into Regexp for matching
+      if options[:skip_files]
+        list = "(?:" << options[:skip_files].map { |f| Regexp.escape f }.join("|") << ")$"
+        new(root, Regexp.new(list))
+      else
+        new(root)
+      end
+    end
+
+    def initialize(root, skip_files = nil)
+      @root = root
+      @skip_files = skip_files
+    end
+
+    def expand_path(path)
+      File.expand_path(path, @root)
+    end
+
+    def read(path)
+      File.read(File.join(@root, path))
+    end
+
+    # This variation requires full paths instead of paths based
+    # off the project root. I'd prefer to get all the code outside
+    # of AppTree using project-root based paths (e.g. app/models/user.rb)
+    # instead of full paths, but I suspect it's an incompatible change.
+    def read_path(path)
+      File.read(path)
+    end
+
+    def exists?(path)
+      File.exists?(File.join(@root, path))
+    end
+
+    # This is a pair for #read_path. Again, would like to kill these
+    def path_exists?(path)
+      File.exists?(path)
+    end
+
+    def initializer_paths
+      @initializer_paths ||= find_paths("config/initializers")
+    end
+
+    def controller_paths
+      @controller_paths ||= find_paths("app/controllers")
+    end
+
+    def model_paths
+      @model_paths ||= find_paths("app/models")
+    end
+
+    def template_paths
+      @template_paths ||= find_paths("app/views", "*.{#{VIEW_EXTENSIONS}}")
+    end
+
+    def layout_exists?(name)
+      pattern = "#{@root}/app/views/layouts/#{name}.html.{erb,haml}"
+      !Dir.glob(pattern).empty?
+    end
+
+    def lib_paths
+      @lib_files ||= find_paths("lib")
+    end
+
+  private
+
+    def find_paths(directory, extensions = "*.rb")
+      pattern = @root + "/#{directory}/**/#{extensions}"
+
+      Dir.glob(pattern).sort.tap do |paths|
+        reject_skipped_files(paths)
+      end
+    end
+
+    def reject_skipped_files(paths)
+      return unless @skip_files
+      paths.reject! { |f| @skip_files.match f }
+    end
+
+  end
+end

lib/brakeman/checks.rb

@@ -75,16 +75,16 @@ class Brakeman::Checks
 
   #Run all the checks on the given Tracker.
   #Returns a new instance of Checks with the results.
-  def self.run_checks tracker
+  def self.run_checks(app_tree, tracker)
     if tracker.options[:parallel_checks]
-      self.run_checks_parallel tracker
+      self.run_checks_parallel(app_tree, tracker)
     else
-      self.run_checks_sequential tracker
+      self.run_checks_sequential(app_tree, tracker)
     end
   end
 
   #Run checks sequentially
-  def self.run_checks_sequential tracker
+  def self.run_checks_sequential(app_tree, tracker)
     check_runner = self.new :min_confidence => tracker.options[:min_confidence]
 
     @checks.each do |c|
@@ -96,7 +96,7 @@ class Brakeman::Checks
 
         Brakeman.notify " - #{check_name}"
 
-        check = c.new(tracker)
+        check = c.new(app_tree, tracker)
 
         begin
           check.run_check
@@ -118,7 +118,7 @@ class Brakeman::Checks
   end
 
   #Run checks in parallel threads
-  def self.run_checks_parallel tracker
+  def self.run_checks_parallel(app_tree, tracker)
     threads = []
     error_mutex = Mutex.new
 
@@ -134,7 +134,7 @@ class Brakeman::Checks
         Brakeman.notify " - #{check_name}"
 
         threads << Thread.new do
-          check = c.new(tracker)
+          check = c.new(app_tree, tracker)
 
           begin
             check.run_check

lib/brakeman/checks/base_check.rb

@@ -14,8 +14,9 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
   Match = Struct.new(:type, :match)
 
   #Initialize Check with Checks.
-  def initialize tracker
+  def initialize(app_tree, tracker)
     super()
+    @app_tree = app_tree
     @results = [] #only to check for duplicates
     @warnings = []
     @tracker = tracker
@@ -451,7 +452,7 @@ class Brakeman::BaseCheck < Brakeman::SexpProcessor
   end
 
   def gemfile_or_environment
-    if File.exist? File.expand_path "#{tracker.options[:app_path]}/Gemfile"
+    if @app_tree.exists?("Gemfile")
       "Gemfile"
     else
       "config/environment.rb"

lib/brakeman/processor.rb

@@ -12,8 +12,9 @@ module Brakeman
   class Processor
     include Util
 
-    def initialize options
-      @tracker = Tracker.new self, options
+    def initialize(app_tree, options)
+      @app_tree = app_tree
+      @tracker = Tracker.new(@app_tree, self, options)
     end
 
     def tracked_events
@@ -38,7 +39,7 @@ module Brakeman
     #Process controller source. +file_name+ is used for reporting
     def process_controller src, file_name
       if contains_class? src
-        ControllerProcessor.new(@tracker).process_controller src, file_name
+        ControllerProcessor.new(@app_tree, @tracker).process_controller src, file_name
       else
         LibraryProcessor.new(@tracker).process_library src, file_name
       end
@@ -47,7 +48,7 @@ module Brakeman
     #Process variable aliasing in controller source and save it in the
     #tracker.
     def process_controller_alias name, src, only_method = nil
-      ControllerAliasProcessor.new(@tracker, only_method).process_controller name, src
+      ControllerAliasProcessor.new(@app_tree, @tracker, only_method).process_controller name, src
     end
 
     #Process a model source

lib/brakeman/processors/controller_alias_processor.rb

@@ -9,8 +9,9 @@ class Brakeman::ControllerAliasProcessor < Brakeman::AliasProcessor
   #If only_method is specified, only that method will be processed,
   #other methods will be skipped.
   #This is for rescanning just a single action.
-  def initialize tracker, only_method = nil
+  def initialize app_tree, tracker, only_method = nil
     super()
+    @app_tree = app_tree
     @only_method = only_method
     @tracker = tracker
     @rendered = false
@@ -46,7 +47,7 @@ class Brakeman::ControllerAliasProcessor < Brakeman::AliasProcessor
       methods.each do |name|
         #Need to process the method like it was in a controller in order
         #to get the renders set
-        processor = Brakeman::ControllerProcessor.new(@tracker)
+        processor = Brakeman::ControllerProcessor.new(@app_tree, @tracker)
         method = mixin[:public][name]
 
         if node_type? method, :methdef

lib/brakeman/processors/controller_processor.rb

@@ -4,8 +4,9 @@ require 'brakeman/processors/base_processor'
 class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
   FORMAT_HTML = Sexp.new(:call, Sexp.new(:lvar, :format), :html)
 
-  def initialize tracker
-    super 
+  def initialize app_tree, tracker
+    super(tracker)
+    @app_tree = app_tree
     @controller = nil
     @current_method = nil
     @current_module = nil
@@ -89,7 +90,7 @@ class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
             #layout "some_layout"
 
             name = args.last.value.to_s
-            unless Dir.glob("#{@tracker.options[:app_path]}/app/views/layouts/#{name}.html.{erb,haml}").empty?
+            if @app_tree.layout_exists?(name)
               @controller[:layout] = "layouts/#{name}"
             else
               Brakeman.debug "[Notice] Layout not found: #{name}"
@@ -174,7 +175,7 @@ class Brakeman::ControllerProcessor < Brakeman::BaseProcessor
     name = underscore(@controller[:name].to_s.split("::")[-1].gsub("Controller", ''))
 
     #There is a layout for this Controller
-    unless Dir.glob("#{@tracker.options[:app_path]}/app/views/layouts/#{name}.html.{erb,haml}").empty?
+    if @app_tree.layout_exists?(name)
       @controller[:layout] = "layouts/#{name}"
     end
   end

lib/brakeman/report.rb

@@ -40,7 +40,8 @@ class Brakeman::Report
                      "<span class='med-confidence'>Medium</span>",
                      "<span class='weak-confidence'>Weak</span>" ]
 
-  def initialize tracker
+  def initialize(app_tree, tracker)
+    @app_tree = app_tree
     @tracker = tracker
     @checks = tracker.checks
     @element_id = 0 #Used for HTML ids
@@ -542,7 +543,7 @@ HEADER
 
   #Generate HTML for warnings, including context show/hidden via Javascript
   def with_context warning, message
-    context = context_for warning
+    context = context_for(@app_tree, warning)
     full_message = nil
 
     if tracker.options[:message_limit] and
@@ -657,7 +658,7 @@ HEADER
         else
           w.code = ""
         end
-        w.context = context_for(w).join("\n")
+        w.context = context_for(@app_tree, w).join("\n")
       end
     end
 

lib/brakeman/rescanner.rb

@@ -12,7 +12,7 @@ class Brakeman::Rescanner < Brakeman::Scanner
   def initialize options, processor, changed_files
     super(options, processor)
 
-    @paths = changed_files.map {|f| File.expand_path f, tracker.options[:app_path] }
+    @paths = changed_files.map {|f| @app_tree.expand_path(f) }
     @old_results = tracker.checks  #Old warnings from previous scan
     @changes = nil                 #True if files had to be rescanned
     @reindex = Set.new
@@ -66,7 +66,7 @@ class Brakeman::Rescanner < Brakeman::Scanner
   def rescan_file path, type = nil
     type ||= file_type path
 
-    unless File.exist? path
+    unless @app_tree.path_exists?(path)
       return rescan_deleted_file path, type
     end
 
@@ -128,7 +128,7 @@ class Brakeman::Rescanner < Brakeman::Scanner
   end
 
   def rescan_template path
-    return unless path.match KNOWN_TEMPLATE_EXTENSIONS and File.exist? path
+    return unless path.match KNOWN_TEMPLATE_EXTENSIONS and @app_tree.path_exists?(path)
 
     template_name = template_path_to_name(path)
 
@@ -177,7 +177,7 @@ class Brakeman::Rescanner < Brakeman::Scanner
   def rescan_model path
     num_models = tracker.models.length
     tracker.reset_model path
-    process_model path if File.exists? path
+    process_model path if @app_tree.path_exists?(path)
 
     #Only need to rescan other things if a model is added or removed
     if num_models != tracker.models.length
@@ -190,7 +190,7 @@ class Brakeman::Rescanner < Brakeman::Scanner
   end
 
   def rescan_lib path
-    process_lib path if File.exists? path
+    process_lib path if @app_tree.path_exists?(path)
 
     lib = nil
 

lib/brakeman/scanner.rb

@@ -9,6 +9,7 @@ begin
   require 'erb'
   require 'erubis'
   require 'brakeman/processor'
+  require 'brakeman/app_tree'
   require 'brakeman/parsers/rails2_erubis'
   require 'brakeman/parsers/rails2_xss_plugin_erubis'
   require 'brakeman/parsers/rails3_erubis'
@@ -28,19 +29,19 @@ class Brakeman::Scanner
   #Pass in path to the root of the Rails application
   def initialize options, processor = nil
     @options = options
-    @report_progress = options[:report_progress]
-    @path = options[:app_path]
-    @app_path = File.join(@path, "app")
-    @processor = processor || Brakeman::Processor.new(options)
-    @skip_files = nil
+    @app_tree = Brakeman::AppTree.from_options(options)
 
-    #Convert files into Regexp for matching
-    if options[:skip_files]
-      list = "(?:" << options[:skip_files].map { |f| Regexp.escape f }.join("|") << ")$"
-      @skip_files = Regexp.new(list)
+    if !@app_tree.root || !@app_tree.exists?("app")
+      abort("Please supply the path to a Rails application.")
+    end
+
+    if @app_tree.exists?("script/rails")
+      options[:rails3] = true
+      Brakeman.notify "[Notice] Detected Rails 3 application"
     end
 
     @ruby_parser = ::RubyParser
+    @processor = processor || Brakeman::Processor.new(@app_tree, options)
   end
 
   #Returns the Tracker generated from the scan
@@ -83,7 +84,7 @@ class Brakeman::Scanner
       process_config_file "gems.rb"
     end
 
-    if File.exists? "#@path/vendor/plugins/rails_xss" or
+    if @app_tree.exists?("vendor/plugins/rails_xss") or
       options[:rails3] or options[:escape_html]
 
       tracker.config[:escape_html] = true
@@ -92,8 +93,10 @@ class Brakeman::Scanner
   end
 
   def process_config_file file
-    if File.exists? "#@path/config/#{file}"
-      @processor.process_config(parse_ruby(File.read("#@path/config/#{file}")))
+    path = "config/#{file}"
+
+    if @app_tree.exists?(path)
+      @processor.process_config(parse_ruby(@app_tree.read(path)))
     end
 
   rescue Exception => e
@@ -105,11 +108,11 @@ class Brakeman::Scanner
 
   #Process Gemfile
   def process_gems
-    if File.exists? "#@path/Gemfile"
-      if File.exists? "#@path/Gemfile.lock"
-        @processor.process_gems(parse_ruby(File.read("#@path/Gemfile")), File.read("#@path/Gemfile.lock"))
+    if @app_tree.exists? "Gemfile"
+      if @app_tree.exists? "Gemfile.lock"
+        @processor.process_gems(parse_ruby(@app_tree.read("Gemfile")), @app_tree.read("Gemfile.lock"))
       else
-        @processor.process_gems(parse_ruby(File.read("#@path/Gemfile")))
+        @processor.process_gems(parse_ruby(@app_tree.read("Gemfile")))
       end
     end
   rescue Exception => e
@@ -121,10 +124,7 @@ class Brakeman::Scanner
   #
   #Adds parsed information to tracker.initializers
   def process_initializers
-    initializer_files = Dir.glob(@path + "/config/initializers/**/*.rb").sort
-    initializer_files.reject! { |f| @skip_files.match f } if @skip_files
-
-    initializer_files.each do |f|
+    @app_tree.initializer_paths.each do |f|
       process_initializer f
     end
   end
@@ -132,7 +132,7 @@ class Brakeman::Scanner
   #Process an initializer
   def process_initializer path
     begin
-      @processor.process_initializer(path, parse_ruby(File.read(path)))
+      @processor.process_initializer(path, parse_ruby(@app_tree.read_path(path)))
     rescue Racc::ParseError => e
       tracker.error e, "could not parse #{path}. There is probably a typo in the file. Test it with 'ruby_parse #{path}'"
     rescue Exception => e
@@ -149,19 +149,13 @@ class Brakeman::Scanner
       return
     end
 
-    lib_files = Dir.glob(@path + "/lib/**/*.rb").sort
-    lib_files.reject! { |f| @skip_files.match f } if @skip_files
-
-    total = lib_files.length
+    total = @app_tree.lib_paths.length
     current = 0
 
-    lib_files.each do |f|
+    @app_tree.lib_paths.each do |f|
       Brakeman.debug "Processing #{f}"
-      if @report_progress
-        $stderr.print " #{current}/#{total} files processed\r"
+      report_progress(current, total)
       current += 1
-      end
-
       process_lib f
     end
   end
@@ -169,7 +163,7 @@ class Brakeman::Scanner
   #Process a library
   def process_lib path
     begin
-      @processor.process_lib parse_ruby(File.read(path)), path
+      @processor.process_lib parse_ruby(@app_tree.read_path(path)), path
     rescue Racc::ParseError => e
       tracker.error e, "could not parse #{path}. There is probably a typo in the file. Test it with 'ruby_parse #{path}'"
     rescue Exception => e
@@ -181,9 +175,9 @@ class Brakeman::Scanner
   #
   #Adds parsed information to tracker.routes
   def process_routes
-    if File.exists? "#@path/config/routes.rb"
+    if @app_tree.exists?("config/routes.rb")
       begin
-        @processor.process_routes parse_ruby(File.read("#@path/config/routes.rb"))
+        @processor.process_routes parse_ruby(@app_tree.read("config/routes.rb"))
       rescue Exception => e
         tracker.error e.exception(e.message + "\nWhile processing routes.rb"), e.backtrace
         Brakeman.notify "[Notice] Error while processing routes - assuming all public controller methods are actions."
@@ -198,19 +192,13 @@ class Brakeman::Scanner
   #
   #Adds processed controllers to tracker.controllers
   def process_controllers
-    controller_files = Dir.glob(@app_path + "/controllers/**/*.rb").sort
-    controller_files.reject! { |f| @skip_files.match f } if @skip_files
-
-    total = controller_files.length
+    total = @app_tree.controller_paths.length
     current = 0
 
-    controller_files.each do |f|
+    @app_tree.controller_paths.each do |f|
       Brakeman.debug "Processing #{f}"
-      if @report_progress
-        $stderr.print " #{current}/#{total} files processed\r"
+      report_progress(current, total)
       current += 1
-      end
-
       process_controller f
     end
 
@@ -221,11 +209,8 @@ class Brakeman::Scanner
 
     tracker.controllers.sort_by{|name| name.to_s}.each do |name, controller|
       Brakeman.debug "Processing #{name}"
-      if @report_progress
-        $stderr.print " #{current}/#{total} controllers processed\r"
+      report_progress(current, total, "controllers")
       current += 1
-      end
-
       @processor.process_controller_alias name, controller[:src]
     end
 
@@ -235,7 +220,7 @@ class Brakeman::Scanner
 
   def process_controller path
     begin
-      @processor.process_controller(parse_ruby(File.read(path)), path)
+      @processor.process_controller(parse_ruby(@app_tree.read_path(path)), path)
     rescue Racc::ParseError => e
       tracker.error e, "could not parse #{path}. There is probably a typo in the file. Test it with 'ruby_parse #{path}'"
     rescue Exception => e
@@ -247,23 +232,15 @@ class Brakeman::Scanner
   #
   #Adds processed views to tracker.views
   def process_templates
-
-    views_path = @app_path + "/views/**/*.{html.erb,html.haml,rhtml,js.erb}"
     $stdout.sync = true
-    count = 0
-
-    template_files = Dir.glob(views_path).sort
-    template_files.reject! { |f| @skip_files.match f } if @skip_files
 
-    total = template_files.length
+    count = 0
+    total = @app_tree.template_paths.length
 
-    template_files.each do |path|
+    @app_tree.template_paths.each do |path|
       Brakeman.debug "Processing #{path}"
-      if @report_progress
-        $stderr.print " #{count}/#{total} files processed\r"
+      report_progress(count, total)
       count += 1
-      end
-
       process_template path
     end
 
@@ -274,11 +251,8 @@ class Brakeman::Scanner
 
     tracker.templates.keys.dup.sort_by{|name| name.to_s}.each do |name|
       Brakeman.debug "Processing #{name}"
-      if @report_progress
+      report_progress(count, total, "templates")
       count += 1
-        $stderr.print " #{count}/#{total} templates processed\r"
-      end
-
       @processor.process_template_alias tracker.templates[name]
     end
   end
@@ -287,7 +261,7 @@ class Brakeman::Scanner
     type = path.match(KNOWN_TEMPLATE_EXTENSIONS)[1].to_sym
     type = :erb if type == :rhtml
     name = template_path_to_name path
-    text = File.read path
+    text = @app_tree.read_path path
 
     begin
       if type == :erb
@@ -339,27 +313,20 @@ class Brakeman::Scanner
   #
   #Adds the processed models to tracker.models
   def process_models
-    model_files = Dir.glob(@app_path + "/models/**/*.rb").sort
-    model_files.reject! { |f| @skip_files.match f } if @skip_files
-
-    total = model_files.length
+    total = @app_tree.model_paths.length
     current = 0
 
-    model_files.each do |f|
+    @app_tree.model_paths.each do |f|
       Brakeman.debug "Processing #{f}"
-      if @report_progress
-        $stderr.print " #{current}/#{total} files processed\r"
+      report_progress(current, total)
       current += 1
-      end
-
       process_model f
-
     end
   end
 
   def process_model path
     begin
-      @processor.process_model(parse_ruby(File.read(path)), path)
+      @processor.process_model(parse_ruby(@app_tree.read_path(path)), path)
     rescue Racc::ParseError => e
       tracker.error e, "could not parse #{path}"
     rescue Exception => e
@@ -367,6 +334,11 @@ class Brakeman::Scanner
     end
   end
 
+  def report_progress(current, total, type = "files")
+    return unless @options[:report_progress]
+    $stderr.print " #{current}/#{total} #{type} processed\r"
+  end
+
   def index_call_sites
     tracker.index_call_sites
   end

lib/brakeman/tracker.rb

@@ -20,9 +20,11 @@ class Brakeman::Tracker
   #
   #The Processor argument is only used by other Processors
   #that might need to access it.
-  def initialize processor = nil, options = {}
+  def initialize(app_tree, processor = nil, options = {})
+    @app_tree = app_tree
     @processor = processor
     @options = options
+
     @config = {}
     @templates = {}
     @controllers = {}
@@ -67,7 +69,7 @@ class Brakeman::Tracker
   #Run a set of checks on the current information. Results will be stored
   #in Tracker#checks.
   def run_checks
-    @checks = Brakeman::Checks.run_checks(self)
+    @checks = Brakeman::Checks.run_checks(@app_tree, self)
 
     @end_time = Time.now
     @duration = @end_time - @start_time
@@ -147,7 +149,7 @@ class Brakeman::Tracker
 
   #Returns a Report with this Tracker's information
   def report
-    Brakeman::Report.new(self)
+    Brakeman::Report.new(@app_tree, self)
   end
 
   def index_call_sites

lib/brakeman/util.rb

@@ -331,10 +331,10 @@ module Brakeman::Util
 
   #Return array of lines surrounding the warning location from the original
   #file.
-  def context_for warning, tracker = nil
+  def context_for app_tree, warning, tracker = nil
     file = file_for warning, tracker
     context = []
-    return context unless warning.line and file and File.exist? file
+    return context unless warning.line and file and @app_tree.path_exists? file
 
     current_line = 0
     start_line = warning.line - 5

test/tests/test_brakeman.rb

@@ -18,10 +18,12 @@ end
 
 class BaseCheckTests < Test::Unit::TestCase
   FakeTracker = Struct.new(:config)
+  FakeAppTree = Struct.new(:root)
 
   def setup
     @tracker = FakeTracker.new
-    @check = Brakeman::BaseCheck.new @tracker
+    app_tree = FakeAppTree.new
+    @check = Brakeman::BaseCheck.new app_tree, @tracker
   end
 
   def version_between? version, high, low