Possible false positive Mass Attribute update warning

[presidentbeef]

Possible false positive Mass Attribute update warning

[daveworth]

@presidentbeef and @oreoshake, just as a note, I am not creating patches/pull requests for this (or my previous issue) because I am not positive my report is actually correct. If you think that I'm right and that I should submit a patch I will happily do so!

[presidentbeef]

Yeah, I'm pretty sure this was just me being overzealous.

[daveworth]

Cool. I can remove the checks for #update_attribute (singular ;-)) and create a pull request if you'd like.

[presidentbeef]

Sure...with a test? :D

[daveworth]

Sure... I guess I was kinda sloppy on my annotations branch eh? Hard to test doesn't mean it shouldn't be. 💣

[presidentbeef]

Haha, I wasn't implying anything - I was only thinking we should add a test so in the future it looks deliberate.

[presidentbeef]

If you have any questions about how to go about writing the tests for this, please let me know.

[oreoshake]

I heard my name so I should say something. Yes, removing update_attribute sounds like the right thing and I'm not sure that the bypassing of validations is something brakeman should be concerned with only because I can't come up with a reasonable scenario to warn on without a crazy high FP rate or a lot of configuration. Or maybe I'm misunderstanding the concern. I just assumed you were thinking along the lines of

class Donkey < ActiveRecord::Base
  validates_security_of :secure_stuffs  #made up validator
end

def some_action
   donkey = Donkey.find(params[:id])
   donkey.update_attribute(:secure_stuffs, params[:secure_stuffs]) # DANGER, WILL ROBINSON!
end

[oreoshake]

And the winner for run-on comment of the year goes to...

[daveworth]

I caused commotion without meaning to... by "biggest worry regarding #update_attribute" I meant in general, not in a security context. I recently had to make a decision between querying object.valid? after an update_attribute or changing the call to update_attributes with a hash I entirely controlled (which I chose) because of the unclear semantics of the singular version not using validators... it's just weird.

[presidentbeef]

Closed by #83 - thanks Dave!