SQL quoting for :order parameter


#1

When I construct a query using variables I do something like this:

@users = User.find(:all, :conditions => [“location = ?”, location])

I would like to do the same sort of thing in the :order parameter.

@users = User.find(:all, :order => ["? ASC", location])

When I try this though I get a SQL error and I see that the items in the
array were concatenated together and ?s weren’t replaced.

What is the best way to handle this?

Thanks,
Justin


#2

Justin J. wrote:

When I construct a query using variables I do something like this:

@users = User.find(:all, :conditions => [“location = ?”, location])

I would like to do the same sort of thing in the :order parameter.

@users = User.find(:all, :order => ["? ASC", location])

Ordering by a constant makes no sense. I’m pretty sure you just want

    :order => 'location'

What you wrote would get a weird query like:

    SELECT * from tab_name ORDER BY 'some_constant'

#3

On Thu, Feb 23, 2006 at 11:22:29AM -0600, Justin J. wrote:

When I construct a query using variables I do something like this:

@users = User.find(:all, :conditions => [“location = ?”, location])

I would like to do the same sort of thing in the :order parameter.

@users = User.find(:all, :order => ["? ASC", location])

Have you tried this?

@users = User.find :all, :order => “#{location} ASC”

As was pointed out previously you’ll want to make sure that
location is the name of a column in the users table or you’ll
still get a SQL error (or worse).

-steve


#4

On 2/23/06, Steve P. removed_email_address@domain.invalid wrote:

Have you tried this?

@users = User.find :all, :order => “#{location} ASC”

As was pointed out previously you’ll want to make sure that
location is the name of a column in the users table or you’ll
still get a SQL error (or worse).

Yes that works, but my understanding is that this is a SQL injection
risk,
which is why I wanted to use variable binding with a ? in the string.


#5

On 2/23/06, Jeremy E. removed_email_address@domain.invalid wrote:

comparing it against known good values:

:order=>(ModelName.column_names.include?(location) ? “#{location} ASC” :
nil)

This approach works for me. Thanks much.


#6

On Thu, Feb 23, 2006 at 02:51:48PM -0600, Justin J. wrote:

Yes that works, but my understanding is that this is a SQL injection risk,
which is why I wanted to use variable binding with a ? in the string.

Indeed it is if you can’t trust the source of location’s value.
At the very least you have to ‘quote’ its value otherwise you’ll
be wide open to attack.

-steve


#7

On 2/23/06, Steve P. removed_email_address@domain.invalid wrote:

On Thu, Feb 23, 2006 at 02:51:48PM -0600, Justin J. wrote:

Yes that works, but my understanding is that this is a SQL injection
risk,
which is why I wanted to use variable binding with a ? in the string.

Indeed it is if you can’t trust the source of location’s value.
At the very least you have to ‘quote’ its value otherwise you’ll
be wide open to attack.

I should have clarified in my original post. The value of location is
set
by a paramater on the URL. That is why I wanted to quote it to begin
with.
But it does make more sense to just do an explicit check against the
table
columns before including it. No need for quoting.

Thanks everyone.


#8

On 2/23/06, Justin J. removed_email_address@domain.invalid wrote:

Yes that works, but my understanding is that this is a SQL injection risk,
which is why I wanted to use variable binding with a ? in the string.

SQL Injection is mitigating with ? by appropriately quoting input
values. You are trying to substitute and attribute name (not an
attribute value), which will be unquoted, so using ? doesn’t make
sense. Instead, you should check that your location is valid by
comparing it against known good values:

:order=>(ModelName.column_names.include?(location) ? “#{location} ASC” :
nil)


#9

Take a look at this, I think it adds some functionality that you might
find handy.

http://tech.rufy.com/articles/2005/07/06/
hash-to_sql-hash-to_conditions-hash-to_named_conditions-oh-my

_Kevin