RateLimiter is triggered when callback function returns empty string

[alexbog8]

Laravel Version

11.23.5

PHP Version

8.2.28

Database Driver & Version

Mysql 8.4.3

Description

When the callback function returns an empty string the RateLimiter is triggered after the first attempt.
This is due to the following code inside cache/RateLimiter.php

if (is_null($result = $callback())) {
$result = true;
}

is_null() should be replaced with empty() to allow return of empty string.

Steps To Reproduce

The ratelimiter is triggered immediately when the function returns an empty string

$executed = RateLimiter::attempt(
'contact-search-'.Auth::user()->id,
$perMinute = 15,
function() use($request){
$data = $this->contactSearch($request);
return $data;
},
$decayRate = 600
);

[rodrigopedra]

Any value returned from the callback, including null triggers a hit to the rate limiter:

framework/src/Illuminate/Cache/RateLimiter.php

Lines 105 to 118 in 6728659

	public function attempt($key, $maxAttempts, Closure $callback, $decaySeconds = 60)
	{
	if ($this->tooManyAttempts($key, $maxAttempts)) {
	return false;
	}
	if (is_null($result = $callback())) {
	$result = true;
	}
	return tap($result, function () use ($key, $decaySeconds) {
	$this->hit($key, $decaySeconds);
	});
	}

The if statement you mentioned just "casts" a null value to true. It does nothing to prevent hitting the rate limiter, as you can see in the last statement.

The only case a hit is prevented, is if the $this->tooManyAttempts($key, $maxAttempts) condition is true. In that case, the callback is also not even executed.

If the code passes that condition, in other words, every time the callback is called, unless the callback throws an exception, the rate limiter gets a hit, regardless of the callback's return value.

[alexbog8]

Yes, but if the result of the callback is an empty string, is_null will do nothing because empty string is not null. Therefore $result will be empty string and the return of the tap function will be an empty string. The negation of an empty string is true so ! $executed is triggered …

[rodrigopedra]

so ! $executed is triggered

And what does it have to do with hitting or not the rate limiter? Is $executed a variable in your code?

From your original post, I understood you wanted to prevent the rate limiter to get a hit, and assumed that returning null would prevent it, but wanted an empty string to also prevent it, right?

What happens is that, unless the callback throws an exception, if the callback is called, its return value has no deal on preventing the rate limiter to get a hit.

If your callback returns:

• null, rate limiter gets a hit
• "", rate limiter gets a hit
• false, rate limiter gets a hit
• 0, rate limiter gets a hit
• [], rate limiter gets a hit

And for any other value returned, the rate limiter gets a hit.

The reasoning on why the return value from the callback is preserved is laid on this PR #45611

Originally, it always returned true if the callback was run, and false if the first if clause, which checks for too many attempts before executing the callback, evaluated to true.

[rodrigopedra]

I see, you are talking about the sample in the docs:

use Illuminate\Support\Facades\RateLimiter;

$executed = RateLimiter::attempt(
    'send-message:'.$user->id,
    $perMinute = 5,
    function() {
        // Send message...
    }
);

if (! $executed) {
  return 'Too many messages sent!';
}

https://laravel.com/docs/12.x/rate-limiting#basic-usage

In the case the callback returns a falsy value, the check for the callback being executed should be:

if ($executed === false) {
  return 'Too many messages sent!';
}

That can also be misleading if the callback returns false.

Not sure what is the best solution here, if it is to make the docs clear about this, or to send a PR restoring the behavior before PR #45611, or to propose an alternative solution.

Please excuse the misunderstanding.

Although the callback's return still has no impact on if the rate limiter gets a hit or nor, I now understand better your point.