Address false warnings raised for known renderable classes specified with fully qualified paths

[that-jill]

We noticed some instances of Brakeman raising warnings in our application when there wasn't a clear issue. Demonstrating them here instead of in an issue so that I can show failing specs and help tackle the root cause. (Happy to open an issue as well, but failing specs I think are clearer than any written explanation of the situation).

About our app:

it's quite old and large (Brakeman reports finding 1216 controllers and 1865 models). We use CBRA to help manage it, with lots of organized modules / components. It's usual for our developers to specify a fully-qualified path (::MyViewComponent vs MyViewComponent) due to our unique architecture. In this case, the fully-qualified path specifications were causing Brakeman to raise warnings while rendering ViewComponents across the app.

Will start working on a fix; definitely open to questions/opinions on the best way to tackle this.

[dryrunsecurity]

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security	Status	Findings
AppSec Analyzer (beta)	✅	0 findings
Secrets Analyzer (beta)	✅	0 findings
Authn/Authz Analyzer	✅	0 findings
Configured Codepaths Analyzer	✅	0 findings
Sensitive Files Analyzer	✅	0 findings

Note

🟢 Risk threshold not exceeded.

Tip

Get answers to your security questions. Add a comment in this PR starting with @DryRunSecurity. For example...

@dryrunsecurity What are common security issues with web application cookies?

Powered by DryRun Security

[that-jill]

I can't say that I fully understand the impact of this change. However, I see several places where we do string checking -- this one being the most relevant to our issue -- where returning a fully-qualified path like so could cause failures. I also checked this type against the existing test suite, and the only time a this matches against :colon3 seems to be in the ViewComponent cases. All specs pass with this change. This leads me to believe that this type may be largely unused / unchecked-against. @presidentbeef do you know whether this might be the case or if there are any other untested ramifications of such a modification?

[presidentbeef]

No, this change doesn't make sense to me.

A :colon3 node is produced when parsing a constant with a leading ::, e.g. ::X

So what should be updated is the list of representations of ViewComponent classes. But see my other comment on this issue before we go down that path.

[that-jill]

The reason I opted to go with this option is that I see several places in this codebase that do manual checks against classnames -- several instances of places where inheriting from places like ::ActiveRecord::Base instead of ActiveRecord::Base might not work:

(a few examples, though not all that are listed apply)

I can certainly update my application to skip this specific check, but that won't truly address the problem of these paths being handled differently. We'd have to update these lists across brakeman as well as ensure any future representations are updated, which only makes sense if there are places where constants with a leading
:: (ie, fully-qualified) should be treated differently in this gem.

Do you know of instances in which a fully-qualified constant should be handled differently in brakeman?

[presidentbeef]

Coming back to this (sorry for the delay), I can see how that makes sense. In the end, Brakeman isn't really sophisticated enough to do anything different between ::X and X.

[presidentbeef]

I'm really leaning towards either making this entire check optional (non-default) or stop warning about anything that's not string interpolation. It's just becoming too noisy with some of these newer patterns.