fix(throttler): Remove the sleep from the throttler that throwsbugfix/noid/remove-sleep-from-throttler

The sleep is not adding benefit when it's being aborted with 429
in other cases anyway.

Signed-off-by: Joas Schilling <coding@schilljs.com>

6 files changed, 34 insertions, 16 deletions

diff --git a/apps/dav/lib/Connector/Sabre/PublicAuth.php b/apps/dav/lib/Connector/Sabre/PublicAuth.php
index ea59d9efc8f..b5d9ce3db72 100644
--- a/apps/dav/lib/Connector/Sabre/PublicAuth.php
+++ b/apps/dav/lib/Connector/Sabre/PublicAuth.php
@@ -15,6 +15,7 @@ use OCP\Defaults;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\Security\Bruteforce\IThrottler;
+use OCP\Security\Bruteforce\MaxDelayReached;
 use OCP\Share\Exceptions\ShareNotFound;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
@@ -56,6 +57,7 @@ class PublicAuth extends AbstractBasic {
 	 *
 	 * @return array
 	 * @throws NotAuthenticated
+	 * @throws MaxDelayReached
 	 * @throws ServiceUnavailable
 	 */
 	public function check(RequestInterface $request, ResponseInterface $response): array {
@@ -75,7 +77,8 @@ class PublicAuth extends AbstractBasic {
 			}
 
 			return $this->checkToken();
-		} catch (NotAuthenticated $e) {
+		} catch (NotAuthenticated|MaxDelayReached $e) {
+			$this->throttler->registerAttempt(self::BRUTEFORCE_ACTION, $this->request->getRemoteAddress());
 			throw $e;
 		} catch (\Exception $e) {
 			$class = get_class($e);
@@ -94,7 +97,7 @@ class PublicAuth extends AbstractBasic {
 		$path = $this->request->getPathInfo() ?: '';
 		// ['', 'dav', 'files', 'token']
 		$splittedPath = explode('/', $path);
-		
+
 		if (count($splittedPath) < 4 || $splittedPath[3] === '') {
 			throw new NotFound();
 		}
@@ -176,7 +179,7 @@ class PublicAuth extends AbstractBasic {
 					}
 					return true;
 				}
-				
+
 				if ($this->session->exists(PublicAuth::DAV_AUTHENTICATED)
 					&& $this->session->get(PublicAuth::DAV_AUTHENTICATED) === $share->getId()) {
 					return true;
diff --git a/apps/dav/lib/Direct/DirectHome.php b/apps/dav/lib/Direct/DirectHome.php
index 10e1017f5a4..ac411c9b52f 100644
--- a/apps/dav/lib/Direct/DirectHome.php
+++ b/apps/dav/lib/Direct/DirectHome.php
@@ -53,7 +53,7 @@ class DirectHome implements ICollection {
 		} catch (DoesNotExistException $e) {
 			// Since the token space is so huge only throttle on non-existing token
 			$this->throttler->registerAttempt('directlink', $this->request->getRemoteAddress());
-			$this->throttler->sleepDelay($this->request->getRemoteAddress(), 'directlink');
+			$this->throttler->sleepDelayOrThrowOnMax($this->request->getRemoteAddress(), 'directlink');
 
 			throw new NotFound();
 		}
diff --git a/apps/dav/lib/Files/BrowserErrorPagePlugin.php b/apps/dav/lib/Files/BrowserErrorPagePlugin.php
index de86c4995e2..85ed975a409 100644
--- a/apps/dav/lib/Files/BrowserErrorPagePlugin.php
+++ b/apps/dav/lib/Files/BrowserErrorPagePlugin.php
@@ -11,6 +11,7 @@ use OC\AppFramework\Http\Request;
 use OCP\AppFramework\Http\ContentSecurityPolicy;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\IRequest;
+use OCP\Security\Bruteforce\MaxDelayReached;
 use OCP\Template\ITemplateManager;
 use Sabre\DAV\Exception;
 use Sabre\DAV\Server;
@@ -60,6 +61,9 @@ class BrowserErrorPagePlugin extends ServerPlugin {
 		if ($ex instanceof Exception) {
 			$httpCode = $ex->getHTTPCode();
 			$headers = $ex->getHTTPHeaders($this->server);
+		} elseif ($ex instanceof MaxDelayReached) {
+			$httpCode = 429;
+			$headers = [];
 		} else {
 			$httpCode = 500;
 			$headers = [];
@@ -81,7 +85,7 @@ class BrowserErrorPagePlugin extends ServerPlugin {
 		$request = \OCP\Server::get(IRequest::class);
 
 		$templateName = 'exception';
-		if ($httpCode === 403 || $httpCode === 404) {
+		if ($httpCode === 403 || $httpCode === 404 || $httpCode === 429) {
 			$templateName = (string)$httpCode;
 		}
 
diff --git a/apps/dav/tests/unit/Direct/DirectHomeTest.php b/apps/dav/tests/unit/Direct/DirectHomeTest.php
index 1134f0cd3af..06fb48a64d8 100644
--- a/apps/dav/tests/unit/Direct/DirectHomeTest.php
+++ b/apps/dav/tests/unit/Direct/DirectHomeTest.php
@@ -160,7 +160,7 @@ class DirectHomeTest extends TestCase {
 				'1.2.3.4'
 			);
 		$this->throttler->expects($this->once())
-			->method('sleepDelay')
+			->method('sleepDelayOrThrowOnMax')
 			->with(
 				'1.2.3.4',
 				'directlink'
diff --git a/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php b/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php
index 2b3025fccff..b3040673d0f 100644
--- a/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php
+++ b/lib/private/AppFramework/Middleware/PublicShare/PublicShareMiddleware.php
@@ -120,7 +120,7 @@ class PublicShareMiddleware extends Middleware {
 
 	private function throttle($bruteforceProtectionAction, $token): void {
 		$ip = $this->request->getRemoteAddress();
-		$this->throttler->sleepDelay($ip, $bruteforceProtectionAction);
+		$this->throttler->sleepDelayOrThrowOnMax($ip, $bruteforceProtectionAction);
 		$this->throttler->registerAttempt($bruteforceProtectionAction, $ip, ['token' => $token]);
 	}
 }
diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php
index 21d50848641..065f720ba72 100644
--- a/lib/private/Security/Bruteforce/Throttler.php
+++ b/lib/private/Security/Bruteforce/Throttler.php
@@ -127,6 +127,13 @@ class Throttler implements IThrottler {
 	 */
 	public function getDelay(string $ip, string $action = ''): int {
 		$attempts = $this->getAttempts($ip, $action);
+		return $this->calculateDelay($attempts);
+	}
+
+	/**
+	 * {@inheritDoc}
+	 */
+	public function calculateDelay(int $attempts): int {
 		if ($attempts === 0) {
 			return 0;
 		}
@@ -199,25 +206,29 @@ class Throttler implements IThrottler {
 	 * {@inheritDoc}
 	 */
 	public function sleepDelayOrThrowOnMax(string $ip, string $action = ''): int {
-		$delay = $this->getDelay($ip, $action);
-		if (($delay === self::MAX_DELAY_MS) && $this->getAttempts($ip, $action, 0.5) > $this->config->getSystemValueInt('auth.bruteforce.max-attempts', self::MAX_ATTEMPTS)) {
-			$this->logger->info('IP address blocked because it reached the maximum failed attempts in the last 30 minutes [action: {action}, ip: {ip}]', [
+		$attempts = $this->getAttempts($ip, $action, 0.5);
+		if ($attempts > $this->config->getSystemValueInt('auth.bruteforce.max-attempts', self::MAX_ATTEMPTS)) {
+			$this->logger->info('IP address blocked because it reached the maximum failed attempts in the last 30 minutes [action: {action}, attempts: {attempts}, ip: {ip}]', [
 				'action' => $action,
 				'ip' => $ip,
+				'attempts' => $attempts,
 			]);
 			// If the ip made too many attempts within the last 30 mins we don't execute anymore
 			throw new MaxDelayReached('Reached maximum delay');
 		}
-		if ($delay > 100) {
-			$this->logger->info('IP address throttled because it reached the attempts limit in the last 30 minutes [action: {action}, delay: {delay}, ip: {ip}]', [
+
+		$attempts = $this->getAttempts($ip, $action);
+		if ($attempts > 10) {
+			$this->logger->info('IP address throttled because it reached the attempts limit in the last 12 hours [action: {action}, attempts: {attempts}, ip: {ip}]', [
 				'action' => $action,
 				'ip' => $ip,
-				'delay' => $delay,
+				'attempts' => $attempts,
 			]);
 		}
-		if (!$this->config->getSystemValueBool('auth.bruteforce.protection.testing')) {
-			usleep($delay * 1000);
+		if ($attempts > 0) {
+			return $this->calculateDelay($attempts);
 		}
-		return $delay;
+
+		return 0;
 	}
 }