Rate limiter no longer works when compression/serialization is enabled since v11.39.0

[JeppeKnockaert]

Laravel Version

11.39.0

PHP Version

8.4.3

Database Driver & Version

No response

Description

When using Redis as a cache driver combined with compression or serialization, the rate limiter no longer works in the latest release: the increment method always returns 0.

This seems to be related to #54221

More specifically, it seems that adding a key and then incrementing that key is the thing that breaks the rate limiter

$added = $this->cache->add($key, 0, $decaySeconds);
$hits = (int) $this->cache->increment($key, $amount);

doing the above always returns 0.

That's because the incrby function in Redis only works on integers, not on serialized/compressed data.
I can try and submit a PR with a fix, but I'm not sure what the preferred way forward would be.

Steps To Reproduce

I created a test case here: JeppeKnockaert/laravel-cache-bug-report@e983654

To execute the tests:

cp .env.example .env
./vendor/bin/sail artisan key:generate
./vendor/bin/sail up -d    
./vendor/bin/sail test ./tests/Unit/CacheTest.php  

The first test is the only that succeeds, because no compression or serialisation are configured.

[github-actions]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[TheLevti]

Hello, please be aware that compression/serialization never properly worked with the Cache component before this release. It seems like the rate limiter component works on top of the cache component.

Theoretically for rate limiting it makes not much sense to do this serialization/compression overhead. Either use a connection that does not set those config values or let us properly fix the issue.

I can have a look after work today if its not urgent.

@JeppeKnockaert can you provide me the phpredis version you are using?

If its below 5.3.5 then I suspect the issue in the pack method in the class PacksPhpRedisValues. As you mentioned in the pull request, we maybe should not serialize/compress certain operations. Try to update phpredis to >5.3.5, because then the laravel code calls the phpredis's internal pack method, which matches what phpredis does on all method calls.

[JeppeKnockaert]

Nothing urgent. I agree there are other ways to solve the issue that might be better.

For example, we could also make the increment/decrement operations compression/serialization aware instead.

The main issue is that in a clean Laravel project, by setting serialization or compression, rate limiting just breaks. At the very least, it seems prudent to put a big warning in the docs about that so people know to use a different connection in that case.

I'm just using the version included in the most recent Dockerfile published by Laravel Sail, that's 6.1.0 according to phpinfo.

Thanks for looking into it!

[TheLevti]

Nothing urgent. I agree there are other ways to solve the issue that might be better.

For example, we could also make the increment/decrement operations compression/serialization aware instead.

The main issue is that in a clean Laravel project, by setting serialization or compression, rate limiting just breaks. At the very least, it seems prudent to put a big warning in the docs about that so people know to use a different connection in that case.

I'm just using the version included in the most recent Dockerfile published by Laravel Sail, that's 6.1.0 according to phpinfo.

Thanks for looking into it!

I was able to reproduce it locally now. I also use phpredis 6.1.0. Checking now where things go south.

For now I want to ensure that the Cache component works correctly. Inherently everything built on top of this component should also work correctly (e.g. rate limiter). If other components use redis with those settings enabled, they most likely are not working the same way as the Cache component has not been working since forever (e.g. Queue would need some adjustments next). But it makes no sense to fix those for now. Better to explicitly state in the docs that if those settings are set, that only Redis and Cache components support that. The others should use a normal connection.

I will let you know what I find in the coming hours.

Edit:

Test code:

$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));

Working version where Serialization/Compression disabled, yields following output:

