The code protecting against multiple statements is part of Connection::query(), but ::prepareStatement() should be protected too.
Move the protection code to Connection::prepareStatement().
none
none
none
The code protecting against multiple statements that was part of Connection::query(), is moved to a protected method ::preprocessStatement() so that code that calls in sequence ::prepareStatement() and StatementInterface::execute() can benefit from its protection too. See the change record for more details.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 3201266-7.patch | 7.26 KB | mondrake |
| #7 | interdiff_6-7.txt | 6.44 KB | mondrake |
Comments
Comment #2
mondrakeComment #3
mondrakeWith a test.
Comment #4
daffie commentedThe code changes look good to me.
We are moving the code for protection against SQL injections to the method Connection::preparestatement() from the method Connection::query(). In the later method is after the moved code block is the method Connection::preparestatement() called. The moved code block is executed as the first code. Therefore the code order execution has not changed. Only the moved code is now also executed in the method Connection::preparestatement().
There is testing added for the code added to the method Connection::preparestatement().
For me it is RTBC.
Comment #5
mondrakeThanks. Unfortunately it looks like the Sqlite driver needs some changes too.
Comment #6
mondrakeFixes for Sqlite. This adds a bit of code duplication, dunno if it's worth adding another method to the base Connection class to preprocess the SQL statement across different drivers.
Comment #7
mondrakeWent ahead and did #6, adding a protected
Connection::preprocessStatement()method. This will be used in contrib too IMHO.Comment #9
mondrakeComment #10
daffie commentedAll the code changes since my last RTBC look good to me.
The fix for SQLite look great.
Back to RTBC for me.
Comment #11
catchGiven we needed changes in the sqlite driver, I think we should add a release note and change record in case similar changes are needed in contrib drivers.
It's also arguably an API change although only for database drivers so still might be OK in a minor.
Comment #12
mondrakeAdded draft CR and release note snippet.
Comment #13
catchComment #15
catchOK I think the hardening here outweighs the potential bc break, and the new exception is only thrown in a situation that would already throw an exception.
Committed 2c84141 and pushed to 9.2.x. Thanks!
Comment #16
effulgentsia commentedPer #11.