[12.x] [POC] Allow rate limit hits to only be recorded after successful responses

[JosephSilber]

No tests yet, as this just a proof of concept for now. Waiting to hear from @taylorotwell if this is how we should implement it.

---

This PR adds the onSuccess() method to the Limit class, which will only record a hit after a successful response:

RateLimiter::for('signup', function (Request $request) {
    return Limit::perDay(1)
        ->by($request->ip())
        ->onSuccess(); // <-- this is new
});

Background

Imagine the following rate limit:

// In a service provider...
RateLimiter::for('signup', function (Request $request) {
    return Limit::perDay(1)->by($request->ip());
});

// In a routes files...
Route::post('signup', SignupController::class)->middleware('throttle:signup');

This will only allow a single signup per day from a given IP address, which is great.

However, if the user submits the signup form with some validation errors, they will use up their rate limit, and will no longer be able to submit the form again until the next day. That's not what we want.

Solution

By adding onSuccess() to the Limit:

RateLimiter::for('signup', function (Request $request) {
    return Limit::perDay(1)->by($request->ip())->onSuccess();
});

...we can now have a rate limit that only gets a hit after the form is successfully processed. Form submissions with validation errors do not count against the rate limit.

---

It can also be combined with a general, more lax rate limit:

RateLimiter::for('signup', fn (Request $request) => [
    Limit::perMinute(10)->by($request->ip()),
    Limit::perDay(1)->by('.success'.$request->ip())->onSuccess(),
]);

This will allow up to 10 failed requests per minute, but only a single successful request per day.

[Rizky92]

CMIIW, but aren't rate limiter supposedly reset after successful response?

[rodrigopedra]

CMIIW, but aren't rate limiter supposedly reset after successful response?

No, they are managed into the cache and reset when their period ends.

For example, the Limit::perDay(1)->by('.success'.$request->ip())->onSuccess() limiter will allow a single request per day.

If it got reset after a successful response, one could do as many requests as they want in a day, and that limit wouldn't serve any purpose.

[timacdonald]

Totally understand this is a proof of concept PR, so not covering every base right now.

I like the more generic idea of being able to rate limit based on the response.

We do this to protect against enumeration attacks, e.g.,

RateLimiter::for('resources', fn ($request) => [
    Limit::perMinute(60)->by($request->ip()),
    Limit::perMinute(5)->by('.404'.$request->ip())->onNotFound(),
]);

Good requests get 60 per minute. If the user is continually hitting 404s, we assume enumeration attack and block for the remainder of the minute.

So more generally, I like the idea of a low-level hook that all of this is built on top of:

RateLimiter::for('resources', fn ($request) => [
    Limit::perMinute(60)->by($request->ip()),
    Limit::perMinute(5)
        ->by('.404'.$request->ip())
        ->when(function (Response $response) {
            return $response->status() === 404;
        }),
]);

[JosephSilber]

So more generally, I like the idea of a low-level hook that all of this is built on top of

@timacdonald Funnily enough, that was exactly my first thought as well.

However, when I actually started implementing it I realized that illuminate/cache doesn't have access to Response, so I scrapped it.

[timacdonald]

I see.

I don't think it could hurt to add a HTTP layer concern into illuminate/cache without making a hard dependency. We already do this with the Limit::response method underneath your proposed onSuccess method.

framework/src/Illuminate/Cache/RateLimiting/Limit.php

Lines 132 to 143 in bb00401

	/**
	* Set the callback that should generate the response when the limit is exceeded.
	*
	* @param callable $callback
	* @return $this
	*/
	public function response(callable $callback)
	{
	$this->responseCallback = $callback;
	return $this;
	}

The Limit class would remain a plain old value object that is responsible for collecting the callback. The middleware would be responsible for invoking it and passing through the response.

Side note: I wonder if after would be a nice method name for the callback. Feels like it is clearer on its intention.

RateLimiter::for('resources', fn ($request) => [
    Limit::perMinute(60)->by($request->ip()),
    Limit::perMinute(5)
        ->by('.404'.$request->ip())
        ->after(function (Response $response) {
            return $response->status() === 404;
        }),
]);

[JosephSilber]

We already do this with the Limit::response method underneath your proposed onSuccess method.

@timacdonald That callback can return whatever it wants. Its technical return type is mixed, so there's no dependence on Response.

But to implement what we want here, we would have to pass in an instance of Response, which we don't have access to. We could cheat by not providing the exact callable type in the DocBlock (we already do this in RateLimiter::for()), but that doesn't solve the next issue...

Adding our helper methods, such as onSuccess and onNotFound, definitely requires interacting with the Response:

class Limit
{
    // ...

    public function onNotFound()
    {
        $this->onResponse(function (Response $response) {
            return $response->status() == 404;
        });

        return $this;
    }
}

Now... here too we can cheat, by not type-hinting the parameter, but that's even worse 😑

[timacdonald]

Gotcha. Dunno; I don't really see a problem with it, but that is just me.

I'm sure we have other instances of stuff in sub-packages that are really there as holistic framework affordance. Else, we could always add illuminate/http to the sub-package requirements.

But it is all good, I just wanted to share my thoughts, not specifically for there to be change here. Think it is a good feature.

[JosephSilber]

@timacdonald in the past, I put code in a macro in Foundation to get around this problem.

I thought it's overkill here, but... 🤷

[rodrigopedra]

@JosephSilber

I see this is a POC, but is it planned to also have the behavior of the Illuminate\Queue\Middleware\RateLimited job middleware changed? And potentially its RateLimitedWithRedis sibling?

I have a job that hits an external service. It can fail before making the request for some other reasons.

It would be nice to only hit the limiter if the request, and therefore the job, succeeds.

[taylorotwell]

I personally like @timacdonald's idea of the lower-level hook and we should just make that happen. 👍

[JosephSilber]

I personally like @timacdonald's idea of the lower-level hook and we should just make that happen. 👍

I don't know how. Gonna close this out, and let someone else pick it up if they want.

[timacdonald]

Happy to take a look on Monday

[timacdonald]

Turns out I was excited enough to do it today :)

#57125