From 471620cb523450ecd471ce0ae12b36f150232300 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:12:44 +0200 Subject: [PATCH 1/3] Replace (string) casts on JSON payload values with is_string guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two `(string)` casts in `OpenIdConfigurationProvider` coerced JSON-decoded mixed values into strings: the JWK `kid` lookup and the OIDC discovery doc value lookup. Both upstream payloads specify the field as a string by spec (RFC 7517 §4.5 for `kid`; OIDC Discovery for the document values), but a malformed-payload case silently turned an int / bool / array into a useless string and produced confusing downstream failures rather than diagnosing the malformed input. Replace each cast with an `is_string` guard that throws an appropriate typed exception (`KeyException` and `CacheException` respectively). Both already implement `OpenIdConnectExceptionInterface`, so consumers catching the marker pick up the new throw paths without code changes. Adds two tests covering the new branches to preserve 100% coverage (151/151 lines). Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 7 ++ src/Security/OpenIdConfigurationProvider.php | 14 ++-- .../OpenIdConfigurationProviderTest.php | 72 +++++++++++++++++++ 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13d2d49..e08378c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 re-wrapped as `CodeException`. Both implement the marker, so a consumer catching that is unaffected; a consumer catching only `CodeException` will need to widen to the marker for this code path. +- `OpenIdConfigurationProvider` now throws `KeyException` when a JWK + entry is missing a string `kid` (RFC 7517 §4.5), and `CacheException` + when an OIDC discovery document returns a non-string value for a + required key. Previously these were silently coerced to strings via + `(string)` casts and produced confusing downstream failures. The new + thrown types both implement the marker interface, so consumers + catching that are unaffected. ### Added diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index 89ddbe1..e3b4d41 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -370,7 +370,10 @@ private function getJwtVerificationKeys(): array $jwks = $this->fetchJsonResource($keysUri); foreach ($jwks['keys'] as $key) { - $kid = (string) $key['kid']; + if (!is_string($key['kid'] ?? null)) { + throw new KeyException('JWK entry missing string "kid" (RFC 7517 §4.5)'); + } + $kid = $key['kid']; if ('RSA' === $key['kty']) { $e = self::base64urlDecode($key['e']); $n = self::base64urlDecode($key['n']); @@ -468,13 +471,14 @@ private function getConfiguration(string $key): string $this->cacheItemPool->save($item); } - if (isset($configuration[$key])) { - $value = (string) $configuration[$key]; - } else { + if (!isset($configuration[$key])) { throw new CacheException('Required config key not defined: '.$key); } + if (!is_string($configuration[$key])) { + throw new CacheException(sprintf('OIDC discovery document value for "%s" is not a string (got %s)', $key, get_debug_type($configuration[$key]))); + } - return $value; + return $configuration[$key]; } catch (InvalidArgumentException $e) { throw new CacheException($e->getMessage(), 0, $e); } diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 7c91d1e..18d8019 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -581,6 +581,32 @@ public function testGetConfigurationMissingKey(): void $method->invoke($this->provider, 'nonexistent_key'); } + public function testGetConfigurationNonStringValue(): void + { + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(true); + $mockCacheItem->method('get')->willReturn(['authorization_endpoint' => 42]); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + $provider = new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => 'https://some.url/openid-configuration', + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $this->createStub(ClientInterface::class), + ]); + + $this->expectException(CacheException::class); + $this->expectExceptionMessage('OIDC discovery document value for "authorization_endpoint" is not a string (got int)'); + + $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); + $method->invoke($provider, 'authorization_endpoint'); + } + public function testFetchJsonResourceNon200(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; @@ -683,6 +709,52 @@ public function testFetchJsonResourceInvalidJson(): void $provider->getBaseAuthorizationUrl(); } + public function testGetJwtVerificationKeysRejectsNonStringKid(): void + { + $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; + $jwks_uri = 'https://azure_b2c_test.b2clogin.com/azure_b2c_test.onmicrosoft.com/discovery/v2.0/keys?p=test-policy'; + + $mockConfigResponse = $this->getMockHttpSuccessResponse('/../MockData/mockOpenIDConfiguration.json'); + + // JWK with an int `kid` — violates RFC 7517 §4.5 (kid must be a string). + $badJwks = json_encode(['keys' => [['kid' => 42, 'kty' => 'RSA', 'e' => 'AQAB', 'n' => 'abc']]]); + $mockKeysStream = $this->createStub(StreamInterface::class); + $mockKeysStream->method('getContents')->willReturn($badJwks); + + $mockKeysResponse = $this->createStub(ResponseInterface::class); + $mockKeysResponse->method('getStatusCode')->willReturn(200); + $mockKeysResponse->method('getBody')->willReturn($mockKeysStream); + + $mockHttpClient = $this->createStub(ClientInterface::class); + $mockHttpClient->method('request')->willReturnMap([ + ['GET', $openIDConnectMetadataUrl, [], $mockConfigResponse], + ['GET', $jwks_uri, [], $mockKeysResponse], + ]); + + $mockCacheItem = $this->createStub(CacheItemInterface::class); + $mockCacheItem->method('isHit')->willReturn(false); + + $mockCacheItemPool = $this->createStub(CacheItemPoolInterface::class); + $mockCacheItemPool->method('getItem')->willReturn($mockCacheItem); + + $provider = new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => $openIDConnectMetadataUrl, + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $mockHttpClient, + ]); + + $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(KeyException::class); + $this->expectExceptionMessage('JWK entry missing string "kid" (RFC 7517 §4.5)'); + + $provider->validateIdToken('token', self::NONCE); + } + public function testGetJwtVerificationKeysUnsupportedKeyType(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; From a320df6aa748f8605c29aa4378aad60dcaf596bd Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:34:15 +0200 Subject: [PATCH 2/3] Throw JsonException for malformed discovery document, not CacheException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getConfiguration` has two distinct failure boundaries: - `catch (InvalidArgumentException $e) → throw new CacheException(...)` correctly maps PSR-6 cache-layer failures to `CacheException`. - The inline `throw` for "required key missing" / "value is not a string" was *also* using `CacheException`, but those validation failures fire regardless of whether `$configuration` came from cache or from a fresh `fetchJsonResource()` call — the IdP- returned (or previously cached) JSON payload doesn't conform to the OIDC Discovery schema. Calling that a "Cache" exception misleads operators looking at logs. Switch both validation throws to `JsonException`, the existing concrete the library already uses for "JSON payload from the IdP is malformed". The new check (added in this PR for the previously- silent-coerced non-string value case) and the pre-existing missing-key check now share the same type, since they're the same failure category. Both `CacheException` and `JsonException` implement `OpenIdConnectExceptionInterface`, so consumers catching the marker are unaffected. Consumers catching `CacheException` specifically for the missing-key case will need to widen — flagged in the CHANGELOG. Addresses the review comment on the original throw: https://github.com/itk-dev/openid-connect/pull/42#discussion_r3225251704 Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 17 +++++++++++------ src/Security/OpenIdConfigurationProvider.php | 4 ++-- .../OpenIdConfigurationProviderTest.php | 6 +++--- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e08378c..bc2b60b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,12 +41,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 consumer catching that is unaffected; a consumer catching only `CodeException` will need to widen to the marker for this code path. - `OpenIdConfigurationProvider` now throws `KeyException` when a JWK - entry is missing a string `kid` (RFC 7517 §4.5), and `CacheException` - when an OIDC discovery document returns a non-string value for a - required key. Previously these were silently coerced to strings via - `(string)` casts and produced confusing downstream failures. The new - thrown types both implement the marker interface, so consumers - catching that are unaffected. + entry is missing a string `kid` (RFC 7517 §4.5), and `JsonException` + when an OIDC discovery document is missing a required key or has a + non-string value at one. Previously the non-string value was + silently coerced via `(string)` cast, and both validation failures + bubbled as `CacheException` — semantically misleading since the + failure is the IdP-returned payload not conforming to the OIDC + Discovery schema, not the cache layer misbehaving. All three new + throw types implement the marker interface, so consumers catching + that are unaffected; consumers catching `CacheException` specifically + for the missing-key case will need to widen to the marker or to + `JsonException`. ### Added diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index e3b4d41..fcf1d5e 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -472,10 +472,10 @@ private function getConfiguration(string $key): string } if (!isset($configuration[$key])) { - throw new CacheException('Required config key not defined: '.$key); + throw new JsonException('OIDC discovery document missing required key: '.$key); } if (!is_string($configuration[$key])) { - throw new CacheException(sprintf('OIDC discovery document value for "%s" is not a string (got %s)', $key, get_debug_type($configuration[$key]))); + throw new JsonException(sprintf('OIDC discovery document value for "%s" is not a string (got %s)', $key, get_debug_type($configuration[$key]))); } return $configuration[$key]; diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 18d8019..c4a153a 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -574,8 +574,8 @@ public function testGetConfigurationCacheHit(): void public function testGetConfigurationMissingKey(): void { - $this->expectException(CacheException::class); - $this->expectExceptionMessage('Required config key not defined: nonexistent_key'); + $this->expectException(\ItkDev\OpenIdConnect\Exception\JsonException::class); + $this->expectExceptionMessage('OIDC discovery document missing required key: nonexistent_key'); $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); $method->invoke($this->provider, 'nonexistent_key'); @@ -600,7 +600,7 @@ public function testGetConfigurationNonStringValue(): void 'httpClient' => $this->createStub(ClientInterface::class), ]); - $this->expectException(CacheException::class); + $this->expectException(\ItkDev\OpenIdConnect\Exception\JsonException::class); $this->expectExceptionMessage('OIDC discovery document value for "authorization_endpoint" is not a string (got int)'); $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); From 39ac2f843f5aaf3a27bba5be237022a95b81a998 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:40:58 +0200 Subject: [PATCH 3/3] Introduce MetadataException for malformed OIDC discovery documents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `JsonException` stretches its semantic when used for "the JSON parsed fine but the document doesn't conform to the OIDC Discovery spec" — the existing usage of `JsonException` is strictly for `json_decode` failures (the bytes aren't valid JSON). Mixing the two failure modes under one type means a consumer catching `JsonException` to retry on transient parse failures would incorrectly retry a malformed-discovery- document case (the IdP will keep returning the same bad payload — no retry will help). New `MetadataException extends \RuntimeException implements OpenIdConnectExceptionInterface` covers this distinct failure category. The two validation throws in `getConfiguration` (missing required key; non-string value at a required key) switch to it. `fetchJsonResource` still throws `JsonException` for actual parse failures, so the categories stay clean. Updates the `@throws` lists on `getBaseAuthorizationUrl`, `getEndSessionUrl`, `validateIdToken`, and `getConfiguration` to advertise the new transitively-thrown type. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 29 ++++++++++++------- README.md | 2 +- src/Exception/MetadataException.php | 17 +++++++++++ src/Security/OpenIdConfigurationProvider.php | 9 ++++-- tests/Exception/ExceptionHierarchyTest.php | 2 ++ .../OpenIdConfigurationProviderTest.php | 4 +-- 6 files changed, 47 insertions(+), 16 deletions(-) create mode 100644 src/Exception/MetadataException.php diff --git a/CHANGELOG.md b/CHANGELOG.md index bc2b60b..1a5b6f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,17 +41,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 consumer catching that is unaffected; a consumer catching only `CodeException` will need to widen to the marker for this code path. - `OpenIdConfigurationProvider` now throws `KeyException` when a JWK - entry is missing a string `kid` (RFC 7517 §4.5), and `JsonException` - when an OIDC discovery document is missing a required key or has a - non-string value at one. Previously the non-string value was - silently coerced via `(string)` cast, and both validation failures - bubbled as `CacheException` — semantically misleading since the - failure is the IdP-returned payload not conforming to the OIDC - Discovery schema, not the cache layer misbehaving. All three new - throw types implement the marker interface, so consumers catching - that are unaffected; consumers catching `CacheException` specifically - for the missing-key case will need to widen to the marker or to - `JsonException`. + entry is missing a string `kid` (RFC 7517 §4.5), and the new + `MetadataException` when an OIDC discovery document is missing a + required key or has a non-string value at one. Previously the + non-string value was silently coerced via `(string)` cast, and both + validation failures bubbled as `CacheException` — semantically + misleading since the failure is the IdP-returned payload not + conforming to the OIDC Discovery schema, not the cache layer + misbehaving. All three throw types implement the marker interface, + so consumers catching that are unaffected; consumers catching + `CacheException` specifically for the missing-key case will need to + widen to the marker or to `MetadataException`. ### Added @@ -59,6 +59,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 marker for catching every OIDC failure from this library. - `ItkDev\OpenIdConnect\Exception\ConfigurationException` for missing or invalid constructor options. +- `ItkDev\OpenIdConnect\Exception\MetadataException` (extends + `\RuntimeException`, implements the marker) for IdP-returned OIDC + discovery documents that parse as JSON but don't conform to the + OIDC Discovery spec (missing required key, wrong type at a required + key). Distinct from `JsonException` (parse failure) and + `CacheException` (PSR-6 cache layer failure) — different remediation + paths (retry doesn't help; the IdP needs to fix its payload). - `tests/Exception/ExceptionHierarchyTest.php` locks the contract: every concrete implements the marker, extends the correct SPL parent, and is caught by a `catch (OpenIdConnectExceptionInterface $e)` diff --git a/README.md b/README.md index bc9eb0f..e3fc1e0 100644 --- a/README.md +++ b/README.md @@ -218,7 +218,7 @@ category, so a `catch` block scoped to that SPL type will also match: | SPL parent | Concrete types | Category | | ---------- | -------------- | -------- | -| `\RuntimeException` | `CacheException`, `HttpException`, `JsonException`, `DecodeException`, `KeyException`, `CodeException`, `ValidationException`, `ClaimsException` | Network, cache, token validation, claims mismatch — transient or data-shape failures | +| `\RuntimeException` | `CacheException`, `HttpException`, `JsonException`, `DecodeException`, `KeyException`, `CodeException`, `ValidationException`, `ClaimsException`, `MetadataException` | Network, cache, token validation, claims mismatch — transient or data-shape failures | | `\LogicException` | `BadUrlException`, `IllegalSchemeException`, `MissingParameterException` | Programmer/config bugs — should be fixed in code | | `\InvalidArgumentException` | `ConfigurationException`, `NegativeCacheDurationException`, `NegativeLeewayException` | Invalid input to the constructor / setters | diff --git a/src/Exception/MetadataException.php b/src/Exception/MetadataException.php new file mode 100644 index 0000000..f695daf --- /dev/null +++ b/src/Exception/MetadataException.php @@ -0,0 +1,17 @@ + [CodeException::class, \RuntimeException::class]; yield 'ValidationException' => [ValidationException::class, \RuntimeException::class]; yield 'ClaimsException' => [ClaimsException::class, \RuntimeException::class]; + yield 'MetadataException' => [MetadataException::class, \RuntimeException::class]; } /** diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index c4a153a..668c306 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -574,7 +574,7 @@ public function testGetConfigurationCacheHit(): void public function testGetConfigurationMissingKey(): void { - $this->expectException(\ItkDev\OpenIdConnect\Exception\JsonException::class); + $this->expectException(\ItkDev\OpenIdConnect\Exception\MetadataException::class); $this->expectExceptionMessage('OIDC discovery document missing required key: nonexistent_key'); $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); @@ -600,7 +600,7 @@ public function testGetConfigurationNonStringValue(): void 'httpClient' => $this->createStub(ClientInterface::class), ]); - $this->expectException(\ItkDev\OpenIdConnect\Exception\JsonException::class); + $this->expectException(\ItkDev\OpenIdConnect\Exception\MetadataException::class); $this->expectExceptionMessage('OIDC discovery document value for "authorization_endpoint" is not a string (got int)'); $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration');