Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ORM] Database field names are not guarded against ORM method clashes #6329

Open
padbor opened this issue Nov 23, 2016 · 7 comments · May be fixed by #9549
Open

[ORM] Database field names are not guarded against ORM method clashes #6329

padbor opened this issue Nov 23, 2016 · 7 comments · May be fixed by #9549

Comments

@padbor
Copy link

padbor commented Nov 23, 2016

Consider this:

<?php
class book extends DataObject {

	private static $db = array(	
		"Show" => "Boolean",
		"Delete" => "Boolean"
	);

	private static $summary_fields = array (
		"Delete" => "Delete",
		"Show" => "Show"
	);

}

Each time the administration is reloaded, the data for that model is removed from the database.

This removed more than 300 documents from my database.

This is because SilverStripe calls the Delete() method in $summary_fields

Which in this case is a very very sensitive method.

In some cases, this little trick of SilverStripe can go very fast and save lives.

But destroys data now. And it took me 48 hours and think that I was attacking, siphoning ...

For example, you can update the Delete method of the DataObject model to:

function delete ($now = false) {
    If ($now) {
      $this->brokenOnDelete = true;
       $ this->onBeforeDelete();
   }
 }

So to call the Delete method, $now must take true, so that the data is deleted.

PRs

@tractorcow
Copy link
Contributor

tractorcow commented Nov 23, 2016

Alright, so what we need is a better check for reserved keys for properties, similar to how we do for form actions. What you would expect is that framework would crash with a "Invalid property name" error right?

E.g. a property Delete should fail if there is a method delete on the parent record (case insensitive).

@tractorcow tractorcow changed the title Error Deleting Data from Database Database field names are not guarded against ORM method clashes Nov 23, 2016
@tractorcow
Copy link
Contributor

I suggest to put the fix in DataObject::db() method (or DataObjectSchema::fieldSpec method in 4.x).

To improve performance, maybe the check should only be in dev mode.

@dhensby
Copy link
Contributor

dhensby commented Nov 24, 2016

The cause of this is that GridField@getDataFieldValue calls $record->$fieldName(), which it probably shouldn't do - it should stick to getting DB values not invoking methods.

I think reserving all method names from being DB names is a bit much and surely we'd have to also reserve all the controller methods and all methods on the class hierarchy of both model and controllers too? Oh and decorators, then any GlobalTemplateProviders and so on, which all seems unnecessary and impractical.

What happens if I add a method later with the same name or I install a new module which causes a clash? Who's wrong, the method or the DB field?

Brutally honest mode engaged:

  1. Naming a field "Delete" or "Write" or any word that's such a core part of the Model's API is going to cause problems
  2. This would come up pretty quickly in testing and deleting 300 rows from a test DB shouldn't be a big deal
  • if you didn't test, then your backups will be useful here
  • if you didn't back up then you're just scapegoating the framework for the fact you didn't put in any safeguards against you deploying untested code.

What scenario is it even needed to store a boolean on an object called "delete"?

@tractorcow
Copy link
Contributor

I would say:

private static $db = [
	'Name' => 'Type',
];

Would accept getName() as a method, but it should fail if Name is a method on the parent object. DB getters should use the property getters, not the non-getter form of these fields.

Unlike relation names, however, we can't easily guard against these. However, instead we can fail on names that clash with a PARENT class. E.g. function Children() would be fine with a has_one => Children, but it should be a validation error if the function Children() is in a parent class. This would cover relations named delete and so on in subclasses of DataObject.

These errors would be checked on dev/build.

Does this sound reasonable?

@dhensby
Copy link
Contributor

dhensby commented Jan 26, 2018

gah - I'd just tell people to be sensible with their naming ;)

but yes, that sounds reasonable

@sminnee sminnee changed the title Database field names are not guarded against ORM method clashes [ORM] Database field names are not guarded against ORM method clashes Oct 6, 2018
@elliot-sawyer
Copy link
Contributor

Does this imply that destructive methods like delete can be triggered with a GET request?

@jakxnz
Copy link
Contributor

jakxnz commented Jun 16, 2020

Is #9549 a reasonable solution @dhensby ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants