named finders and possible security issues

Hi,

while at a security review we noticed the following design/ security flaw in rails/ activerecord's named finders.

Named finders (ex. find_by_confirmation_token) are used when no or exactly one result is desired. Consider the following code:

account = Account.find_by_confirmation_token(params[:secret_token])

Confirmed accounts have their confirmation_token set to nil. Unconfirmed accounts have some digest there. Now when the controller action is called without secret_token set in the params hash a random confirmed account is returned. This is because there's no check if the underlying query returns more than one row. Of course this is not desired.

To help finding those subtile bugs/ design flaws, I'd suggest to remove the "LIMIT 1" from the generated sql by the named finders and instead raise an exception if the query returns more than one row. Suggestion: "named finders expect the sql to return at most one result, got xx".

This change should not have any backwards compatibility issues because a named finder with a query which selects more than one row is never desired and a sign for bad design.

What do you think?

Corin

This is flatly not true. We use it all the time:

  customer.billings.most_recent_first.find_by_closed_at(nil)

Or more explicitly:

  customer.billings.find_by_closed_at(nil, :order => "read_at DESC")

The behavior that you want is also useful, but is a very different
thing. To handle it, and similar cases when not using dynamic
finders, I added a method #only on enumerable (this predates the arel
method of the same name - I would choose something different now),
which simply gets all results and if the size is more than one, raises
an exception. So in our app we can write:

  customer.billings.find_all_by_closed_at(nil).only

Or without anything to do with dynamic finders:

  customer.billings.all.only

You could easily take this one step further and jack up another
dynamic finder method:

  consumer.billings.find_only_by_closed_at(nil)

But again, I would want to pick another word than 'only' now that Arel
has introduced a different concept by that name.

Regardless, the current find_by behavior is useful and it would not be
correct to change it because there are some times when you want
different behavior.