localhost:7009> MONITOR
OK
1737732214.619024 [0 172.18.0.1:38242] "FLUSHDB"
1737732214.621169 [0 172.18.0.1:38242] "GET" "test_key"
1737732214.633698 [0 172.18.0.1:38242] "EVAL" "return redis.call('exists',KEYS[1])<1 and redis.call('setex',KEYS[1],ARGV[2],ARGV[1])" "1" "test_key:timer" "1737732274" "60"
1737732214.633810 [0 lua] "exists" "test_key:timer"
1737732214.633833 [0 lua] "setex" "test_key:timer" "60" "1737732274"
1737732214.634165 [0 172.18.0.1:38242] "EVAL" "return redis.call('exists',KEYS[1])<1 and redis.call('setex',KEYS[1],ARGV[2],ARGV[1])" "1" "test_key" "0" "60"
1737732214.634174 [0 lua] "exists" "test_key"
1737732214.634176 [0 lua] "setex" "test_key" "60" "0"
1737732214.634224 [0 172.18.0.1:38242] "INCRBY" "test_key" "1"
1737732214.634381 [0 172.18.0.1:38242] "GET" "test_key"
1737732214.634481 [0 172.18.0.1:38242] "GET" "test_key:timer"
1737732214.634670 [0 172.18.0.1:38242] "FLUSHDB"

Failing version where serialization is set to Redis::SERIALIZER_PHP:

localhost:7009> MONITOR
OK
1737732304.430559 [0 172.18.0.1:42958] "FLUSHDB"
1737732304.432333 [0 172.18.0.1:42958] "GET" "test_key"
1737732304.446384 [0 172.18.0.1:42958] "EVAL" "return redis.call('exists',KEYS[1])<1 and redis.call('setex',KEYS[1],ARGV[2],ARGV[1])" "1" "test_key:timer" "i:1737732364;" "60"
1737732304.446452 [0 lua] "exists" "test_key:timer"
1737732304.446477 [0 lua] "setex" "test_key:timer" "60" "i:1737732364;"
1737732304.446928 [0 172.18.0.1:42958] "EVAL" "return redis.call('exists',KEYS[1])<1 and redis.call('setex',KEYS[1],ARGV[2],ARGV[1])" "1" "test_key" "i:0;" "60"
1737732304.446975 [0 lua] "exists" "test_key"
1737732304.446993 [0 lua] "setex" "test_key" "60" "i:0;"
1737732304.447095 [0 172.18.0.1:42958] "INCRBY" "test_key" "1"
1737732304.452072 [0 172.18.0.1:42958] "FLUSHDB"

It seems like when we store a key with an integer value, redis fails to increment it afterwards, because it stored it serialized. It seems like there is a problem on phpredis side. Because when I run $redis->set('test_key', 0);, phpredis also stores the value as i:0;. Then of course you can not increment it anymore.

[TheLevti]

I have talked with the maintainer and contributors of phpredis. It is a known edge case "bug", where you can not increment a variable that has been set with a serialized/compressed connection. As its kind of by design to not distinguish the use case of the application and just pack everything when enabled, we need to fix it here on framework side.

---

Solution is basically similar to what is done here. Instead of globally skipping the pack call for any Cache component usage, we should do this only when used by the RateLimiter component. (for setting the initial counter value)

[TheLevti]

Proposed solution fixes the issue. Regarding globally avoiding packing for numeric values, it won't be an issue. phpredis will return the value as is when it can not deserialize/uncompress a value. So we should be fine with this proposal.

[TheLevti]

I just want to highlight the implications of this fix:

Right now EVAL values won't be serialized/compressed when they are numbers. This potentially can cause the following bugs:

Case 1:

• User sets via set, put, setex a value, which is serialized/compressed by phpredis.
• User then runs an EVAL script that increments or decrements, which will fail, because its not a number

Case 2:

• User wants to overwrite the counter. Sets a serialized value (via set, put, setex, ..), which can no longer be incremented or decremented.

In addition we still have this edge case that will cause the rate limiter to not work. If we get into this if condition, we will set a compressed/serialized value. Future increments won't work.

I will open a second fix later today for this case (should actually never happen? maybe when connection fails on the increment above?), and also one a bit later that proposes a different approach without globally disabling the packing for numbers. The proposal involves to disable the options for this add/put call and set the previous value back afterwards. phpredis maintainer confirmed that its almost a noop, so there is not much overhead changing this setting.

[TheLevti]

Good news, phpredis may add an option to also skip serialization/compression for numbers on extension side. That would then match Laravel's behaviour and fixes all the cases where increment/decrement is required, while compression/serialization is enabled.

Will post here once I know more.