Strong Parameters in Rails — down the rabbit hole

It all started with a bug report (doesn’t it always?). A user was complaining that when they passed an incorrect argument to our API by mistake, they didn’t get any error message. Sounds like a quick fix, right?

Wrong.

Some background first, simplified greatly for the purpose of this post. Our application, like many others, has users. A user can belong to multiple organizations. Our REST API allows updating a user by PUTting a JSON object to the relevant endpoint, for example:

{ "user": { "organization_ids": [1,2,3]}}

But our user had a typo. This is the JSON they sent:

{ "user": { "organization_ids": 1}}

It seemed to work fine — they got no error message. Perhaps it worked even too well: When they tried to pass a non-existing organization ID, it still returned a successful response! In fact, whenever they forgot to wrap the ID in an array, that parameter was simply ignored. No error response, no mention in the logs of something being wrong, nothing.

After I reproduced the bug in my development environment, I started digging in. With some help from pry inside the controller, I saw that while the organization_ids parameter was present in the request, it wasn’t there in the user_params object that contains the allowed parameters for assignment. This pointed me in the direction of Strong Parameters.

Strong Parameters is a feature of Rails that prevents assigning request parameters to objects unless they have been explicitly permitted. It has its own DSL (Domain Specific Language, or in other words, a predefined syntax it understands), that allows you to indicate what parameters should be allowed. It also lets you indicate if each parameter should be a hash, array or scalar (i.e. integer, string, etc.), as well as some other functionality that is not relevant for this post.

Here is an excerpt from our definition of permitted parameters in the users controller:

params.permit :name, :login, :mail, :organization_ids => [], ...

Ok, so Strong Parameters expects an array for the :organization_ids parameter, but it gets an integer instead. So why am I not seeing any indication that it failed in the logs?

Time to dig into the Rails code. Good thing it’s Open Source and I can just read it myself! Here is the function that does the actual filtering:

def permit(*filters)
  params = self.class.new

  filters.flatten.each do |filter|
    case filter
    when Symbol, String
      permitted_scalar_filter(params, filter)
    when Hash
      hash_filter(params, filter)
    end
  end

  unpermitted_parameters!(params) if self.class.action_on_unpermitted_parameters

  params.permit!
end

So looks like it does have some sort of handling for the unpermitted parameters… Lets take another step inside the code:

def unpermitted_parameters!(params)
  unpermitted_keys = unpermitted_keys(params)
  if unpermitted_keys.any?
    case self.class.action_on_unpermitted_parameters
    when :log
      name = "unpermitted_parameters.action_controller"
      ActiveSupport::Notifications.instrument(name, keys: unpermitted_keys)
    when :raise
      raise ActionController::UnpermittedParameters.new(unpermitted_keys)
    end
  end
end

But wait, what is this action_on_unpermitted_parameters thing? Turns out this is a configuration option you can define in your application initialization. So I looked in our application.rb and this is what we had defined:

config.action_controller.action_on_unpermitted_parameters = :strict

Well, this explains why we saw nothing! This isn’t actually a value this option expects!

Digging in git history, turns out the :strict option is actually a left-over from Rails 3 days  —  when it was passed to config.active_record.mass_assignment_sanitizer, which was a previous method of sanitizing input that accepted the :strict option. When we implemented Strong Parameters in preparation for the Rails 5 upgrade, this option was renamed correctly, but its value remained the same. Which means that for about a year and half, we were silently dropping all of the filtered parameters instead of raising an exception.

Great! Now all I have to do is replace :strict with :raise, write a little bit of error handling code to give the user a nice friendly message informing them which of the parameters they passed was wrong and why and I’m done! Shouldn’t take too long…

After a couple of hours of work, I realized that due to the way that Strong Parameters is currently implemented, there is no easy way of telling whether the parameter was filtered out because the name wasn’t permitted or because the value wasn’t the correct type (which was the original issue  —  passing an integer when the filter only accepted an array). OK, no worries, for now let’s just display a general message informing which parameter was the problematic one, that should be enough to help the user figure out what’s wrong.

So I added something like this in the base API controller:

