From 8188f1cf153830f0ac4aefb45209c1eb49767ae8 Mon Sep 17 00:00:00 2001 From: MatTheCat Date: Mon, 15 Aug 2022 19:09:15 +0200 Subject: [PATCH] [HttpFoundation] Prevent accepted rate limits with no remaining token to be preferred over denied ones --- .../AbstractRequestRateLimiter.php | 20 +++++- .../AbstractRequestRateLimiterTest.php | 64 +++++++++++++++++++ .../MockAbstractRequestRateLimiter.php | 34 ++++++++++ .../Component/HttpFoundation/composer.json | 3 +- 4 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 src/Symfony/Component/HttpFoundation/Tests/RateLimiter/AbstractRequestRateLimiterTest.php create mode 100644 src/Symfony/Component/HttpFoundation/Tests/RateLimiter/MockAbstractRequestRateLimiter.php diff --git a/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php b/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php index c91d614fe30bf..a6dd993b7315b 100644 --- a/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php +++ b/src/Symfony/Component/HttpFoundation/RateLimiter/AbstractRequestRateLimiter.php @@ -35,9 +35,7 @@ public function consume(Request $request): RateLimit foreach ($limiters as $limiter) { $rateLimit = $limiter->consume(1); - if (null === $minimalRateLimit || $rateLimit->getRemainingTokens() < $minimalRateLimit->getRemainingTokens()) { - $minimalRateLimit = $rateLimit; - } + $minimalRateLimit = $minimalRateLimit ? self::getMinimalRateLimit($minimalRateLimit, $rateLimit) : $rateLimit; } return $minimalRateLimit; @@ -54,4 +52,20 @@ public function reset(Request $request): void * @return LimiterInterface[] a set of limiters using keys extracted from the request */ abstract protected function getLimiters(Request $request): array; + + private static function getMinimalRateLimit(RateLimit $first, RateLimit $second): RateLimit + { + if ($first->isAccepted() !== $second->isAccepted()) { + return $first->isAccepted() ? $second : $first; + } + + $firstRemainingTokens = $first->getRemainingTokens(); + $secondRemainingTokens = $second->getRemainingTokens(); + + if ($firstRemainingTokens === $secondRemainingTokens) { + return $first->getRetryAfter() < $second->getRetryAfter() ? $second : $first; + } + + return $firstRemainingTokens > $secondRemainingTokens ? $second : $first; + } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/RateLimiter/AbstractRequestRateLimiterTest.php b/src/Symfony/Component/HttpFoundation/Tests/RateLimiter/AbstractRequestRateLimiterTest.php new file mode 100644 index 0000000000000..4790eae183802 --- /dev/null +++ b/src/Symfony/Component/HttpFoundation/Tests/RateLimiter/AbstractRequestRateLimiterTest.php @@ -0,0 +1,64 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpFoundation\Tests\RateLimiter; + +use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\RateLimiter\LimiterInterface; +use Symfony\Component\RateLimiter\RateLimit; + +class AbstractRequestRateLimiterTest extends TestCase +{ + /** + * @dataProvider provideRateLimits + */ + public function testConsume(array $rateLimits, ?RateLimit $expected) + { + $rateLimiter = new MockAbstractRequestRateLimiter(array_map(function (RateLimit $rateLimit) { + $limiter = $this->createStub(LimiterInterface::class); + $limiter->method('consume')->willReturn($rateLimit); + + return $limiter; + }, $rateLimits)); + + $this->assertSame($expected, $rateLimiter->consume(new Request())); + } + + public function provideRateLimits() + { + $now = new \DateTimeImmutable(); + + yield 'Both accepted with different count of remaining tokens' => [ + [ + $expected = new RateLimit(0, $now, true, 1), // less remaining tokens + new RateLimit(1, $now, true, 1), + ], + $expected, + ]; + + yield 'Both accepted with same count of remaining tokens' => [ + [ + $expected = new RateLimit(0, $now->add(new \DateInterval('P1D')), true, 1), // longest wait time + new RateLimit(0, $now, true, 1), + ], + $expected, + ]; + + yield 'Accepted and denied' => [ + [ + new RateLimit(0, $now, true, 1), + $expected = new RateLimit(0, $now, false, 1), // denied + ], + $expected, + ]; + } +} diff --git a/src/Symfony/Component/HttpFoundation/Tests/RateLimiter/MockAbstractRequestRateLimiter.php b/src/Symfony/Component/HttpFoundation/Tests/RateLimiter/MockAbstractRequestRateLimiter.php new file mode 100644 index 0000000000000..0acc918bf4d5c --- /dev/null +++ b/src/Symfony/Component/HttpFoundation/Tests/RateLimiter/MockAbstractRequestRateLimiter.php @@ -0,0 +1,34 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\HttpFoundation\Tests\RateLimiter; + +use Symfony\Component\HttpFoundation\RateLimiter\AbstractRequestRateLimiter; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\RateLimiter\LimiterInterface; + +class MockAbstractRequestRateLimiter extends AbstractRequestRateLimiter +{ + /** + * @var LimiterInterface[] + */ + private $limiters; + + public function __construct(array $limiters) + { + $this->limiters = $limiters; + } + + protected function getLimiters(Request $request): array + { + return $this->limiters; + } +} diff --git a/src/Symfony/Component/HttpFoundation/composer.json b/src/Symfony/Component/HttpFoundation/composer.json index 452281794b615..cb8d59ffed0d5 100644 --- a/src/Symfony/Component/HttpFoundation/composer.json +++ b/src/Symfony/Component/HttpFoundation/composer.json @@ -27,7 +27,8 @@ "symfony/dependency-injection": "^5.4|^6.0", "symfony/http-kernel": "^5.4.12|^6.0.12|^6.1.4", "symfony/mime": "^4.4|^5.0|^6.0", - "symfony/expression-language": "^4.4|^5.0|^6.0" + "symfony/expression-language": "^4.4|^5.0|^6.0", + "symfony/rate-limiter": "^5.2|^6.0" }, "suggest" : { "symfony/mime": "To use the file extension guesser"