Merge pull request ManageIQ#23124 from Fryguy/upgrade_brakeman

Upgrade brakeman to v6

Gemfile

@@ -303,7 +303,7 @@ group :development do
 end
 
 group :test do
-  gem "brakeman",                       "~>5.4",             :require => false
+  gem "brakeman",                       "~>6.2",             :require => false
   gem "bundler-audit",                                       :require => false
   gem "capybara",                       "~>2.5.0",           :require => false
   gem "db-query-matchers",              "~>0.11.0"

config/brakeman.ignore

@@ -1,20 +1,42 @@
 {
   "ignored_warnings": [
+    {
+      "warning_type": "Cross-Site Request Forgery",
+      "warning_code": 86,
+      "fingerprint": "6301c055d2b1a4bc467bcd405b0ba295893f71df183eae355cd1a8b6c0ed0588",
+      "check_name": "ForgerySetting",
+      "message": "`protect_from_forgery` should be configured with `with: :exception`",
+      "file": "(engine:manageiq-ui-classic) app/controllers/application_controller.rb",
+      "line": 13,
+      "link": "https://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/",
+      "code": "protect_from_forgery(:secret => SecureRandom.hex(64), :except => ([:authenticate, :external_authenticate, :kerberos_authenticate, :saml_login, :initiate_saml_login, :oidc_login, :initiate_oidc_login, :csp_report]), :with => :reset_session)",
+      "render_path": null,
+      "location": {
+        "type": "controller",
+        "controller": "ApplicationController"
+      },
+      "user_input": null,
+      "confidence": "Medium",
+      "cwe_id": [
+        352
+      ],
+      "note": "This was intentionally changed from :exception to :reset_session in ManageIQ/manageiq-ui-classic#4901"
+    },
     {
       "warning_type": "Command Injection",
       "warning_code": 14,
-      "fingerprint": "1d1f63511c528d6d28a292a8b3ff2a91b5e0c4104112fb06631a00ca89f03e2f",
+      "fingerprint": "9a58ac820e59b1edb4530e27646edc1f328915a7a356d987397659b48c52239e",
       "check_name": "Execute",
       "message": "Possible command injection",
       "file": "lib/ansible/runner.rb",
-      "line": 409,
+      "line": 422,
       "link": "https://brakemanscanner.org/docs/warning_types/command_injection/",
       "code": "`python#{version} -c 'import site; print(\":\".join(site.getsitepackages()))'`",
       "render_path": null,
       "location": {
         "type": "method",
         "class": "Ansible::Runner",
-        "method": "ansible_python_paths_raw"
+        "method": "s(:self).ansible_python_paths_raw"
       },
       "user_input": "version",
       "confidence": "Medium",
@@ -24,6 +46,6 @@
       "note": "This method is safe because it verifies that the version is in the form #.#."
     }
   ],
-  "updated": "2023-03-20 10:30:55 -0400",
-  "brakeman_version": "5.4.1"
+  "updated": "2024-09-11 16:34:41 -0400",
+  "brakeman_version": "6.2.1"
 }

lib/extensions/brakeman_fingerprint_patch.rb

@@ -0,0 +1,70 @@
+# Brakeman fingerprints account for the file location as part of the fingerprint
+# digest. The fingerprint uses the file's relative path, but assumes that all
+# files being scanned will be under the app_tree root directory, providing a
+# consistent file path regardless of the system it is run on.
+#
+# For engines, however this is not always the case. Engines can come from gems
+# and gems can be installed basically anywhere on the system depending on how
+# ruby is installed and configured, and even depending on which Ruby version
+# manager is used. Additionally, in CI gems are typically installed in a vendor
+# directory, and locally gems can be configured as git-based or path-based.
+# Because of this, the file path and the fingerprint can vary widely between
+# local dev and CI, causing problems when trying to ignore issues. For example,
+#
+#   git-based engine locally     | ../../.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb
+#   git-based engine on CI       | vendor/bundle/ruby/3.0.0/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb
+#   path-based engine locally    | ../manageiq-ui-classic/app/controllers/application_controller.rb
+#   version-based engine locally | ../../.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/app/controllers/application_controller.rb
+#
+# This patch introduces a way to "remove" the leading path for files that reside
+# in engines. This removal provides a consistent file path for the fingerprint
+# method. For example, all of the above will appear like
+#
+#   (engine:manageiq-ui-classic) app/controllers/application_controller.rb
+#
+# NOTE: This patch only modifies what is necessary to make the
+# test:security:brakeman test suite work, namely fingerprint and the json
+# reporter (which leverages to_hash). It does not modify things such as the
+# the text reporter (CLI output) nor the interactive ignore.
+module BrakemanFingerprintPatch
+  def self.rails_engine_paths
+    @rails_engine_paths ||= ::Rails::Engine.subclasses.map { |e| e.root.to_s << "/" }
+  end
+
+  # Removes any leading engine paths if the file is an engine, and replaces with
+  # `(engine:<engine_name>)`
+  #
+  # NOTE: Ideally this code should use the in_engine_paths? method (that is
+  # patched in brakeman_excludes_patch.rb), however Brakeman::Warning doesn't
+  # have a reference to the app_tree where that method is defined, as warnings
+  # are standalone objects.
+  def file_string
+    engine_path = BrakemanFingerprintPatch.rails_engine_paths.detect { |p| self.file.absolute.start_with?(p) }
+    if engine_path
+      engine_name = File.basename(engine_path).sub(/-\h+$/, "").sub(/-(?:\d+\.)+\d+$/, "")
+      engine_relative = self.file.absolute.sub(engine_path, "")
+      "(engine:#{engine_name}) #{engine_relative}"
+    else
+      self.file.relative
+    end
+  end
+
+  # This method is copied from brakeman (https://github.com/presidentbeef/brakeman/blob/e4f49f64d263f8001bac62eec182ad417152776d/lib/brakeman/warning.rb#L250-L257)
+  # in order to modify the file_string component of the digest to account for engine support.
+  def fingerprint
+    loc = self.location
+    location_string = loc && loc.sort_by { |k, v| k.to_s }.inspect
+    warning_code_string = sprintf("%03d", @warning_code)
+    code_string = @code.inspect
+
+    Digest::SHA2.new(256).update("#{warning_code_string}#{code_string}#{location_string}#{file_string}#{self.confidence}").to_s
+  end
+
+  # This method overrides the .to_hash method from brakeman (https://github.com/presidentbeef/brakeman/blob/e4f49f64d263f8001bac62eec182ad417152776d/lib/brakeman/warning.rb#L288-L310)
+  # in order to modify the file value to account for engine support.
+  def to_hash(absolute_paths: true)
+    super.tap do |h|
+      h[:file] = (absolute_paths ? self.file.absolute : file_string)
+    end
+  end
+end

lib/tasks/test_security_helper.rb

@@ -12,6 +12,11 @@ def self.brakeman(format: "human")
     require Rails.root.join('lib/extensions/brakeman_excludes_patch')
     Brakeman::AppTree.prepend(BrakemanExcludesPatch)
 
+    # Brakeman's fingerprint check does not work properly with engines
+    require "brakeman/warning"
+    require Rails.root.join('lib/extensions/brakeman_fingerprint_patch')
+    Brakeman::Warning.prepend(BrakemanFingerprintPatch)
+
     app_path = Rails.root.to_s
     engine_paths = Vmdb::Plugins.paths.except(ManageIQ::Schema::Engine).values