Guard cache regions against corrupted data

For some bizarre reason the underlying cache drivers are returning unexpected values, which are leaking to the cache objects and causing them to error. This makes our cache regions much more strict about the types that are fetched from the cache provider, ensuring that no invalid information is ever sent to the hydrators.
doctrine · Aug 14, 2019 · 80503c4 · 80503c4
1 parent 3577064
commit 80503c4
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 2 deletions.
diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultMultiGetRegion.php
@@ -21,6 +21,7 @@
 namespace Doctrine\ORM\Cache\Region;
 
 use Doctrine\Common\Cache\MultiGetCache;
+use Doctrine\ORM\Cache\CacheEntry;
 use Doctrine\ORM\Cache\CollectionCacheEntry;
 
 /**
@@ -67,7 +68,12 @@ public function getMultiple(CollectionCacheEntry $collection)
         }
 
         $returnableItems = [];
+
         foreach ($keysToRetrieve as $index => $key) {
+            if (! $items[$key] instanceof CacheEntry) {
+                return null;
+            }
+
             $returnableItems[$index] = $items[$key];
         }
 

diff --git a/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php b/lib/Doctrine/ORM/Cache/Region/DefaultRegion.php
@@ -94,7 +94,13 @@ public function contains(CacheKey $key)
      */
     public function get(CacheKey $key)
     {
-        return $this->cache->fetch($this->getCacheEntryKey($key)) ?: null;
+        $entry = $this->cache->fetch($this->getCacheEntryKey($key));
+
+        if (! $entry instanceof CacheEntry) {
+            return null;
+        }
+
+        return $entry;
     }
 
     /**
@@ -108,7 +114,7 @@ public function getMultiple(CollectionCacheEntry $collection)
             $entryKey   = $this->getCacheEntryKey($key);
             $entryValue = $this->cache->fetch($entryKey);
 
-            if ($entryValue === false) {
+            if (! $entryValue instanceof CacheEntry) {
                 return null;
             }
 

diff --git a/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/DefaultRegionTest.php
@@ -95,6 +95,32 @@ public function testGetMulti()
         $this->assertEquals($value1, $actual[0]);
         $this->assertEquals($value2, $actual[1]);
     }
+
+    /**
+     * @test
+     * @group GH7266
+     */
+    public function corruptedDataDoesNotLeakIntoApplicationWhenGettingSingleEntry() : void
+    {
+        $key1 = new CacheKeyMock('key.1');
+        $this->cache->save($this->region->getName() . '_' . $key1->hash, 'a-very-invalid-value');
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertNull($this->region->get($key1));
+    }
+
+    /**
+     * @test
+     * @group GH7266
+     */
+    public function corruptedDataDoesNotLeakIntoApplicationWhenGettingMultipleEntries() : void
+    {
+        $key1 = new CacheKeyMock('key.1');
+        $this->cache->save($this->region->getName() . '_' . $key1->hash, 'a-very-invalid-value');
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertNull($this->region->getMultiple(new CollectionCacheEntry([$key1])));
+    }
 }
 
 /**

diff --git a/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php b/tests/Doctrine/Tests/ORM/Cache/MultiGetRegionTest.php
@@ -39,4 +39,17 @@ public function testGetMulti()
         $this->assertEquals($value1, $actual[0]);
         $this->assertEquals($value2, $actual[1]);
     }
+
+    /**
+     * @test
+     * @group GH7266
+     */
+    public function corruptedDataDoesNotLeakIntoApplication() : void
+    {
+        $key1 = new CacheKeyMock('key.1');
+        $this->cache->save($this->region->getName() . '_' . $key1->hash, 'a-very-invalid-value');
+
+        self::assertTrue($this->region->contains($key1));
+        self::assertNull($this->region->getMultiple(new CollectionCacheEntry([$key1])));
+    }
 }