bug #58757 [RateLimiter] Fix DateInterval normalization (danydev)

nicolas-grekas · nicolas-grekas · commit d2ba257846b3 · 2024-11-05T14:17:03.000+01:00
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 intuitive, 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
diff --git a/src/Symfony/Component/RateLimiter/RateLimiterFactory.php b/src/Symfony/Component/RateLimiter/RateLimiterFactory.php
@@ -69,7 +69,11 @@ protected static function configureOptions(OptionsResolver $options): void
     {
         $intervalNormalizer = static function (Options $options, string $interval): \DateInterval {
             try {
-                return (new \DateTimeImmutable())->diff(new \DateTimeImmutable('+'.$interval));
+                // Create DateTimeImmutable from unix timesatmp, so the default timezone is ignored and we don't need to
+                // deal with quirks happening when modifying dates using a timezone with DST.
+                $now = \DateTimeImmutable::createFromFormat('U', time());
+
+                return $now->diff($now->modify('+'.$interval));
             } catch (\Exception $e) {
                 if (!preg_match('/Failed to parse time string \(\+([^)]+)\)/', $e->getMessage(), $m)) {
                     throw $e;
diff --git a/src/Symfony/Component/RateLimiter/Tests/RateLimiterFactoryTest.php b/src/Symfony/Component/RateLimiter/Tests/RateLimiterFactoryTest.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\RateLimiter\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Bridge\PhpUnit\ClockMock;
 use Symfony\Component\OptionsResolver\Exception\MissingOptionsException;
 use Symfony\Component\RateLimiter\Policy\FixedWindowLimiter;
 use Symfony\Component\RateLimiter\Policy\NoLimiter;
@@ -76,4 +77,37 @@ public static function invalidConfigProvider()
             'policy' => 'token_bucket',
         ]];
     }
+
+    /**
+     * @group time-sensitive
+     */
+    public function testExpirationTimeCalculationWhenUsingDefaultTimezoneRomeWithIntervalAfterCETChange()
+    {
+        $originalTimezone = date_default_timezone_get();
+        try {
+            // Timestamp for 'Sun 27 Oct 2024 12:59:40 AM UTC' that's just 20 seconds before switch CEST->CET
+            ClockMock::withClockMock(1729990780);
+
+            // This is a prerequisite for the bug to happen
+            date_default_timezone_set('Europe/Rome');
+
+            $storage = new InMemoryStorage();
+            $factory = new RateLimiterFactory(
+                [
+                    'id' => 'id_1',
+                    'policy' => 'fixed_window',
+                    'limit' => 30,
+                    'interval' => '21 seconds',
+                ],
+                $storage
+            );
+            $rateLimiter = $factory->create('key');
+            $rateLimiter->consume(1);
+            $limiterState = $storage->fetch('id_1-key');
+            // As expected the expiration is equal to the interval we defined
+            $this->assertSame(21, $limiterState->getExpirationTime());
+        } finally {
+            date_default_timezone_set($originalTimezone);
+        }
+    }
 }
diff --git a/src/Symfony/Component/RateLimiter/Util/TimeUtil.php b/src/Symfony/Component/RateLimiter/Util/TimeUtil.php
@@ -20,7 +20,7 @@ final class TimeUtil
 {
     public static function dateIntervalToSeconds(\DateInterval $interval): int
     {
-        $now = new \DateTimeImmutable();
+        $now = \DateTimeImmutable::createFromFormat('U', time());
 
         return $now->add($interval)->getTimestamp() - $now->getTimestamp();
     }

Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ final class TimeUtil`
`20`	`20`	`{`
`21`	`21`	`public static function dateIntervalToSeconds(\DateInterval $interval): int`
`22`	`22`	`{`
`23`		`- $now = new \DateTimeImmutable();`
	`23`	`+ $now = \DateTimeImmutable::createFromFormat('U', time());`
`24`	`24`
`25`	`25`	`return $now->add($interval)->getTimestamp() - $now->getTimestamp();`
`26`	`26`	`}`