Grape and Protecting Against Mass Assignment Abuse

[jonallured]

tl;dr: it's too easy to write Grape endpoints that are not protected against Mass Assignment abuse.

I've created an example Rails application called PretendGravity, an homage to the main API server at Artsy called simply Gravity. I added a model called Artwork and then implemented CRUD endpoints without using Grape - just plain old Rails controllers. Then I installed Grape on the project and implemented those same CRUD operations using a Grape endpoint. I wrote request specs to verify that the two sets of endpoints behave the same way.

Then I created a PR that implements something new - adds a new field to the Artwork model that should not be allowed to be set via the API. I used this PR to demonstrate that it's too easy for a Grape user to write endpoint code that is not protected from Mass Assignment abuse. The last commit on that PR adds a failing test that shows the abuse in action. That same type of test was written on the Rails side and passes because over there we have protection via Strong Params from Rails.

Then I wrote a series of mitigations that might be attempted.

Mitigation One: use except

The first mitigation I attempted was using the except method that removes a key from a hash. This passes the failing test but I'm not a fan because:

• every new field that's added would need to be removed
• an engineer would have to know that they have to use this method to keep their params safe

Mitigation Two: use declared

The second mitigation uses a helper from Grape called declared. There were two steps here:

• specify the params for the create and update endpoints
• wrap the params with the declared method and specify include_missing as false

This approach filters out params that are unknown and passes our failing tests. Even better it's future proof in this endpoint so that should a field be added in the future then it does not have to be manually added to the list of exceptions like in the first mitigation.

Still, as in the first mitigation, an engineer would have to know that they should use declared and future endpoints have no build-in protections. This one is better than the last mitigation but imho is still insufficient.

Mitigation Three: use Strong Params

The third mitigation brings patterns from Rails into Grape by using Strong Params and an artwork_params helper to pass the failing specs.

Failing tests are back to passing but we still have only addressed this particular endpoint and haven't done enough to protect our future selves from making similar mistakes.

Mitigation Four: set Strong Params as param_builder

The fourth mitigation takes the idea of using Strong Params a step further. Using the param_builder configuration option of Grape I created a shim module that turns all params objects into ActionController::Parameters objects. I still had to add a helper for artwork_params and call permit with the allowed params as both the third mitigation and Rails controller must.

What seems better here is that our current endpoint and all future endpoints now benefit from the built-in protection offered by Strong Params. It's not just the filtering of params, it's also that if used incorrectly an exception is raised:

params = ActionController::Parameters.new({})
artwork = Artwork.create(params)
# => ActiveModel::ForbiddenAttributesError raised!

Proposal for Adding Strong Params to Grape

So this was all in service of this: I propose we add Strong Params to Grape. There are already 3 options so there are patterns we could follow. Docs could be updated and should probably have a section about Mass Assignment and the stance of the Grape project. I think this would be a great first step in improving protections for Mass Assignment abuse.

I would take it further though. I would propose that eventually Strong Params should be the default and then even remove the other options altogether. Maybe it happens over the course of a few major releases to ease the upgrade pain but I believe it should happen. Rails had to go through this pain but now lots of apps are much more secure than they were before and I think Grape should have this reckoning too.

Am I Missing Something?

I've looked at this every which way I could think of but maybe I'm missing something - are there other mitigations that I'm not thinking of? Is there a better way to address Mass Assignment abuse in Grape? How would you tackle this one?

And it's possible that adding Strong Params to Grape has already been discussed - let me know if there's something I can catch up on! Ultimately what I'm after is better Mass Assignment protection in Grape. I believe Strong Params is the right way to address this but def open to hearing more.

[dblock]

First, thank you for writing such a detailed and thorough overview of the problem you’re trying to solve, and code that demonstrates the issue so succinctly.

What would you like the DSL for strong parameters to be?

I think the proposal in #810 is one option.

[jonallured]

Ok great! Glad to hear that there's some interest in these ideas. And thanks for mentioning #810, that helped me find #1603, #1609, and #2358 which in turn helped me find these PRs:

• added undeclared params validator #810 #1158 by @hobofan in 2015
• Add validator 'declared_only' to reject undeclared params #1606 by @jlfaber in 2017
• Add validator 'declared_only' to reject undeclared params #1729 by @seanhandley in 2018

Bummer that these efforts fizzled. There's some variance but generally speaking these approaches look something like this:

params undeclared: :error do
  optional :title, type: String
end

Also supported was a :warn option there in case you didn't want to fail the api call. There were also some ideas of using a method like declared_only but I sorta like how they evolved into an option to params instead. Either way, these approaches are close but not quite what I had in mind.

What I feel is lacking from these approaches are:

• I don't want to fail the api call if undeclared params are passed in
• I do want to raise an exception while an engineer is writing an endpoint with unsafe params
• I want to have a place to define which params are able to be passed to the model

Mitigation Five: add Strong Params to Grape on a fork

After reading and poking around Grape a bit more it occurred to me that I could probably add Strong Params myself pretty easily. So Mitigation Five is done with a branch on my fork. All I did was follow the patterns I saw and:

• create an extension module that implements build_params
• update the main grape.rb file to autoload that new extension
• update the gemspec to add actionpack as a dependency

I'm unsure about that last bit - is adding another piece of Rails to Grape frowned upon?

Spiking on Permitted Params

From here I did a little spiking that I also wanted to share. If I have Strong Params how would I really want to use it? I think I'd want to do something to dry up the list of permitted params and the definition of the params that are safe to use.

So I started by moving the list of permitted params to the model with jonallured/pretend_gravity@a838d97 and then extracted a base class to help with defining a Grape helper method with jonallured/pretend_gravity@c099992. These ideas are only half-baked but maybe help express the direction I'm trying to head.

What do you think? How would you address this stuff? Very open to other thoughts!

[dchandekstark]

Based on my level of understanding of this issue, I am not convinced that Strong Params is the best answer for Grape, for these reasons:

• The ability to enforce declared globally on params could be explored further.
• Strong Params would add another gem dependency, which could require more frequent patching of Grape.
• Some of the benefits depend on using Rails models (AR/AM).
• Some benefits for Rails models could be achieved by implementing permitted? on params (false) and the hash returned by declared (true).

[dblock]

@dchandekstark We added contract support in #2386. Is that a workable alternative?

[dchandekstark]

@dblock I hadn't seen that, but it looks promising, and is perhaps another point against the current proposal -- at least insofar as it wants to replace current features with strong params.

[dblock]

@dchandekstark we haven't shipped the contract support yet (2.1.0), will you give it a try? would love your opinion

[dchandekstark]

@dchandekstark we haven't shipped the contract support yet (2.1.0), will you give it a try? would love your opinion

Yes, I would like to try the contract approach, mainly due to the nicer encapsulation as compared to the default params. I am wondering if this statement in the doc is accurate though, because, if so, I don't understand it (I am not that familiar with dry-schema, however):

"The latter will define a coercing schema (Dry::Schema.Params). When using the former approach, it's up to you to decide whether the input will need coercing."

[dblock]

@jcagarcia ^