Support for extra whitelisted params

[naps62]

I'm building an app based on both suspenders and administrate.
This means I have config.action_controller.action_on_unpermitted_parameters = :raise, and I'd like to keep it that way

Another thing I have is the administrate panel under a subdomain:

constraints subdomain: "admin" do
  scope module: "admin", as: "admin" do
    resources :sessions, only: [:new, :create]

    # ... admin dashboard routes here
  end
end

This creates an unfortunate combination, where subdomain: "admin" is automatically added to the list of params, and the app later fails because administrate's own functions sanitized_order_params and clear_search_params don't expect extra params to be there, causing an exception

So far, I had to override those methods on Admin::ApplicationController:

    include Admin::ApplicationHelper
    helper_method :clear_search_params, :sanitized_order_params

    # Overriden from administrate's original source code
    # to include :subdomain param in exception list
    def sanitized_order_params(page, current_field_name)
      collection_names = page.item_includes + [current_field_name]
      association_params = collection_names.map { |assoc_name|
        {assoc_name => %i[order direction page per_page]}
      }
      params.except(:subdomain).permit(:search, :id, :page, :per_page, association_params)
    end

    # Overriden from administrate's original source code
    # to include :subdomain param in exception list
    def clear_search_params
      params.except(:search, :page, :subdomain).permit(
        :per_page, resource_name => %i[order direction]
      )
    end

But I'm guessing a good solution would be to add a configurable list of permitted params, and have those two functions take that into account.
Or is there a better solution I might be missing?

PS: I'm willing to implement this myself, it's fairly straightforward. But I wanted feedback on the idea first

[nickcharlton]

Oh, interesting! I don't think I've tried a subdomain for many years.

I wonder if a configurable list is the best option here. Would you be able to provide an example of what that might look like? It'd be easier to think about with a sample.

[naps62]

@nickcharlton I actually had a 2nd possible solution that I came up with in the meantime. So I'll leave both here

1 configurable list

My thinking would be something along these lines:

class Administrate::ApplicationController

  protected

  def ignored_params
    []
  end
end

naming up for debate of course

This would be the default implemented by the gem, which would give me the ability to override that method to return [:subdomain]
That array would have to be excluded via params.except on the two calls I've overriden above

2 Don't use strong parameters at all

I was diving a bit more into administrate's code, and noticed that, while the strong_parameters API is used internally only within those 2 functions, the plain version (weak parameters?) is used in several places, to refer to params[:id], params[:per_page] and so on

So it actually looks to me like these functions are using strong_parameters, not for the security benefits, but for automatically filtering only the params they're interested in.

So this 2nd proposal would be to refactor those two functions to not use strong_parameters, but instead use Enumerable#reject / Enumerable#select to achieve the filtering

This would both fix the original issue, as well as make the gem's code more consistent, and compatible with suspenders' defaults (which, with both being thoughtbot projects, I imagine is something you'd want)

Thoughts?

[pablobm]

Removing strong parameters does have some merit here, after looking at the code. I'm not seeing any obvious security issues if done correctly, and I'd prefer it over adding new interfaces. If only strong parameters had #weak_permit (or something) method that didn't raise exceptions...

[nickcharlton]

I'd support the idea of removing strong parameters as the right solution here. Is this something you might be able to take a look at, @naps62?

[naps62]

I'm not actively using administrate anymore, but this is simple enough. I can give it a shot 👍

[naps62]

@nickcharlton After going a bit through the code, and the documentation for ActionController::Parameters I'm a bit less confident about how easy & ergonomic it would be to remove strong parameters

Taking one example:

    def sanitized_order_params(page, current_field_name)
      collection_names = page.item_includes + [current_field_name]
      association_params = collection_names.map do |assoc_name|
        { assoc_name => %i[order direction page per_page] }
      end
      params.permit(:search, :id, :_page, :per_page, association_params)
    end

We don't want to just revert to using raw params, as that would have the original security implications.
But we can't also do a simple params.slice(:search_id, ...) because of the nesting with association_params

so the "easy" way to get the same behaviour, but still allow for the possibility of extra params, would be the weird:

permitted = [:search, :id, :_page, :per_page, association_params]

# take out the extra params before the #permit call
params.slice(*permitted).permit(*permitted)

Which looks... weird, to say the least

Thoughts?

[nickcharlton]

Thanks for looking into this! Yeah, agreed. It does look a little weird.

I think my biggest concern here is opening up ourselves to a security vulnerability because we’re needing to work around something Rails does for us — which I’d expect an explicit allow list wouldn’t have (unless, I suppose, you allowed something you shouldn’t, but that’s on the user).

For your original problem, would setting that sub domain parameter allowed on Administrate’s ApplicationController solved your problem?

[naps62]

I think it would yes

[pablobm]

Sounds good to me. Want to provide a PR?

[blocknotes]

Does someone have news about this feature request? 🙏

I'm in a similar situation: I have an Administrate plugin to use Ransack for some extra filters, but it requires a q parameter in the query to work.

So as a workaround I have to override the sanitized_order_params helper method in order to have the column header sorting work.

I also think that removing strong parameters from index page queries looks like a good improvement.

[pablobm]

There may be a way to make this work while the "proper" feature is being developed. I just had to deal with it myself, and it may be of use to others.

I had a param special-order, and this was raising an error on unpermitted params. The way I solved it was by reading the param in a before_action, and deleting it from the params hash:

class Admin::ApplicationController < Administrate::ApplicationController
  # ...

  before_action :read_special_order

  def read_special_order
    @special_order = params.key?("special-order")
    params.delete("special-order")
  end
end

Would this solve your problem?

[blocknotes]

@pablobm: nice idea, thanks! 👍

[blocknotes]

Nevermind, in my case removing the query parameter in before_action in not optimal because in this way the header column sort links will miss my custom parameter (and lose the search filters).

[pablobm]

@blocknotes - It looks like the progress of that PR has stalled. Would you be able to provide your own one?