propelorm / Propel2

Propel2 is an open-source high-performance Object-Relational Mapping (ORM) for modern PHP
http://propelorm.org/
MIT License
1.26k stars 399 forks source link

Start experiment with 'literal-string' type #1788

Closed craigfrancis closed 3 years ago

craigfrancis commented 3 years ago

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

This checks that a string has been created by the developer (defined in the source code), so mistakes that lead to Injection Vulnerabilities can be identified, e.g.

$users = UserQuery::create()->where('id = ' . $_GET['id'])->find(); // INSECURE
$users = UserQuery::create()->where('id = ?', $_GET['id'])->find(); // Correct

By setting the $clause parameter to literal-string|array, while the array version won't be checked, at least the string can be (ensuring it does not contain unsafe user data).

And literal-string is quite forgiving, in that it works with variables, and allows string concatenation (assuming both are literal-string values), so things like conditional where clauses are fine, e.g.

$type_where = 'type ' . ($like ? 'LIKE' : 'NOT LIKE') . ' ?';

$users = UserQuery::create()->where($type_where, $_GET['type'])->find(); // Fine

That said, I still want to be cautious (I don't want to create issues for the users of Propel). Which is why I'd like to start with ->where(), see what feedback we get (I'm happy to help with that), and if we get a positive response, we can update other methods. The last one will probably be $statement in $con->prepare(), as I appreciate that its input is a bit more complicated.


@dereuromark, we briefly talked about this concept in April, in regards to the is_literal RFC. While it didn't pass (I'll try again next year, once I've addressed the concerns people had), Psalm/PHPStan have now added support, so we can introduce this check for developers that use Psalm (level 3 or stricter) or PHPStan (level 7 or stricter)... where it will help them identify mistakes often created by junior developers making a "quick edit".

dereuromark commented 3 years ago

Almost it seems: https://github.com/propelorm/Propel2/pull/1788/checks?check_run_id=3861848381#step:8:18

craigfrancis commented 3 years ago

Yep, sorry, being lazy and didn't re-run phpcs locally.

craigfrancis commented 3 years ago

Thanks @dereuromark ... I'll try to keep an eye out for any issues that are reported, and if everything's ok in a few months time, I'll make a new PR that covers a few more methods.