From 69af9e0dbdefd8c6c951e8afbe2bba141d454beb Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 20 May 2024 14:00:58 +0100 Subject: [PATCH] Routes: Added throttling to a range of auth-related endpoints Some already throttled in some means, but this adds a simple ip-based non-request-specific layer to many endpoints. Related to #4993 --- .../Controllers/ForgotPasswordController.php | 5 +++ .../Controllers/ResetPasswordController.php | 9 ++-- app/App/Providers/RouteServiceProvider.php | 4 ++ routes/web.php | 12 +++--- tests/Auth/RegistrationTest.php | 29 +++++++++++++ tests/Auth/ResetPasswordTest.php | 42 +++++++++++++++++++ tests/Auth/UserInviteTest.php | 20 +++++++++ 7 files changed, 109 insertions(+), 12 deletions(-) diff --git a/app/Access/Controllers/ForgotPasswordController.php b/app/Access/Controllers/ForgotPasswordController.php index 86fbe8fa3..36dd97755 100644 --- a/app/Access/Controllers/ForgotPasswordController.php +++ b/app/Access/Controllers/ForgotPasswordController.php @@ -6,6 +6,7 @@ use BookStack\Activity\ActivityType; use BookStack\Http\Controller; use Illuminate\Http\Request; use Illuminate\Support\Facades\Password; +use Illuminate\Support\Sleep; class ForgotPasswordController extends Controller { @@ -32,6 +33,10 @@ class ForgotPasswordController extends Controller 'email' => ['required', 'email'], ]); + // Add random pause to the response to help avoid time-base sniffing + // of valid resets via slower email send handling. + Sleep::for(random_int(1000, 3000))->milliseconds(); + // We will send the password reset link to this user. Once we have attempted // to send the link, we will examine the response then see the message we // need to show to the user. Finally, we'll send out a proper response. diff --git a/app/Access/Controllers/ResetPasswordController.php b/app/Access/Controllers/ResetPasswordController.php index a8a45dddf..3af65d17f 100644 --- a/app/Access/Controllers/ResetPasswordController.php +++ b/app/Access/Controllers/ResetPasswordController.php @@ -15,14 +15,11 @@ use Illuminate\Validation\Rules\Password as PasswordRule; class ResetPasswordController extends Controller { - protected LoginService $loginService; - - public function __construct(LoginService $loginService) - { + public function __construct( + protected LoginService $loginService + ) { $this->middleware('guest'); $this->middleware('guard:standard'); - - $this->loginService = $loginService; } /** diff --git a/app/App/Providers/RouteServiceProvider.php b/app/App/Providers/RouteServiceProvider.php index 3a155920e..d7c1cb737 100644 --- a/app/App/Providers/RouteServiceProvider.php +++ b/app/App/Providers/RouteServiceProvider.php @@ -81,5 +81,9 @@ class RouteServiceProvider extends ServiceProvider RateLimiter::for('api', function (Request $request) { return Limit::perMinute(60)->by($request->user()?->id ?: $request->ip()); }); + + RateLimiter::for('public', function (Request $request) { + return Limit::perMinute(10)->by($request->ip()); + }); } } diff --git a/routes/web.php b/routes/web.php index 03595288f..58b8f4e54 100644 --- a/routes/web.php +++ b/routes/web.php @@ -317,8 +317,8 @@ Route::get('/register/confirm', [AccessControllers\ConfirmEmailController::class Route::get('/register/confirm/awaiting', [AccessControllers\ConfirmEmailController::class, 'showAwaiting']); Route::post('/register/confirm/resend', [AccessControllers\ConfirmEmailController::class, 'resend']); Route::get('/register/confirm/{token}', [AccessControllers\ConfirmEmailController::class, 'showAcceptForm']); -Route::post('/register/confirm/accept', [AccessControllers\ConfirmEmailController::class, 'confirm']); -Route::post('/register', [AccessControllers\RegisterController::class, 'postRegister']); +Route::post('/register/confirm/accept', [AccessControllers\ConfirmEmailController::class, 'confirm'])->middleware('throttle:public'); +Route::post('/register', [AccessControllers\RegisterController::class, 'postRegister'])->middleware('throttle:public'); // SAML routes Route::post('/saml2/login', [AccessControllers\Saml2Controller::class, 'login']); @@ -338,16 +338,16 @@ Route::get('/oidc/callback', [AccessControllers\OidcController::class, 'callback Route::post('/oidc/logout', [AccessControllers\OidcController::class, 'logout']); // User invitation routes -Route::get('/register/invite/{token}', [AccessControllers\UserInviteController::class, 'showSetPassword']); -Route::post('/register/invite/{token}', [AccessControllers\UserInviteController::class, 'setPassword']); +Route::get('/register/invite/{token}', [AccessControllers\UserInviteController::class, 'showSetPassword'])->middleware('throttle:public'); +Route::post('/register/invite/{token}', [AccessControllers\UserInviteController::class, 'setPassword'])->middleware('throttle:public'); // Password reset link request routes Route::get('/password/email', [AccessControllers\ForgotPasswordController::class, 'showLinkRequestForm']); -Route::post('/password/email', [AccessControllers\ForgotPasswordController::class, 'sendResetLinkEmail']); +Route::post('/password/email', [AccessControllers\ForgotPasswordController::class, 'sendResetLinkEmail'])->middleware('throttle:public'); // Password reset routes Route::get('/password/reset/{token}', [AccessControllers\ResetPasswordController::class, 'showResetForm']); -Route::post('/password/reset', [AccessControllers\ResetPasswordController::class, 'reset']); +Route::post('/password/reset', [AccessControllers\ResetPasswordController::class, 'reset'])->middleware('throttle:public'); // Metadata routes Route::view('/help/wysiwyg', 'help.wysiwyg'); diff --git a/tests/Auth/RegistrationTest.php b/tests/Auth/RegistrationTest.php index 60ae17573..42d1120e4 100644 --- a/tests/Auth/RegistrationTest.php +++ b/tests/Auth/RegistrationTest.php @@ -203,4 +203,33 @@ class RegistrationTest extends TestCase $resp = $this->followRedirects($resp); $this->withHtml($resp)->assertElementExists('form input[name="username"].text-neg'); } + + public function test_registration_endpoint_throttled() + { + $this->setSettings(['registration-enabled' => 'true']); + + for ($i = 0; $i < 11; $i++) { + $response = $this->post('/register/', [ + 'name' => "Barry{$i}", + 'email' => "barry{$i}@example.com", + 'password' => "barryIsTheBest{$i}", + ]); + auth()->logout(); + } + + $response->assertStatus(429); + } + + public function test_registration_confirmation_throttled() + { + $this->setSettings(['registration-enabled' => 'true']); + + for ($i = 0; $i < 11; $i++) { + $response = $this->post('/register/confirm/accept', [ + 'token' => "token{$i}", + ]); + } + + $response->assertStatus(429); + } } diff --git a/tests/Auth/ResetPasswordTest.php b/tests/Auth/ResetPasswordTest.php index d2af17b9c..026f8c5ba 100644 --- a/tests/Auth/ResetPasswordTest.php +++ b/tests/Auth/ResetPasswordTest.php @@ -4,11 +4,19 @@ namespace Tests\Auth; use BookStack\Access\Notifications\ResetPasswordNotification; use BookStack\Users\Models\User; +use Carbon\CarbonInterval; use Illuminate\Support\Facades\Notification; +use Illuminate\Support\Sleep; use Tests\TestCase; class ResetPasswordTest extends TestCase { + protected function setUp(): void + { + parent::setUp(); + Sleep::fake(); + } + public function test_reset_flow() { Notification::fake(); @@ -75,6 +83,17 @@ class ResetPasswordTest extends TestCase ->assertSee('The password reset token is invalid for this email address.'); } + public function test_reset_request_with_not_found_user_still_has_delay() + { + $this->followingRedirects()->post('/password/email', [ + 'email' => 'barrynotfoundrandomuser@example.com', + ]); + + Sleep::assertSlept(function (CarbonInterval $duration): bool { + return $duration->totalMilliseconds > 999; + }, 1); + } + public function test_reset_page_shows_sign_links() { $this->setSettings(['registration-enabled' => 'true']); @@ -98,4 +117,27 @@ class ResetPasswordTest extends TestCase Notification::assertSentTimes(ResetPasswordNotification::class, 1); $resp->assertSee('A password reset link will be sent to ' . $editor->email . ' if that email address is found in the system.'); } + + public function test_reset_request_with_not_found_user_is_throttled() + { + for ($i = 0; $i < 11; $i++) { + $response = $this->post('/password/email', [ + 'email' => 'barrynotfoundrandomuser@example.com', + ]); + } + + $response->assertStatus(429); + } + + public function test_reset_call_is_throttled() + { + for ($i = 0; $i < 11; $i++) { + $response = $this->post('/password/reset', [ + 'email' => "arandomuser{$i}@example.com", + 'token' => "randomtoken{$i}", + ]); + } + + $response->assertStatus(429); + } } diff --git a/tests/Auth/UserInviteTest.php b/tests/Auth/UserInviteTest.php index a9dee0007..434de6aa6 100644 --- a/tests/Auth/UserInviteTest.php +++ b/tests/Auth/UserInviteTest.php @@ -137,4 +137,24 @@ class UserInviteTest extends TestCase $setPasswordPageResp->assertRedirect('/password/email'); $setPasswordPageResp->assertSessionHas('error', 'This invitation link has expired. You can instead try to reset your account password.'); } + + public function test_set_password_view_is_throttled() + { + for ($i = 0; $i < 11; $i++) { + $response = $this->get("/register/invite/tokenhere{$i}"); + } + + $response->assertStatus(429); + } + + public function test_set_password_post_is_throttled() + { + for ($i = 0; $i < 11; $i++) { + $response = $this->post("/register/invite/tokenhere{$i}", [ + 'password' => 'my test password', + ]); + } + + $response->assertStatus(429); + } }