AR Serialization shouldn't fall back to base opts for includes #1344

merged 2 commits into from May 26, 2011


None yet
2 participants

jmileham commented May 26, 2011

It seems wrong that when I do the following:

User.first.as_json(:only => :first_name, :include => :posts)

that :only => :first_name will apply to to :posts as well.

The present behavior has been around essentially forever, and the current nuances of the behavior arrived in two phases, apparently initially to preserve backward compatibility when it became possible to specify opts for includes in the first place, and then when app developers got the ability to override to_xml on their models.

As far as I can tell it was just initially decided that passing the base's options along to the include is better than having the included model be serialized in an unconstrained way, but the remote likelihood that the same column names are likely to usefully exist on the two models makes me think that's worth reconsidering.

The current behavior isn't covered by tests, so I'm hoping nobody is intentionally relying on it. That said, because this patch is less restrictive than the old approach, this could cause security issues for anybody who was relying on it unwittingly (included models without explicit options hashes would start dumping all attributes out over JSON and XML REST APIs instead of the seemingly random intersection of their column and method names with those from the base opts). That may make this pull request a no go until AR (or really, AM) serialization has a mandatory security layer analogous to attr_accessible (mandatory may be far-fetched given the cool reception to mandatory attr_accessible, but I'd be in favor of it), or this behavior change comes with a configuration option that must be turned on consciously by an upgrading app maintainer in order to take effect.

I'm working on a generic AM serialization security layer for a project (and this patch is a prerequisite for it, in fact -- otherwise it's challenging to keep opts on base from trickling down to included models). If there's interest in pulling that into core, I'd be happy to clean it up and submit. Especially if it would make this patch more palatable.

Thanks for considering.

josevalim merged commit d341d16 into rails:master May 26, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment