Question about the index policy in Pundit and its impact on authorization

[tagliala]

Hello,

apologies for the long post, I hope that this is clear

The (potential) problem

We've encountered a "problem" in our project related to the index policy implementation that might lead to unauthorized access and information disclosure. In our policies, we typically have:

def index
  true
end

to override the false value from the default policy template

And in our controllers we verify the policies like this:

after_action :verify_authorized, except: :index
after_action :verify_policy_scoped, only: :index

This setup means that we are not verifying that index is being authorized, which could lead to information disclosure. From the Pundit documentation, we see examples suggesting similar configurations:

class ApplicationController < ActionController::Base
  include Pundit::Authorization
  after_action :verify_pundit_authorization

  def verify_pundit_authorization
    if action_name == "index"
      verify_policy_scoped
    else
      verify_authorized
    end
  end
end

which is different from what we had before, but it has the same effect: the index policy is not being verified

The documentation states:

This is mostly useful for controller actions like index which find collections with a scope and don't authorize individual instances.

However, this can be confusing and may result in implementing insecure policies in controllers. For instance, consider the following situation:

def index
  admin?
end

def destroy
  record.draft?
end

Using the suggested approach in the controller, the policy unit specs might pass, but in reality:

1. We do not verify that the user is an admin, allowing non-admins to access the index action if authorize is not called.
2. We do not verify the policy scope on destroy, permitting users to destroy drafts from other users if the policy scope is not called.

In most cases, we want to verify the policy scope and authorization everywhere. Currently, most of our index methods are set to true, for documentation rather than functionality or security

Questions

• Should the index? action be removed from the default application policy template?
• Should documentation be improved to clarify potential issues with the index action?

[orbanbotond]

It depends how the responsability of pundit policy class is defined by team/community.

If We define the responsibility of pundit policy classes as being responsible to decide if the passed user has controll over the passed record then:

IMO:
Having index? in the policy classes is bad by design. Becase we are not dealing with any record, but rather a Relation. Therefore this method should be dropped. If anybody tries to check the users access to the index action then that is a mix of responsabilities according to the above (hypothetical/fictional) responsbility, therefore SRP is violated/mixed.

As for destroy? pundit method:
I would implement it like this:

def destroy?
   record.owned_by(user) && record.draft?

Also if the team/community can't define/communicate the responsibility of the pundit class then a bit of more though/attention is needed to make a bit more clear Objects with clear Responsibility.