diff --git a/CHANGELOG.md b/CHANGELOG.md index f4aa62c..3c314d0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,18 @@ 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 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 974d038..a23f4ef 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,46 @@ 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. + * + * 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 + * + * @throws \UnexpectedValueException from JWT::decode(), wrapped by the caller + */ + private function decodeWithLeeway(string $idToken, array $keys): \stdClass + { + $previousLeeway = JWT::$leeway; + JWT::$leeway = $this->leeway; + + try { + return JWT::decode($idToken, $keys); + } finally { + JWT::$leeway = $previousLeeway; + } + } + /** * Assert that a claim is present and numeric. * diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 15b9c6d..fd7ce31 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -306,6 +306,54 @@ 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 = 5; + + $observedDuringDecode = null; + $mockClaims = $this->getMockClaims(); + + $mockJWT = $this->overloadJwt(); + $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, $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 { $mockJWT = $this->overloadJwt();