presidentbeef/brakeman — pull request #440 by presidentbeef (2014-02-05 03:20:14+00:00)
Title: Better handling of SQL strings and fewer false positives

This branch improves the detection of "safe" SQL string building.

For example, this:
```ruby
find_by_sql("SELECT * FROM stuff WHERE thing = " + self.class.sanitize_sql(thing))
```
would have generated a warning which indicated `"SELECT * FROM stuff WHERE thing = " + self.class.sanitize_sql(thing)` was a dangerous value. Now it will not warn at all.

As another example, code like this
```ruby
options = {}

if params[:sort_order] == 'ascending'
sort_order = 'ASC'
else
sort_order = 'DESC'
end

options[:order] = 'updated_at ' + sort_order
Test.all(options)
```
would create a warning about `("updated_at " + ("ASC" or "DESC"))`. Now Brakeman will recognize that these are all just strings and not warn.

Many warnings will also just be more accurate:
```ruby
query = "SELECT sum(stuff) " +
"FROM (SELECT other_stuff FROM #{table} WHERE id = #{id}) " +
"AS item, bgs " +
"WHERE ST_Contains(item.geometry, bgs.the_point);"

Test.find_by_sql(query)
```
This used to warn on the entire query! Now it will just warn about `table`.

`#to_s` calls are ignored now and their targets considered instead. Even interpolated `#to_s` calls which drive me nuts.

Additionally, Brakeman shouldn't warn about values which are method calls ending in `_id`, since those generally refer to foreign keys. Note, however, that local variables ending in `_id` will still produce warnings.

In general, fingerprints should not change for existing warnings, since the `user_input` value is not included in the fingerprint.

Maybe one day someone will clean up `CheckSQL`...it's a real mess.