Skip to content

[11.x] Fix the RateLimiter issue when using dynamic keys#53763

Merged
taylorotwell merged 2 commits into
laravel:11.xfrom
MilesChou:unique-limiter-key-when-empty
Dec 6, 2024
Merged

[11.x] Fix the RateLimiter issue when using dynamic keys#53763
taylorotwell merged 2 commits into
laravel:11.xfrom
MilesChou:unique-limiter-key-when-empty

Conversation

@MilesChou

@MilesChou MilesChou commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

In Laravel v11.29.0, a new modification was introduced to the RateLimiter via PR #53177, which provides a fallback key when no unique key is specified. However, this mechanism can cause issues in scenarios where the key is determined dynamically using a closure.

For example, when implementing rate limiting based on dynamic userId values:

$rateLimiter->for('user_limiter', fn (string $userId) => [
    Limit::perSecond(3)->by($userId),
    Limit::perMinute(5)->by($userId),
]);

$userId1 = '123';
$userId2 = '456';

$limiterForUser1 = $rateLimiter->limiter('user_limiter')($userId1);
$limiterForUser2 = $rateLimiter->limiter('user_limiter')($userId2);

In version 11.29.0, the Limit::by($userId) method is ignored, and the fallback key is used instead, causing rate limiting records for different userId values to interfere with each other.

This fix ensures that if the original limit has a recorded key, it uses the original key as a prefix. For more details, please refer to the PR.

@MilesChou MilesChou force-pushed the unique-limiter-key-when-empty branch 2 times, most recently from 165838b to e11ff7d Compare December 5, 2024 06:19
@MilesChou MilesChou changed the title [11.x] Fix the RateLimite issue when generating dynamic keys [11.x] Fix the RateLimite issue when using dynamic keys Dec 5, 2024
@MilesChou MilesChou changed the title [11.x] Fix the RateLimite issue when using dynamic keys [11.x] Fix the RateLimiter issue when using dynamic keys Dec 5, 2024
@MilesChou MilesChou force-pushed the unique-limiter-key-when-empty branch from e11ff7d to 4f23c78 Compare December 5, 2024 06:25
@gofish543

Copy link
Copy Markdown

@MilesChou Thank you for getting this PR in, was just about to submit one myself as a chunk of an application's unit tests are failing due to this change...

<?php

namespace App\RateLimiters;

use Illuminate\Cache\RateLimiting\Limit;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\RateLimiter;

class Web
{
    public static function register(): void
    {
        RateLimiter::for('web', function (Request $request) {
            $user = $request->user();
            $ip = $request->ip();

            return [
                Limit::perSecond(5)
                    ->by($user ? $user->id : $ip),

                Limit::perMinute(20)
                    ->by($user ? $user->id : $ip),

                Limit::perHour(80)
                    ->by($user ? $user->id : $ip),

                Limit::perDay(320)
                    ->by($user ? $user->id : $ip),
            ];
        });
    }
}
test('limited by user id', function () {
    $rateLimiter = RateLimiter::limiter('web');
    $user = User::factory()->make();
    $user->setUniqueIds();

    $request = Request::instance();
    $request->setUserResolver(fn () => $user);

    $limiters = $rateLimiter($request);

    dd(collect($limiters)->pluck('key'));
    expect($limiters)->toHaveCount(4);
    expect($limiters[0]->key)->toEqual($user->id);
});
Illuminate\Support\Collectio#2249
  #items: array:4 [
    0 => "attempts:5:decay:1"
    1 => "attempts:20:decay:60"
    2 => "attempts:80:decay:3600"
    3 => "attempts:320:decay:86400"
  ]
  #escapeWhenCastingToString: false
}

@timacdonald Any word on getting this merged?

@taylorotwell taylorotwell merged commit 5ed48d3 into laravel:11.x Dec 6, 2024
@MilesChou MilesChou deleted the unique-limiter-key-when-empty branch December 7, 2024 13:19
@timacdonald

Copy link
Copy Markdown
Member

Sorry about this one, folks.

browner12 pushed a commit to browner12/framework that referenced this pull request Dec 10, 2024
* Fix the RateLimite issue when generating dynamic keys

* Adjust limit condition in Limit::fallbackKey() method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants