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.

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

Interactive ignore tries to call empty? on a Brakeman::FilePath and fails #1622

Closed
bradparker opened this issue Jul 19, 2021 · 8 comments
Closed

Comments

@bradparker
Copy link
Contributor

bradparker commented Jul 19, 2021

Background

Brakeman version: 5.1.0
Rails version: 6.1.4
Ruby version: 2.7.3

Link to Rails application code: Sorry, it's a private repo but I'm not sure this is source related.

Issue

When trying to run with --interactive-ignore I get the following error:

$BUNDLE_PATH/ruby/2.7.0/gems/brakeman-5.1.0/lib/brakeman/report/ignore/interactive.rb:39:in `block (2 levels) in file_menu': undefined method `empty?' for #<Brakeman::FilePath:0x00007fae2cbf4c08> (NoMethodError)

I think this change may have introduced it: 52bfc9b.

@bradparker
Copy link
Contributor Author

Reverting that commit does stop that error from happening, and I get an otherwise successful run. From looking at the associated PR it doesn't seem to me that the refactor was required for the feature that was being implemented. Wonder if reverting for now is an OK way to go.

@presidentbeef
Copy link
Owner

Hi @bradparker - how are you triggering this issue? I understand why the code is failing but I'd like to be able to reproduce it.

@bradparker
Copy link
Contributor Author

bradparker commented Jul 19, 2021

@presidentbeef I'm running:

$ bin/brakeman --interactive-ignore

bin/brakeman is the usual bin stub I think:

#!/usr/bin/env ruby
# frozen_string_literal: true

#
# This file was generated by Bundler.
#
# The application 'brakeman' is installed as part of a gem, and
# this file is here to facilitate running it.
#

require "pathname"
ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../../Gemfile",
  Pathname.new(__FILE__).realpath)

bundle_binstub = File.expand_path("../bundle", __FILE__)

if File.file?(bundle_binstub)
  if File.read(bundle_binstub, 300) =~ /This file was generated by Bundler/
    load(bundle_binstub)
  else
    abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run.
Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.")
  end
end

require "rubygems"
require "bundler/setup"

load Gem.bin_path("brakeman", "brakeman")

@donncha
Copy link

donncha commented Jul 20, 2021

This change also broke ignore files in general. I am now seeing failing CI runs with
[Notice] Could not find ignore configuration in brakeman/et-rpm-whitelist.json
No issues in v5.0.4

@donncha
Copy link

donncha commented Jul 20, 2021

OK, the change to File.exists? file.absolute in lib/brakeman/report/ignore/config.rb means that the ignore file is now relative to the directory being scanned, not the directory the command was run from, so

$ brakeman dir_to_scan -i ignore_file.json

expects the ignore file to be at ./dir_to_scan/ignore_file.json instead of ./ignore_file.json in 5.0.4

@eliblock
Copy link
Contributor

eliblock commented Jul 20, 2021

@presidentbeef - based on @donncha's feedback (which I've validated), I'd be supportive of a revert here.

I updated #1624's PR comment to expand a bit on the original intention of including the ignore file pathing refactor in #1620, but the side effect makes the behavior of -i diverge from that of -c, which seems undesired.

@bradparker
Copy link
Contributor Author

bradparker commented Jul 20, 2021

Have a revert ready to go in case it's useful: #1623.

@presidentbeef
Copy link
Owner

Thanks @bradparker and @eliblock!

I've released 5.1.1 with the revert.

In the meantime, if anyone wants to contribute test coverage for --interactive-ignore... 🥰

Repository owner locked and limited conversation to collaborators Jan 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants