Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix brakeman fingerprint patch on clones #23191

Merged
merged 2 commits into from
Sep 18, 2024
Merged

Fix brakeman fingerprint patch on clones #23191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4ad486
Add specs for BrakemanFingerprintPatch
Fryguy Sep 17, 2024
00b8966
Fix issue when running from an engine with a cloned spec/manageiq
Fryguy Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions lib/extensions/brakeman_fingerprint_patch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ def self.rails_engine_paths
# are standalone objects.
def file_string
engine_path = BrakemanFingerprintPatch.rails_engine_paths.detect { |p| self.file.absolute.start_with?(p) }
if engine_path
if engine_path.nil? || (Rails.root.to_s.start_with?(engine_path) && self.file.absolute.start_with?(Rails.root.to_s))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, when have a core cloned underneath the engine we have 2 cases

  • issue in an engine file, such as: /Users/user/dev/manageiq-ui-classic/app/controllers/application_controller.rb
  • issue in a core file, such as /Users/user/dev/manageiq-ui-classic/spec/manageiq/lib/ansible/runner.rb

In both of those cases, we get an engine_path, but in reality, only the issue in an engine file one is actually in the engine, whereas the core file just happens to be under the engine directory, but is actually an issue in core. So, we do additional checks to

  • see if the Rails root starts with the engine_path, which tells us we are in the core cloned underneath the engine case
  • then see if the issue path starts with the rails root, which tells us that the file is actually a core issue and not an engine issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's too bad we need to check it like this in a patch of brakeman.

Just checking... Do we still think it's worth checking all of the engines here? I know it's been a bit frustrating that an engine issue was causing unrelated core test failures. Is it just more convenient to pull in all the engines and test each one?

Honestly, if we tested in UI classic only, the rest of the engines could probably be included in the core tests. UI classic is likely the noisest offender for brakeman issues outside of core. I'm just wondering if it's worth it.

Copy link
Member Author

@Fryguy Fryguy Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a really good idea. I think what I'd like to do is get this merged to get us green then I can investigate that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I researched this a little, and I thikn the way we have it is the best way. The brakeman app technically always runs from the main app, and what we do is just tell it more engines to check. From core, we just loop through all of the engines and add all of them. From an engine, we just add the engine itself only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think the problem is where the problem is reported. If we add something in an engine that breaks core, it makes core red and we become hesitant to merge while it's being fixed whereas the engine is not aware of the problem. We can look at it as a followup, I just wanted to make sure the complexity still made sense.

self.file.relative
else
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

Expand All @@ -65,6 +65,8 @@ def fingerprint
def to_hash(absolute_paths: true)
super.tap do |h|
h[:file] = (absolute_paths ? self.file.absolute : file_string)
h[:file_rel] = self.file.relative
h[:file_abs] = self.file.absolute
end
end
end
168 changes: 168 additions & 0 deletions spec/lib/extensions/brakeman_fingerprint_patch_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
RSpec.describe BrakemanFingerprintPatch do
subject do
Class.new do
include BrakemanFingerprintPatch
attr_accessor :file
def initialize = @warning_code = 0
def confidence = "High"
def location = {}
end.new
end

shared_examples_for "handles brakeman pathing" do
before do
allow(Rails).to receive(:root).and_return(Pathname.new(core_path))
allow(described_class).to receive(:rails_engine_paths).and_return([engine_path])
end

it "with an issue in the engine" do
subject.file = engine_issue

expect(subject.file_string).to eq("(engine:manageiq-ui-classic) app/controllers/application_controller.rb")
expect(subject.fingerprint).to eq("1ea1c06c8976493622ad8c668f56df3a44aac997fabd57ab58fcf59a37712e56")
end

it "with an issue in core" do
subject.file = core_issue

expect(subject.file_string).to eq("lib/ansible/runner.rb")
expect(subject.fingerprint).to eq("f06e25d3b6fa417a80313b2ebd451fbbeac3670f03897e26e86983a5c29635c1")
end
end

let(:core_path) { "/Users/user/dev/manageiq/" }
let(:core_issue) do
instance_double("Brakeman::FilePath",
:relative => "lib/ansible/runner.rb",
:absolute => "/Users/user/dev/manageiq/lib/ansible/runner.rb"
)
end

context "running from the core repo" do
context "with a git-based engine" do
context "in the system gem location" do
let(:engine_path) { "/Users/user/.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "../../.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb",
:absolute => "/Users/user/.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end

context "in a vendored gem location inside of the core repo" do # This is the core CI case
let(:engine_path) { "/Users/user/dev/manageiq/vendor/bundle/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "vendor/bundle/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq/vendor/bundle/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end
end

context "with a gem-based engine" do
context "in the system gem location" do
let(:engine_path) { "/Users/user/.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "../../.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/app/controllers/application_controller.rb",
:absolute => "/Users/user/.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end

context "in a vendored gem location inside of the core repo" do # This is a core CI case in a future where we might use a versioned gem
let(:engine_path) { "/Users/user/dev/manageiq/vendor/bundle/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "vendor/bundle/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq/vendor/bundle/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end
end

context "with a path-based engine" do
context "which is a sibling of the core repo" do
let(:engine_path) { "/Users/user/dev/manageiq-ui-classic/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "../manageiq-ui-classic/app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq-ui-classic/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end

context "which is inside of the core repo" do
let(:engine_path) { "/Users/user/dev/manageiq/plugins/manageiq-ui-classic/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "plugins/manageiq-ui-classic/app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq/plugins/manageiq-ui-classic/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end
end
end

context "running from an engine repo" do
context "with a symlinked spec/manageiq dir" do # When symlinked, the paths appear the same as a path-based gem, so these are copies of the path-based tests above
context "which is a sibling of the core repo" do
let(:engine_path) { "/Users/user/dev/manageiq-ui-classic/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "../manageiq-ui-classic/app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq-ui-classic/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end

context "which is inside of the core repo" do
let(:engine_path) { "/Users/user/dev/manageiq/plugins/manageiq-ui-classic/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "plugins/manageiq-ui-classic/app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq/plugins/manageiq-ui-classic/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end
end

context "with a cloned spec/manageiq dir" do # This is also the CI case for a plugin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie When I ran these tests without the code fix, all of them passed except this one, and within this one the engine one passed but the core case did not. So, the code fix specifically fixes this case without breaking the other cases.

let(:core_path) { "/Users/user/dev/manageiq-ui-classic/spec/manageiq/" }
let(:core_issue) do
instance_double("Brakeman::FilePath",
:relative => "lib/ansible/runner.rb",
:absolute => "/Users/user/dev/manageiq-ui-classic/spec/manageiq/lib/ansible/runner.rb"
)
end

let(:engine_path) { "/Users/user/dev/manageiq-ui-classic/" }
let(:engine_issue) do
instance_double("Brakeman::FilePath",
:relative => "../../app/controllers/application_controller.rb",
:absolute => "/Users/user/dev/manageiq-ui-classic/app/controllers/application_controller.rb"
)
end

include_examples "handles brakeman pathing"
end
end
end