presidentbeef/brakeman — pull request #82 by presidentbeef (2012-04-19 09:00:36+00:00)
Title: Fix mass assignment and SQLi dupes in views

So, here's the story:

### Chapter 1

Sometimes, like in the case of XSS, we don't care where a value originally came from, just where it goes. For example, if a parameter query is stored in a variable in a controller:

```ruby
@user_pref = params[:user][:pref]
```

It doesn't matter how many times that gets used in a view, we want to warn on each of them (assuming unescaped):

```erb
<%= @user_pref %>

<%= @user_pref.some_other_thing %>
```

But for other issues, we only want to warn on the original call, not all of them. Imagine if this were in the controller:

```ruby
@user_pref = User.find(:first, :conditions => "name = '#{params[:name]}'").user_pref
```

In this case, we only want to warn about SQL injection once, in the controller. Not every time `@user_pref` is used!

### Chapter 2

One way of differentiating between the original value and a use of that value in a variable is via `Sexp#original_line`. This value is set when the Sexp is used to replace a variable (or other value). It allows Brakeman to track where the value came from, while not messing up tracking where the value is being used (which is at `Sexp#line`).

The SQL check was already using the `original_line` value to avoid warning on SQL injection in views, and it worked. Most of the time. Not always. But well enough that it fooled most of us.

But while the SQL check maintained its deception, the `original_line` continued to be the prime suspect. It *should* work, so why wasn't it?

### Chapter 3

Checks use a rather hairy method for determining if a warning has already been issued for a piece of code. @oreoshake recently noticed that the mass assignment checks were not using this method. In fact, the original mass assignment check was written before the method even existed, and had not been updated in all this time.

So, as makes sense, he updated it.

Suddenly, a wild continuous integration failure appeared! Tests were broken! The screaming! The *horror!*

### Chapter 4

Sensing a deeper problem, @presidentbeef bravely decided to look into the broken tests. Indeed, it did seem that changing the mass assignment checks to use the proper duplicate detection was causing some strange behavior. But there was no reason why those checks should get special treatment - after all, the SQL check was working, right? (Not really.)

After some time, it was realized that the `original_line` appeared to be at fault. It was clearly *not* being set while it obviously *should* be. Something was amiss. But what? Where? Very little code touched the actual `original_line` value, but TONS of code could disrupt its orderly flow from the AliasProcessor to the checks themselves.

### Chapter 5

Careful stalking of the dangerous `original_line` beast indicated that it was being properly tended for nearly its entire journey...in almost all cases. But the exact case in which it disappeared was puzzling. Was it when the value was used as a parameter? Or only when not output? Or perhaps when methods were called on the value? Different combinations of situations led sometimes to one result, and sometimes to another. Soon all the attempts at divining a single, specific scenario failed.

Failure followed failure, until patiently-laid traps eventually revealed the culprit: *after* the templates are processed to propagate variables and values through them, they are passed once again trough the regular template processing to extract output values. It was at this point that some - not all - calls were repacked in a fresh Sexp, leaving the crucial `original_line` to the ravaging of the garbage collector.

### Conclusion

Order restored, it became apparent this problem had already manifest itself in previous tests, but had been ignored. These tests were quickly righted, and the continuous integration monster would soon be sated.

While it seemed peace had once again come to the Brakeman world, the adventure had also uncovered some unpleasant code which should, someday, be refactored. And so the epic quest for practical static analysis continues...

---
*Sorry, I've been watching Frozen Planet*

:penguin: