Browse Source

Don't manipulate keys, throw exceptions instead.

Replacing special characters can result in cache key collisions which
can lead to unexpected results. Instead we can throw exceptions on
invalid data.
Mark Story 7 years ago
parent
commit
dc612c58e2
2 changed files with 33 additions and 13 deletions
  1. 9 6
      src/Cache/Engine/FileEngine.php
  2. 24 7
      tests/TestCase/Cache/Engine/FileEngineTest.php

+ 9 - 6
src/Cache/Engine/FileEngine.php

@@ -16,7 +16,7 @@ declare(strict_types=1);
 namespace Cake\Cache\Engine;
 
 use Cake\Cache\CacheEngine;
-use Cake\Utility\Inflector;
+use Cake\Cache\InvalidArgumentException;
 use CallbackFilterIterator;
 use Exception;
 use LogicException;
@@ -422,11 +422,14 @@ class FileEngine extends CacheEngine
     {
         $key = parent::_key($key);
 
-        return Inflector::underscore(str_replace(
-            [DIRECTORY_SEPARATOR, '/', '.', '<', '>', '?', ':', '|', '*', '"'],
-            '_',
-            (string)$key
-        ));
+        if (preg_match('/[\/\\<>?:|*"]/', (string)$key)) {
+            throw new InvalidArgumentException(
+                "Cache key `{$key}` contains invalid characters. " .
+                'You cannot use /, \\, <, >, ?, :, |, *, or " in cache keys.'
+            );
+        }
+
+        return $key;
     }
 
     /**

+ 24 - 7
tests/TestCase/Cache/Engine/FileEngineTest.php

@@ -16,6 +16,7 @@ declare(strict_types=1);
 namespace Cake\Test\TestCase\Cache\Engine;
 
 use Cake\Cache\Cache;
+use Cake\Cache\InvalidArgumentException;
 use Cake\Cache\Engine\FileEngine;
 use Cake\Core\Configure;
 use Cake\TestSuite\TestCase;
@@ -292,21 +293,37 @@ class FileEngineTest extends TestCase
     {
         $result = Cache::write('views.countries.something', 'here', 'file_test');
         $this->assertTrue($result);
-        $this->assertFileExists(TMP . 'tests/cake_views_countries_something');
+        $this->assertFileExists(TMP . 'tests/cake_views.countries.something');
 
         $result = Cache::read('views.countries.something', 'file_test');
         $this->assertEquals('here', $result);
 
         $result = Cache::clear('file_test');
         $this->assertTrue($result);
+    }
 
-        $result = Cache::write('domain.test.com:8080', 'here', 'file_test');
-        $this->assertTrue($result);
-        $this->assertFileExists(TMP . 'tests/cake_domain_test_com_8080');
+    /**
+     * Test invalid key() containing :
+     *
+     * @return void
+     */
+    public function testInvalidKeyColon()
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('contains invalid characters');
+        Cache::write('domain.test.com:8080', 'here', 'file_test');
+    }
 
-        $result = Cache::write('command>dir|more', 'here', 'file_test');
-        $this->assertTrue($result);
-        $this->assertFileExists(TMP . 'tests/cake_command_dir_more');
+    /**
+     * Test invalid key() containing >
+     *
+     * @return void
+     */
+    public function testInvalidKeyAngleBracket()
+    {
+        $this->expectException(InvalidArgumentException::class);
+        $this->expectExceptionMessage('contains invalid characters');
+        Cache::write('command>dir|more', 'here', 'file_test');
     }
 
     /**