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

Namespaced classes that are not fully qualified can cause difference in false positives/negatives (WIP) #1523

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ChrisNJ58
Copy link

Hello!

A coworker and I have recently been working on trying to resolve an issue reported into Brakeman a little over a year ago, related to the namespacing of classes causing potential false negatives. There was an issue brought up related to this issue here if a refresher on the topic is needed.

This PR contains a fix that you had been working on yourself, before eventually putting aside, due to the results coming back with a number of false positives and weird results, specifically when analyzing controllers. I've taken your fix and put it on top the latest 4.10.0 version of Brakeman, including fixing a few tests that had been failing.

When I and my coworker have been testing this fix, we do still some false positive results when running against some of our repositories (particularly where a number of previously ignored warnings are being re-raised, due to this fix changing the confidence levels on warnings), but overall it looks like the changes made in this PR resolves the greater namespacing issue. I know it's likely been a long time since this problem was looked at, but I was hoping to get some of your insight into what other issues you were experiencing when you initially worked on this PR, particularly around weird results you were finding on controller, and see if I could pick your brain on possible ideas that I could implement to get this problem solved!

@presidentbeef
Copy link
Owner

Whew, thanks for picking this back up! I will have to run some tests to remember what the issues were that I saw before.

@ChrisNJ58
Copy link
Author

ChrisNJ58 commented Oct 20, 2020

That sounds great! We've been running Brakeman with this fix on top on a number of our projects, and haven't been able to find anything strange or concerning from our results, so I'm really interested to hear if there's something that we're missing, and getting a chance to dig into it a bit deeper!

@kfinn-clio
Copy link

One thing my co-worker and I noticed with this namespace change is previous ignored warnings become obsolete. After investigation these obsolete warnings are still valid but can be ignored. The obsolete warnings were in both models and controllers. Had these warnings not been ignored they would have not been flagged with the namespace change.
Is this something you noticed when previously working on this change?

@ChrisNJ58
Copy link
Author

Hello again!

I was wondering if you have had the chance to get reacquainted with this namespacing issue? It looks like since I posted this PR it's no longer passing codeclimate, but I would like to hear your thoughts if it would resolve the potential namespacing problem.

@presidentbeef
Copy link
Owner

Hi folks, my priority right now is getting Brakeman 5.0 out. I apologize for the lack of reply.

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

Successfully merging this pull request may close these issues.

3 participants