From 9ff98dfb59ab9f30ae8583e98033e5337543f246 Mon Sep 17 00:00:00 2001 From: Nico Hoffmann Date: Wed, 24 Jul 2024 12:00:58 +0200 Subject: [PATCH 1/3] Cache classes: fix method order --- src/Cache/Cache.php | 11 +--- src/Cache/FileCache.php | 115 +++++++++++++++++++------------------- src/Cache/MemCached.php | 57 +++++++++---------- src/Cache/MemoryCache.php | 44 +++++++-------- src/Cache/NullCache.php | 38 ++++++------- src/Cache/Value.php | 24 +++----- 6 files changed, 139 insertions(+), 150 deletions(-) diff --git a/src/Cache/Cache.php b/src/Cache/Cache.php index 5b2a05bf86..8099244310 100644 --- a/src/Cache/Cache.php +++ b/src/Cache/Cache.php @@ -18,17 +18,12 @@ */ abstract class Cache { - /** - * Stores all options for the driver - */ - protected array $options = []; - /** * Sets all parameters which are needed to connect to the cache storage */ - public function __construct(array $options = []) - { - $this->options = $options; + public function __construct( + protected array $options = [] + ) { } /** diff --git a/src/Cache/FileCache.php b/src/Cache/FileCache.php index 8181f192b3..97d1f149a5 100644 --- a/src/Cache/FileCache.php +++ b/src/Cache/FileCache.php @@ -30,8 +30,9 @@ class FileCache extends Cache * 'prefix' (default: none) * 'extension' (file extension for cache files, default: none) */ - public function __construct(array $options) - { + public function __construct( + array $options + ) { parent::__construct([ 'root' => null, 'prefix' => null, @@ -51,20 +52,28 @@ public function __construct(array $options) } /** - * Returns whether the cache is ready to - * store values + * Checks when the cache has been created; + * returns the creation timestamp on success + * and false if the item does not exist */ - public function enabled(): bool + public function created(string $key): int|false { - return is_writable($this->root) === true; + // use the modification timestamp + // as indicator when the cache has been created/overwritten + clearstatcache(); + + // get the file for this cache key + $file = $this->file($key); + return file_exists($file) ? filemtime($file) : false; } /** - * Returns the full root including prefix + * Returns whether the cache is ready to + * store values */ - public function root(): string + public function enabled(): bool { - return $this->root; + return is_writable($this->root) === true; } /** @@ -116,47 +125,19 @@ protected function file(string $key): string } /** - * Writes an item to the cache for a given number of minutes and - * returns whether the operation was successful - * - * - * // put an item in the cache for 15 minutes - * $cache->set('value', 'my value', 15); - * - */ - public function set(string $key, $value, int $minutes = 0): bool - { - $file = $this->file($key); - - return F::write($file, (new Value($value, $minutes))->toJson()); - } - - /** - * Internal method to retrieve the raw cache value; - * needs to return a Value object or null if not found - */ - public function retrieve(string $key): Value|null - { - $file = $this->file($key); - $value = F::read($file); - - return $value ? Value::fromJson($value) : null; - } - - /** - * Checks when the cache has been created; - * returns the creation timestamp on success - * and false if the item does not exist + * Flushes the entire cache and returns + * whether the operation was successful */ - public function created(string $key): int|false + public function flush(): bool { - // use the modification timestamp - // as indicator when the cache has been created/overwritten - clearstatcache(); + if ( + Dir::remove($this->root) === true && + Dir::make($this->root) === true + ) { + return true; + } - // get the file for this cache key - $file = $this->file($key); - return file_exists($file) ? filemtime($file) : false; + return false; // @codeCoverageIgnore } /** @@ -209,18 +190,38 @@ protected function removeEmptyDirectories(string $dir): void } /** - * Flushes the entire cache and returns - * whether the operation was successful + * Internal method to retrieve the raw cache value; + * needs to return a Value object or null if not found */ - public function flush(): bool + public function retrieve(string $key): Value|null { - if ( - Dir::remove($this->root) === true && - Dir::make($this->root) === true - ) { - return true; - } + $file = $this->file($key); + $value = F::read($file); - return false; // @codeCoverageIgnore + return $value ? Value::fromJson($value) : null; + } + + /** + * Returns the full root including prefix + */ + public function root(): string + { + return $this->root; + } + + /** + * Writes an item to the cache for a given number of minutes and + * returns whether the operation was successful + * + * + * // put an item in the cache for 15 minutes + * $cache->set('value', 'my value', 15); + * + */ + public function set(string $key, $value, int $minutes = 0): bool + { + $file = $this->file($key); + + return F::write($file, (new Value($value, $minutes))->toJson()); } } diff --git a/src/Cache/MemCached.php b/src/Cache/MemCached.php index 0609e95076..6d8a95af5b 100644 --- a/src/Cache/MemCached.php +++ b/src/Cache/MemCached.php @@ -32,8 +32,9 @@ class MemCached extends Cache * 'port' (default: 11211) * 'prefix' (default: null) */ - public function __construct(array $options = []) - { + public function __construct( + array $options = [] + ) { parent::__construct([ 'host' => 'localhost', 'port' => 11211, @@ -58,48 +59,48 @@ public function enabled(): bool } /** - * Writes an item to the cache for a given number of minutes and - * returns whether the operation was successful - * - * - * // put an item in the cache for 15 minutes - * $cache->set('value', 'my value', 15); - * + * Flushes the entire cache and returns + * whether the operation was successful; + * WARNING: Memcached only supports flushing the whole cache at once! */ - public function set(string $key, $value, int $minutes = 0): bool + public function flush(): bool { - $key = $this->key($key); - $value = (new Value($value, $minutes))->toJson(); - $expires = $this->expiration($minutes); - return $this->connection->set($key, $value, $expires); + return $this->connection->flush(); } /** - * Internal method to retrieve the raw cache value; - * needs to return a Value object or null if not found + * Removes an item from the cache and returns + * whether the operation was successful */ - public function retrieve(string $key): Value|null + public function remove(string $key): bool { - $value = $this->connection->get($this->key($key)); - return Value::fromJson($value); + return $this->connection->delete($this->key($key)); } /** - * Removes an item from the cache and returns - * whether the operation was successful + * Internal method to retrieve the raw cache value; + * needs to return a Value object or null if not found */ - public function remove(string $key): bool + public function retrieve(string $key): Value|null { - return $this->connection->delete($this->key($key)); + $value = $this->connection->get($this->key($key)); + return Value::fromJson($value); } /** - * Flushes the entire cache and returns - * whether the operation was successful; - * WARNING: Memcached only supports flushing the whole cache at once! + * Writes an item to the cache for a given number of minutes and + * returns whether the operation was successful + * + * + * // put an item in the cache for 15 minutes + * $cache->set('value', 'my value', 15); + * */ - public function flush(): bool + public function set(string $key, $value, int $minutes = 0): bool { - return $this->connection->flush(); + $key = $this->key($key); + $value = (new Value($value, $minutes))->toJson(); + $expires = $this->expiration($minutes); + return $this->connection->set($key, $value, $expires); } } diff --git a/src/Cache/MemoryCache.php b/src/Cache/MemoryCache.php index 55fdbc1298..b8d563e682 100644 --- a/src/Cache/MemoryCache.php +++ b/src/Cache/MemoryCache.php @@ -28,29 +28,15 @@ public function enabled(): bool } /** - * Writes an item to the cache for a given number of minutes and - * returns whether the operation was successful - * - * - * // put an item in the cache for 15 minutes - * $cache->set('value', 'my value', 15); - * + * Flushes the entire cache and returns + * whether the operation was successful */ - public function set(string $key, $value, int $minutes = 0): bool + public function flush(): bool { - $this->store[$key] = new Value($value, $minutes); + $this->store = []; return true; } - /** - * Internal method to retrieve the raw cache value; - * needs to return a Value object or null if not found - */ - public function retrieve(string $key): Value|null - { - return $this->store[$key] ?? null; - } - /** * Removes an item from the cache and returns * whether the operation was successful @@ -66,12 +52,26 @@ public function remove(string $key): bool } /** - * Flushes the entire cache and returns - * whether the operation was successful + * Internal method to retrieve the raw cache value; + * needs to return a Value object or null if not found */ - public function flush(): bool + public function retrieve(string $key): Value|null { - $this->store = []; + return $this->store[$key] ?? null; + } + + /** + * Writes an item to the cache for a given number of minutes and + * returns whether the operation was successful + * + * + * // put an item in the cache for 15 minutes + * $cache->set('value', 'my value', 15); + * + */ + public function set(string $key, $value, int $minutes = 0): bool + { + $this->store[$key] = new Value($value, $minutes); return true; } } diff --git a/src/Cache/NullCache.php b/src/Cache/NullCache.php index 7c2211d276..829bb5b0e1 100644 --- a/src/Cache/NullCache.php +++ b/src/Cache/NullCache.php @@ -23,42 +23,42 @@ public function enabled(): bool } /** - * Writes an item to the cache for a given number of minutes and - * returns whether the operation was successful - * - * - * // put an item in the cache for 15 minutes - * $cache->set('value', 'my value', 15); - * + * Flushes the entire cache and returns + * whether the operation was successful */ - public function set(string $key, $value, int $minutes = 0): bool + public function flush(): bool { return true; } /** - * Internal method to retrieve the raw cache value; - * needs to return a Value object or null if not found + * Removes an item from the cache and returns + * whether the operation was successful */ - public function retrieve(string $key): Value|null + public function remove(string $key): bool { - return null; + return true; } /** - * Removes an item from the cache and returns - * whether the operation was successful + * Internal method to retrieve the raw cache value; + * needs to return a Value object or null if not found */ - public function remove(string $key): bool + public function retrieve(string $key): Value|null { - return true; + return null; } /** - * Flushes the entire cache and returns - * whether the operation was successful + * Writes an item to the cache for a given number of minutes and + * returns whether the operation was successful + * + * + * // put an item in the cache for 15 minutes + * $cache->set('value', 'my value', 15); + * */ - public function flush(): bool + public function set(string $key, $value, int $minutes = 0): bool { return true; } diff --git a/src/Cache/Value.php b/src/Cache/Value.php index 2fc64a5bb5..28a497fa01 100644 --- a/src/Cache/Value.php +++ b/src/Cache/Value.php @@ -17,18 +17,6 @@ */ class Value { - /** - * Cached value - */ - protected mixed $value; - - /** - * the number of minutes until the value expires - * @todo Rename this property to $expiry to reflect - * both minutes and absolute timestamps - */ - protected int $minutes; - /** * Creation timestamp */ @@ -41,11 +29,15 @@ class Value * or an absolute UNIX timestamp * @param int|null $created the UNIX timestamp when the value has been created * (defaults to the current time) + * + * @todo Rename $minutes property to $expiry to reflect + * both minutes and absolute timestamps */ - public function __construct($value, int $minutes = 0, int|null $created = null) - { - $this->value = $value; - $this->minutes = $minutes; + public function __construct( + protected mixed $value, + protected int $minutes = 0, + int|null $created = null + ) { $this->created = $created ?? time(); } From 32d11a9dc2bed0edb8b8176d86a0e4f78011ad46 Mon Sep 17 00:00:00 2001 From: Nico Hoffmann Date: Wed, 24 Jul 2024 12:26:19 +0200 Subject: [PATCH 2/3] `Cache::isEmpty()` --- src/Cache/ApcuCache.php | 15 +++++++++++++++ src/Cache/Cache.php | 6 ++++++ src/Cache/FileCache.php | 9 +++++++++ src/Cache/MemCached.php | 9 +++++++++ src/Cache/MemoryCache.php | 9 +++++++++ src/Cache/NullCache.php | 9 +++++++++ tests/Cache/ApcuCacheTest.php | 12 ++++++++++++ tests/Cache/FileCacheTest.php | 14 ++++++++++++++ tests/Cache/MemCachedTest.php | 12 ++++++++++++ tests/Cache/MemoryCacheTest.php | 12 ++++++++++++ tests/Cache/NullCacheTest.php | 12 ++++++++++++ tests/Cache/mocks.php | 27 +++++++-------------------- 12 files changed, 126 insertions(+), 20 deletions(-) diff --git a/src/Cache/ApcuCache.php b/src/Cache/ApcuCache.php index ee132153a7..fff617fe47 100644 --- a/src/Cache/ApcuCache.php +++ b/src/Cache/ApcuCache.php @@ -45,6 +45,21 @@ public function flush(): bool return apcu_clear_cache(); } + /** + * Whether the cache has any entry, + * irrespective whether the entries have expired or not + */ + public function isEmpty(): bool + { + if (empty($this->options['prefix']) === false) { + $iterator = new APCUIterator('!^' . preg_quote($this->options['prefix']) . '!'); + } else { + $iterator = new APCUIterator(); + } + + return $iterator->getTotalCount() === 0; + } + /** * Removes an item from the cache and returns * whether the operation was successful diff --git a/src/Cache/Cache.php b/src/Cache/Cache.php index 8099244310..745a82837e 100644 --- a/src/Cache/Cache.php +++ b/src/Cache/Cache.php @@ -177,6 +177,12 @@ public function getOrSet( return $result; } + /** + * Whether the cache has any entry, + * irrespective whether the entries have expired or not + */ + abstract public function isEmpty(): bool; + /** * Adds the prefix to the key if given */ diff --git a/src/Cache/FileCache.php b/src/Cache/FileCache.php index 97d1f149a5..650c5bf8d9 100644 --- a/src/Cache/FileCache.php +++ b/src/Cache/FileCache.php @@ -140,6 +140,15 @@ public function flush(): bool return false; // @codeCoverageIgnore } + /** + * Whether the cache has any entry, + * irrespective whether the entries have expired or not + */ + public function isEmpty(): bool + { + return Dir::isEmpty($this->root); + } + /** * Removes an item from the cache and returns * whether the operation was successful diff --git a/src/Cache/MemCached.php b/src/Cache/MemCached.php index 6d8a95af5b..1dcb938ada 100644 --- a/src/Cache/MemCached.php +++ b/src/Cache/MemCached.php @@ -68,6 +68,15 @@ public function flush(): bool return $this->connection->flush(); } + /** + * Whether the cache has any entry, + * irrespective whether the entries have expired or not + */ + public function isEmpty(): bool + { + return count($this->connection->getAllKeys()) === 0; + } + /** * Removes an item from the cache and returns * whether the operation was successful diff --git a/src/Cache/MemoryCache.php b/src/Cache/MemoryCache.php index b8d563e682..bea59dbdba 100644 --- a/src/Cache/MemoryCache.php +++ b/src/Cache/MemoryCache.php @@ -37,6 +37,15 @@ public function flush(): bool return true; } + /** + * Whether the cache has any entry, + * irrespective whether the entries have expired or not + */ + public function isEmpty(): bool + { + return count($this->store) === 0; + } + /** * Removes an item from the cache and returns * whether the operation was successful diff --git a/src/Cache/NullCache.php b/src/Cache/NullCache.php index 829bb5b0e1..438cd016f2 100644 --- a/src/Cache/NullCache.php +++ b/src/Cache/NullCache.php @@ -31,6 +31,15 @@ public function flush(): bool return true; } + /** + * Whether the cache has any entry, + * irrespective whether the entries have expired or not + */ + public function isEmpty(): bool + { + return true; + } + /** * Removes an item from the cache and returns * whether the operation was successful diff --git a/tests/Cache/ApcuCacheTest.php b/tests/Cache/ApcuCacheTest.php index 6f0cb59614..371a3f2d4b 100644 --- a/tests/Cache/ApcuCacheTest.php +++ b/tests/Cache/ApcuCacheTest.php @@ -29,6 +29,18 @@ public function testEnabled() $this->assertTrue($cache->enabled()); } + /** + * @covers ::isEmpty + */ + public function testIsEmpty() + { + $cache = new ApcuCache(); + + $this->assertTrue($cache->isEmpty()); + $this->assertTrue($cache->set('foo', 'A basic value')); + $this->assertFalse($cache->isEmpty()); + } + /** * @covers ::set * @covers ::exists diff --git a/tests/Cache/FileCacheTest.php b/tests/Cache/FileCacheTest.php index 57edcffbe7..a61d15fe15 100644 --- a/tests/Cache/FileCacheTest.php +++ b/tests/Cache/FileCacheTest.php @@ -208,6 +208,20 @@ public function testFile() ); } + /** + * @covers ::isEmpty + */ + public function testIsEmpty() + { + $cache = new FileCache([ + 'root' => static::TMP + ]); + + $this->assertTrue($cache->isEmpty()); + $this->assertTrue($cache->set('foo', 'A basic value')); + $this->assertFalse($cache->isEmpty()); + } + /** * @covers ::set * @covers ::created diff --git a/tests/Cache/MemCachedTest.php b/tests/Cache/MemCachedTest.php index 8f5da5b2d7..1afb8ed091 100644 --- a/tests/Cache/MemCachedTest.php +++ b/tests/Cache/MemCachedTest.php @@ -40,6 +40,18 @@ public function testEnabled() $this->assertTrue($cache->enabled()); } + /** + * @covers ::isEmpty + */ + public function testIsEmpty() + { + $cache = new MemCached(); + + $this->assertTrue($cache->isEmpty()); + $this->assertTrue($cache->set('foo', 'A basic value')); + $this->assertFalse($cache->isEmpty()); + } + /** * @covers ::set * @covers ::retrieve diff --git a/tests/Cache/MemoryCacheTest.php b/tests/Cache/MemoryCacheTest.php index 6b20b19ada..e5dd570d48 100644 --- a/tests/Cache/MemoryCacheTest.php +++ b/tests/Cache/MemoryCacheTest.php @@ -19,6 +19,18 @@ public function testEnabled() $this->assertTrue($cache->enabled()); } + /** + * @covers ::isEmpty + */ + public function testIsEmpty() + { + $cache = new MemoryCache(); + + $this->assertTrue($cache->isEmpty()); + $this->assertTrue($cache->set('foo', 'A basic value')); + $this->assertFalse($cache->isEmpty()); + } + /** * @covers ::set * @covers ::retrieve diff --git a/tests/Cache/NullCacheTest.php b/tests/Cache/NullCacheTest.php index a1ffda82dd..cf00dcfe96 100644 --- a/tests/Cache/NullCacheTest.php +++ b/tests/Cache/NullCacheTest.php @@ -19,6 +19,18 @@ public function testEnabled() $this->assertFalse($cache->enabled()); } + /** + * @covers ::isEmpty + */ + public function testIsEmpty() + { + $cache = new NullCache(); + + $this->assertTrue($cache->isEmpty()); + $this->assertTrue($cache->set('foo', 'A basic value')); + $this->assertTrue($cache->isEmpty()); + } + /** * @covers ::set * @covers ::retrieve diff --git a/tests/Cache/mocks.php b/tests/Cache/mocks.php index ad03f34110..7aabb17221 100644 --- a/tests/Cache/mocks.php +++ b/tests/Cache/mocks.php @@ -18,31 +18,18 @@ function time(): int return 1337; } -class TestCache extends Cache +class TestCache extends MemoryCache { public array $store = []; - public function set(string $key, $value, int $minutes = 0, int|null $created = null): bool - { + public function set( + string $key, + $value, + int $minutes = 0, + int|null $created = null + ): bool { $value = new Value($value, $minutes, $created); $this->store[$key] = $value; return true; } - - public function retrieve(string $key): Value|null - { - return $this->store[$key] ?? null; - } - - public function remove(string $key): bool - { - unset($this->store[$key]); - return true; - } - - public function flush(): bool - { - $this->store = []; - return true; - } } From db11136049918b2f4a692f0db6c0b030c5cffeff Mon Sep 17 00:00:00 2001 From: Nico Hoffmann Date: Wed, 24 Jul 2024 12:38:01 +0200 Subject: [PATCH 3/3] Permalinks: look up from index if cache empty --- config/routes.php | 17 ++++++++++++----- tests/Uuid/PermalinksTest.php | 32 ++++++++++++++++++++------------ 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/config/routes.php b/config/routes.php index 0699ef1328..37f2f89430 100644 --- a/config/routes.php +++ b/config/routes.php @@ -8,6 +8,7 @@ use Kirby\Panel\Plugins; use Kirby\Toolkit\Str; use Kirby\Uuid\Uuid; +use Kirby\Uuid\Uuids; return function (App $kirby) { $api = $kirby->option('api.slug', 'api'); @@ -135,11 +136,17 @@ 'method' => 'ALL', 'env' => 'site', 'action' => function (string $type, string $id) use ($kirby) { - // try to resolve to model, but only from UUID cache; - // this ensures that only existing UUIDs can be queried - // and attackers can't force Kirby to go through the whole - // site index with a non-existing UUID - if ($model = Uuid::for($type . '://' . $id)?->model(true)) { + // try to resolve to model but if the UUID cache exists + // only allow lookup from the cache; + // only if the cache doesn't exist, use the index; + // this ensures that attackers can't force Kirby to go through + // the whole site index with a non-existing UUID + $lazy = Uuids::cache()->isEmpty() === false; + + if ($model = Uuid::for($type . '://' . $id)?->model($lazy)) { + /** + * @var \Kirby\Cms\Page|\Kirby\Cms\File $model + */ return $kirby ->response() ->redirect($model->url()); diff --git a/tests/Uuid/PermalinksTest.php b/tests/Uuid/PermalinksTest.php index 8b9039d742..8283513849 100644 --- a/tests/Uuid/PermalinksTest.php +++ b/tests/Uuid/PermalinksTest.php @@ -14,29 +14,37 @@ public function testRoute() [ 'slug' => 'a', 'content' => ['uuid' => 'my-id'] + ], + [ + 'slug' => 'b', + 'content' => ['uuid' => 'my-other-id'] ] ] ] ]); - // not cached, should fail (redirect to error) - $response = $app->call('/@/page/my-id'); - $this->assertFalse($response); + $this->assertTrue(Uuids::cache()->isEmpty()); + $uuid = $app->page('a')->uuid(); - // cached, should redirect to page A - $app->page('a')->uuid()->populate(); - $response = $app->call('/@/page/my-id')->send(); + // not cached, but cache is empty => using index to find it + $this->assertFalse($uuid->isCached()); + $response = $app->call('/@/page/my-id'); $this->assertSame(302, $response->code()); $this->assertSame('https://getkirby.com/a', $response->header('Location')); - // check if ->url() populates cache - $uuid = $app->page('a')->uuid(); - $uuid->clear(); + // now cached, redirect from cache + $this->assertTrue($uuid->isCached()); $response = $app->call('/@/page/my-id'); - $this->assertFalse($response); - $uuid->url(); - $response = $app->call('/@/page/my-id')->send(); $this->assertSame(302, $response->code()); $this->assertSame('https://getkirby.com/a', $response->header('Location')); + + // not cached but cache isn't empty => fail to prevent attacks + $uuid->clear(); + $app->page('b')->uuid()->populate(); + $this->assertFalse($uuid->isCached()); + $this->assertFalse(Uuids::cache()->isEmpty()); + + $response = $app->call('/@/page/my-id'); + $this->assertSame(false, $response); } }