Framework: Same throttle limit counter for all routes

[carduz]

• Laravel Version: 5.5.32
• PHP Version: 7.1
• Database Driver & Version: Sqlite 3.22

Description:

With pull request https://github.com/laravel/framework/pull/19807 an important issue was introduced. In fact the fingerprint, for authenticated users, depends only form the user (sha1($user->getAuthIdentifier())) not from the request, like was before ($request->fingerprint()). So you cannot restrict some routes (for example if you want a lower limit on a specific route), since the rate limit counter is in common (same key).
A possible solution could be reintroduce $request->fingerprint().
Take a look at method resolveRequestSignature of the class ThrottleRequests.

Steps To Reproduce:

Try to create two routes for authenticated user with rate limit, you will see that the limit counter is in common.
Example:

• route A: rate limit 3:1
• route B: rate limit 60:1

I try to call route B 3 times, then call route A 1 time, in the same minute. The route A says that the rate limit is reached, even if I haven't called it before, but in the headers the rate limit counter has 3 as value. Of course if I call route B it replies correctly.

[Kyslik]

This sure is interesting, I wonder if there is general opinion on this and why wasn't this discussed while doing #19807.

[sisve]

Consider the built-in throttling a default, and you can just derive from it and override the resolveRequestSignature to implement any logic you want.

[carduz]

Ok, but this issue was introduced with a minor release, causing problems in different apps. My propose is to do rollback.

[sisve]

You're probably misreading the GitHub interface. The commit is at https://github.com/laravel/framework/pull/19807/commits/8b7a7a7b881c973c45426fd5fce6f2b261beacbd and you can expand all the tags (releases) that has it. The first tag that has this commit is the 5.5.0 tag, which would be the release of Laravel 5.5. It was introduced in a new major Laravel version over 6 months ago.

[carduz]

Sorry my mistake, anyway it is not documented in the upgrade guide

[ntzm]

The upgrade guides are open to pull requests!

[arjasco]

You could subclass the middleware and override the resolveRequestSignature method. It could just look like this.

protected function resolveRequestSignature($request)
{
    if (! $route = $request->route()) {
        throw new RuntimeException('Unable to generate the request signature. Route unavailable.');
    }

    if ($user = $request->user()) {
        return sha1(implode('|', array_merge(
            $route->methods(), [$route->getDomain(), $route->uri(), $request->ip(), $user->getAuthIdentifier()]
        )));            
    }

    return $request->fingerprint();
}

This way throttling will be on a per route basis for auth'd and not auth'd, you can then do stuff like this.

Route::get('one', function () {
    return 'route one';
})->middleware('throttle:10,1');

Route::get('two', function () {
    return 'route two';
})->middleware('throttle:20,1');

Route::get('three', function () {
    return 'route three which requires auth';
})->middleware('auth:api', 'throttle:10,1');

In it's current implementation because it doesn't create a SHA1 including the path and request method then you don't get that more granular level of control.

Also if you was to hit the 10 limit on the first endpoint, the second endpoint will only allow 10 requests regardless of defining 20, because there isn't enough information to create an endpoint unique SHA1.. obviously.

Feels like this could do with a tidy up to be fair. The sha1($user->getAuthIdentifier()) on the current implementation makes me feel weird. I know it's keeping the consistency of using SHA1 but hashing a single integer thats more than likely just your auto incrementing primary key anyway, meh... 😕

[themsaid]

Sorry that breaking change wasn't documented, will make sure it is.

[trevorgehman]

This seems like an oversight, and I doubt many people are aware of it. I certainly wasn't. This basically means that you can only use the throttle middleware on one route reliably.

[lucasmichot]

Anyone can extend it and adapt it to specific needs.

[trevorgehman]

In case anyone is interested, here is how we extended the throttle middleware. Now we can use it for two situations:

• throttle:60,1: Will throttle a specific route, on its own
• throttle:60,1,api: Will throttle all routes together that use the "api" identifier

Now we can, for example, throttle our entire API by placing throttle:120,1,api on the top-level route group, and we can also throttle specific routes within the API if necessary by adding throttle:15,1 to just a single route.

class ThrottleRequests extends BaseMiddleware
{
    /**
     * The identifier to be used in the throttle signature.
     *
     * @var string
     */
    protected $slug;

    public function handle($request, Closure $next, $maxAttempts = 60, $decayMinutes = 1, $slug = null)
    {
        $this->slug = $slug;

        return parent::handle($request, $next, $maxAttempts, $decayMinutes);
    }

    protected function resolveRequestSignature($request)
    {
        if (! $route = $request->route()) {
            throw new RuntimeException('Unable to generate the request signature. Route unavailable.');
        }

        // With a provided identifier we can throttle a group of routes, so that all the routes in the group
        // will have the same throttle signature.
        if ($this->slug) {
            return $this->generateGroupSignature($request);
        }

        return $this->generateRouteSignature($request, $route);
    }

    protected function generateGroupSignature($request)
    {
        $identifier = ($user = $request->user())
            ? $user->getAuthIdentifier()
            : $request->ip();

        return sha1($this->slug . '|' . $identifier);
    }

    protected function generateRouteSignature($request, $route)
    {
        if ($user = $request->user()) {
            return sha1(implode('|', array_merge(
                $route->methods(),
                [$route->getDomain(), $route->uri(), $user->getAuthIdentifier()]
            )));
        }

        return $request->fingerprint();
    }