Connection parameters are cached hashed #3031
Conversation
| @@ -99,10 +101,14 @@ public function getCacheKey() | |||
| */ | |||
| public function generateCacheKeys($query, $params, $types, array $connectionParams = []) | |||
| { | |||
| $hashedParams = array_map(function ($param) { | |||
| return crypt($param, md5($param)); | |||
There was a problem hiding this comment.
This approach assumes that all $params are scalar which may be true for now but not necessarily true in the future. Why not just hash the serialized representation once instead of hashing parameters individually?
There was a problem hiding this comment.
I'm not a security expert but md5($param) is not random (the salt should be random). Should something like random_bytes() be used instead, or can we just get away with something like md5() or sha1() for this purpose?
There was a problem hiding this comment.
First one seems legit, since I don't know where is it really called I didn't want to change too much stuff. If it's ok I'll change it.
Salt is not random because with same strings we want same outputs!
There was a problem hiding this comment.
No crypto needed here: this should just be a plain hash of all parameters. sha256 would be more than sufficient.
| $realCacheKey = 'query=' . $query . | ||
| '¶ms=' . serialize($params) . | ||
| '&types=' . serialize($types) . | ||
| '&connectionParams=' . serialize($connectionParams); | ||
| '&connectionParams=' . serialize($hashedParams); |
There was a problem hiding this comment.
Serializing the hash is not needed anymore, while hashing the serialized parameters would be simpler
| @@ -99,10 +101,14 @@ public function getCacheKey() | |||
| */ | |||
| public function generateCacheKeys($query, $params, $types, array $connectionParams = []) | |||
| { | |||
| $hashedParams = array_map(function ($param) { | |||
| return crypt($param, md5($param)); | |||
There was a problem hiding this comment.
No crypto needed here: this should just be a plain hash of all parameters. sha256 would be more than sufficient.
| @@ -5,65 +5,61 @@ | |||
| use Doctrine\DBAL\Cache\QueryCacheProfile; | |||
| use Doctrine\DBAL\ParameterType; | |||
| use Doctrine\Tests\DbalTestCase; | |||
| use function parse_str; | |||
| use function unserialize; | |||
|
|
|||
| class QueryCacheProfileTest extends DbalTestCase | |||
There was a problem hiding this comment.
A new test method is needed, in which we verify that none of the connection parameters are to be found in the key.
There was a problem hiding this comment.
I made it, it's called testConnectionParamsShouldBeHashed
There was a problem hiding this comment.
Thanks! Didn't see it due to diff noise :)
| @@ -20,18 +20,19 @@ | |||
| namespace Doctrine\DBAL\Cache; | |||
|
|
|||
| use Doctrine\Common\Cache\Cache; | |||
| use function serialize; | |||
| use function sha1; | |||
There was a problem hiding this comment.
lines 103, 104 and 109!
There was a problem hiding this comment.
Aha, I just saw the diff, thanks 👍
| @@ -102,7 +102,7 @@ public function generateCacheKeys($query, $params, $types, array $connectionPara | |||
| $realCacheKey = 'query=' . $query . | |||
| '¶ms=' . serialize($params) . | |||
| '&types=' . serialize($types) . | |||
| '&connectionParams=' . serialize($connectionParams); | |||
| '&connectionParams=' . hash('sha256', serialize($connectionParams)); | |||
| private $types = [ParameterType::INTEGER]; | ||
|
|
||
| /** @var array<string> */ | ||
| private $connectionParams = [ |
There was a problem hiding this comment.
Alignment went a bit bonkers here. Also, the type is string[], as most tooling doesn't work with array<foo>
|
|
||
| $connectionParams = array( | ||
| /** @var array<string> */ |
| $query = 'SELECT * FROM foo WHERE bar = ?'; | ||
| $params = [666]; | ||
| $types = [ParameterType::INTEGER]; | ||
| /** @var array<int> */ |
|
|
||
| class QueryCacheProfileTest extends DbalTestCase | ||
| { | ||
| const LIFETIME = 3600; | ||
| const CACHE_KEY = 'user_specified_cache_key'; | ||
| public const LIFETIME = 3600; |
| const LIFETIME = 3600; | ||
| const CACHE_KEY = 'user_specified_cache_key'; | ||
| public const LIFETIME = 3600; | ||
| public const CACHE_KEY = 'user_specified_cache_key'; |
| @@ -77,67 +73,64 @@ public function testShouldGenerateAnAutomaticKeyIfNoKeyHasBeenGiven() | |||
|
|
|||
| public function testShouldGenerateDifferentKeysForSameQueryAndParamsAndDifferentConnections() | |||
| { | |||
| $query = 'SELECT * FROM foo WHERE bar = ?'; | |||
There was a problem hiding this comment.
Some in the diffs in the test seem completely wrong/unrelated. Also, as I mentioned before, a new method verifying that the connection parameters cannot be found in the key is needed.
There was a problem hiding this comment.
Yes, I tried to make it more dry!
| self::assertArrayHasKey('connectionParams', $params); | ||
|
|
||
| foreach ($this->connectionParams as $param) { | ||
| self::assertEquals(false, strpos($param, $params['connectionParams'])); |
There was a problem hiding this comment.
Is there something like assertStringNotContains that you can use here?
.gitignore
Outdated
| @@ -8,3 +8,4 @@ vendor/ | |||
| composer.lock | |||
| /phpunit.xml | |||
| /.phpcs-cache | |||
| .idea | |||
There was a problem hiding this comment.
This should be in your global .gitignore, and not in the project
|
Some CS issues remain, but I'll not hassle you with them :) |
| @@ -20,18 +20,19 @@ | |||
| namespace Doctrine\DBAL\Cache; | |||
|
|
|||
| use Doctrine\Common\Cache\Cache; | |||
| use function hash; | |||
| use function serialize; | |||
| use function sha1; | |||
There was a problem hiding this comment.
This one is now redundant (will fail build imho). Forget it, it's used on another place.
|
🚢 |
|
Thanks @fullbl! |
As per doctrine/orm#7086 (I didn't know it was related to dbal and not doctrine2), parameters should be hashed because they can have passwords inside.