Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache Key Validation #4637

Merged
merged 3 commits into from
May 5, 2021
Merged

Cache Key Validation #4637

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
01f8b4b
Add cache key validation and hashing
MGatner May 4, 2021
490abe8
Apply review recommendations
MGatner May 5, 2021
cf52dda
[ci skip] Add method to UG
MGatner May 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions system/Cache/Handlers/BaseHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,63 @@
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.
* From 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP_INT_MAX is an excessively long amount. Can we have a reasonable number here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically meant to be "unlimited". I couldn't find limits for all the handlers, but P/Redis stores the keys as binary strings with a limit of 512 MB - huge, in other words: https://stackoverflow.com/questions/5606106/what-is-the-maximum-value-size-you-can-store-in-redis

I'm open to other suggestions, or varying the specific handlers more, but that's how I got there.


/**
* 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.
* From https://github.com/symfony/cache/blob/7b024c6726af21fd4984ac8d1eae2b9f3d90de88/CacheItem.php#L158
*
* @param string $key The key to validate
* @param string $prefix Optional prefix to include in length calculations
*
* @throws InvalidArgumentException When $key is not valid
*/
public static function validateKey($key, $prefix = ''): string
lonnieezell marked this conversation as resolved.
Show resolved Hide resolved
{
if (! is_string($key))
{
throw new InvalidArgumentException('Cache key must be a string');
lonnieezell marked this conversation as resolved.
Show resolved Hide resolved
}
paulbalandan marked this conversation as resolved.
Show resolved Hide resolved
if ($key === '')
{
throw new InvalidArgumentException('Cache key cannot be empty.');
lonnieezell marked this conversation as resolved.
Show resolved Hide resolved
}
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;
paulbalandan marked this conversation as resolved.
Show resolved Hide resolved
}

//--------------------------------------------------------------------

/**
* Get an item from the cache, or execute the given Closure and store the result.
*
Expand Down
37 changes: 16 additions & 21 deletions system/Cache/Handlers/FileHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = PHP_MAXPATHLEN;

/**
* Where to store cached files on the disk.
Expand Down Expand Up @@ -94,8 +92,7 @@ public function initialize()
*/
public function get(string $key)
{
$key = $this->prefix . $key;

$key = static::validateKey($key, $this->prefix);
MGatner marked this conversation as resolved.
Show resolved Hide resolved
$data = $this->getItem($key);

return is_array($data) ? $data['data'] : null;
Expand All @@ -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(),
Expand Down Expand Up @@ -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);
}
Expand All @@ -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))
{
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
{
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 6 additions & 14 deletions system/Cache/Handlers/MemcachedHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,6 @@
*/
class MemcachedHandler extends BaseHandler
{
/**
* Prefixed to all cache names.
*
* @var string
*/
protected $prefix;

/**
* The memcached object
*
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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'])
{
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
25 changes: 16 additions & 9 deletions system/Cache/Handlers/PredisHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,6 @@
*/
class PredisHandler extends BaseHandler
{
/**
* Prefixed to all cache names.
*
* @var string
*/
protected $prefix;

/**
* Default config
*
Expand Down Expand Up @@ -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'])
);

Expand Down Expand Up @@ -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':
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}

Expand Down Expand Up @@ -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)
Expand Down
Loading