Protector not restricting pluck() calls

[mwalsher]

It looks like calls to pluck(), such as...

OrderType.restrict!([current_user, current_account, current_accounts_user, current_organization]).pluck(:name)

... do not trigger protector.

Here is the protect block, for example:

protect do |subjects, table|
  current_user, current_account, current_accounts_user, current_organization = subjects

  puts " >>> Protecting"

  if current_accounts_user
    scope {
      where account_id: current_account.id
    }

    can :view
    can :create
    can :update
    can :destroy
  else
    # Allow nothing
    scope { none }
  end
end

There is no output of ">>> Protecting" and the resulting query is:

SELECT order_types.name FROM order_types

I didn't see any existing issues for this so I'm assuming it hasn't already been fixed, if it is in fact an issue? (mind you I'm using an outdated version of Protector (0.6.1) so it may in fact have been fixed already... hoping so as it's a fairly significant bug otherwise)

Thanks!

[inossidabile]

It's not a bug. Please read README carefully:

The last thing is really important to understand. No matter if you can read a field or not, methods like .pluck are still capable of reading any of your fields and if you tell your model to skip validation it will also skip an ACL check.

[mwalsher]

Yikes. What is the rationale behind this decision? It seems rather dangerous to leave these methods unprotected when the expected behaviour is otherwise. And what other methods are there like this? (I'm assuming there are others since you say "methods like .pluck"?)

At the very least, perhaps there should be a configurable option for something like this.

[inossidabile]

They are any methods making raw SQL queries avoiding the internal data model of ActiveRecord. Pluck is simply a shortcut for making raw selection. Protector doesn't work at this level currently.

We might want to add such option in future. I'm ok to cooperate on such pull request. But on our side we are not going to implement it ourselves until 1.0.

[anazar]

+1 Ran into this issue as well Pluck should definitely use the scopes being applied on the models. Seems pretty risky not to allow restrict in this case.

[anazar]

@inossidabile - any chance of reopening this? I keep running into this issue.

[inossidabile]

No. This is so by design.

[anazar]

Can you explain the reasoning for this? Doesn't make sense to me.

Why should I get different results for the following:

User.restrict!(current_user).select(:id).limit(5)
User.restrict!(current_user).limit(5).pluck(:id)

That seems pretty insecure.

[anazar]

I realize in the readme you mention that pluck can still read fields which is fine... but I think it should still be adding to the active record chain... if you have something like where(client_id: current_user.client_id) that's pretty important and you'd still want that filter to apply so they can't see all users.

[inossidabile]

The logic behind this is that with pluck you directly request a field. Protector should not stay in your way when you explicitly ask for something. It saves you from accidental security issues. Nothing can save you from intentional bugs. The behavior of pluck will remain the same.