Framework: Throttling individual routes is unintuitive / behaviour change

[garygreen]

• Laravel Version: 5.8
• PHP Version: 7.2

Description:

Since PR https://github.com/laravel/framework/pull/19807 which changed the throttlers request signature, all routes are essentially overriding the previous throttler settings.

This means that even internal things like email verification (which adds throttling by default essentially OVERRIDES your apps throttling because throttling is no longer a route-level thing.

This is completely counter intuitive - throttling has now become essentially global, regardless of what options you specify in individual routes e.g 6,10, it all goes in one massive pool.

I understand the intentions behind the original PR were good, and that you can alter this logic by overriding the resolveRequestSignature - but the fact that Laravel is internally using throttling on routes means this is likely unintentionally broken to some degree and interfering with peoples app.

If we still support:

Route::get('some-route')->middleware('throttle:X,X');
Route::get('another-route')->middleware('throttle:X,X');

Then we need to make it clear that those throttling options are NOT specific to those routes. It seems counter intuitive, but that's now the behaviour.

[laurencei]

ping @lucasmichot

[lucasmichot]

I guess this throttling mechanism misses a "bucket" feature.

Nevertheless, the throttling mechanism is different for each application and by simply extending this middleware you can have fine-grained control over it.

See https://github.com/laravel/framework/issues/22929

[garygreen]

@lucasmichot

Nevertheless, the throttling mechanism is different for each application and by simply extending this middleware you can have fine-grained control over it.

You shouldn't have to extend the middleware to fix the problem of not being able to adjust throttling setting on a per-route basis.

Really, your original PR should of been some "global" domain-based setting/feature. I'm surprised it even got merged because its a big breaking change that only really suits your specific use case of needing domain-level throttling, it should of been a separate feature.

I mean, you may as well now just get rid of the route parameters and have it on the global level:

Route::throttle(6, 10);

...because that's how it works now, unfortunately.

[driesvints]

I realize that this could perhaps be done better but atm I believe this is just "how it works". If this isn't documented properly then it's best to send in a PR to the docs. Otherwise an issue on the ideas repo can be opened to improve this behavior or change it.

So at the moment we're not considering this as an actual bug. Feel free to discuss this further on the ideas repository.

[garygreen]

@driesvints yet again you've misunderstood the problem and closed an issue preemptively. This is getting tiring.

[garygreen]

I'll send a PR to the docs. This is going to be interesting, as the docs will essentially explain the feature is broken.

[lucasmichot]

Yes please do so

[garygreen]

Pretty sure Taylor is going to be confused by it, as he's used throttling in the verification controller, so I am confident even he is unaware this is broken at the moment. But hopefully it'll highlight this and re-open.

[lucasmichot]

👌🏻

[garygreen]

There's even throttling on the api out the box in Laravel, which means that ANYTHING will overwrite this and you can't have seperate web / api based throttle settings.

protected $middlewareGroups = [
    'web' => [
        \App\Http\Middleware\EncryptCookies::class,
        \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class,
        \Illuminate\Session\Middleware\StartSession::class,
        // \Illuminate\Session\Middleware\AuthenticateSession::class,
        \Illuminate\View\Middleware\ShareErrorsFromSession::class,
        \App\Http\Middleware\VerifyCsrfToken::class,
        \Illuminate\Routing\Middleware\SubstituteBindings::class,
    ],
    'api' => [
        'throttle:60,1',
        'bindings',
    ],
];

https://github.com/laravel/laravel/blob/v5.8.3/app/Http/Kernel.php#L41

@lucasmichot

Could you help with explaining this change on the docs in a user-friendly way?

[garygreen]

Here's what I've come up with so far. It sounds totally ridiculous, but that's just the way it works now. 😕

---

Note: the given throttle settings apply on a domain-level globally and not on a per-route basis. So for example, if you have multiple routes that define throttle settings:

Route::get('users')->middleware('throttle:6,10');
Route::get('posts')->middleware('throttle:10,5);

Whichever route loaded first will take priority and use those throttle settings.

[lucasmichot]

@garygreen - I actually cannot, as I already use a custom throttling that would not fit anyone expectation.
But feel free to ask Dries, he will be very happy to help you 😀

[garygreen]

The first example on the docs would need to change as well, as it's not applying throttle settings to just that route group. It's pretty much all over the docs that it operates on a per-route basis.

@driesvints I would appreciate if you can you at least check with Taylor first to see if this change was intentional or not. The docs explain it to be working on a per-route basis and if I did send a PR to change this it would overwrite a lot of whats currently on the docs.

[driesvints]

@driesvints yet again you've misunderstood the problem and closed an issue preemptively. This is getting tiring.

I didn't. Throttling is currently performed on domain/ip level by default in a non-authed state and not by "slug" as you expect it: https://github.com/laravel/framework/blob/5.8/src/Illuminate/Routing/Middleware/ThrottleRequests.php#L94-L105

I suggested a PR to the docs because I do agree that those are confusing atm because they don't make the above reasoning clear.

I would appreciate if you can you at least check with Taylor first

I'll check with him.

[garygreen]

I've sent a PR to the docs: https://github.com/laravel/docs/pull/5152

Let's see what Taylor has to say. Thanks for reaching out to him, it's much appreciated 😃

[taylorotwell]

Looked it over. I agree that is a kinda counter-intuitive and not how I would expect things to work based on my interactions with other APIs.

If someone has a suggested fix to improve it could you please send a PR? We will need to somehow tie the rate limiting into a unique route identifier I guess.

[garygreen]

If someone has a suggested fix to improve it could you please send a PR? We will need to somehow tie the rate limiting into a unique route identifier I guess.

Well before the original PR was merged it was using a unique route identifier essentially by using the $request->fingerprint() method. This uses the http verb/method, domain, route url and ip. The PR changed that so it operates only on a domain level.

My suggestion would be to revert back to using request->fingerprint(), as this is the expected and documented behaviour of throttling. Then maybe a separate PR to add "domain based" throttling (to suit @lucasmichot use case) - maybe this can be added like so:

Route::throttle('throttle-name', 5, 10);
Route::get('users')->domainThrottle('throttle-name');

Seems a bit clumsy though, domain based throttling should probably be done as more of a middleware-thing.

I'm guessing any PR should be targeted @ 5.9?

What do you think @taylorotwell ?

[trevorgehman]

Just for reference, I had this same back-and-forth a couple months ago, and made the same argument. The bottom line is it's odd to apply a middleware to a route that isn't route-specific.

Here's how I extended the base middleware: https://github.com/laravel/framework/issues/22929#issuecomment-463282531

This basically allows two use-cases:

• 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. (Good use case is to apply this to the top-level route group for your entire API.)

[garygreen]

@trevorgehman thanks for the info. There is a PR I'm working on and I think we've come to agree with the same implementation of being able to specify an identifier you can group routes by. I'll get round to updating the PR to support this hopefully this weekend. It's targeted at 5.9 but hopefully it'll make throttling more intuitive again.