Require allowlisting the attributes that are mass-assignable, in each action

[sevenseacat]

Is your feature request related to a problem? Please describe.

The current behaviour is to accept any public attributes that exist on the resource, for mass-assigning in actions (as documented in this example - https://ash-hq.org/docs/guides/ash/latest/topics/actions#create-update-destroy-actions). This leads to potential security holes for users, similar to the old optional attr_accessible functionality in Rails - attributes that weren't meant to be public, were accidentally left public and thus could be exploited.

(See http://homakov.blogspot.com/2012/03/how-to.html for a super-brief description for how this was exploited on GitHub - homakov was able to push commits directly to Rails master.)

Describe the solution you'd like

Instead of public attributes being permitted by default, all mass-assignable attributes should be required to be listed in the accept list for an action. This would require users to opt-in to public attributes being mass-assignable.

eg. accept [:name, :email] (but not :admin or any other similar fields, even if they are public)

Additional context

As discussed on Slack/Discord, this could be done as a breaking change in Ash 3.0 or earlier with a config option that users have to specifically enable to get this functionality.

[zachdaniel]

Agreed, that would be a much better default. It can be achieved now on a resource by resource basis using

actions do
  default_accept []
  
  ...
end

But it should be inverted, such that you do

actions do
  default_accept [:some, :basic, :fields] # because the default would then be `[]`
  
  ...
end

What I'd like to do here is provide either:

1. a print out of what the default accept of each resource ought to be based on the old behavior, so that it could be dropped in to each resource

2. a script that just updates your resources w/ the default accept (which you should of course review)

[sevenseacat]

That relies on people knowing to set default_accept though, and ensuring they do it on every resource - and if they don't, the issue remains.

[zachdaniel]

Agreed, my point was that users who want to retain the old behavior (so they can iteratively migrate) won't need to alter every single action to do so, just one line per resource.

[zachdaniel]

Paste the following snippet in iex to find affected resources:

(:my_otp_app
|> Application.get_env(:ash_apis)
|> Enum.flat_map(&Ash.Api.Info.resources/1)
|> Enum.reject(fn resource ->
  resource.spark_dsl_config()
  |> Spark.Dsl.Transformer.get_option([:actions], :default_accept)
end)
|> Enum.map_join("\n", fn resource ->
  path =
    resource.module_info(:compile)
    |> Keyword.get(:source)
    |> Path.relative_to(File.cwd!())

  "#{path} - #{inspect(resource)}"
end)
|> IO.puts())

[zachdaniel]

So, the minimal diff for implementing this can be found here: https://github.com/ash-project/ash/pull/new/default-accept

However, we will need to update all the guides with the new required config (temporarily until we do a 3.0 release) and update any examples of actions to include an accept list, most likely.

[zachdaniel]

This is complete in the 3.0 branch <3