[11.x] Proper rate limiter fix with phpredis serialization/compressio…

…n enabled (#54372) * Properly resolve RateLimiter not working while redis serialization or compression is enabled * Fix code style * Fix test * Skip outdated test * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
laravel · Jan 27, 2025 · 2d10f2a · 2d10f2a
1 parent 96302bf
commit 2d10f2a
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 7 deletions.
diff --git a/src/Illuminate/Cache/RateLimiter.php b/src/Illuminate/Cache/RateLimiter.php
@@ -4,6 +4,7 @@
 
 use Closure;
 use Illuminate\Contracts\Cache\Repository as Cache;
+use Illuminate\Redis\Connections\PhpRedisConnection;
 use Illuminate\Support\Collection;
 use Illuminate\Support\InteractsWithTime;
 
@@ -165,12 +166,16 @@ public function increment($key, $decaySeconds = 60, $amount = 1)
             $key.':timer', $this->availableAt($decaySeconds), $decaySeconds
         );
 
-        $added = $this->cache->add($key, 0, $decaySeconds);
+        $added = $this->withoutSerializationOrCompression(
+            fn () => $this->cache->add($key, 0, $decaySeconds)
+        );
 
         $hits = (int) $this->cache->increment($key, $amount);
 
         if (! $added && $hits == 1) {
-            $this->cache->put($key, 1, $decaySeconds);
+            $this->withoutSerializationOrCompression(
+                fn () => $this->cache->put($key, 1, $decaySeconds)
+            );
         }
 
         return $hits;
