false positive when using .order("FIELD(id, ...)")

[devvmh]

I have some code that takes a list of ids from params and uses it to order a query. For instance:

ids = params[:ids].map(&:to_i) # I'm hoping the to_i closes any security holes
@posts = Post.where(id: ids).order("FIELD(id, #{ids.join(', ')})")

I definitely don't like this code, but it's in production so it's gotta stay haha EDIT: this code is growing on me

I originally tried using ActiveRecord::Base.sanitize, but it just puts quotes around the list of ids, leading to a query like this:

SELECT * from posts WHERE id IN (2, 1, 3) ORDER BY FIELD(id, '2, 1, 3')

which will silently fail and produce the wrong order.

The first code block above is my current proposed solution, but brakeman gives me a warning for it. I'm open to any solutions that can preserve the behaviour, but I may just not know enough about SQL. Perhaps there is a solution that can be tailored more towards what brakeman is expecting.

Thanks for looking!

[presidentbeef]

Hi Devin, sorry for the delay.

I think what you have may be the best you can do with ActiveRecord, but I'll take a look.

[presidentbeef]

I should point out your code does still allow determining how many columns are available in the table (sometimes useful) and possibly discovering values of columns via sorting (see http://rails-sqli.org/#order). Probably not a huge concern for a Post model though.

[presidentbeef]

Just kidding, not the value discovery. Just arbitrary column sorting (probably fine) and determining how many columns there are (probably fine for a Post table).

[devvmh]

Now we are writing new code with this pattern, and we have to add it to brakeman.ignore each time. I can see why this allows arbitrary column sorting (in fact, this is the purpose of the code), but I can't see how it lets you determine how many columns are in the table.

I do understand how this code might let an attacker determine how many rows are in the table, which I suppose might be a problem in a different app.

Is there an existing mechanism to handle a case where we want the behaviour to exist, but other apps might not? Or are we stuck using brakeman.ignore forever?

[JasonBarnabe]

In this scenario, you can use this:

@posts = Post.where(id: ids).order(ActiveRecord::Base.send(:sanitize_sql_array, ['FIELD(id, ?)', ids]))

And brakeman's happy with it.

[dvandersluis]

FYI (and not that it's brakeman's fault), but @JasonBarnabe's solution triggers the Rails 6 deprecation warning from https://github.com/rails/rails/issues/32995.

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "FIELD(id, 1,2,3,4,5)". Non-attribute arguments will be disallowed in Rails 6.0. This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql().