check for potential HTTP verb confusion warns for `unless request.get?`

[afdev82]

Background

I'm writing because I get a warning related to the new check for potential HTTP verb confusion (#1432).

Brakeman version: 5.0.0 Rails version: 5.2.4.4 Ruby version: 2.7.2

False Positive

Full warning from Brakeman:

Confidence: Weak Category: HTTP Verb Confusion Message: Potential HTTP verb confusion. HEAD is routed like GET but request.get? will return false Code: return unless request.get? File: app/controllers/application_controller.rb Line: 65

Relevant code:

def store_location
    return unless request.get?
    ...
end

Why might this be a false positive? Because it's a guard clause, the code after that gets executed only for get requests, so it should be safe. I think the rule should consider that and ignore it.

[presidentbeef]

Agree. This specific case will be easy to address.

[marvinthepa]

Two very similar false positives:

return super if request.head?
return super if request.get?

and

return super if request.get?
return super if request.head?

should be fine IMHO. @presidentbeef do you want an extra issue for that?

Edit:

return super if request.get? || request.head?

works fine and does the same.

[aakindur]

@afdev82 We are going through the brakeman ignore file, and noticed that this entry is still present. Are we still ignoring this vulnerability finding, and if so, we’d need to document the exception. Can you please provide (or point us in the right direction) information detailing why this exception is needed? Otherwise, we will plan to remove the scanning exception form the brakeman ignore file

[afdev82]

The exception is needed because the code following the line return unless request.get? is safe, because it gets executed only for GET requests, since request.get? is TRUE only for GET requests and FALSE otherwise. But the check doesn't consider this case and also the others in the previous comments, so the check needs to be ignored.