Dangerous query method deprecation warning on Rails 5.2.3

Question

I am in the process of upgrading my Rails app to 5.2.3

I am using the following code in my app.

MyModel.order('LOWER(name) ASC')

It raises the following deprecation warning:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "LOWER(name)". 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()

I have changed the above as the deprecation warning recommends and the warning gone away:

MyModel.order(Arel.sql('LOWER(name) ASC'))

I have surfed about related discussion here. It seems this change is introduced to disallow the SQL injections.

But the order clause LOWER(name) ASC doesn't contains any user input. Why this ordering is considered as insecure? Is this the intended behavior or Am I missing anything here?

Answer 1

This is intended behavior, you linked to right discussion and that is exactly what it is. I can re-elaborate it a bit more though so it is easy to understand.

First, re-explaining sql injection, just for reference, doing this:

MyModel.order('LOWER(name) ASC')

Means people can pass any arbitrary string in the order function, this string might contain column names and/or order type input from user.

Now lets say, there is a dropdown in your web app, where user selects column and another one where user selects desc or asc and it submits. the data.

On the controller action what one might be doing is:

order_sql = "#{params[:column_name]} #{params[:column_order]}"

This is exactly where sql injection can take place, a malicious user can edit the form submission data and instead of sending asc or desc in column_order param, he can send some sql script something like: asc; delete from table_name_user_guessed_or_knows causing SQL injections, this is why rails want users to be cautious when using sql in order functions. And allow specifically the safe sql with user of Arel.

Now the second part, why name asc is allowed as input and LOWER(name) asc is not?

Deprecation warning reads:

DEPRECATION WARNING: Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "LOWER(name) asc". 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()

Focus on the words: non-attribute argument(s), non attribute arguments is anything which is not attribute, be it any extra sql appended at end for an SQL injection or be it some method call on the attribute, because methods calls can also be used to alter intended behavior of the SQL.

Next, you asked:

the order clause LOWER(name) ASC doesn't contains any user input

Rails simply has no way to know how a string got formed, it only knows it's a string being passed. Thats why it complains and want developers to be cautious.

This is why name asc is allowed, because it is simple attribute argument. While LOWER(name) asc is throwing warning because its not simple attribute argument, there is a method call on this argument which can potentially be used for SQL injection.
(Obviously an attacker wont probably use simple LOWER function for attacks, but rather he will use some special functions, maybe one he defined with similar injection approach in some previous or even same call himself).