diff --git a/system/Cache/Handlers/BaseHandler.php b/system/Cache/Handlers/BaseHandler.php index b2d17d369c2e..ea70efb22661 100644 --- a/system/Cache/Handlers/BaseHandler.php +++ b/system/Cache/Handlers/BaseHandler.php @@ -14,12 +14,65 @@ use Closure; use CodeIgniter\Cache\CacheInterface; use Exception; +use InvalidArgumentException; /** * Base class for cache handling */ abstract class BaseHandler implements CacheInterface { + /** + * Reserved characters that cannot be used in a key or tag. + * + * @see https://github.com/symfony/cache-contracts/blob/c0446463729b89dd4fa62e9aeecc80287323615d/ItemInterface.php#L43 + */ + public const RESERVED_CHARACTERS = '{}()/\@:'; + + /** + * Maximum key length. + */ + public const MAX_KEY_LENGTH = PHP_INT_MAX; + + /** + * Prefix to apply to cache keys. + * May not be used by all handlers. + * + * @var string + */ + protected $prefix; + + /** + * Validates a cache key according to PSR-6. + * Keys that exceed MAX_KEY_LENGTH are hashed. + * + * @param string $key The key to validate + * @param string $prefix Optional prefix to include in length calculations + * + * @throws InvalidArgumentException When $key is not valid + * + * @see https://github.com/symfony/cache/blob/7b024c6726af21fd4984ac8d1eae2b9f3d90de88/CacheItem.php#L158 + */ + public static function validateKey($key, $prefix = ''): string + { + if (! is_string($key)) + { + throw new InvalidArgumentException('Cache key must be a string'); + } + if ($key === '') + { + throw new InvalidArgumentException('Cache key cannot be empty.'); + } + if (strpbrk($key, self::RESERVED_CHARACTERS) !== false) + { + throw new InvalidArgumentException('Cache key contains reserved characters ' . self::RESERVED_CHARACTERS); + } + + // If the key with prefix exceeds the length then return the hashed version + return strlen($prefix . $key) > static::MAX_KEY_LENGTH ? $prefix . md5($key) : $prefix . $key; + } + + //-------------------------------------------------------------------- + /** * Get an item from the cache, or execute the given Closure and store the result. * diff --git a/system/Cache/Handlers/FileHandler.php b/system/Cache/Handlers/FileHandler.php index 426a7fb92306..b507390c78cf 100644 --- a/system/Cache/Handlers/FileHandler.php +++ b/system/Cache/Handlers/FileHandler.php @@ -21,11 +21,9 @@ class FileHandler extends BaseHandler { /** - * Prefixed to all cache names. - * - * @var string + * Maximum key length. */ - protected $prefix; + public const MAX_KEY_LENGTH = 255; /** * Where to store cached files on the disk. @@ -94,8 +92,7 @@ public function initialize() */ public function get(string $key) { - $key = $this->prefix . $key; - + $key = static::validateKey($key, $this->prefix); $data = $this->getItem($key); return is_array($data) ? $data['data'] : null; @@ -114,7 +111,7 @@ public function get(string $key) */ public function save(string $key, $value, int $ttl = 60) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); $contents = [ 'time' => time(), @@ -152,7 +149,7 @@ public function save(string $key, $value, int $ttl = 60) */ public function delete(string $key) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); return is_file($this->path . $key) && unlink($this->path . $key); } @@ -170,7 +167,7 @@ public function deleteMatching(string $pattern) { $deleted = 0; - foreach (glob($this->path . $pattern, GLOB_NOSORT) as $filename) + foreach (glob($this->path . $pattern, GLOB_NOSORT) as $filename) { if (is_file($filename) && @unlink($filename)) { @@ -193,8 +190,7 @@ public function deleteMatching(string $pattern) */ public function increment(string $key, int $offset = 1) { - $key = $this->prefix . $key; - + $key = static::validateKey($key, $this->prefix); $data = $this->getItem($key); if ($data === false) @@ -222,12 +218,11 @@ public function increment(string $key, int $offset = 1) * @param string $key Cache ID * @param integer $offset Step/value to increase by * - * @return bool + * @return boolean */ public function decrement(string $key, int $offset = 1) { - $key = $this->prefix . $key; - + $key = static::validateKey($key, $this->prefix); $data = $this->getItem($key); if ($data === false) @@ -288,7 +283,7 @@ public function getCacheInfo() */ public function getMetaData(string $key) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); if (! is_file($this->path . $key)) { @@ -342,26 +337,26 @@ public function isSupported(): bool * Does the heavy lifting of actually retrieving the file and * verifying it's age. * - * @param string $key + * @param string $filename * * @return boolean|mixed */ - protected function getItem(string $key) + protected function getItem(string $filename) { - if (! is_file($this->path . $key)) + if (! is_file($this->path . $filename)) { return false; } - $data = unserialize(file_get_contents($this->path . $key)); + $data = unserialize(file_get_contents($this->path . $filename)); // @phpstan-ignore-next-line if ($data['ttl'] > 0 && time() > $data['time'] + $data['ttl']) { // If the file is still there then remove it - if (is_file($this->path . $key)) + if (is_file($this->path . $filename)) { - unlink($this->path . $key); + unlink($this->path . $filename); } return false; diff --git a/system/Cache/Handlers/MemcachedHandler.php b/system/Cache/Handlers/MemcachedHandler.php index 0d22069f570e..00cfcb203c65 100644 --- a/system/Cache/Handlers/MemcachedHandler.php +++ b/system/Cache/Handlers/MemcachedHandler.php @@ -22,13 +22,6 @@ */ class MemcachedHandler extends BaseHandler { - /** - * Prefixed to all cache names. - * - * @var string - */ - protected $prefix; - /** * The memcached object * @@ -166,7 +159,7 @@ public function initialize() */ public function get(string $key) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); if ($this->memcached instanceof Memcached) { @@ -206,7 +199,7 @@ public function get(string $key) */ public function save(string $key, $value, int $ttl = 60) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); if (! $this->config['raw']) { @@ -242,7 +235,7 @@ public function save(string $key, $value, int $ttl = 60) */ public function delete(string $key) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); return $this->memcached->delete($key); } @@ -278,7 +271,7 @@ public function increment(string $key, int $offset = 1) return false; } - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); // @phpstan-ignore-next-line return $this->memcached->increment($key, $offset, $offset, 60); @@ -301,7 +294,7 @@ public function decrement(string $key, int $offset = 1) return false; } - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); //FIXME: third parameter isn't other handler actions. // @phpstan-ignore-next-line @@ -349,8 +342,7 @@ public function getCacheInfo() */ public function getMetaData(string $key) { - $key = $this->prefix . $key; - + $key = static::validateKey($key, $this->prefix); $stored = $this->memcached->get($key); // if not an array, don't try to count for PHP7.2 diff --git a/system/Cache/Handlers/PredisHandler.php b/system/Cache/Handlers/PredisHandler.php index f66a1f9d2436..3a89173c0c73 100644 --- a/system/Cache/Handlers/PredisHandler.php +++ b/system/Cache/Handlers/PredisHandler.php @@ -22,13 +22,6 @@ */ class PredisHandler extends BaseHandler { - /** - * Prefixed to all cache names. - * - * @var string - */ - protected $prefix; - /** * Default config * @@ -101,8 +94,12 @@ public function initialize() */ public function get(string $key) { - $data = array_combine( - ['__ci_type', '__ci_value'], + $key = static::validateKey($key); + + $data = array_combine([ + '__ci_type', + '__ci_value', + ], $this->redis->hmget($key, ['__ci_type', '__ci_value']) ); @@ -141,6 +138,8 @@ public function get(string $key) */ public function save(string $key, $value, int $ttl = 60) { + $key = static::validateKey($key); + switch ($dataType = gettype($value)) { case 'array': @@ -179,6 +178,8 @@ public function save(string $key, $value, int $ttl = 60) */ public function delete(string $key) { + $key = static::validateKey($key); + return $this->redis->del($key) === 1; } @@ -215,6 +216,8 @@ public function deleteMatching(string $pattern) */ public function increment(string $key, int $offset = 1) { + $key = static::validateKey($key); + return $this->redis->hincrby($key, 'data', $offset); } @@ -230,6 +233,8 @@ public function increment(string $key, int $offset = 1) */ public function decrement(string $key, int $offset = 1) { + $key = static::validateKey($key); + return $this->redis->hincrby($key, 'data', -$offset); } @@ -273,6 +278,8 @@ public function getCacheInfo() */ public function getMetaData(string $key) { + $key = static::validateKey($key); + $data = array_combine(['__ci_value'], $this->redis->hmget($key, ['__ci_value'])); if (isset($data['__ci_value']) && $data['__ci_value'] !== false) diff --git a/system/Cache/Handlers/RedisHandler.php b/system/Cache/Handlers/RedisHandler.php index 6b9175bce1f1..58fcec83d864 100644 --- a/system/Cache/Handlers/RedisHandler.php +++ b/system/Cache/Handlers/RedisHandler.php @@ -21,13 +21,6 @@ */ class RedisHandler extends BaseHandler { - /** - * Prefixed to all cache names. - * - * @var string - */ - protected $prefix; - /** * Default config * @@ -134,8 +127,7 @@ public function initialize() */ public function get(string $key) { - $key = $this->prefix . $key; - + $key = static::validateKey($key, $this->prefix); $data = $this->redis->hMGet($key, ['__ci_type', '__ci_value']); if (! isset($data['__ci_type'], $data['__ci_value']) || $data['__ci_value'] === false) @@ -173,7 +165,7 @@ public function get(string $key) */ public function save(string $key, $value, int $ttl = 60) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); switch ($dataType = gettype($value)) { @@ -216,7 +208,7 @@ public function save(string $key, $value, int $ttl = 60) */ public function delete(string $key) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); return $this->redis->del($key) === 1; } @@ -233,7 +225,7 @@ public function delete(string $key) public function deleteMatching(string $pattern) { $matchedKeys = []; - $iterator = null; + $iterator = null; do { @@ -266,7 +258,7 @@ public function deleteMatching(string $pattern) */ public function increment(string $key, int $offset = 1) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); return $this->redis->hIncrBy($key, 'data', $offset); } @@ -283,7 +275,7 @@ public function increment(string $key, int $offset = 1) */ public function decrement(string $key, int $offset = 1) { - $key = $this->prefix . $key; + $key = static::validateKey($key, $this->prefix); return $this->redis->hIncrBy($key, 'data', -$offset); } @@ -328,8 +320,7 @@ public function getCacheInfo() */ public function getMetaData(string $key) { - $key = $this->prefix . $key; - + $key = static::validateKey($key, $this->prefix); $value = $this->get($key); if ($value !== null) diff --git a/system/Cache/Handlers/WincacheHandler.php b/system/Cache/Handlers/WincacheHandler.php index cc49103a0df0..3c8af823d384 100644 --- a/system/Cache/Handlers/WincacheHandler.php +++ b/system/Cache/Handlers/WincacheHandler.php @@ -21,15 +21,6 @@ */ class WincacheHandler extends BaseHandler { - /** - * Prefixed to all cache names. - * - * @var string - */ - protected $prefix; - - //-------------------------------------------------------------------- - /** * Constructor. * @@ -60,6 +51,7 @@ public function initialize() */ public function get(string $key) { + $key = static::validateKey($key); $success = false; $data = wincache_ucache_get($this->prefix . $key, $success); @@ -81,6 +73,8 @@ public function get(string $key) */ public function save(string $key, $value, int $ttl = 60) { + $key = static::validateKey($key); + return wincache_ucache_set($this->prefix . $key, $value, $ttl); } @@ -95,6 +89,8 @@ public function save(string $key, $value, int $ttl = 60) */ public function delete(string $key) { + $key = static::validateKey($key); + return wincache_ucache_delete($this->prefix . $key); } @@ -124,6 +120,8 @@ public function deleteMatching(string $pattern) */ public function increment(string $key, int $offset = 1) { + $key = static::validateKey($key); + return wincache_ucache_inc($this->prefix . $key, $offset); } @@ -139,6 +137,8 @@ public function increment(string $key, int $offset = 1) */ public function decrement(string $key, int $offset = 1) { + $key = static::validateKey($key); + return wincache_ucache_dec($this->prefix . $key, $offset); } @@ -183,6 +183,8 @@ public function getCacheInfo() */ public function getMetaData(string $key) { + $key = static::validateKey($key); + if ($stored = wincache_ucache_info(false, $this->prefix . $key)) { $age = $stored['ucache_entries'][1]['age_seconds']; diff --git a/tests/_support/Cache/RestrictiveHandler.php b/tests/_support/Cache/RestrictiveHandler.php new file mode 100644 index 000000000000..c74e3771190b --- /dev/null +++ b/tests/_support/Cache/RestrictiveHandler.php @@ -0,0 +1,26 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Tests\Support\Cache; + +use CodeIgniter\Cache\Handlers\DummyHandler; + +/** + * Handler with unnecessarily restrictive + * key limit for testing validateKey. + */ +class RestrictiveHandler extends DummyHandler +{ + /** + * Maximum key length. + */ + public const MAX_KEY_LENGTH = 10; +} diff --git a/tests/system/Cache/Handlers/BaseHandlerTest.php b/tests/system/Cache/Handlers/BaseHandlerTest.php new file mode 100644 index 000000000000..4a65c115fd88 --- /dev/null +++ b/tests/system/Cache/Handlers/BaseHandlerTest.php @@ -0,0 +1,66 @@ +expectException('InvalidArgumentException'); + $this->expectExceptionMessage('Cache key must be a string'); + + BaseHandler::validateKey(false); + } + + public function invalidTypeProvider(): array + { + return [ + [true], + [false], + [null], + [42], + [new stdClass], + ]; + } + + public function testValidateKeySuccess() + { + $string = 'banana'; + $result = BaseHandler::validateKey($string); + + $this->assertSame($string, $result); + } + + public function testValidateKeySuccessWithPrefix() + { + $string = 'banana'; + $result = BaseHandler::validateKey($string, 'prefix'); + + $this->assertSame('prefix' . $string, $result); + } + + public function testValidateExcessiveLength() + { + $string = 'MoreThanTenCharacters'; + $expected = md5($string); + + $result = RestrictiveHandler::validateKey($string); + + $this->assertSame($expected, $result); + } + + public function testValidateExcessiveLengthWithPrefix() + { + $string = 'MoreThanTenCharacters'; + $expected = 'prefix' . md5($string); + + $result = RestrictiveHandler::validateKey($string, 'prefix'); + + $this->assertSame($expected, $result); + } +} diff --git a/tests/system/Cache/Handlers/FileHandlerTest.php b/tests/system/Cache/Handlers/FileHandlerTest.php index e9465aadcd00..bd2fb0ce13e3 100644 --- a/tests/system/Cache/Handlers/FileHandlerTest.php +++ b/tests/system/Cache/Handlers/FileHandlerTest.php @@ -51,6 +51,8 @@ protected function setUp(): void public function tearDown(): void { + parent::tearDown(); + if (is_dir($this->config->file['storePath'])) { chmod($this->config->file['storePath'], 0777); @@ -125,6 +127,17 @@ public function testSave() $this->assertFalse($this->fileHandler->save(self::$key2, 'value')); } + public function testSaveExcessiveKeyLength() + { + $key = str_repeat('a', 300); + $file = $this->config->file['storePath'] . DIRECTORY_SEPARATOR . md5($key); + + $this->assertTrue($this->fileHandler->save($key, 'value')); + $this->assertFileExists($file); + + unlink($file); + } + public function testDelete() { $this->fileHandler->save(self::$key1, 'value');