Introduce 'literal-string' to QueryBuilder::where($predicates)

[craigfrancis]

Both Psalm 4.8 and PHPStan 0.12.97 support the literal-string type.

This is useful for method arguments that expect the input string to be defined by the developer (in the source code), allowing mistakes like these to be easily identified via Static Analsys:

$result = $qb
  ->select('u')
  ->from('User', 'u')
  ->where('u.id = ' . $_GET['id']); // INSECURE

While literal-string can be used by other methods, I want to start with where(), to see if we get any feedback (which I'm happy to help with), and if it goes well, I can carefully/slowly update other methods (I don't want to cause issues for developers using Doctrine, but I do want to stop Injection Vulnerabilities being introduced by the thousands of developers who can easily use Doctrine incorrectly).

We're able to change the type from mixed to literal-string|object|array, because the where() method calls $this->add() and that already uses string|object|array $dqlPart (we can look at the object/array inputs at a future date).

The literal-string type does support string concatenation, to reduce false positives.

It will get developers to correctly use ->setParameter() for values (e.g. not relying on the integer type for safety, as noted by Grégoire Paris) - because user values should always be parameterised, so they can be sent to the database separately (database/driver permitting).

For user defined identifiers, comparison operators, etc - while these will hopefully be rare, they should already be using an allow-list approach, like the following, and both of these work with literal-string (I'm using $_GET to clearly show the un-trusted/un-safe values):

$fields = [
  'name' => 'u.full_name',
  'email' => 'u.email_address',
  'address' => 'u.postal_address',
];

$field = ($fields[$_GET['field']] ?? 'u.full_name');

// or

$fields = ['name', 'address', 'email'];

$field_id = array_search($_GET['field'], $fields);

$field = $fields[$field_id];

// Used with

$qb
  ->select('u')
  ->from('User', 'u')
  ->where($field . ' = :my_value')
  ->setParameter('my_value', $_GET['value']);

I would still consider providing a setIdentifier() method, but I believe Sergei Morozov is already intending to add "support for handling identifiers as first-class citizens" via #4357 and #4772 (this isn't necessary, but it might help some edge cases).

Because this is being done by Static Analysis (how Sergei wanted to implement this check), you can still tell Psalm or PHPStan to ignore any errors - i.e. for those who have been using this method incorrectly, and don't have the time to fix their code.

Ref Issue 8933.

The base branch was changed.

[derrabus]

I still have mixed feelings about this because passing a concatenated string to this method is not inherently wrong from my PoV. But I won't block merging it if others think it's a good idea.

However, this is not a bugfix, so I'd suggest to target 2.12.x instead.

[craigfrancis]

Thanks @derrabus, but just to confirm, concatenated literal-strings are fine (I specially asked the static analysis tools to allow that, to help adoption). It’s only when a non-literal string (e.g. a user value) is included does it complain (as it should), where developers should be using setParameter(), or something like the identifier examples above.

And I’m happy to target any version; you’re right it’s not a big fix, I’m mostly wanting to be sure it doesn’t cause any problems.

[craigfrancis]

To test, Doctrine extensions for PHPStan (1.2.10), when using bleedingEdge, will check QueryBuilder::where() only accepts literal-string|object|array<mixed> for $predicates . Like this patch, it won't check object/array inputs have used a literal-string, but it should give us some feedback.

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[github-actions]

This pull request was closed due to inactivity.