From 3a6bc7aba2625dd4144e25033fabe8b8f42afb42 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 11 May 2023 09:17:30 +0200 Subject: [PATCH] fix(middleware): Also abort the request when reaching max delay in afterController Signed-off-by: Joas Schilling --- .../Security/BruteForceMiddleware.php | 52 +++++++++++-------- .../Security/BruteForceMiddlewareTest.php | 14 ++--- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php index ba8c7f45b49c3..2ecd26a68e1ff 100644 --- a/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/BruteForceMiddleware.php @@ -90,32 +90,40 @@ public function beforeController($controller, $methodName) { */ public function afterController($controller, $methodName, Response $response) { if ($response->isThrottled()) { - if ($this->reflector->hasAnnotation('BruteForceProtection')) { - $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action'); - $ip = $this->request->getRemoteAddress(); - $this->throttler->sleepDelay($ip, $action); - $this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata()); - } else { - $reflectionMethod = new ReflectionMethod($controller, $methodName); - $attributes = $reflectionMethod->getAttributes(BruteForceProtection::class); - - if (!empty($attributes)) { + try { + if ($this->reflector->hasAnnotation('BruteForceProtection')) { + $action = $this->reflector->getAnnotationParameter('BruteForceProtection', 'action'); $ip = $this->request->getRemoteAddress(); - $metaData = $response->getThrottleMetadata(); - - foreach ($attributes as $attribute) { - /** @var BruteForceProtection $protection */ - $protection = $attribute->newInstance(); - $action = $protection->getAction(); - - if (!isset($metaData['action']) || $metaData['action'] === $action) { - $this->throttler->sleepDelay($ip, $action); - $this->throttler->registerAttempt($action, $ip, $metaData); + $this->throttler->registerAttempt($action, $ip, $response->getThrottleMetadata()); + $this->throttler->sleepDelayOrThrowOnMax($ip, $action); + } else { + $reflectionMethod = new ReflectionMethod($controller, $methodName); + $attributes = $reflectionMethod->getAttributes(BruteForceProtection::class); + + if (!empty($attributes)) { + $ip = $this->request->getRemoteAddress(); + $metaData = $response->getThrottleMetadata(); + + foreach ($attributes as $attribute) { + /** @var BruteForceProtection $protection */ + $protection = $attribute->newInstance(); + $action = $protection->getAction(); + + if (!isset($metaData['action']) || $metaData['action'] === $action) { + $this->throttler->registerAttempt($action, $ip, $metaData); + $this->throttler->sleepDelayOrThrowOnMax($ip, $action); + } } + } else { + $this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.'); } - } else { - $this->logger->debug('Response for ' . get_class($controller) . '::' . $methodName . ' got bruteforce throttled but has no annotation nor attribute defined.'); } + } catch (MaxDelayReached $e) { + if ($controller instanceof OCSController) { + throw new OCSException($e->getMessage(), Http::STATUS_TOO_MANY_REQUESTS); + } + + return new TooManyRequestsResponse(); } } diff --git a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php index 388b2fbf53439..faf2d24d17289 100644 --- a/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/BruteForceMiddlewareTest.php @@ -157,7 +157,7 @@ public function testAfterControllerWithAnnotationAndThrottledRequest(): void { ->willReturn('127.0.0.1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('127.0.0.1', 'login'); $this->throttler ->expects($this->once()) @@ -181,7 +181,7 @@ public function testAfterControllerWithAnnotationAndNotThrottledRequest(): void ->method('getRemoteAddress'); $this->throttler ->expects($this->never()) - ->method('sleepDelay'); + ->method('sleepDelayOrThrowOnMax'); $this->throttler ->expects($this->never()) ->method('registerAttempt'); @@ -209,7 +209,7 @@ public function testAfterControllerWithSingleAttribute(): void { ->willReturn('::1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('::1', 'single'); $this->throttler ->expects($this->once()) @@ -239,7 +239,7 @@ public function testAfterControllerWithMultipleAttributesGeneralMatch(): void { ->willReturn('::1'); $this->throttler ->expects($this->exactly(2)) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->withConsecutive( ['::1', 'first'], ['::1', 'second'], @@ -275,7 +275,7 @@ public function testAfterControllerWithMultipleAttributesSpecificMatch(): void { ->willReturn('::1'); $this->throttler ->expects($this->once()) - ->method('sleepDelay') + ->method('sleepDelayOrThrowOnMax') ->with('::1', 'second'); $this->throttler ->expects($this->once()) @@ -293,7 +293,7 @@ public function testAfterControllerWithoutAnnotation(): void { ->method('getRemoteAddress'); $this->throttler ->expects($this->never()) - ->method('sleepDelay'); + ->method('sleepDelayOrThrowOnMax'); $controller = new TestController('test', $this->request); $this->reflector->reflect($controller, 'testMethodWithoutAnnotation'); @@ -308,7 +308,7 @@ public function testAfterControllerWithThrottledResponseButUnhandled(): void { ->method('getRemoteAddress'); $this->throttler ->expects($this->never()) - ->method('sleepDelay'); + ->method('sleepDelayOrThrowOnMax'); $controller = new TestController('test', $this->request); $this->reflector->reflect($controller, 'testMethodWithoutAnnotation');