4 ms·

It's a vulnerability in Rails, which is what GitHub is built on. The vulnerability was demonstrated by adding a commit to the Rails project on GitHub, indicati

It's a vulnerability in Rails, which is what GitHub is built on. The vulnerability was demonstrated by adding a commit to the Rails project on GitHub, indicating that GitHub suffers from the vulnerability. Here's the relevant issue. It might clarify things a bit better: https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228

phillmv 15y ago
Well… strictly speaking it's not a vulnerability in that you aren't inherently unsafe just because you use rails. It is a bug in that it's a usability issue; maybe it should be turned on by default, much like the auto escaping to prevent XSS that came in rails 3.
Scriptor 15y ago
The issue seems to be a result of an entire hash of the parameters passed in the request being sent to the new method for models. Is this really common practice in Rails code? Sure you can specify in the model that certain attributes can't be changed. But shouldn't this stuff be checked when validating form input? Normally I'd have a hash of filters, with the field/column name mapped to the appropriate set of rules for that field. Anything that's not specified in the filters doesn't go to the model's new method. In this case, it's like the equivalent of PHP code where you santizie the data in $_POST and just send that whole variable to the database. Choosing which fields are accepted shouldn't just be a model security issue, it's a form validation issue. This makes choosing whether an admin can change a field or not trivial as well. If a request is made as an admin, (maybe through a form that's only accessible by someone with an admin role) then you just apply the validation rules for an admin. Otherwise you apply the rules for a user.
- cdcarter 15y ago
  It's common in tutorials and quickstart guides, usually with a note saying that you really should be protecting things and not using mass-assignment. It just makes the blog in 5 minutes videos cleaner.
  - Scriptor 15y ago
    Reminds me of all the PHP tutorials floating around that only treat security as a side note. Essentially, the default configuration with Rails seems to be that users can set the data for any database column. It's almost as bad as register_globals...
- winstonm 15y ago
  Yes, mass assignment with Model.new(params) and model.attributes=params is a best practice for professional production Rails websites. Business and security rules for field updates are coded in the model (attr_accessible/attr_protected/validates). That's how it's been since Rails 1. Which is cool. But it's error-prone for newbies, especially when Rails's model and controller generators make all attrs writeable by default, with nary a generated comment about how or why to lock things down. In a culture of convention over configuration, attrs should be locked down by default: "config.active_record.whitelist_attributes=true" for new apps, and throw a helpful message when I mass assign to a model that has no accessible attrs configured yet.

phillmv 15y ago

Well… strictly speaking it's not a vulnerability in that you aren't inherently unsafe just because you use rails. It is a bug in that it's a usability issue; maybe it should be turned on by default, much like the auto escaping to prevent XSS that came in rails 3.

Scriptor 15y ago

The issue seems to be a result of an entire hash of the parameters passed in the request being sent to the new method for models. Is this really common practice in Rails code? Sure you can specify in the model that certain attributes can't be changed. But shouldn't this stuff be checked when validating form input? Normally I'd have a hash of filters, with the field/column name mapped to the appropriate set of rules for that field. Anything that's not specified in the filters doesn't go to the model's new method. In this case, it's like the equivalent of PHP code where you santizie the data in $_POST and just send that whole variable to the database. Choosing which fields are accepted shouldn't just be a model security issue, it's a form validation issue. This makes choosing whether an admin can change a field or not trivial as well. If a request is made as an admin, (maybe through a form that's only accessible by someone with an admin role) then you just apply the validation rules for an admin. Otherwise you apply the rules for a user.

cdcarter 15y ago
It's common in tutorials and quickstart guides, usually with a note saying that you really should be protecting things and not using mass-assignment. It just makes the blog in 5 minutes videos cleaner.
- Scriptor 15y ago
  Reminds me of all the PHP tutorials floating around that only treat security as a side note. Essentially, the default configuration with Rails seems to be that users can set the data for any database column. It's almost as bad as register_globals...
winstonm 15y ago
Yes, mass assignment with Model.new(params) and model.attributes=params is a best practice for professional production Rails websites. Business and security rules for field updates are coded in the model (attr_accessible/attr_protected/validates). That's how it's been since Rails 1. Which is cool. But it's error-prone for newbies, especially when Rails's model and controller generators make all attrs writeable by default, with nary a generated comment about how or why to lock things down. In a culture of convention over configuration, attrs should be locked down by default: "config.active_record.whitelist_attributes=true" for new apps, and throw a helpful message when I mass assign to a model that has no accessible attrs configured yet.