API Create orderBy() method to handle raw SQL

[emteknetnz]

Issue #10449

Note: unit-tests won't pass until silverstripe/silverstripe-versioned#385 is merged in - but this was run though an installer CI run: https://github.com/emteknetnz/silverstripe-installer/actions/runs/3617074332

I've only implemented this at the DataList level. If we wanted to implement this deeper then for sort() we'd probably want to do this at the SQLSelect::addOrderBy() which is ultimately the only thing called by DataQuery::sort() - however it feels as though there's going to be too much raw SQL flowing through there to make this practical given that Silverstripe loves to use CASE WHEN statements when constructing queries. SQLSelect::addOrderBy() currently has a lot of logic to handle raw sql. At some level we need to fully allow raw SQL because that's the language the database uses.

My assumption is that DataList::sort() is the entry point for SQL injection attacks because it's the abstraction used for interacting with the ORM i.e. MyDataObject::get()->filter($filter)->sort($sort)->column('Title') uses DataList::sort() - while DataQuery is one abstraction level below and SQLSelect is another abstraction below again.

The most recently disclosed SQL injection vulnerability was via DataList - 4308a93

The following public methods on DataList do accept raw sql

sort()

• This PR tightens this up and splits off orderBy() which is the raw SQL version

where() and whereAny()

• I've split off a new card to handle the following:
• var_dump(SiteTree::get()->where('MOD(ID,3) = 1')->column('ID'));
• the major call to these is the $filter param in public static DataObject::get($callerClass, $filter, ...) and get_one() - which will use call DataList::where() e.g. DataObject::get(MyDataObject::class, $rawSql). We should simply change the calls here from ->where() to ->filter() in CMS5 to tighten this up.
• where() / whereAny() are essentially the "raw" versions of 'filter() / filterAny()`, so this is similar to sort() / orderBy()
• PartialMatchFilter::applyMany() is the only call to whereAny() that needs further investigation
• Group::Member($filter = '') will call where($filter)

leftJoin() / - innerJoin()

• var_dump(SiteTree::get()->leftJoin('Member', 'SiteTree.ID = Member.ID AND SiteTree.ID < 5 AND MOD(SiteTree.ID,3) = 1')->column('Member.Email'));
• I've had a look through all the calls to innerJoin() and leftJoin() in core and there didn't look to be anything unsafe e.g. could be altered via a url parameter.
• I don't know if it's viable to filter out raw SQL since its includes a param for raw sql for the join clause.

The following public methods on DataList do not accept raw sql:

filter() / filterAny() / find() / exclude()

• var_dump(SiteTree::get()->exclude(['MOD(ID,3)' => 1])->column('ID'));
• Unknown column 'MOD(ID,3)' in 'where clause'

map()

• var_dump(SiteTree::get()->map('ID', 'MOD(ID,3)')->toArray());
• will simply iterate over the results of a database query and only return a value if key exists i.e. doesn't query database with MOD()

max() / min() / avg() / sum()

• var_dump(SiteTree::get()->max('MOD(ID,3)'));
• Unknown column 'MOD(ID,3)' in 'field list'

column() / columnUnique()

• var_dump(SiteTree::get()->column('MOD(ID,3)'));
• Invalid column name MOD(ID,3)

[GuySartorelli]

Should #10582 be revisited as a part of this work?
In my mind we should either set GridFieldSortableHeader to use orderBy() (because a developer could want to use some aggregate or join sort column, as discussed in the original PR for that fix), or we could remove the logic that validates SortColumn exists and still use sort().

[emteknetnz]

Should #10582 be revisited as a part of this work?

Out of scope IMO, we can look at this with a new card if need be. It can happen post 5.0.0

[GuySartorelli]

@emteknetnz The CI failure is for an invalid column name which doesn't seem to be related to the other PRs - can you please take a look? Was the installer CI run after the most recent changes?

[GuySartorelli]

Just a couple of minor changes - I should have caught two of these last review so sorry about that.

[GuySartorelli]

This will still match against something like "Column Name" - it's funtionally equivalent to using explode for any string with only one space in it.
If the column name has a space in it, and no direction was supplied, it should not match here so that the default direction can be added in the else block.

I think we should explicitly be checking for asc and desc here instead.

Suggested change

	if (preg_match('/^(.+) ([^"]+)$/i', trim($colDir), $matches)) {
	if (preg_match('/^(.+) (asc|desc)$/i', trim($colDir), $matches)) {

[emteknetnz]

It won't match "Column Name" - it's pinned with $ at the end of the regex

It would match Column Name - however this will fail on validateSortDirection()

[GuySartorelli]

Apologies, I should have omitted the quote marks which were intended to mean "this is the whole string"

My point is that it shouldn't fail on validateSortDirection(), because Column on its own won't fail that.
So currently passing Column is fine but Column Name is not fine, even if they're both valid columns.

[emteknetnz]

I don't think think we need to over think for edge cases like this. An exception will still be thrown and it should be pretty obvious at that point they need to wrap their column in quotes since they'll get a 'Invalid sort direction Name' exception message

Even if we change this to (asc|desc), then it'll still fail on the edge case of someone calling naming column MySortDir ASC. There's a workaround available which is to wrap the weird column in double quotes e.g. ->sort('"Column Name" ASC')

I think it's reasonable to put the onus on end users to do this here - they'll be well used to do extra steps to get things to actually work. Other parts of Silverstripe will already be broken e.g. calling $myDataObject->{'Column Name'} to get values (I'm assuming that actually works)

[GuySartorelli]

I disagree but I'll merge it 'cause it's not worth arguing about. Someone can raise an issue when it causes problems for them.