Strong_parameters is borked

Development
1

Hello,
Following up on a bug, I found out that our current handling of strong_parameters is seriously flawed. Currently, if a request parameter is filtered out, there is no indication that it happened (except in the log only in production). This causes three significant issues:

  1. Users using the API who mistype a parameter name or mistakenly pass a value of the wrong type (e.g. integer when array was expected) assume their command was successful, when in fact, the parameter was filtered out completely leading to unexpected results which are hard to debug (no indication of the wrong parameter other than an easily missed message in the log)
  2. Developers wasting hours and days attempting to debug issues without realizing that a parameter they were expecting was filtered out with no indication at all, since in development environment we don’t even log filtered parameters. Based on True Story™.
  3. Tests passing when they should have failed since some of the parameters were filtered and not applied.

To mitigate this, I have started working on changing this behavior to raise an error when a parameter is filtered, and present a user-friendly message indicating what went wrong:

While this is still WIP, this has caused many (547) tests to fail - some of them which should have failed already, others due to various request params that are being passed in but not permitted. I will continue fixing all our tests to handle this.
However, once this is merged in core, all plugins may need to make some other changes as well. Plugin maintainers might be able to proactively fix their tests or filters by setting config.action_controller.action_on_unpermitted_parameters = :raise and seeing what requests break.
At this time, we sadly don’t have the possibility of indicating whether the failure was caused by bad name or value due to the way ActionController does the filtering. This might be possible to handle in core in a follow-up PR in the future, or by changing it in Rails.

3 Likes
2

This topic has come up often in the past. The basic premise is, “As an API user, I can PUT a modified GET json.” There are variations on this such as angular.js sending the entire json back with an update. I’m not against the idea at all and would certainly welcome the enforcing if it was feasible.

With the recent discussions about a v3 API, perhaps this should be added to the requirements.

3

For those interested in a more in-depth dive into the details, I just published a blog post about it - https://medium.com/@tbrisker/strong-parameters-in-rails-down-the-rabbit-hole-2426d331625

5 Likes
4

It took a week, but core PR tests are now green.
Plugin maintainers, please run your tests with it to see what changes need to be made in the plugins. I’d be happy to help, but I won’t be able to fix all plugins on my own.

1 Like
5

BTW, what’s the status here, especially regarding plugins? I’m still seeing this as a 1.18 candiate (or, when that’s unwanted at least as mergeable to develop now, as 1.18 is branched).

6

A large part of this (the non-breaking changes) was already merged, but this will not get into 1.18 as it will break lots of plugins. I am trying to get this in mergable condition so we can start early in the 1.19 cycle on fixing all the related changes this will need in time.

2 Likes