fix(middleware): Also abort the request when reaching max delay in af…

…terController Signed-off-by: Joas Schilling <coding@schilljs.com>
nextcloud · May 15, 2023 · 3a6bc7a · 3a6bc7a
1 parent b9026ac
commit 3a6bc7a
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 29 deletions.
diff --git 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
@@ -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');