Check forgery setting in all direct subclasses of ActionController::Base #858
Conversation
… of ActionController::Base
| :confidence => CONFIDENCE[:high], | ||
| :file => app_controller.file | ||
| if tracker.config.allow_forgery_protection? | ||
| warn :controller => name, |
There was a problem hiding this comment.
I don't think we want to generate this warning for every controller.
There was a problem hiding this comment.
It will only generate a warning for a controller that directly inherits from ActionController::Base. Will that be an issue? I would imagine that there may be apps that has more than one controllers that inherits from ActionController::Base. Like say the ApplicationController and AdminController.
There was a problem hiding this comment.
It's warning about a global setting (that probably no one ever uses), so it doesn't make sense to warn about it for every controller that inherits from ActionController::Base. Actually it doesn't make sense to tie it to a controller at all, but this code was written over five years ago and I didn't know what I was doing.
There was a problem hiding this comment.
I factored out that check from the .each iteration. One side effect of that is that for the rails3.rb test, we prioritize the warning of CVE20110447 over the missing protect_from_forgery call. See https://github.com/presidentbeef/brakeman/pull/858/files#diff-29d5ade8f0be60f47a3073ebab75b8d4L386. Which kind of makes sense in my opinion because there's no point asking the user to add the call to when rails need upgrading or patching in the first place.
|
Closing, will merge with #953 |
This fixes #848. This is the second part to fix #664. Instead of just scanning controllers named
ApplicationController, we scan direct subclasses ofActionController::Baseinstead.Once we have both this PR and #857 merged, we can close #664. 😉