diff --git a/CHANGELOG.md b/CHANGELOG.md index 13d2d49..1a5b6f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,18 @@ 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 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 @@ -47,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 @@ +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']); @@ -451,6 +458,7 @@ private function fetchJsonResource(string $resourceUrl): array * @throws CacheException * @throws HttpException * @throws JsonException + * @throws MetadataException */ private function getConfiguration(string $key): string { @@ -468,13 +476,14 @@ private function getConfiguration(string $key): string $this->cacheItemPool->save($item); } - if (isset($configuration[$key])) { - $value = (string) $configuration[$key]; - } else { - throw new CacheException('Required config key not defined: '.$key); + if (!isset($configuration[$key])) { + throw new MetadataException('OIDC discovery document missing required key: '.$key); + } + if (!is_string($configuration[$key])) { + throw new MetadataException(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/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 658cd65..aa26923 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -12,6 +12,7 @@ use ItkDev\OpenIdConnect\Exception\IllegalSchemeException; use ItkDev\OpenIdConnect\Exception\JsonException; use ItkDev\OpenIdConnect\Exception\KeyException; +use ItkDev\OpenIdConnect\Exception\MetadataException; use ItkDev\OpenIdConnect\Exception\MissingParameterException; use ItkDev\OpenIdConnect\Exception\NegativeCacheDurationException; use ItkDev\OpenIdConnect\Exception\NegativeLeewayException; @@ -62,6 +63,7 @@ public static function concreteProvider(): iterable yield 'CodeException' => [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 7c91d1e..668c306 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -574,13 +574,39 @@ 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\MetadataException::class); + $this->expectExceptionMessage('OIDC discovery document missing required key: nonexistent_key'); $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'getConfiguration'); $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(\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'); + $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';