From dc3af44659ed294fdb0efcb37a9461d38737ec17 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 12:46:37 +0200 Subject: [PATCH 1/2] refactor: contain the JWT::$leeway static write in one method firebase/php-jwt exposes leeway only as JWT::$leeway, a process-global static with no per-call alternative. The write already sat immediately before the decode, but only a comment said it had to, and nothing stopped a future edit from putting a cache read or an HTTP call between them. decodeWithLeeway() now does nothing but set the static and decode, so the invariant has a single home and a stated rationale: no suspension point may come between the two, which is why the verification keys are resolved by the caller before it is entered. Under PHP-FPM that ordering is a formality; under cooperative concurrency it is what stops a fibre decoding with a sibling provider's leeway, and under a preemptive model it would not be enough at all. Behaviour is unchanged. The test suite already carried MockJWT::$leeway for this and never asserted it, so there is now a test pinning the configured leeway to what reaches the decode. The extraction also needed the return type spelled as \stdClass rather than object: `object` erased the narrowing JWT::decode() provides and PHPStan lost $claims->aud. --- CHANGELOG.md | 7 ++++ src/Security/OpenIdConfigurationProvider.php | 41 +++++++++++++++---- .../OpenIdConfigurationProviderTest.php | 19 +++++++++ 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4aa62c..c89d200 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 newly escaped mutant fails the build rather than being absorbed by headroom, and pull requests stay free of `--logger-github` annotations unless they genuinely regress the score +- The write to `firebase/php-jwt`'s process-global `JWT::$leeway` is contained in + a single private `decodeWithLeeway()` method, which does nothing but set the + static and decode. Behaviour is unchanged; the point is that the constraint + — nothing that could suspend may come between the write and the decode — now + has one home and a stated rationale instead of being an ordering that happened + to hold. Under PHP-FPM this is a formality, but it is what a fibre-based or + worker runtime would depend on - The discovery document and the JWKS are capped at 1 MiB each, raising `HttpException` when a response exceeds it. Both are a few kilobytes in practice, so a hostile or misconfigured endpoint could previously hand over an diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index 974d038..546d50a 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -234,10 +234,9 @@ public function getEndSessionUrl(?string $postLogoutRedirectUri = null, ?string * constant-time: it is the only claim compared against a value the caller * holds, so a timing signal would leak that secret. * - * Note: JWT::$leeway is a static property, so in environments with multiple - * OpenIdConfigurationProvider instances (e.g. multi-tenant setups in long-running - * processes), the leeway value set by the last provider to call validateIdToken() - * will apply globally until overwritten. + * Note: the leeway is applied through a process-global static in + * firebase/php-jwt, so with several providers configured differently the + * value belongs to whichever called last. See decodeWithLeeway(). * * @param string $idToken Raw id token * @param string $nonce Nonce @@ -257,10 +256,7 @@ public function validateIdToken(string $idToken, string $nonce): object { try { $keys = $this->getJwtVerificationKeys(); - // NB: JWT::$leeway is a static property shared across all instances. - // Always set it immediately before decode to ensure the correct value. - JWT::$leeway = $this->leeway; - $claims = JWT::decode($idToken, $keys); + $claims = $this->decodeWithLeeway($idToken, $keys); // "exp" and "iat" are REQUIRED by OIDC Core §2, but // firebase/php-jwt only validates them when they are present — a @@ -298,6 +294,35 @@ public function validateIdToken(string $idToken, string $nonce): object } } + /** + * Decode an ID token, applying the configured leeway. + * + * INVARIANT: firebase/php-jwt exposes leeway only as `JWT::$leeway`, a + * process-global static with no per-call alternative, so this method is the + * single place that writes it. Nothing may come between the write and + * `JWT::decode()` that could suspend — no HTTP, no cache read, no I/O of any + * kind. That is why the verification keys are resolved by the caller before + * this is entered, and why this method does nothing else. + * + * Under PHP-FPM, where a request owns its process, that ordering is a + * formality. Under cooperative concurrency it is what stops a fiber from + * decoding with a sibling provider's leeway, and under a preemptive model it + * would not be sufficient at all. Revisit before adopting FrankenPHP + * workers or any other long-lived concurrent runtime. + * + * @param array $keys Verification keys, indexed by JWK `kid` + * + * @return \stdClass The token's payload, as JWT::decode() returns it + * + * @throws \UnexpectedValueException from JWT::decode(), wrapped by the caller + */ + private function decodeWithLeeway(string $idToken, array $keys): \stdClass + { + JWT::$leeway = $this->leeway; + + return JWT::decode($idToken, $keys); + } + /** * Assert that a claim is present and numeric. * diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 15b9c6d..1cf8f90 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -306,6 +306,25 @@ public function testValidateIdTokenSuccess(): void $this->assertEquals(self::CLIENT_ID, $claims->aud); } + /** + * The configured leeway must reach firebase/php-jwt. It travels through the + * process-global JWT::$leeway, which is why MockJWT declares that static — + * asserting it here is what pins decodeWithLeeway() to writing the + * provider's value rather than leaving whatever was there before. + */ + public function testValidateIdTokenAppliesConfiguredLeeway(): void + { + MockJWT::$leeway = null; + + $mockJWT = $this->overloadJwt(); + $mockJWT->shouldReceive('decode')->andReturn($this->getMockClaims()); + + $this->provider->validateIdToken('token', self::NONCE); + + // 30 is the leeway the provider is constructed with in setUp(). + $this->assertSame(30, MockJWT::$leeway); + } + public function testValidateIdTokenFailure(): void { $mockJWT = $this->overloadJwt(); From df212a425d2fbf2bcbbda3c868cde1cf47e40210 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Wed, 26 Aug 2026 12:54:28 +0200 Subject: [PATCH 2/2] fix: restore the previous JWT::$leeway after each decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setting the process-global is unavoidable — firebase/php-jwt offers no per-call leeway — but leaving it set is a choice. Under PHP-FPM the mutated static dies with the request. In a worker process it persists for the life of the process and silently applies to every other firebase/php-jwt consumer in it that never sets its own leeway, which makes this library a bad neighbour in exactly the runtime it otherwise suits. The restore is in a finally block, so a rejected token cannot leave our leeway applied either. Both paths are tested: the value in effect during the decode, and the value left behind after a success and after a SignatureInvalidException. --- CHANGELOG.md | 5 +++ src/Security/OpenIdConfigurationProvider.php | 13 ++++++- .../OpenIdConfigurationProviderTest.php | 35 +++++++++++++++++-- 3 files changed, 49 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c89d200..3c314d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 has one home and a stated rationale instead of being an ordering that happened to hold. Under PHP-FPM this is a formality, but it is what a fibre-based or worker runtime would depend on +- The previous `JWT::$leeway` value is restored after each decode. Under PHP-FPM + the mutated static died with the request; in a worker process it persisted for + the life of the process and silently applied to any other `firebase/php-jwt` + consumer that never set its own leeway. Writing a process-global is + unavoidable given the upstream API, but leaving it written is not - The discovery document and the JWKS are capped at 1 MiB each, raising `HttpException` when a response exceeds it. Both are a few kilobytes in practice, so a hostile or misconfigured endpoint could previously hand over an diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index 546d50a..a23f4ef 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -310,6 +310,12 @@ public function validateIdToken(string $idToken, string $nonce): object * would not be sufficient at all. Revisit before adopting FrankenPHP * workers or any other long-lived concurrent runtime. * + * The previous value is restored afterwards. Under FPM the mutated static + * dies with the request either way, but in a worker it would persist for the + * life of the process and silently apply to any other firebase/php-jwt + * consumer in it that never sets its own leeway. Writing a process-global is + * unavoidable here; leaving it written is not. + * * @param array $keys Verification keys, indexed by JWK `kid` * * @return \stdClass The token's payload, as JWT::decode() returns it @@ -318,9 +324,14 @@ public function validateIdToken(string $idToken, string $nonce): object */ private function decodeWithLeeway(string $idToken, array $keys): \stdClass { + $previousLeeway = JWT::$leeway; JWT::$leeway = $this->leeway; - return JWT::decode($idToken, $keys); + try { + return JWT::decode($idToken, $keys); + } finally { + JWT::$leeway = $previousLeeway; + } } /** diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 1cf8f90..fd7ce31 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -314,15 +314,44 @@ public function testValidateIdTokenSuccess(): void */ public function testValidateIdTokenAppliesConfiguredLeeway(): void { - MockJWT::$leeway = null; + MockJWT::$leeway = 5; + + $observedDuringDecode = null; + $mockClaims = $this->getMockClaims(); $mockJWT = $this->overloadJwt(); - $mockJWT->shouldReceive('decode')->andReturn($this->getMockClaims()); + $mockJWT->shouldReceive('decode')->andReturnUsing( + function () use (&$observedDuringDecode, $mockClaims): \stdClass { + $observedDuringDecode = MockJWT::$leeway; + + return $mockClaims; + } + ); $this->provider->validateIdToken('token', self::NONCE); // 30 is the leeway the provider is constructed with in setUp(). - $this->assertSame(30, MockJWT::$leeway); + $this->assertSame(30, $observedDuringDecode, 'Configured leeway must be in effect for the decode'); + $this->assertSame(5, MockJWT::$leeway, 'Any pre-existing leeway must be restored afterwards'); + } + + /** + * The static is restored even when the decode throws, so a rejected token + * cannot leave this library's leeway applied to the rest of the process. + */ + public function testValidateIdTokenRestoresLeewayWhenDecodeFails(): void + { + MockJWT::$leeway = 5; + + $mockJWT = $this->overloadJwt(); + $mockJWT->shouldReceive('decode')->andThrow(SignatureInvalidException::class, 'Signature verification failed'); + + try { + $this->provider->validateIdToken('token', self::NONCE); + $this->fail('Expected ValidationException was not thrown'); + } catch (ValidationException) { + $this->assertSame(5, MockJWT::$leeway, 'Leeway must be restored on the failure path too'); + } } public function testValidateIdTokenFailure(): void