codebrew / backbone-rails

Easily use backbone.js with rails 3.1
MIT License
1.62k stars 255 forks source link

Overriding Backbone.sync breaks Rails' strong parameters #188

Open jarrett opened 9 years ago

jarrett commented 9 years ago

Line 19 in backbone_rails_sync.js reads:

data = JSON.stringify(options.attrs || model.toJSON(options));

It looks like this variable is always overwritten in the next few lines:

if (model.paramRoot) {
  data = {};
  data[model.paramRoot] = model.toJSON(options);
} else {
  data = model.toJSON();
}

So line 19 seems to do nothing. If I'm correct, It's a few CPU cycles gone to waste.

More importantly though, options.attrs is ignored. That's bad because it breaks calls like the following:

myModelInstance.save({name: "example"}, {patch: true});

The above should send only {name: "example"} rather than the whole attributes hash in the HTTP request. This is very important in Rails, where strong parameters is the officially supported way to whitelist attributes. In Rails, you want to be able to set config.action_controller.action_on_unpermitted_parameters = :raise to catch your own mistakes, but it breaks when backbone-rails forces you to send every attribute with every request.

jarrett commented 9 years ago

Proposed fix in pull request #189.

jarrett commented 9 years ago

I should add that overriding Backbone.Model#toJSON is another approach to working with Strong Parameters. Nonetheless I think it's worthwhile to ensure backbone-rails preserves the documented functionality of Backbone.Model#save.

kamaljoshi commented 8 years ago

This behaviour by the JS patch is very annoying and it seems like that it can be easily fixed by considering options.attrs here:

if (model.paramRoot) {
  data = {};
  data[model.paramRoot] = model.toJSON(options);
} else {
  data = model.toJSON();
}

Why hasn't this been fixed yet?

reidcooper commented 8 years ago

+1 to bump