rescue_from ActionController::UnpermittedParameters do |error|
  message = "Invalid parameter: %s. " % error.params.to_sentence
  message << 'Please verify that the parameter name is valid and the values are the correct type.'
  render_error 'param_error', :status => :bad_request, :locals => { :exception => error, :message => message }
end

Excellent! Good day’s work, let’s open a PR and see what the CI says!

About an hour later…

(we have an extensive test suite that is run against different Ruby versions and different databases)

Jenkins reports that tests have failed. 636 failed tests to be exact. Yikes! I guess a lot of code was changed in the year-and-half since we made the change to Strong Parameters.

Digging one more level down the Rails code, the unpermitted_keys that raise an exception are calculated this way:

def unpermitted_keys(params)
  keys - params.keys - always_permitted_parameters
end

Luckily, turns out always_permitted_parameters is actually another config option. Despite what its name suggests, it is actually a list of parameters that are filtered out but don’t raise an error when they are. By default, it is set to %w( controller action ), since these two parameters are always sent by Rails, but it can easily be overridden in the application config. This is useful for parameters that you want to pass but don’t need to be assigned to the models —  for example, number of entries to display per page in index views. I added a few common parameters to this list, like so:

config.action_controller.always_permitted_parameters = %w(controller action format page per_page search locale utf8 _method authenticity_token locale _ie_support apiv id)

This got me down to 237 failing tests. Not bad for a quick, one line change.

However, now the failing tests will need individual handling. Some may be easily fixed by adding some more common parameters that I didn’t think about to the list, but some of them are actually broken and have been for a while —  we just didn’t realize it because we were silently filtering out many parameters which may have caused the tests to fail. In the following clean-up, we may even find some new bugs that were missed because of this.

On the positive side, getting this fixed properly will give us several benefits:

1. Our users will receive informative feedback when they send an invalid parameter to the API. Even though at this point we still won’t be able to tell them what’s wrong, they will at least know which parameter(s) are causing issues, and won’t think an action succeeded when in fact some of the parameters were silently filtered out.
2. Developers will have an easier time when debugging. Instead of wasting hours trying to figure out why the parameter they expected wasn’t available in the controller, they will get a clear exception that they can handle.
3. Our tests will be better. Silently failing during tests leads us to an false feeling that everything is fine. Since we have a large test suite with good coverage (sadly not 100% yet), we often rely on tests when doing code review as an indication that new code isn’t introducing a regression.

I think any one of these is in itself worth my effort, not to mention getting all three at once.

My key takeaways from all of this:

• Failing silently is a Bad Idea™. In this case, we missed the fact that after the change to Strong Parameters, the :strict option no longer produces the desired outcome —  raising an error when filtering out parameters.
• Open Source is awesome. Thanks to the fact that the Rails code is open, I was able to figure out the cause of this bug in just a few hours.
• Don’t be afraid to dig in deep. Even if that means going into the code of some of your dependencies or their dependencies. I learned a whole lot in my career from digging deep -  both in terms of understanding our stack better and of reading some great code written by very talented developers. If you find a bug in some dependency’s code — fix it and send a PR! That way everyone will benefit from the fix, and you won’t have to create an ugly workaround in your code.
• Tests are important. Making sure they work the way you expect them to work is even more important. Don’t just check the request completed successfully, check that it did what it was supposed to do.
• Code review is important. If this issue was caught in code review 1.5 years ago, we wouldn’t have to fix hundreds of tests now. Even though the PR did undergo review, it was a huge PR — 257 files changed, which makes sense when changing such a fundamental aspect that touches practically every part of the application. However, such a large amount of changes makes it easy to miss something when reviewing. Whenever possible, small PRs are always better, as the chance that something will be missed is much smaller.
• Good documentation is important, especially if you are writing a library others depend on. Thanks to the Rails docs, I was quickly able to understand what each function does and identify which configuration options I need to set.

Thank you for reading this! I would be happy for any feedback in the comments, or by DM on Twitter.

P.S. Thanks Dafna Rosenblum for writing the post that led me to write this one, about 2 years after I told myself I should start a blog.