Browse Source
fix(ratelimit): Allow to bypass rate-limit from bruteforce allowlist
fix(ratelimit): Allow to bypass rate-limit from bruteforce allowlist
Signed-off-by: Joas Schilling <coding@schilljs.com>pull/50234/head
Joas Schilling
9 months ago
No known key found for this signature in database
GPG Key ID: F72FA5B49FFA96B0
10 changed files with 246 additions and 293 deletions
-
5build/psalm-baseline.xml
-
1lib/composer/composer/autoload_classmap.php
-
1lib/composer/composer/autoload_static.php
-
10lib/private/AppFramework/DependencyInjection/DIContainer.php
-
9lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php
-
69lib/private/Security/Bruteforce/Throttler.php
-
60lib/private/Security/Ip/BruteforceAllowList.php
-
10tests/lib/AppFramework/Middleware/Security/RateLimitingMiddlewareTest.php
-
212tests/lib/Security/Bruteforce/ThrottlerTest.php
-
162tests/lib/Security/Ip/BruteforceAllowListTest.php
@ -0,0 +1,60 @@ |
|||
<?php |
|||
|
|||
declare(strict_types=1); |
|||
|
|||
/** |
|||
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors |
|||
* SPDX-License-Identifier: AGPL-3.0-or-later |
|||
*/ |
|||
namespace OC\Security\Ip; |
|||
|
|||
use OCP\IAppConfig; |
|||
use OCP\Security\Ip\IFactory; |
|||
|
|||
class BruteforceAllowList { |
|||
/** @var array<string, bool> */ |
|||
protected array $ipIsAllowListed = []; |
|||
|
|||
public function __construct( |
|||
private readonly IAppConfig $appConfig, |
|||
private readonly IFactory $factory, |
|||
) { |
|||
} |
|||
|
|||
/** |
|||
* Check if the IP is allowed to bypass bruteforce protection |
|||
*/ |
|||
public function isBypassListed(string $ip): bool { |
|||
if (isset($this->ipIsAllowListed[$ip])) { |
|||
return $this->ipIsAllowListed[$ip]; |
|||
} |
|||
|
|||
try { |
|||
$address = $this->factory->addressFromString($ip); |
|||
} catch (\InvalidArgumentException) { |
|||
$this->ipIsAllowListed[$ip] = false; |
|||
return false; |
|||
} |
|||
|
|||
$keys = $this->appConfig->getKeys('bruteForce'); |
|||
$keys = array_filter($keys, static fn ($key): bool => str_starts_with($key, 'whitelist_')); |
|||
|
|||
foreach ($keys as $key) { |
|||
$rangeString = $this->appConfig->getValueString('bruteForce', $key); |
|||
try { |
|||
$range = $this->factory->rangeFromString($rangeString); |
|||
} catch (\InvalidArgumentException) { |
|||
continue; |
|||
} |
|||
|
|||
$allowed = $range->contains($address); |
|||
if ($allowed) { |
|||
$this->ipIsAllowListed[$ip] = true; |
|||
return true; |
|||
} |
|||
} |
|||
|
|||
$this->ipIsAllowListed[$ip] = false; |
|||
return false; |
|||
} |
|||
} |
|||
@ -1,212 +0,0 @@ |
|||
<?php |
|||
|
|||
declare(strict_types=1); |
|||
|
|||
/** |
|||
* SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors |
|||
* SPDX-License-Identifier: AGPL-3.0-or-later |
|||
*/ |
|||
|
|||
namespace Test\Security\Bruteforce; |
|||
|
|||
use OC\Security\Bruteforce\Backend\DatabaseBackend; |
|||
use OC\Security\Bruteforce\Throttler; |
|||
use OCP\AppFramework\Utility\ITimeFactory; |
|||
use OCP\IConfig; |
|||
use OCP\IDBConnection; |
|||
use Psr\Log\LoggerInterface; |
|||
use Test\TestCase; |
|||
|
|||
/** |
|||
* Based on the unit tests from Paragonie's Airship CMS |
|||
* Ref: https://github.com/paragonie/airship/blob/7e5bad7e3c0fbbf324c11f963fd1f80e59762606/test/unit/Engine/Security/AirBrakeTest.php |
|||
* |
|||
* @package Test\Security\Bruteforce |
|||
*/ |
|||
class ThrottlerTest extends TestCase { |
|||
/** @var Throttler */ |
|||
private $throttler; |
|||
/** @var IDBConnection */ |
|||
private $dbConnection; |
|||
/** @var ITimeFactory */ |
|||
private $timeFactory; |
|||
/** @var LoggerInterface */ |
|||
private $logger; |
|||
/** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ |
|||
private $config; |
|||
|
|||
protected function setUp(): void { |
|||
$this->dbConnection = $this->createMock(IDBConnection::class); |
|||
$this->timeFactory = $this->createMock(ITimeFactory::class); |
|||
$this->logger = $this->createMock(LoggerInterface::class); |
|||
$this->config = $this->createMock(IConfig::class); |
|||
|
|||
$this->throttler = new Throttler( |
|||
$this->timeFactory, |
|||
$this->logger, |
|||
$this->config, |
|||
new DatabaseBackend($this->dbConnection) |
|||
); |
|||
parent::setUp(); |
|||
} |
|||
|
|||
public function dataIsIPWhitelisted() { |
|||
return [ |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.0/24', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.11/31', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.9/31', |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.15/29', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
'whitelist_2' => 'deaf:beef:cafe:1234::/64' |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
'whitelist_2' => 'deaf:beef::/64' |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
'whitelist_2' => 'deaf:cafe::/8' |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1111', |
|||
[ |
|||
'whitelist_0' => 'dead:beef:cafe::1100/123', |
|||
|
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'invalid', |
|||
[], |
|||
false, |
|||
], |
|||
]; |
|||
} |
|||
|
|||
/** |
|||
* @param string $ip |
|||
* @param string[] $whitelists |
|||
* @param bool $isWhiteListed |
|||
* @param bool $enabled |
|||
*/ |
|||
private function isIpWhiteListedHelper($ip, |
|||
$whitelists, |
|||
$isWhiteListed, |
|||
$enabled) { |
|||
$this->config->method('getAppKeys') |
|||
->with($this->equalTo('bruteForce')) |
|||
->willReturn(array_keys($whitelists)); |
|||
$this->config |
|||
->expects($this->once()) |
|||
->method('getSystemValueBool') |
|||
->with('auth.bruteforce.protection.enabled', true) |
|||
->willReturn($enabled); |
|||
|
|||
$this->config->method('getAppValue') |
|||
->willReturnCallback(function ($app, $key, $default) use ($whitelists) { |
|||
if ($app !== 'bruteForce') { |
|||
return $default; |
|||
} |
|||
if (isset($whitelists[$key])) { |
|||
return $whitelists[$key]; |
|||
} |
|||
return $default; |
|||
}); |
|||
|
|||
$this->assertSame( |
|||
($enabled === false) ? true : $isWhiteListed, |
|||
self::invokePrivate($this->throttler, 'isBypassListed', [$ip]) |
|||
); |
|||
} |
|||
|
|||
/** |
|||
* @dataProvider dataIsIPWhitelisted |
|||
* |
|||
* @param string $ip |
|||
* @param string[] $whitelists |
|||
* @param bool $isWhiteListed |
|||
*/ |
|||
public function testIsIpWhiteListedWithEnabledProtection($ip, |
|||
$whitelists, |
|||
$isWhiteListed): void { |
|||
$this->isIpWhiteListedHelper( |
|||
$ip, |
|||
$whitelists, |
|||
$isWhiteListed, |
|||
true |
|||
); |
|||
} |
|||
|
|||
/** |
|||
* @dataProvider dataIsIPWhitelisted |
|||
* |
|||
* @param string $ip |
|||
* @param string[] $whitelists |
|||
* @param bool $isWhiteListed |
|||
*/ |
|||
public function testIsIpWhiteListedWithDisabledProtection($ip, |
|||
$whitelists, |
|||
$isWhiteListed): void { |
|||
$this->isIpWhiteListedHelper( |
|||
$ip, |
|||
$whitelists, |
|||
$isWhiteListed, |
|||
false |
|||
); |
|||
} |
|||
} |
|||
@ -0,0 +1,162 @@ |
|||
<?php |
|||
|
|||
declare(strict_types=1); |
|||
|
|||
/** |
|||
* SPDX-FileCopyrightText: 2016 Nextcloud GmbH and Nextcloud contributors |
|||
* SPDX-License-Identifier: AGPL-3.0-or-later |
|||
*/ |
|||
|
|||
namespace Test\Security\Ip; |
|||
|
|||
use OC\Security\Ip\BruteforceAllowList; |
|||
use OC\Security\Ip\Factory; |
|||
use OCP\IAppConfig; |
|||
use OCP\Security\Ip\IFactory; |
|||
use PHPUnit\Framework\MockObject\MockObject; |
|||
use Test\TestCase; |
|||
|
|||
/** |
|||
* Based on the unit tests from Paragonie's Airship CMS |
|||
* Ref: https://github.com/paragonie/airship/blob/7e5bad7e3c0fbbf324c11f963fd1f80e59762606/test/unit/Engine/Security/AirBrakeTest.php |
|||
* |
|||
* @package Test\Security\Bruteforce |
|||
*/ |
|||
class BruteforceAllowListTest extends TestCase { |
|||
/** @var IAppConfig|MockObject */ |
|||
private $appConfig; |
|||
/** @var IFactory|MockObject */ |
|||
private $factory; |
|||
/** @var BruteforceAllowList */ |
|||
private $allowList; |
|||
|
|||
protected function setUp(): void { |
|||
parent::setUp(); |
|||
|
|||
$this->appConfig = $this->createMock(IAppConfig::class); |
|||
$this->factory = new Factory(); |
|||
|
|||
$this->allowList = new BruteforceAllowList( |
|||
$this->appConfig, |
|||
$this->factory, |
|||
); |
|||
} |
|||
|
|||
public function dataIsBypassListed(): array { |
|||
return [ |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.0/24', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.11/31', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.9/31', |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'10.10.10.10', |
|||
[ |
|||
'whitelist_0' => '10.10.10.15/29', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
'whitelist_2' => 'deaf:beef:cafe:1234::/64' |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
'whitelist_2' => 'deaf:beef::/64' |
|||
], |
|||
false, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1', |
|||
[ |
|||
'whitelist_0' => '192.168.0.0/16', |
|||
'whitelist_1' => '10.10.10.0/24', |
|||
'whitelist_2' => 'deaf:cafe::/8' |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'dead:beef:cafe::1111', |
|||
[ |
|||
'whitelist_0' => 'dead:beef:cafe::1100/123', |
|||
], |
|||
true, |
|||
], |
|||
[ |
|||
'invalid', |
|||
[], |
|||
false, |
|||
], |
|||
]; |
|||
} |
|||
|
|||
/** |
|||
* @dataProvider dataIsBypassListed |
|||
* |
|||
* @param string[] $allowList |
|||
*/ |
|||
public function testIsBypassListed( |
|||
string $ip, |
|||
array $allowList, |
|||
bool $isAllowListed, |
|||
): void { |
|||
$this->appConfig->method('getKeys') |
|||
->with($this->equalTo('bruteForce')) |
|||
->willReturn(array_keys($allowList)); |
|||
|
|||
$this->appConfig->method('getValueString') |
|||
->willReturnCallback(function ($app, $key, $default) use ($allowList) { |
|||
if ($app !== 'bruteForce') { |
|||
return $default; |
|||
} |
|||
if (isset($allowList[$key])) { |
|||
return $allowList[$key]; |
|||
} |
|||
return $default; |
|||
}); |
|||
|
|||
$this->assertSame( |
|||
$isAllowListed, |
|||
$this->allowList->isBypassListed($ip) |
|||
); |
|||
} |
|||
} |
|||
Write
Preview
Loading…
Cancel
Save
Reference in new issue