@@ -199,7 +204,7 @@ public function attempts($key)
     {
         $key = $this->cleanRateLimiterKey($key);
 
-        return $this->cache->get($key, 0);
+        return $this->withoutSerializationOrCompression(fn () => $this->cache->get($key, 0));
     }
 
     /**
@@ -282,6 +287,29 @@ public function cleanRateLimiterKey($key)
         return preg_replace('/&([a-z])[a-z]+;/i', '$1', htmlentities($key));
     }
 
+    /**
+     * Execute the given callback without serialization or compression when applicable.
+     *
+     * @param  callable  $callback
+     * @return mixed
+     */
+    protected function withoutSerializationOrCompression(callable $callback)
+    {
+        $store = $this->cache->getStore();
+
+        if (! $store instanceof RedisStore) {
+            return $callback();
+        }
+
+        $connection = $store->connection();
+
+        if (! $connection instanceof PhpRedisConnection) {
+            return $callback();
+        }
+
+        return $connection->withoutSerializationOrCompression($callback);
+    }
+
     /**
      * Resolve the rate limiter name.
      *

diff --git a/src/Illuminate/Cache/RedisStore.php b/src/Illuminate/Cache/RedisStore.php
@@ -432,10 +432,6 @@ public function setPrefix($prefix)
     protected function pack($value, $connection)
     {
         if ($connection instanceof PhpRedisConnection) {
-            if ($this->shouldBeStoredWithoutSerialization($value)) {
-                return $value;
-            }
-
             if ($connection->serialized()) {
                 return $connection->pack([$value])[0];
             }

diff --git a/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php b/src/Illuminate/Redis/Connections/PacksPhpRedisValues.php
@@ -82,6 +82,43 @@ public function pack(array $values): array
         return array_map($processor, $values);
     }
 
+    /**
+     * Execute the given callback without serialization or compression when applicable.
+     *
+     * @param  callable  $callback
+     * @return mixed
+     */
+    public function withoutSerializationOrCompression(callable $callback)
+    {
+        $client = $this->client;
+
+        $oldSerializer = null;
+
+        if ($this->serialized()) {
+            $oldSerializer = $client->getOption($client::OPT_SERIALIZER);
+            $client->setOption($client::OPT_SERIALIZER, $client::SERIALIZER_NONE);
+        }
+
+        $oldCompressor = null;
+
+        if ($this->compressed()) {
+            $oldCompressor = $client->getOption($client::OPT_COMPRESSION);
+            $client->setOption($client::OPT_COMPRESSION, $client::COMPRESSION_NONE);
+        }
+
+        try {
+            return $callback();
+        } finally {
+            if ($oldSerializer !== null) {
+                $client->setOption($client::OPT_SERIALIZER, $oldSerializer);
+            }
+
+            if ($oldCompressor !== null) {
+                $client->setOption($client::OPT_COMPRESSION, $oldCompressor);
+            }
+        }
+    }
+
     /**
      * Determine if serialization is enabled.
      *

diff --git a/tests/Cache/CacheRateLimiterTest.php b/tests/Cache/CacheRateLimiterTest.php
@@ -2,6 +2,7 @@
 
 namespace Illuminate\Tests\Cache;
 
+use Illuminate\Cache\ArrayStore;
 use Illuminate\Cache\RateLimiter;
 use Illuminate\Contracts\Cache\Repository as Cache;
 use Mockery as m;
@@ -20,6 +21,7 @@ public function testTooManyAttemptsReturnTrueIfAlreadyLockedOut()
         $cache->shouldReceive('get')->once()->with('key', 0)->andReturn(1);
         $cache->shouldReceive('has')->once()->with('key:timer')->andReturn(true);
         $cache->shouldReceive('add')->never();
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $this->assertTrue($rateLimiter->tooManyAttempts('key', 1));
@@ -31,6 +33,7 @@ public function testHitProperlyIncrementsAttemptCount()
         $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1)->andReturn(true);
         $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(true);
         $cache->shouldReceive('increment')->once()->with('key', 1)->andReturn(1);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $rateLimiter->hit('key', 1);
@@ -42,6 +45,7 @@ public function testIncrementProperlyIncrementsAttemptCount()
         $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1)->andReturn(true);
         $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(true);
         $cache->shouldReceive('increment')->once()->with('key', 5)->andReturn(5);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $rateLimiter->increment('key', 1, 5);
@@ -53,6 +57,7 @@ public function testDecrementProperlyDecrementsAttemptCount()
         $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1)->andReturn(true);
         $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(true);
         $cache->shouldReceive('increment')->once()->with('key', -5)->andReturn(-5);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $rateLimiter->decrement('key', 1, 5);
@@ -65,6 +70,7 @@ public function testHitHasNoMemoryLeak()
         $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturn(false);
         $cache->shouldReceive('increment')->once()->with('key', 1)->andReturn(1);
         $cache->shouldReceive('put')->once()->with('key', 1, 1);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $rateLimiter->hit('key', 1);
@@ -74,6 +80,7 @@ public function testRetriesLeftReturnsCorrectCount()
     {
         $cache = m::mock(Cache::class);
         $cache->shouldReceive('get')->once()->with('key', 0)->andReturn(3);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $this->assertEquals(2, $rateLimiter->retriesLeft('key', 5));
@@ -84,6 +91,7 @@ public function testClearClearsTheCacheKeys()
         $cache = m::mock(Cache::class);
         $cache->shouldReceive('forget')->once()->with('key');
         $cache->shouldReceive('forget')->once()->with('key:timer');
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $rateLimiter->clear('key');
@@ -93,6 +101,7 @@ public function testAvailableInReturnsPositiveValues()
     {
         $cache = m::mock(Cache::class);
         $cache->shouldReceive('get')->andReturn(now()->subSeconds(60)->getTimestamp(), null);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $this->assertTrue($rateLimiter->availableIn('key:timer') >= 0);
@@ -106,6 +115,7 @@ public function testAttemptsCallbackReturnsTrue()
         $cache->shouldReceive('add')->once()->with('key:timer', m::type('int'), 1);
         $cache->shouldReceive('add')->once()->with('key', 0, 1)->andReturns(1);
         $cache->shouldReceive('increment')->once()->with('key', 1)->andReturn(1);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
 
         $executed = false;
 
@@ -124,6 +134,7 @@ public function testAttemptsCallbackReturnsCallbackReturn()
         $cache->shouldReceive('add')->times(6)->with('key:timer', m::type('int'), 1);
         $cache->shouldReceive('add')->times(6)->with('key', 0, 1)->andReturns(1);
         $cache->shouldReceive('increment')->times(6)->with('key', 1)->andReturn(1);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
 
         $rateLimiter = new RateLimiter($cache);
 
@@ -157,6 +168,7 @@ public function testAttemptsCallbackReturnsFalse()
         $cache = m::mock(Cache::class);
         $cache->shouldReceive('get')->once()->with('key', 0)->andReturn(2);
         $cache->shouldReceive('has')->once()->with('key:timer')->andReturn(true);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
 
         $executed = false;
 
@@ -174,6 +186,7 @@ public function testKeysAreSanitizedFromUnicodeCharacters()
         $cache->shouldReceive('get')->once()->with('john', 0)->andReturn(1);
         $cache->shouldReceive('has')->once()->with('john:timer')->andReturn(true);
         $cache->shouldReceive('add')->never();
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
         $rateLimiter = new RateLimiter($cache);
 
         $this->assertTrue($rateLimiter->tooManyAttempts('jôhn', 1));
@@ -190,6 +203,7 @@ public function testKeyIsSanitizedOnlyOnce()
         $cache->shouldReceive('get')->once()->with($cleanedKey, 0)->andReturn(1);
         $cache->shouldReceive('has')->once()->with("$cleanedKey:timer")->andReturn(true);
         $cache->shouldReceive('add')->never();
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
 
         $this->assertTrue($rateLimiter->tooManyAttempts($key, 1));
     }

diff --git a/tests/Cache/RedisCacheIntegrationTest.php b/tests/Cache/RedisCacheIntegrationTest.php
@@ -2,6 +2,7 @@
 
 namespace Illuminate\Tests\Cache;
 
+use Illuminate\Cache\RateLimiter;
 use Illuminate\Cache\RedisStore;
 use Illuminate\Cache\Repository;
 use Illuminate\Foundation\Testing\Concerns\InteractsWithRedis;
@@ -37,6 +38,22 @@ public function testRedisCacheAddTwice($driver)
         $this->assertGreaterThan(3500, $this->redis[$driver]->connection()->ttl('k'));
     }
 
+    /**
+     * @param  string  $driver
+     */
+    #[DataProvider('redisDriverProvider')]
+    public function testRedisCacheRateLimiter($driver)
+    {
+        $store = new RedisStore($this->redis[$driver]);
+        $repository = new Repository($store);
+        $rateLimiter = new RateLimiter($repository);
+
+        $this->assertFalse($rateLimiter->tooManyAttempts('key', 1));
+        $this->assertEquals(1, $rateLimiter->hit('key', 60));
+        $this->assertTrue($rateLimiter->tooManyAttempts('key', 1));
+        $this->assertFalse($rateLimiter->tooManyAttempts('key', 2));
+    }
+
     /**
      * Breaking change.
      *

diff --git a/tests/Integration/Cache/RedisStoreTest.php b/tests/Integration/Cache/RedisStoreTest.php
@@ -252,6 +252,8 @@ public function testPutManyCallsPutWhenClustered()
 
     public function testIncrementWithSerializationEnabled()
     {
+        $this->markTestSkipped('Test makes no sense anymore. Application must explicitly wrap such code in runClean() when used with serialization/compression enabled.');
+
         /** @var \Illuminate\Cache\RedisStore $store */
         $store = Cache::store('redis');
         /** @var \Redis $client */

diff --git a/tests/Integration/Queue/RateLimitedTest.php b/tests/Integration/Queue/RateLimitedTest.php
@@ -63,6 +63,7 @@ public function testRateLimitedJobsAreNotExecutedOnLimitReached2()
         $cache->shouldReceive('add')->andReturn(true, true);
         $cache->shouldReceive('increment')->andReturn(1);
         $cache->shouldReceive('has')->andReturn(true);
+        $cache->shouldReceive('getStore')->andReturn(new ArrayStore);
 
         $rateLimiter = new RateLimiter($cache);
         $this->app->instance(RateLimiter::class, $rateLimiter);