From 38391f9711c296c1fb60fd049d573a42e1f307fb Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:56:58 +0200 Subject: [PATCH 1/2] Narrow JSON payload accesses in getJwtVerificationKeys and getIdToken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two methods read fields out of JSON payloads (JWKS, token endpoint response) where PHPStan could only see `mixed`: - `getJwtVerificationKeys` walks `$jwks['keys']` and reads `kid`, `kty`, `e`, `n` on each entry without validating the structure. Spec-compliant payloads from real IdPs work, but a malformed payload silently produces garbage (a `Key` constructed from empty/wrong inputs) or trips a downstream type error in `XMLSecurityKey::convertRSA`. - `getIdToken` reads `$payload['id_token']` without checking that `$payload` is an array or that the field is a string, returning whatever it finds. Each access is now gated by an explicit `is_array` / `is_string` guard at the actual error boundary, throwing the appropriate typed exception with a precise message: - JWKS missing array `keys` → `KeyException`. - JWK entry not a JSON object → `KeyException`. - JWK entry missing string `kty` → `KeyException`. - RSA JWK missing string `e` / `n` → `KeyException`. - Token endpoint response missing string `id_token` → `CodeException`. Adds a `createProviderWithCustomJwks(string $jwksJson)` helper to the test and uses it for the four new JWKS test cases (the existing `testGetJwtVerificationKeysRejectsNonStringKid` / `testGetJwtVerificationKeysUnsupportedKeyType` cases keep their existing inline setup — out of scope to refactor). One additional test covers the new getIdToken throw. 100% coverage maintained (161/161 lines). Removes 7 errors when PHPStan is run at `level: max`. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 16 ++ src/Security/OpenIdConfigurationProvider.php | 17 +++ .../OpenIdConfigurationProviderTest.php | 141 ++++++++++++++++++ 3 files changed, 174 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a5b6f3..b91557d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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`. +- `OpenIdConfigurationProvider::getJwtVerificationKeys` now validates + the JWKS payload at each level before reading values: the top-level + `keys` property must be an array (`KeyException` otherwise), each + entry must be a JSON object (`KeyException` otherwise), each entry's + `kty` must be a string (`KeyException` otherwise), and for RSA keys + the `e` and `n` modulus/exponent values must both be strings + (`KeyException` otherwise). Previously these dynamic fields were + accessed without checking and would either silently produce a + garbage `Key`, trigger a PHP type error in the base64 decode, or + fail downstream in `XMLSecurityKey::convertRSA`. The new behaviour + fails at the malformed-payload boundary with a precise message. +- `OpenIdConfigurationProvider::getIdToken` now throws `CodeException` + when the token endpoint's JSON response is missing a string + `id_token`. Previously this would have returned `mixed` from + `$payload['id_token']` and produced confusing errors at the call + site. ### Added diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index ed697e6..3a536ce 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -269,6 +269,10 @@ public function getIdToken(string $code): string $payload = json_decode((string) $response->getBody(), true, 512, JSON_THROW_ON_ERROR); + if (!is_array($payload) || !is_string($payload['id_token'] ?? null)) { + throw new CodeException('Token endpoint response missing string "id_token"'); + } + return $payload['id_token']; } catch (IdentityProviderException|ClientExceptionInterface|\JsonException $e) { // Narrow boundary: IdentityProviderException from league's checkResponse, @@ -373,12 +377,25 @@ private function getJwtVerificationKeys(): array $keysUri = $this->getConfiguration('jwks_uri'); $jwks = $this->fetchJsonResource($keysUri); + if (!isset($jwks['keys']) || !is_array($jwks['keys'])) { + throw new KeyException('JWKS payload missing array "keys" property (RFC 7517 §5)'); + } + foreach ($jwks['keys'] as $key) { + if (!is_array($key)) { + throw new KeyException('JWK entry is not a JSON object'); + } if (!is_string($key['kid'] ?? null)) { throw new KeyException('JWK entry missing string "kid" (RFC 7517 §4.5)'); } $kid = $key['kid']; + if (!is_string($key['kty'] ?? null)) { + throw new KeyException('JWK entry missing string "kty" for key id: '.$kid); + } if ('RSA' === $key['kty']) { + if (!is_string($key['e'] ?? null) || !is_string($key['n'] ?? null)) { + throw new KeyException('JWK RSA entry missing string "e"/"n" for key id: '.$kid); + } $e = self::base64urlDecode($key['e']); $n = self::base64urlDecode($key['n']); $publicKey = XMLSecurityKey::convertRSA($n, $e); diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 668c306..e79070e 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -545,6 +545,58 @@ public function testGetIdTokenFailure(): void $provider->getIdToken('test-code'); } + public function testGetIdTokenRejectsResponseWithoutStringIdToken(): void + { + $tokenEndpoint = 'https://azure_b2c_test.b2clogin.com/azure_b2c_test.onmicrosoft.com/oauth2/v2.0/token?p=test-policy'; + $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; + + $mockConfigResponse = $this->getMockHttpSuccessResponse('/../MockData/mockOpenIDConfiguration.json'); + + // Spec-compliant token endpoint returns JSON with `id_token`. + // Here the IdP returns a JSON object that's missing it entirely. + $malformedTokenResponseBody = (string) json_encode(['access_token' => 'not-an-id-token']); + $mockTokenStream = $this->createStub(StreamInterface::class); + $mockTokenStream->method('getContents')->willReturn($malformedTokenResponseBody); + $mockTokenStream->method('__toString')->willReturn($malformedTokenResponseBody); + + $mockTokenResponse = $this->createStub(ResponseInterface::class); + $mockTokenResponse->method('getStatusCode')->willReturn(200); + $mockTokenResponse->method('getBody')->willReturn($mockTokenStream); + + $mockHttpClient = $this->createStub(ClientInterface::class); + $mockHttpClient->method('request')->willReturnMap([ + ['GET', $openIDConnectMetadataUrl, [], $mockConfigResponse], + ['POST', $tokenEndpoint, ['form_params' => [ + 'client_id' => self::CLIENT_ID, + 'client_secret' => self::CLIENT_SECRET, + 'redirect_uri' => self::REDIRECT_URI, + 'grant_type' => 'authorization_code', + 'code' => 'test-code', + ]], $mockTokenResponse], + ]); + + $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, + ]); + + $this->expectException(CodeException::class); + $this->expectExceptionMessage('Token endpoint response missing string "id_token"'); + + $provider->getIdToken('test-code'); + } + public function testGetConfigurationCacheHit(): void { $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); @@ -709,6 +761,54 @@ public function testFetchJsonResourceInvalidJson(): void $provider->getBaseAuthorizationUrl(); } + public function testGetJwtVerificationKeysRejectsJwksMissingKeysArray(): void + { + $provider = $this->createProviderWithCustomJwks((string) json_encode(['something_else' => 1])); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(KeyException::class); + $this->expectExceptionMessage('JWKS payload missing array "keys" property (RFC 7517 §5)'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsNonObjectJwkEntry(): void + { + $provider = $this->createProviderWithCustomJwks((string) json_encode(['keys' => [42]])); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(KeyException::class); + $this->expectExceptionMessage('JWK entry is not a JSON object'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsNonStringKty(): void + { + $provider = $this->createProviderWithCustomJwks( + (string) json_encode(['keys' => [['kid' => 'key-1', 'kty' => 42]]]), + ); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(KeyException::class); + $this->expectExceptionMessage('JWK entry missing string "kty" for key id: key-1'); + + $provider->validateIdToken('token', self::NONCE); + } + + public function testGetJwtVerificationKeysRejectsRsaWithoutStringExpOrModulus(): void + { + $provider = $this->createProviderWithCustomJwks( + (string) json_encode(['keys' => [['kid' => 'key-1', 'kty' => 'RSA', 'e' => 42, 'n' => 'abc']]]), + ); + \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); + + $this->expectException(KeyException::class); + $this->expectExceptionMessage('JWK RSA entry missing string "e"/"n" for key id: key-1'); + + $provider->validateIdToken('token', self::NONCE); + } + public function testGetJwtVerificationKeysRejectsNonStringKid(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; @@ -983,6 +1083,47 @@ private function loadMockFixture(string $filename): array return $decoded; } + /** + * Build a provider whose JWKS endpoint returns the given raw JSON body. + * Used by the JWKS validation tests to feed deliberately-malformed + * payloads through `getJwtVerificationKeys`. + */ + private function createProviderWithCustomJwks(string $jwksJson): OpenIdConfigurationProvider + { + $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'); + + $mockKeysStream = $this->createStub(StreamInterface::class); + $mockKeysStream->method('getContents')->willReturn($jwksJson); + $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); + + return new OpenIdConfigurationProvider([ + 'openIDConnectMetadataUrl' => $openIDConnectMetadataUrl, + 'cacheItemPool' => $mockCacheItemPool, + 'clientId' => self::CLIENT_ID, + 'clientSecret' => self::CLIENT_SECRET, + 'redirectUri' => self::REDIRECT_URI, + ], [ + 'httpClient' => $mockHttpClient, + ]); + } + private function getMockHttpSuccessResponse(string $mockResponseDataPath): ResponseInterface { $mockResponseData = file_get_contents(__DIR__.$mockResponseDataPath); From 6dae797d519a6cbc74c560e7a56033fbf17ed200 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 12:07:17 +0200 Subject: [PATCH 2/2] Document, rename, and tighten the exception system MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related changes to the exception layer: 1. **Document every concrete.** Each of the 15 concretes in `src/Exception/` now has a class-level PHPDoc describing what it represents, when it's thrown, the rationale for its SPL parent (`\RuntimeException` / `\LogicException` / `\InvalidArgumentException`), and the boundary against related concrete types — most importantly: `DecodeException` vs `JwksException` (bytes-level vs shape-level failure inside the JWK); `JsonException` vs `MetadataException` (parse failure vs parsed-but-non-conformant document); `JwksException` vs `MetadataException` (JWKS vs OIDC-discovery document, both IdP-side spec violations at different layers); `CodeException` vs `ValidationException` vs `ClaimsException` (different stages of the auth flow: token-exchange vs signature vs claim-value). 2. **Rename `KeyException` → `JwksException`** for symmetry with `MetadataException` and clearer scope: the type fires for both document-level errors (`keys` array missing) and entry-level errors (missing `kid` / `kty` / `e` / `n`). Naming after the document type (JWKS) rather than the individual key is more accurate; matches Symfony's `JwkSet` PascalCase convention. 3. **Narrow JSON payload accesses** in `getJwtVerificationKeys` and `getIdToken`. Each previously-dynamic field read now has an explicit `is_array` / `is_string` guard at the actual error boundary, throwing the appropriate typed exception with a precise message: `JwksException` for malformed JWKS structure; `CodeException` for a token endpoint response missing string `id_token`. Adds a `createProviderWithCustomJwks(string)` test helper used by four new JWKS validation tests plus one inline getIdToken test. 100% coverage maintained (161/161 lines, up from 151). Audit confirms each of the 15 concretes covers a distinct failure category — no two would be handled identically by a reasonable consumer (per ADR 001's granularity rule). The overall count is higher than the ADR's "three to five per package" target; consolidation candidates are flagged via the cross-references but not collapsed (any rename or removal is a deferred MAJOR break). Static analysis: PHPStan at `level: max` drops 7 errors (was 26 on this branch tip, now 19). Remaining 19 covered by PR C. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 41 ++++++++++++++++--- README.md | 2 +- src/Exception/BadUrlException.php | 10 +++++ src/Exception/CacheException.php | 13 ++++++ src/Exception/ClaimsException.php | 13 ++++++ src/Exception/CodeException.php | 16 ++++++++ src/Exception/ConfigurationException.php | 10 +++-- src/Exception/DecodeException.php | 11 +++++ src/Exception/HttpException.php | 14 +++++++ src/Exception/IllegalSchemeException.php | 9 ++++ src/Exception/JsonException.php | 10 +++++ src/Exception/JwksException.php | 20 +++++++++ src/Exception/KeyException.php | 7 ---- src/Exception/MissingParameterException.php | 7 ++++ .../NegativeCacheDurationException.php | 9 ++++ src/Exception/NegativeLeewayException.php | 9 ++++ src/Exception/ValidationException.php | 13 ++++++ src/Security/OpenIdConfigurationProvider.php | 16 ++++---- tests/Exception/ExceptionHierarchyTest.php | 4 +- .../OpenIdConfigurationProviderTest.php | 14 +++---- 20 files changed, 214 insertions(+), 34 deletions(-) create mode 100644 src/Exception/JwksException.php delete mode 100644 src/Exception/KeyException.php diff --git a/CHANGELOG.md b/CHANGELOG.md index b91557d..e405cdb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 marker interface, extends `\Throwable`). Concrete exception classes now extend the SPL type that best describes the failure category: `\RuntimeException` (network/cache/data — `CacheException`, - `HttpException`, `JsonException`, `DecodeException`, `KeyException`, + `HttpException`, `JsonException`, `DecodeException`, `JwksException`, `CodeException`, `ValidationException`, `ClaimsException`), `\LogicException` (programmer/config bug — `BadUrlException`, `IllegalSchemeException`, `MissingParameterException`), @@ -40,7 +40,7 @@ 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 +- `OpenIdConfigurationProvider` now throws `JwksException` 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 @@ -54,11 +54,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 widen to the marker or to `MetadataException`. - `OpenIdConfigurationProvider::getJwtVerificationKeys` now validates the JWKS payload at each level before reading values: the top-level - `keys` property must be an array (`KeyException` otherwise), each - entry must be a JSON object (`KeyException` otherwise), each entry's - `kty` must be a string (`KeyException` otherwise), and for RSA keys + `keys` property must be an array (`JwksException` otherwise), each + entry must be a JSON object (`JwksException` otherwise), each entry's + `kty` must be a string (`JwksException` otherwise), and for RSA keys the `e` and `n` modulus/exponent values must both be strings - (`KeyException` otherwise). Previously these dynamic fields were + (`JwksException` otherwise). Previously these dynamic fields were accessed without checking and would either silently produce a garbage `Key`, trigger a PHP type error in the base64 decode, or fail downstream in `XMLSecurityKey::convertRSA`. The new behaviour @@ -68,6 +68,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `id_token`. Previously this would have returned `mixed` from `$payload['id_token']` and produced confusing errors at the call site. +- Renamed `KeyException` → `JwksException` for symmetry with + `MetadataException` and clearer scope: the type fires on both + JWKS-document-level errors (`keys` array missing) and JWK-entry- + level errors (missing `kid` / `kty` / `e` / `n`), so naming it + after the document type rather than the individual key is more + accurate. Consumers catching the marker are unaffected; consumers + catching the concrete class need to swap the name. + +### Documentation + +- Added class-level PHPDoc to every concrete exception in + `src/Exception/` describing what it represents, when it's thrown, + the rationale for its SPL parent type, and the boundary against + related concrete types. The audit confirms each of the 15 concretes + covers a distinct failure category — none would be handled + identically by a reasonable consumer: + - `\LogicException` family — `BadUrlException` (URL syntax), + `IllegalSchemeException` (http without opt-in), + `MissingParameterException` (caller omitted state/nonce). + - `\InvalidArgumentException` family — `ConfigurationException` + (missing required ctor option), `NegativeCacheDurationException` + (value out of range), `NegativeLeewayException` (value out of range). + - `\RuntimeException` family — `CacheException` (PSR-6 layer), + `HttpException` (transport + PSR-18 `ClientExceptionInterface`), + `JsonException` (parse failure), `DecodeException` (JWK base64 + bytes), `JwksException` (JWKS structure), `CodeException` (token + exchange), `ValidationException` (JWT signature/decode), + `ClaimsException` (claim values), `MetadataException` (discovery + doc structure). ### Added diff --git a/README.md b/README.md index e3fc1e0..e7f5f06 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`, `MetadataException` | Network, cache, token validation, claims mismatch — transient or data-shape failures | +| `\RuntimeException` | `CacheException`, `HttpException`, `JsonException`, `DecodeException`, `JwksException`, `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/BadUrlException.php b/src/Exception/BadUrlException.php index ee00e77..2ec409b 100644 --- a/src/Exception/BadUrlException.php +++ b/src/Exception/BadUrlException.php @@ -2,6 +2,16 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Thrown when `openIDConnectMetadataUrl` fails URL syntax validation + * (`parse_url` rejects it because no scheme can be parsed). A programmer + * error — the value is hard-coded or comes from misread configuration, so + * fixing it requires editing code or env config, not retrying at runtime. + * Hence `\LogicException`. + * + * Distinct from {@see IllegalSchemeException} (URL parses successfully but + * uses an `http://` scheme without `allowHttp: true`). + */ class BadUrlException extends \LogicException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CacheException.php b/src/Exception/CacheException.php index 89e6b45..f4d2cbd 100644 --- a/src/Exception/CacheException.php +++ b/src/Exception/CacheException.php @@ -2,6 +2,19 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Wraps PSR-6 cache layer failures. Specifically thrown when the injected + * `Psr\Cache\CacheItemPoolInterface` raises `Psr\Cache\InvalidArgumentException` + * from `getItem` / `save` / `deleteItem` — typically because the cache key + * contains a character the backend rejects, or the backend itself is + * unhealthy. The original exception is chained via `$previous`. Hence + * `\RuntimeException` (transient — a different cache backend or a sanitized + * key may resolve it). + * + * Strictly cache-layer failures only. Discovery-document validation problems + * are {@see MetadataException}; JWKS validation problems are + * {@see JwksException}; JSON parse failures are {@see JsonException}. + */ class CacheException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ClaimsException.php b/src/Exception/ClaimsException.php index 86ec72f..612259d 100644 --- a/src/Exception/ClaimsException.php +++ b/src/Exception/ClaimsException.php @@ -2,6 +2,19 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Thrown from `validateIdToken()` when the decoded ID token's claims + * don't match expectations — wrong `aud` (audience does not contain + * our client id), wrong `iss` (issuer doesn't match the discovery + * document), or wrong `nonce` (didn't match the value we sent on the + * authorization request). Hence `\RuntimeException` (typically requires + * either re-authenticating, or auditing why the IdP issued a token + * meant for someone else — security-relevant if persistent). + * + * Distinct from {@see ValidationException} (token cryptographically + * invalid — bad signature, expired) and from {@see CodeException} + * (failure obtaining the token in the first place, before decoding). + */ class ClaimsException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CodeException.php b/src/Exception/CodeException.php index 82d7186..d95693c 100644 --- a/src/Exception/CodeException.php +++ b/src/Exception/CodeException.php @@ -2,6 +2,22 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Thrown by `getIdToken()` when the OAuth authorization-code exchange + * fails: the token endpoint returned a transport error + * (`Psr\Http\Client\ClientExceptionInterface`), an OAuth error response + * (`League\OAuth2\Client\Provider\Exception\IdentityProviderException`), + * non-JSON body (`\JsonException`), or a JSON body missing a string + * `id_token`. The originating exception is chained via `$previous`. + * Hence `\RuntimeException` (often transient — typical causes are an + * expired or already-used authorization code, or a brief IdP outage). + * + * Distinct from {@see HttpException} (general HTTP transport failures + * for the discovery / JWKS endpoints, not the token-exchange POST). And + * distinct from {@see ValidationException} / {@see ClaimsException}, + * which fire later in the flow on a successfully-received but invalid + * token. + */ class CodeException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ConfigurationException.php b/src/Exception/ConfigurationException.php index 45279cd..50009ab 100644 --- a/src/Exception/ConfigurationException.php +++ b/src/Exception/ConfigurationException.php @@ -3,10 +3,14 @@ namespace ItkDev\OpenIdConnect\Exception; /** - * Thrown when the bundle is misconfigured (missing required constructor option, invalid value, etc). + * Thrown from the `OpenIdConfigurationProvider` constructor when a required + * option (`cacheItemPool`, `openIDConnectMetadataUrl`) is missing from the + * `$options` array. Invalid input to a public constructor — fixable in + * calling code only. Hence `\InvalidArgumentException`. * - * Extends `\InvalidArgumentException` because the failure is invalid input to a public constructor; - * fixable in calling code, not at runtime. + * Distinct from {@see NegativeCacheDurationException} and + * {@see NegativeLeewayException}, which fire when a numeric option is + * present but out of range. */ class ConfigurationException extends \InvalidArgumentException implements OpenIdConnectExceptionInterface { diff --git a/src/Exception/DecodeException.php b/src/Exception/DecodeException.php index ee95e12..24bdbe1 100644 --- a/src/Exception/DecodeException.php +++ b/src/Exception/DecodeException.php @@ -2,6 +2,17 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Thrown by the internal base64url decoder when a JWK's `e` (exponent) or + * `n` (modulus) string contains bytes that fail strict base64 decoding. + * The JWK is structurally OK (per RFC 7517) but its contents are + * unparseable. Hence `\RuntimeException`. + * + * Distinct from {@see JwksException} (the JWK structure itself is wrong + * — missing `kid` / `kty`, non-array key entry, etc.). Both can fire + * while loading the JWKS, but at different levels: JwksException at the + * shape level, DecodeException at the bytes level. + */ class DecodeException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/HttpException.php b/src/Exception/HttpException.php index d40ff45..cbefde4 100644 --- a/src/Exception/HttpException.php +++ b/src/Exception/HttpException.php @@ -4,6 +4,20 @@ use Psr\Http\Client\ClientExceptionInterface; +/** + * Wraps HTTP transport failures while fetching the OIDC discovery document + * or the JWKS — non-200 responses, network errors, or Guzzle-thrown + * `Psr\Http\Client\ClientExceptionInterface` from the underlying HTTP + * client (chained via `$previous`). Hence `\RuntimeException` (transient — + * the IdP being briefly unreachable typically resolves on retry). + * + * Also implements PSR-18's {@see ClientExceptionInterface} as part of the + * public contract: PSR-18-aware consumers can catch HTTP failures from + * this library via the standard PSR marker. + * + * Distinct from {@see CodeException} (failure during the OAuth code + * exchange POST to the token endpoint). + */ class HttpException extends \RuntimeException implements OpenIdConnectExceptionInterface, ClientExceptionInterface { } diff --git a/src/Exception/IllegalSchemeException.php b/src/Exception/IllegalSchemeException.php index 790e55b..e3311e3 100644 --- a/src/Exception/IllegalSchemeException.php +++ b/src/Exception/IllegalSchemeException.php @@ -2,6 +2,15 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Thrown when `openIDConnectMetadataUrl` uses the `http://` scheme without + * the explicit `allowHttp: true` opt-in. OIDC requires TLS; plain HTTP is + * only acceptable for local IdP mocks during development. A programmer + * error — both the URL and the opt-in are configuration, fixed in code or + * env. Hence `\LogicException`. + * + * Distinct from {@see BadUrlException} (URL syntax is unparseable). + */ class IllegalSchemeException extends \LogicException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/JsonException.php b/src/Exception/JsonException.php index f10e527..8ce776e 100644 --- a/src/Exception/JsonException.php +++ b/src/Exception/JsonException.php @@ -2,6 +2,16 @@ namespace ItkDev\OpenIdConnect\Exception; +/** + * Thrown when `json_decode` fails on an IdP response body — the bytes + * didn't parse as JSON at all (raw `\JsonException` from PHP's JSON + * extension, chained via `$previous`). Hence `\RuntimeException`. + * + * Distinct from {@see MetadataException} (JSON parses fine but doesn't + * conform to the OIDC Discovery spec). The remediation differs: a parse + * failure may be transient (corrupted bytes, retry might help), while a + * malformed discovery document is a persistent IdP-configuration issue. + */ class JsonException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/JwksException.php b/src/Exception/JwksException.php new file mode 100644 index 0000000..51d35b6 --- /dev/null +++ b/src/Exception/JwksException.php @@ -0,0 +1,20 @@ +fetchJsonResource($keysUri); if (!isset($jwks['keys']) || !is_array($jwks['keys'])) { - throw new KeyException('JWKS payload missing array "keys" property (RFC 7517 §5)'); + throw new JwksException('JWKS payload missing array "keys" property (RFC 7517 §5)'); } foreach ($jwks['keys'] as $key) { if (!is_array($key)) { - throw new KeyException('JWK entry is not a JSON object'); + throw new JwksException('JWK entry is not a JSON object'); } if (!is_string($key['kid'] ?? null)) { - throw new KeyException('JWK entry missing string "kid" (RFC 7517 §4.5)'); + throw new JwksException('JWK entry missing string "kid" (RFC 7517 §4.5)'); } $kid = $key['kid']; if (!is_string($key['kty'] ?? null)) { - throw new KeyException('JWK entry missing string "kty" for key id: '.$kid); + throw new JwksException('JWK entry missing string "kty" for key id: '.$kid); } if ('RSA' === $key['kty']) { if (!is_string($key['e'] ?? null) || !is_string($key['n'] ?? null)) { - throw new KeyException('JWK RSA entry missing string "e"/"n" for key id: '.$kid); + throw new JwksException('JWK RSA entry missing string "e"/"n" for key id: '.$kid); } $e = self::base64urlDecode($key['e']); $n = self::base64urlDecode($key['n']); $publicKey = XMLSecurityKey::convertRSA($n, $e); $keys[$kid] = new Key($publicKey, 'RS256'); } else { - throw new KeyException('Unsupported key data for key id: '.$kid); + throw new JwksException('Unsupported key data for key id: '.$kid); } } diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index aa26923..501858b 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -11,7 +11,7 @@ use ItkDev\OpenIdConnect\Exception\HttpException; use ItkDev\OpenIdConnect\Exception\IllegalSchemeException; use ItkDev\OpenIdConnect\Exception\JsonException; -use ItkDev\OpenIdConnect\Exception\KeyException; +use ItkDev\OpenIdConnect\Exception\JwksException; use ItkDev\OpenIdConnect\Exception\MetadataException; use ItkDev\OpenIdConnect\Exception\MissingParameterException; use ItkDev\OpenIdConnect\Exception\NegativeCacheDurationException; @@ -59,7 +59,7 @@ public static function concreteProvider(): iterable yield 'HttpException' => [HttpException::class, \RuntimeException::class]; yield 'JsonException' => [JsonException::class, \RuntimeException::class]; yield 'DecodeException' => [DecodeException::class, \RuntimeException::class]; - yield 'KeyException' => [KeyException::class, \RuntimeException::class]; + yield 'JwksException' => [JwksException::class, \RuntimeException::class]; yield 'CodeException' => [CodeException::class, \RuntimeException::class]; yield 'ValidationException' => [ValidationException::class, \RuntimeException::class]; yield 'ClaimsException' => [ClaimsException::class, \RuntimeException::class]; diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index e79070e..d3be35f 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -12,7 +12,7 @@ use ItkDev\OpenIdConnect\Exception\CodeException; use ItkDev\OpenIdConnect\Exception\HttpException; use ItkDev\OpenIdConnect\Exception\IllegalSchemeException; -use ItkDev\OpenIdConnect\Exception\KeyException; +use ItkDev\OpenIdConnect\Exception\JwksException; use ItkDev\OpenIdConnect\Exception\MissingParameterException; use ItkDev\OpenIdConnect\Exception\NegativeCacheDurationException; use ItkDev\OpenIdConnect\Exception\NegativeLeewayException; @@ -766,7 +766,7 @@ public function testGetJwtVerificationKeysRejectsJwksMissingKeysArray(): void $provider = $this->createProviderWithCustomJwks((string) json_encode(['something_else' => 1])); \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('JWKS payload missing array "keys" property (RFC 7517 §5)'); $provider->validateIdToken('token', self::NONCE); @@ -777,7 +777,7 @@ public function testGetJwtVerificationKeysRejectsNonObjectJwkEntry(): void $provider = $this->createProviderWithCustomJwks((string) json_encode(['keys' => [42]])); \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('JWK entry is not a JSON object'); $provider->validateIdToken('token', self::NONCE); @@ -790,7 +790,7 @@ public function testGetJwtVerificationKeysRejectsNonStringKty(): void ); \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('JWK entry missing string "kty" for key id: key-1'); $provider->validateIdToken('token', self::NONCE); @@ -803,7 +803,7 @@ public function testGetJwtVerificationKeysRejectsRsaWithoutStringExpOrModulus(): ); \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('JWK RSA entry missing string "e"/"n" for key id: key-1'); $provider->validateIdToken('token', self::NONCE); @@ -849,7 +849,7 @@ public function testGetJwtVerificationKeysRejectsNonStringKid(): void $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('JWK entry missing string "kid" (RFC 7517 §4.5)'); $provider->validateIdToken('token', self::NONCE); @@ -894,7 +894,7 @@ public function testGetJwtVerificationKeysUnsupportedKeyType(): void $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); - $this->expectException(KeyException::class); + $this->expectException(JwksException::class); $this->expectExceptionMessage('Unsupported key data for key id: ec-key-1'); $provider->validateIdToken('token', self::NONCE);