Login throttle ternary condition inside `AuthenticatedSessionController` seems incorrect

brnquester commented 3 months ago

Fortify Version

1.21.3

Laravel Version

10.48.12

PHP Version

8.2

Database Driver & Version

MySQL 8.0 Docker container mysql/mysql-server:8.0

Description

It seems that the ternary condition inside of AuthenticatedSessionController is inverted for EnsureLoginIsNotThrottled verification.

Location: https://github.com/laravel/fortify/blob/a725684d17959c4750f3b441ff2e94ecde7793a1/src/Http/Controllers/AuthenticatedSessionController.php#L85-L91

It should like below:

        return (new Pipeline(app()))->send($request)->through(array_filter([
            config('fortify.limiters.login') ? EnsureLoginIsNotThrottled::class : null,
            config('fortify.lowercase_usernames') ? CanonicalizeUsername::class : null,
            Features::enabled(Features::twoFactorAuthentication()) ? RedirectIfTwoFactorAuthenticatable::class : null,
            AttemptToAuthenticate::class,
            PrepareAuthenticatedSession::class,
        ]));

Fortify own lockout handler is needed for different scenarios

Ideal scenario / my proposal

Have both protections by having different throttle configs for the Laravel middleware and Fortify own handler. We can have a smaller throttle rate for Fortify's own handler and a greater one for the middleware just in case of brute force.

Steps To Reproduce

Make sure to have the limiters defined in config/fortify.php as follows:
```
'limiters' => [
    'login' => 5,
    'two-factor' => 5,
],
```
Remove the throttle middleware from the Route::post(RoutePath::for('login', '/login') keeping everything else as is, see below: (Fortify has its own throttle error handlers, it shouldn't need to rely on Laravel defaults throttle middleware)
```
Route::post(RoutePath::for('login', '/login'), [AuthenticatedSessionController::class, 'store'])
    ->middleware(array_filter([
        'guest:'.config('fortify.guard')
    ]));
```
Now attempt multiple logins passing the limiter mark, you can try even 20 times. You'll never see the error "Too many login attempts. Please try again in X seconds.".
Now go to the default AuthenticatedSessionController in Fortify changing the line for the EnsureLoginIsNotThrottled below to config('fortify.limiters.login') ? EnsureLoginIsNotThrottled::class : null. Just invert it. https://github.com/laravel/fortify/blob/a725684d17959c4750f3b441ff2e94ecde7793a1/src/Http/Controllers/AuthenticatedSessionController.php#L85-L87
Now try to log in multiple times, at some point, you'll see the error "Too many requests. Please try again in X seconds.".

github-actions[bot] commented 3 months ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

taylorotwell commented 3 months ago

See my response here: https://github.com/laravel/fortify/pull/545

brnquester commented 3 months ago

See my response here: #545

I'm sorry @taylorotwell, but this issue is still a problem and shouldn't be closed. Provided you a descriptive answer here: https://github.com/laravel/fortify/pull/545#issuecomment-2180895589

brnquester commented 3 months ago

@taylorotwell - new PR opened solving all the conflicts and confusions: https://github.com/laravel/fortify/pull/547

laravel / fortify