Bug Report #4497

Whitelist properties in ORM

Added by John Heathco over 2 years ago. Updated 8 months ago.

Status:ClosedStart date:04/16/2012
Priority:HighDue date:
Assignee:Lorenzo Pisani% Done:

100%

Category:-
Target version:Kohana v3.x - v3.4.0
Resolution:fixed Points:1

Description

After the attack on github recently showed the vulnerabilities associated with having models allow all-access to their properties, the Rails community decided to lock up their models by defaulting to a whitelist way of allowing access to properties.

We should do the same. Currently, I'm sure many users do the following:

// In controller action
$model = ORM::factory('model');
$model->values($this->request->post('model'))->save();

The problem here lies in the fact that a user may inject additional POST values in order to set values in the model that your application was not expecting. If you don't lock these down manually, you open yourself up to allowing a malicious user to modify SQL fields they shouldn't have access to.

A possible solution (similar to what was implemented in Rails) is to create a mandatory whitelist property where you specify the attributes of the model that can be changed using the mass setting methods (such as values)


Related issues

Related to ORM - Patch #4466: Reconsider ORM::values() behavior and documentation Closed 03/04/2012

Associated revisions

Revision 106a1220
Added by Kemal Delalic 8 months ago

Refs #4497: ORM::values() second param isn't optional anymore

Revision bce74b4d
Added by Lorenzo Pisani 8 months ago

Merge pull request #96 from kemo/3.4/develop

Refs #4497: ORM::values() second param isn't optional anymore

History

#1 Updated by Isaiah DeRose-Wilson over 2 years ago

  • Project changed from Kohana v3.x to ORM
  • Category deleted (Modules:ORM)

#2 Updated by Isaiah DeRose-Wilson over 2 years ago

  • Target version changed from v3.3.0 to Unscheduled

#3 Updated by dp de over 2 years ago

Just use

// In controller action
$model = ORM::factory('model');
$model->values($this->request->post('model'), array('allowedfield1', 'allowedfield2', '...'))->save();

instead of
// In controller action
$model = ORM::factory('model');
$model->values($this->request->post('model'))->save();

I do not think there is a reason for this whitelist property. In my point of view its better to make the second parameter of the values method required.

#4 Updated by Lorenzo Pisani about 1 year ago

  • Status changed from New to Assigned
  • Assignee set to Lorenzo Pisani
  • Target version changed from Unscheduled to v3.4.0

Hopefully everyone switched to passing in the fields as the second parameter to values()… I'm going to remove the default behavior and make this required in 3.4.0.

#5 Updated by Kemal Delalic about 1 year ago

@Lorenzo array $expected should probably be renamed to $columns or $keys

#6 Updated by Kemal Delalic 8 months ago

What's the current status of this one? Should NULL still remain allowed as second param for the lazy ones?

#7 Updated by Lorenzo Pisani 8 months ago

We should make the change to make it required, feel free to send me a PR and I'll merge it in.

#8 Updated by Lorenzo Pisani 8 months ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100
  • Resolution set to fixed

Also available in: Atom PDF