8000 bug #58757 [RateLimiter] Fix DateInterval normalization (danydev) · symfony/symfony@d2ba257 · GitHub
[go: up one dir, main page]
Skip to content

Commit d2ba257

Browse files
nicolas-grekasnicolas-grekas
committed
bug #58757 [RateLimiter] Fix DateInterval normalization (danydev)
This PR was merged into the 5.4 branch. Discussion ---------- [RateLimiter] Fix DateInterval normalization | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | n.a. | License | MIT This PR fixes a nasty bug (actually 2 different bugs) that caused our API to be unavailable for more than 24 hours. We needed to apply a manual mitigation to resolve the issue (see more below). Specifically, the PR fixes a bug that causes the wrong calculation of the TTL keys containing the information of the rate limits for a `fixed window` limiter. By doing so, our API consumers that respected the limit, actually were (wrongly) blocked by the RateLimiter, because the RateLimiter relies on the fact that the keys expire after the window defined by the configuration, so by using a wrong TTL, it ends up to a wrong evaluation of the consumed tokens. To make it even worse, the TTL is reset at its original value every time a new rate limiter evaluation occurs for that key, so since our clients continued to do the requests (that without the bug would be accepted) the "denial of service" lasted indefinitely until we manually removed all the affected keys in Redis. **Bug 1** Prerequisites: - Using as global php timezone one that has DST (in our case `Europe/Rome`) - Executing code within one hour from the switch CEST->CET or in other words between `27 October 2024 2:00AM CEST` and `27 October 2024 2:59AM CEST` (e.g. `27 October 2024 2:02AM CEST`) - Using a fixed window rate limiter - Doing the 1st request for a given key (so that the TTL is calculated and set) - Using PHP >= 8.1 (read more in the next comment) **Bug 2** Prerequisites: - Using as global php timezone one that has DST (in our case `Europe/Rome`) - Executing code in a CEST time where by adding the rate limiter interval the time goes to CEST, (e.g. `27 October 2024 2:50AM CEST` using an rate limiter interval of 20 minutes) - Using a fixed window rate limiter - Doing the 1st request for a given key (so that the TTL is calculated and set) - Using PHP >= 8.3 (read more in the next comment) Those bugs are caused by a php behaviour that it's not optimal nor intuiti 8000 ve, I feel like **Bug 1** could be defined as a PHP bug, while **Bug 2** is intended behaviour, anyway it's like this, and we need to deal with it at the application level. Commits ------- d81af98 [RateLimiter] Fix DateInterval normalization
2 parents c905bb4 + d81af98 commit d2ba257

File tree

3 files changed

+40
-2
lines changed

3 files changed

+40
-2
lines changed

‎src/Symfony/Component/RateLimiter/RateLimiterFactory.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,11 @@ protected static function configureOptions(OptionsResolver $options): void
6969
{
7070
$intervalNormalizer = static function (Options $options, string $interval): \DateInterval {
7171
try {
72-
return (new \DateTimeImmutable())->diff(new \DateTimeImmutable('+'.$interval));
72+
// Create DateTimeImmutable from unix timesatmp, so the default timezone is ignored and we don't need to
73+
// deal with quirks happening when modifying dates using a timezone with DST.
74+
$now = \DateTimeImmutable::createFromFormat('U', time());
75+
76+
return $now->diff($now->modify('+'.$interval));
7377
} catch (\Exception $e) {
7478
if (!preg_match('/Failed to parse time string \(\+([^)]+)\)/', $e->getMessage(), $m)) {
7579
throw $e;

‎src/Symfony/Component/RateLimiter/Tests/RateLimiterFactoryTest.php

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line 8000 numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\RateLimiter\Tests;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Bridge\PhpUnit\ClockMock;
1516
use Symfony\Component\OptionsResolver\Exception\MissingOptionsException;
1617
use Symfony\Component\RateLimiter\Policy\FixedWindowLimiter;
1718
use Symfony\Component\RateLimiter\Policy\NoLimiter;
@@ -76,4 +77,37 @@ public static function invalidConfigProvider()
7677
'policy' => 'token_bucket',
7778
]];
7879
}
80+
81+
/**
82+
* @group time-sensitive
83+
*/
84+
public function testExpirationTimeCalculationWhenUsingDefaultTimezoneRomeWithIntervalAfterCETChange()
85+
{
86+
$originalTimezone = date_default_timezone_get();
87+
try {
88+
// Timestamp for 'Sun 27 Oct 2024 12:59:40 AM UTC' that's just 20 seconds before switch CEST->CET
89+
ClockMock::withClockMock(1729990780);
90+
91+
// This is a prerequisite for the bug to happen
92+
date_default_timezone_set('Europe/Rome');
93+
94+
$storage = new InMemoryStorage();
95+
$factory = new RateLimiterFactory(
96+
[
97+
'id' => 'id_1',
98+
'policy' => 'fixed_window',
99+
'limit' => 30,
100+
'interval' => '21 seconds',
101+
],
102+
$storage
103+
);
104+
$rateLimiter = $factory->create('key');
105+
$rateLimiter->consume(1);
106+
$limiterState = $storage->fetch('id_1-key');
107+
// As expected the expiration is equal to the interval we defined
108+
$this->assertSame(21, $limiterState->getExpirationTime());
109+
} finally {
110+
date_default_timezone_set($originalTimezone);
111+
}
112+
}
79113
}

‎src/Symfony/Component/RateLimiter/Util/TimeUtil.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ final class TimeUtil
2020
{
2121
public static function dateIntervalToSeconds(\DateInterval $interval): int
2222
{
23-
$now = new \DateTimeImmutable();
23+
$now = \DateTimeImmutable::createFromFormat('U', time());
2424

2525
return $now->add($interval)->getTimestamp() - $now->getTimestamp();
2626
}

0 commit comments

Comments
 (0)
0