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

False negatives due to --skip-libs ignoring app/ files. #1839

Open
kevinjacobs opened this issue Apr 23, 2024 · 2 comments
Open

False negatives due to --skip-libs ignoring app/ files. #1839

kevinjacobs opened this issue Apr 23, 2024 · 2 comments
Milestone

Comments

@kevinjacobs
Copy link

kevinjacobs commented Apr 23, 2024

Background

Brakeman version: 5.4.0
Rails version: 4.0.8
Ruby version: 3.1.2

Issue

I’d like to report some unexpected false negatives noticed when running with the --skip-libs option.

Documentation [1] states that to “To skip processing of the lib/ directory…”, one should add --skip-libs. However this results in Brakeman ignoring much of the app/ directory as well, in fact it appears that only the contents of app/models/ and app/controllers/ are included in this mode. If one wants to skip the lib/ directory, --skip-files lib/ seems to be a better approach.

I believe this is due to file type detection at [2] assuming code is “library” code unless it fits into a small number of alternative classifications. The last sentence in [3] seems to support this.

Admittedly, options.MD also includes the following:

brakeman --faster
This will disable some features, but will probably be much faster (currently it is the same as --skip-libs --no-branching). WARNING: This may cause Brakeman to miss some vulnerabilities.

But given the other mention of --skip-libs applying to lib/, a reasonable reader might assume that --no-branching is the cause for the above warning.

Reproducer:

git clone https://github.com/railstutorial/sample_app_rails_4.git && cd sample_app_rails_4
brakeman -f json --skip-libs -o skip_libs.json
brakeman -f json --skip-files lib/ -o skip_files.json
diff skip_libs.json skip_files.json

Expected results: Given that there are no warnings from lib/, both outputs should include the same warnings.

Actual results: skip_libs.json misses a warning in app/helpers/sessions_helper.rb.

If this behavior is intended (as it appears to be), the documentation should more clearly state the potential impact of running with --skip-libs.

Thanks!

[1] https://brakemanscanner.org/docs/options/
[2] https://github.com/presidentbeef/brakeman/blob/main/lib/brakeman/processors/lib/file_type_detector.rb#L16
[3] #1554

@presidentbeef
Copy link
Owner

presidentbeef commented Apr 23, 2024

Hi @kevinjacobs - you are right, thank you for reporting this. It should be equivalent to --skip-files lib/ (or maybe --skip-files /lib/) but due to the change to scan (almost) every Ruby file, it no longer matches.

@presidentbeef
Copy link
Owner

Should remove --skip-libs 🤔 in Brakeman 7.0 I guess.

@presidentbeef presidentbeef added this to the 7.0 milestone Jul 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants