Skip to content

Start experiment with 'literal-string' type#1788

Merged
dereuromark merged 4 commits intopropelorm:masterfrom
craigfrancis:iteral-string
Oct 11, 2021
Merged

Start experiment with 'literal-string' type#1788
dereuromark merged 4 commits intopropelorm:masterfrom
craigfrancis:iteral-string

Conversation

@craigfrancis
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor

@craigfrancis
Copy link
Copy Markdown
Contributor Author

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

@dereuromark dereuromark merged commit 4b1da87 into propelorm:master Oct 11, 2021
@craigfrancis
Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants