QueryLogger strips some characters from queries #10181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10181 +/- ##
============================================
- Coverage 94.94% 94.94% -0.01%
Complexity 11613 11613
============================================
Files 417 417
Lines 28734 28738 +4
============================================
+ Hits 27281 27284 +3
- Misses 1453 1454 +1
Continue to review full report at Codecov.
|
| "'" => "''", | ||
| ]; | ||
|
|
||
| $p = strtr($p, $replacements); |
There was a problem hiding this comment.
Instead of manually replacing specific characters it would be better to instead use preg_quote() when generating the $keys array below.
There was a problem hiding this comment.
As written in preg_quote()
Note that
preg_quote()is not meant to be applied to the$replacementstring(s) ofpreg_replace()etc.
There was a problem hiding this comment.
My mistake, I misread the code. You are modifying the $params var not $keys.
|
|
||
| $logger->expects($this->once())->method('_log')->with($query); | ||
| $logger->log($query); | ||
| $expected = "duration=0 rows=0 SELECT a FROM b where a = '\$2y\$10\$dUAIj' AND b = '\$0.23' AND c = 'a\\\\0b\\\\1c\\\\d' AND d = 'a''b'"; |
There was a problem hiding this comment.
Looks like the \ have been doubled up. Shouldn't the value match the parameters?
There was a problem hiding this comment.
The final SQL is
SELECT a FROM b where a = '$2y$10$dUAIj' AND b = '$0.23' AND c = 'a\\0b\\1c\\d' AND d = 'a''b'
And the final strings after SQL unescape are c = a\0b\1c\d and d = a'b - c has backslash and d has single-quote and matches 'p3' => 'a\\0b\\1c\\d', 'p4' => "a'b" after PHP unescape.
If it would be \\ instead of \\\\, SQL would get \0 which would result to null-byte after SQL unescape.
This is light version of PDO::quote() - only escapes \ to \\ and ' to ''.
While it is not complete solution for safe SQL escape, but developer needs less modifications before testing SQL elsewhere.
The ' escape to '' is supported by all 4 CakePHP core SQL drivers.
The \ escape to \\ is supported by MySQL and PostgreSQL, but in Sqlite and SQL Server the \ in string literals has no special meaning.
- The commit fixes preg_replace part
$replacements = [
'$' => '\\$',
'\\' => '\\\\',
];
- Then enhances generated SQL string quoting
$replacements = [
'\\\\' => '\\\\\\\\',
"'" => "''",
];
Should I:
- leave only part 1
- leave part 1, and then escape only
'to''(MySQL/PostgreSQL developer will need escape\manually) - or stay with current commit - leave part 1, and then escape both
'and\(Sqlite/SQL Server developer will need revert\\back to\manually)?
There was a problem hiding this comment.
These generated strings are never used as queries though. Wouldn't we want to emulate what the SQL server would be see?
There was a problem hiding this comment.
SQL search for string It's m\n\o is:
SELECT a FROM b WHERE c = 'It's m\n\o'- current QuerryLogger emulated querySELECT a FROM b WHERE c = 'It''s m\\n\\o'- valid in MySQL/PostgreSQLSELECT a FROM b WHERE c = 'It''s m\n\o'- valid in Sqlite/SQL Server
My commit changed SQL to 2nd variant because I work with MySQL and I didn't thought about other DBs. But after I checked syntax of string literal in other DBs, now I'm thinking which variant is best.
QueryLogger is not DB specific. Emulated queries are not executed so security is low priority. But sometimes we need test queries externally if CakePHP works not as expected. I want it to be as much as possible evenly helpful for every DB engine developer.
So I ask which variant is the best? Do CakePHP have higher priorities to some DB engine?
There was a problem hiding this comment.
So I ask which variant is the best? Do CakePHP have higher priorities to some DB engine?
Not really. Given that the current output works for some/most DBs, we'll go with what is present now.
When using DebugKit SqlLog, I noticed that when saving hashed user password, log contained
UPDATE users SET password = 'y$uEFBAE...' WHERE id = 123which is not in valid Bcrypt format.But after checking database, password was saved correctly as
'$2a$10$uEFBAE...'.I enabled logging to text file - still same result, so not fault of DebugKit.
After inspecting code, problem is that QueryLogger interpolates bind parameters using
preg_replace: butpreg_replacereplacement string uses$1or\\1for backreferences.$2and$10in hash refers to invalid backrefences, so replaced with empty strings.Another possible common string could be "$0.12" which would replace to
":p1.12"because$0refers to bind parameter matched by the whole pattern.My commit escapes
$and\to\$and\\to ignore them by preg_replace.Also I replace
'to double single-quote''and\to\\to be even more valid SQL string. This is why there is\\to\\\\\\\\replacement (preg_replace and SQL escape).