Enhance Security: add fieldList to patchEntity calls in controller->add() and controller->edit()

[schrolli]

Hi,

I'm wondering, if it is a good security enhancement, if all add and edit actions have the fieldList-array populated with all fields, that are no primary key and not 'created' or 'modified'.

In my applications, i have many examples, where the user mustn't add or edit special fields. So when the fieldList is prefilled, one can simply remove the form-input and the element in the fieldList. When it is not prefilled, the array has to be build manually. And with approx 30 tables in one big project, that is no fun;)

Furthermore I feel confident, that people get aware of this important Security measure. So when they don't know about fieldList and simply remove the form-helper input() call, one can fiddle the post-message and alter the field, that wasn't supposed to be edited by the user...

I am willing to create a pr for this. But i wanted to ask beforehand, if this is a good idea;)

[markstory]

We have had problems in the past where we've enabled strict modes on features as new developers and often experienced ones forget that they are enabled. The most recent example was $_accessible in the entities. It used to contain a specific whitelist. However, after many issues/questions on stackoverflow about 'new fields not saving' we reverted to a more relaxed list of fields. (#134)

While I agree it is nice to encourage safer practices I worry that we'll have similar problems in introducing fieldList as we had with $_accessible in the entities.

[dereuromark]

I have similar concerns, this also be quite dangerous as fields not in the list, but added later in the form can lead to strange effects, like fields for some reason then not being validated and not being persisted without feedback.
If such a thing was to be added, it should rather throw exceptions for fields included the data but not whitelisted. It needs to be more transparent, not less.

That said, I already tried to educate people back in 2010 about the topic and sure think way too many people still don't know about the importance of it.
But maybe we should focus on making the docs here more clear than trying to force this into the baked code, leading to one more source of things that can annoy the developers because of its obvious un-DRY drawbacks.

[thinkingmedia]

What if we added a property to the Entity class called lock which would contain a hash of the schema. When that property is set by the developer or bake, then you could safely perform strict field rules.

If the database schema ever changed the lock hash would no longer match, and a run-time error could be thrown. Forcing the developer to assign a new hash value to verify the issue was resolved.

Yes, this is a hard core developer feature. Not sure I would use it. It was just my first thought when reading this thread.

[dereuromark]

Could this be provided as plugin opt-in solution? I think that would be the best way for now.

[thinkingmedia]

What if we added a strict option to newEntity/patchEntity that could be assigned a class name of an object to handle extra fields. The default (configurable) would be to just ignore (current behavior).

Developers would then alter the default or pass a custom handler to newEntity/patchEntity.

strict could also accept a callback function for quick edge cases.

[schrolli]

i think this is a conceptual error. when you want to make the patch-entity form-fiddling-save, then you have to declare somewhere, which fields could be changed or which shouldn't. Currently the only option is to list all the acceptable fields in the fieldList option. And that isn't DRY by design. Probably from the pure design-view, a blacklist of fields not to save would be the solution. But that's out of my scope, if this is a good idea from the security perspective.

[thinkingmedia]

a blacklist of fields not to save would be the solution

That's exactly what $_accessible on an entity does. If you don't want to risk password getting hacked add it to that list, and then on your user's reset password action you can override that option with accessibleFields when you call patchEntity.

It's complicated enough as it is. I remember struggling with this when I upgraded from Cake 2 to 3.

To be honest, I think a simple explicit mode is best. When enabled newEntity has to have a fieldList and if $data does not match (to many or to few) it simply fails validation. Errors contains the reason.

I can think of some open front-end forms on a website where you don't want to mess around with security.

[dereuromark]

Yep @thinkingmedia is right.
The admin crud backend and therefore most the forms usually also don't want or need those kind of things out of the box, no need to protect the admin from himself.
The rest is up to the developer how far he wants to secure things.

[thinkingmedia]

@markstory has said in the past that he doesn't want to have security features where a hacker could sniff a post action to see what is allowed and what isn't, but I think if you have a form it's reasonable that the URL gives a negative response if the posted data does not match the form.

[inoas]

CakePHP/CakePHP

I don't think blacklists are very save. The only thing where I can see them are actually HoneyPots.
There are ways to go through the entity and through save/validate whitelists.
The only thing I see missing are different sets of visibilities (accessible, hidden, etc) like there are different sets of validators.

CakePHP/Bake

I think it is reasonable to list all fields in accessible explicitly aside those that are currently set to false. This will make sure you know that if you don't change that (or use fieldLists) then security is broken. It is very quick to say foo => false, * => true.

Edit: just read markstorys comment above. So setting them explicitly by default is a no go for now, it seems, in bake that is. Maybe we could introduce a flag like --secureEntities to do it.
Anyway I still think the most important feature to me would be an easy way to toggle/switch around different sets of visibilities and mass assign privileges.