Respect the StrongParameters permitted flag in the 4.0.0 allow/deny listing

[stillhart]

This issue is regarding #1273 and #1400 and #authorization-allowlistingdenylisting

I just run into the issue/topic of allow/deny lists during the 4.0.0 (#1402) upgrade of ransack. I really wonder why this change was made, and I'm slightly unhappy. Aren't we supposed to use StrongParameters in order to filter ransack search queries? Or is that not enough and I am missing something?

I always used something in the below fashion in each controller. Moving this logic into the controller also seems to be the right place, since it can be customized depending on the action. Passing params directly into anything is considered bad practice in my world view. Now I have to add this whitelist to each model, which feels like the wrong place to me. The controller also contains the user, which would avoid passing around the authorization object out of it's scope into each model.

class ThingsController < ApplicationController
  def index
    # Good
    @q = Things.all.joins(:user).ransack(search_params[:q])
    # Bad
    @q = Things.all.joins(:user).ransack(params[:q])
    ...
  end
  private
  def search_params
    params.permit(:page, :per, q: [:name_or_description_or_user_uid_or_id_eq, :name_or_description_cont, :user_uid_cont])
  end
end

I do get the security case for these allow/deny lists, and that's why I choose StrongParameters to do the job for me.
In this context, I personally would prefer if these new allow/deny lists would first respect whether the params are permitted. If not, apply the accept/deny lists, if yes, don't apply them.
#<ActionController::Parameters ... permitted: true>

As said, maybe I'm missing something fundamental and I'm happy to discuss this.

[deivid-rodriguez]

Interesting, I will give that some thought!

[acflint]

I think that's a great idea. It reinforces the existing best practice of using StrongParameters, and puts all of the parameter security in one place for easy visibility.

One questions I'm not totally clear about: Would you have to permit params for each specific type of query, or could you permit the attribute over all? For example, could params.permit(:page, :per, q: [:name, :description, :user_uid]) allow any type of search (contains, equals, etc.) on the name and description attributes, as well as the associated user.uid attribute?

Unless I'm overlooking something (totally possible), I don't think there's any reason to limit HOW you can query each attribute within ransack. The options will be limited by the available ransack query types already.

[lukas-eu]

I like about the approach here that there is more fine-grained control over which combinations of attributes, associations, and search matchers are available per endpoint.

However, the solution implemented in #1400 seems better equipped to detect when an explicit allow list has been set and forcing the developer to define one if absent.
I might be missing something, but I don't see an elegant way to replicate this kind of behavior with the approach outlined here without making strong assumptions about where the query parameters come from and what part of them is user input

• Some applications might use dynamically created hashes as query parameters that are not or only indirectly linked to user input
• Not every application uses the default q GET parameter to collect the relevant user input

[priit]

By the way, we have already been there in Rails ecosystem where strong attributes where defined at model level. There was a real reason why today we define strong attributes at controller level. Same logic applies to Ransack.

Let me explain. When we have superuser controller/UI where we allow superuser to search and sort many attributes, then at the same time we don't want similar advance UI for other users. Current situation is even worse for associations, where it leaks all association allowed attributes even when some controller/UI it's allowed to access only one or two attributes.

Thus it's not a good idea to define attributes/shorting at model level but it belongs to controller level (and/or defined by view helper).

[iainbeeston]

Could ransack check whether it has been given strong parameters and if those attributes have been permitted, then fall back to the allow listing on the model if not? Something like what rails does:

      def sanitize_for_mass_assignment(attributes)
        if attributes.respond_to?(:permitted?)
          raise ActiveModel::ForbiddenAttributesError if !attributes.permitted?
          attributes.to_h
        else
          attributes
        end
      end

Even if not it'd be good to have a configuration option to turn off the new behaviour for those of us using strong parameters

[jrochkind]

I agree that if ransack IS going to require this, I would have much preferred something at the controller-level than the model level. I don't like embedding this kind of logic in the model.

At this point 4.0 has been released though, so I'm not sure to what extent you want to change things without a 5.0.

[eric-hemasystems]

Just putting my vote in for this. I was basically already doing this:

def search_params
  params.slice *%i[updated_at_gteq expiration_at_gteq expiration_at_lteq status_eq search]
end

Then my action called ransack search_params (didn't use strong params because I didn't need nested objects since my params were not namespaced but if it would auto-whitelist me and I not need to add anything to my models I would be happy to use strong params).

[ohbarye]

📝 Just a note for those stuck with v3 with this issue. It may be possible to upgrade your ransack to v4 with keeping the v3's behavior by adding ransackable_attributes and ransackable_associations to app/models/application_record.rb. This is the simplest way that I can think of for now.

class ApplicationRecord < ActiveRecord::Base
  primary_abstract_class

  def self.ransackable_attributes(auth_object = nil)
    column_names + _ransackers.keys
  end

  def self.ransackable_associations(auth_object = nil)
    reflect_on_all_associations.map { |a| a.name.to_s }
  end
end

[oskarkook]

Another approach is to use the existing methods in ransack:

  def self.ransackable_attributes(_auth_object = nil)
    authorizable_ransackable_attributes
  end

  def self.ransackable_associations(_auth_object = nil)
    authorizable_ransackable_associations
  end

[Augustin-Grenne]

I'd like to express my support for the idea to use strong params as an alternative to a separate whitelist in the model. As @priit explained the model certainly doesn't feel like the right place for this. Aside from the fact that, upgrading to 4.x, requires us to put this two methods, with slightly different sets of attributes each time, in very many models, it also does not cover all our use cases. For instance, we make certain attributes of Blobs searchable/sortable with ransacker, effectively requiring us to alter ActiveStorage::Blob. Meaning also the otherwise fine workarounds by @ohbarye / @oskarkook, of putting a catch-all whitelist in an ApplicationController is insufficient. This initially lead us to adopt a patch approach as described in this fine article: https://gorails.com/blog/patching-models-for-ransack-4-0-0-extending-activerecord-for-gem-compatibility

# config > initializers > ransack_patch.rb

module RansackPatch
  def ransackable_attributes(auth_object = nil)
    authorizable_ransackable_attributes
  end

  def ransackable_associations(auth_object = nil)
    authorizable_ransackable_associations
  end
end

ActiveSupport.on_load(:active_record) do
  extend RansackPatch
end

Which worked fine for a few weeks but now, only in production and only periodically users seem to encounter the raised 'attributes need to be explicitly allowlisted' error. It's unclear to us, and possibly unrelated to ransack, why on most request it works fine and then sometimes it just doesn't. Since we were unable to consistently reproduce the issue and patch the patch. Our only option now is to downgrade ransack to 3.2.1 and lock it at that version. Something I hate to do but, we've already spent too much time on this breaking change.