From 167a4c7a0ad7927ae11dc099b705bed88d0a2c9d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 23 Apr 2021 10:21:14 +0200 Subject: [PATCH 1/4] Fix ratelimit template Signed-off-by: Joas Schilling --- .../Security/RateLimitingMiddleware.php | 21 +++++++------------ .../Security/RateLimitingMiddlewareTest.php | 20 ++++++++---------- 2 files changed, 16 insertions(+), 25 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php index 712becb3be..f596088054 100644 --- a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php @@ -27,7 +27,7 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\RateLimiting\Exception\RateLimitExceededException; use OC\Security\RateLimiting\Limiter; -use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; use OCP\IRequest; @@ -110,21 +110,14 @@ class RateLimitingMiddleware extends Middleware { public function afterException($controller, $methodName, \Exception $exception) { if ($exception instanceof RateLimitExceededException) { if (stripos($this->request->getHeader('Accept'),'html') === false) { - $response = new JSONResponse( - [ - 'message' => $exception->getMessage(), - ], - $exception->getCode() - ); + $response = new DataResponse([], $exception->getCode()); } else { $response = new TemplateResponse( - 'core', - '403', - [ - 'file' => $exception->getMessage() - ], - 'guest' - ); + 'core', + '429', + [], + TemplateResponse::RENDER_AS_GUEST + ); $response->setStatus($exception->getCode()); } diff --git a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php index 3a06531eef..30d51eda96 100644 --- a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php @@ -26,13 +26,16 @@ use OC\AppFramework\Utility\ControllerMethodReflector; use OC\Security\RateLimiting\Exception\RateLimitExceededException; use OC\Security\RateLimiting\Limiter; use OCP\AppFramework\Controller; -use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\IRequest; use OCP\IUser; use OCP\IUserSession; use Test\TestCase; +/** + * @group DB + */ class RateLimitingMiddlewareTest extends TestCase { /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ private $request; @@ -250,11 +253,7 @@ class RateLimitingMiddlewareTest extends TestCase { ->willReturn('JSON'); $result = $this->rateLimitingMiddleware->afterException($controller, 'testMethod', new RateLimitExceededException()); - $expected = new JSONResponse( - [ - 'message' => 'Rate limit exceeded', - ], - 429 + $expected = new DataResponse([], 429 ); $this->assertEquals($expected, $result); } @@ -271,13 +270,12 @@ class RateLimitingMiddlewareTest extends TestCase { $result = $this->rateLimitingMiddleware->afterException($controller, 'testMethod', new RateLimitExceededException()); $expected = new TemplateResponse( 'core', - '403', - [ - 'file' => 'Rate limit exceeded', - ], - 'guest' + '429', + [], + TemplateResponse::RENDER_AS_GUEST ); $expected->setStatus(429); $this->assertEquals($expected, $result); + $this->assertIsString($result->render()); } } From aa178f9e253e762e156c1206d55f4b37daf43407 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 23 Apr 2021 10:54:45 +0200 Subject: [PATCH 2/4] Do not allow to overwrite some variables Signed-off-by: Joas Schilling --- lib/private/Template/Base.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/private/Template/Base.php b/lib/private/Template/Base.php index 04d03b6e6f..856d09ad4e 100644 --- a/lib/private/Template/Base.php +++ b/lib/private/Template/Base.php @@ -168,7 +168,9 @@ class Base { if (!is_null($additionalParams)) { $_ = array_merge($additionalParams, $this->vars); foreach ($_ as $var => $value) { - ${$var} = $value; + if (!isset(${$var})) { + ${$var} = $value; + } } } From f585fbc39127d1f001882d872febf2245bed5751 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 28 Apr 2021 09:59:53 +0200 Subject: [PATCH 3/4] Fix Nextcloud19 compatibility Signed-off-by: Joas Schilling --- .../Middleware/Security/RateLimitingMiddleware.php | 2 +- .../Middleware/Security/RateLimitingMiddlewareTest.php | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php index f596088054..de628bd9ca 100644 --- a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php @@ -116,7 +116,7 @@ class RateLimitingMiddleware extends Middleware { 'core', '429', [], - TemplateResponse::RENDER_AS_GUEST + 'guest' ); $response->setStatus($exception->getCode()); } diff --git a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php index 30d51eda96..4e564f3eea 100644 --- a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php @@ -232,7 +232,7 @@ class RateLimitingMiddlewareTest extends TestCase { $this->rateLimitingMiddleware->beforeController($controller, 'testMethod'); } - + public function testAfterExceptionWithOtherException() { $this->expectException(\Exception::class); $this->expectExceptionMessage('My test exception'); @@ -272,7 +272,7 @@ class RateLimitingMiddlewareTest extends TestCase { 'core', '429', [], - TemplateResponse::RENDER_AS_GUEST + 'guest' ); $expected->setStatus(429); $this->assertEquals($expected, $result); From 43d6921772ee3f20b8a242095867470f2f5177ed Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Thu, 29 Apr 2021 08:44:53 +0200 Subject: [PATCH 4/4] 429 template is NC20+ and fix getDelay for CLI Signed-off-by: Joas Schilling --- .../Middleware/Security/RateLimitingMiddleware.php | 6 ++++-- lib/private/Security/Bruteforce/Throttler.php | 4 ++++ .../Middleware/Security/RateLimitingMiddlewareTest.php | 6 ++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php index de628bd9ca..38872c9ffc 100644 --- a/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php @@ -114,8 +114,10 @@ class RateLimitingMiddleware extends Middleware { } else { $response = new TemplateResponse( 'core', - '429', - [], + '403', + [ + 'message' => $exception->getMessage(), + ], 'guest' ); $response->setStatus($exception->getCode()); diff --git a/lib/private/Security/Bruteforce/Throttler.php b/lib/private/Security/Bruteforce/Throttler.php index 1bece6a05d..d1a5f6d448 100644 --- a/lib/private/Security/Bruteforce/Throttler.php +++ b/lib/private/Security/Bruteforce/Throttler.php @@ -212,6 +212,10 @@ class Throttler { return 0; } + if ($ip === '') { + return 0; + } + $cutoffTime = (new \DateTime()) ->sub($this->getCutoff(43200)) ->getTimestamp(); diff --git a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php index 4e564f3eea..2804b8cd90 100644 --- a/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php @@ -270,8 +270,10 @@ class RateLimitingMiddlewareTest extends TestCase { $result = $this->rateLimitingMiddleware->afterException($controller, 'testMethod', new RateLimitExceededException()); $expected = new TemplateResponse( 'core', - '429', - [], + '403', + [ + 'message' => 'Rate limit exceeded', + ], 'guest' ); $expected->setStatus(429);