From 4f0f6edc1280af22a27961dc5fe9184c83b97b59 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Mon, 11 May 2026 15:16:26 +0200 Subject: [PATCH 01/23] =?UTF-8?q?Introduce=20marker-interface=20exception?= =?UTF-8?q?=20contract=20(5.0=20=E2=80=94=20BREAKING)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The public exception contract becomes a marker interface — `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface extends \Throwable` — instead of the abstract `ItkOpenIdConnectException` class. Every concrete exception is re-parented to the SPL type that best describes its failure category (`\RuntimeException` for transient/data-shape failures; `\LogicException` for programmer/config bugs; `\InvalidArgumentException` for invalid constructor input) and implements the marker. Consumers can now catch every OIDC failure with `catch (OpenIdConnectExceptionInterface $e)` or scope to a specific SPL parent. Concretes no longer extend the abstract `ItkOpenIdConnectException`; the abstract is kept for 5.x as a `@deprecated` alias that still implements the marker, but existing `catch (ItkOpenIdConnectException $e)` blocks will not match anything thrown by 5.0+ code. This is the user-visible BC break behind the MAJOR bump. The README "Exception handling" section walks through the consumer migration. Adds `ConfigurationException` (extending `\InvalidArgumentException`) for missing/invalid constructor options, replacing the raw `\InvalidArgumentException` previously thrown from the constructor. The new type still extends `\InvalidArgumentException`, so existing catches at that level keep matching. Narrows `getIdToken`'s `catch (\Exception $e)` boundary to `IdentityProviderException|ClientExceptionInterface|\JsonException` — the three actually-thrown families — so unexpected failures are no longer silently wrapped as `CodeException`. Cache failures during `getConfiguration` (called for the token-endpoint lookup) now propagate as `CacheException` rather than being re-wrapped. Adds `tests/Exception/ExceptionHierarchyTest.php` to lock the contract: every concrete is verified to implement the marker, extend the correct SPL parent, and be catchable via the marker. Failing this test class is the early warning that the public contract has drifted. Updates the README with a new "Exception handling" subsection (marker interface, SPL parent table, PSR-18 co-implementation note on `HttpException`, and 4.x → 5.0 catch-block migration) and updates the `validateIdToken` example to catch the marker interface. Co-Authored-By: Claude Opus 4.7 (1M context) --- .gitignore | 3 + CHANGELOG.md | 60 ++++++++ README.md | 45 +++++- src/Exception/BadUrlException.php | 2 +- src/Exception/CacheException.php | 2 +- src/Exception/ClaimsException.php | 2 +- src/Exception/CodeException.php | 2 +- src/Exception/ConfigurationException.php | 13 ++ src/Exception/DecodeException.php | 2 +- src/Exception/HttpException.php | 2 +- src/Exception/IllegalSchemeException.php | 2 +- src/Exception/ItkOpenIdConnectException.php | 8 +- src/Exception/JsonException.php | 2 +- src/Exception/KeyException.php | 2 +- src/Exception/MissingParameterException.php | 2 +- .../NegativeCacheDurationException.php | 2 +- src/Exception/NegativeLeewayException.php | 2 +- .../OpenIdConnectExceptionInterface.php | 15 ++ src/Exception/ValidationException.php | 2 +- src/Security/OpenIdConfigurationProvider.php | 23 +-- tests/Exception/ExceptionHierarchyTest.php | 131 ++++++++++++++++++ .../OpenIdConfigurationProviderTest.php | 10 +- 22 files changed, 305 insertions(+), 29 deletions(-) create mode 100644 src/Exception/ConfigurationException.php create mode 100644 src/Exception/OpenIdConnectExceptionInterface.php create mode 100644 tests/Exception/ExceptionHierarchyTest.php diff --git a/.gitignore b/.gitignore index ae0dcca..15e3791 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,6 @@ phpcs-report.xml .php-cs-fixer.cache coverage/ yarn.lock + +# Per-developer Claude Code context — local tooling, not part of the public source. +/CLAUDE.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 557a083..470efa0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,66 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed (BREAKING) + +- **Exception hierarchy reworked.** Every exception thrown from a public + method now implements + `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` (new + 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`, + `CodeException`, `ValidationException`, `ClaimsException`), + `\LogicException` (programmer/config bug — `BadUrlException`, + `IllegalSchemeException`, `MissingParameterException`), + `\InvalidArgumentException` (invalid input — `ConfigurationException`, + `NegativeCacheDurationException`, `NegativeLeewayException`). + Consumers catching `ItkOpenIdConnectException` should migrate to + `OpenIdConnectExceptionInterface`; the abstract class is kept as a + `@deprecated` alias and still implements the marker, but **concrete + exceptions no longer extend it**, so existing `catch + (ItkOpenIdConnectException $e)` blocks will not match anything thrown + by 5.0+ code. +- `OpenIdConfigurationProvider::__construct` now throws + `ConfigurationException` (new, `\InvalidArgumentException`-typed) + instead of a raw `\InvalidArgumentException` when a required option + is missing. The new type implements the marker; existing + `catch (\InvalidArgumentException $e)` blocks continue to match. +- `OpenIdConfigurationProvider::getIdToken` narrowed its boundary + `catch` from `\Exception` to + `IdentityProviderException|ClientExceptionInterface|\JsonException`. + Cache failures during `getConfiguration` (called for the token + endpoint lookup) now propagate as `CacheException` rather than being + 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. + +### Added + +- `ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` + marker for catching every OIDC failure from this library. +- `ItkDev\OpenIdConnect\Exception\ConfigurationException` for missing + or invalid constructor options. +- `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)` + block. Failing this test class is the early warning that the public + contract has drifted. + +### Deprecated + +- `ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException` abstract + class (catch `OpenIdConnectExceptionInterface` instead). Kept through + 5.x; removal scheduled for 6.0. + +### Documentation + +- Added a new "Exception handling" section to `README.md` describing the + marker interface, the SPL parents of each concrete, the PSR-18 + co-implementation on `HttpException`, and the 4.x → 5.0 catch-block + migration. Also fixed the `validateIdToken` example to catch the + marker interface instead of the now-deprecated abstract. + ## [4.1.2] - 2026-05-11 - Chained `previous` consistently in `OpenIdConfigurationProvider` catch diff --git a/README.md b/README.md index 852c83d..bc9eb0f 100644 --- a/README.md +++ b/README.md @@ -185,17 +185,58 @@ if (!$sessionState || $request->query->get('state') !== $sessionState) { // Validate the id token. This will validate the token against the keys published by the // provider (Azure AD B2C). If the token is invalid or the nonce doesn't match an -// exception will thrown. +// exception will be thrown. try { $claims = $provider->validateIdToken($request->query->get('id_token'), $session->get('oauth2nonce')); // Authentication successful -} catch (ItkOpenIdConnectException $exception) { +} catch (OpenIdConnectExceptionInterface $exception) { // Handle failed authentication } finally { $this->session->remove('oauth2nonce'); } ``` +### Exception handling + +Every exception thrown from a public method of this library implements +`\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface`. Catch the +marker to handle any OIDC failure with a single block, or scope to a more +specific type when you need to discriminate: + +```php +use ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface; + +try { + $claims = $provider->validateIdToken($idToken, $nonce); +} catch (OpenIdConnectExceptionInterface $e) { + // Cause is preserved via $e->getPrevious() +} +``` + +Concrete exception classes extend the SPL type that describes the failure +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 | +| `\LogicException` | `BadUrlException`, `IllegalSchemeException`, `MissingParameterException` | Programmer/config bugs — should be fixed in code | +| `\InvalidArgumentException` | `ConfigurationException`, `NegativeCacheDurationException`, `NegativeLeewayException` | Invalid input to the constructor / setters | + +`HttpException` additionally implements PSR-18's +`Psr\Http\Client\ClientExceptionInterface`, so existing PSR-18-aware +consumers can keep catching on the standard PSR marker. + +Every wrap site preserves the underlying cause via `$previous`, so +`$e->getPrevious()` walks back to the originating Guzzle, firebase/php-jwt +or PSR-6 cache exception. + +> **Upgrading from 4.x:** the concrete exceptions no longer extend the +> abstract `ItkOpenIdConnectException`. Catches written as +> `catch (ItkOpenIdConnectException $e)` will not match anything thrown +> by 5.0+ code — migrate to `catch (OpenIdConnectExceptionInterface $e)`. +> The abstract class itself is kept through 5.x as a documented alias +> (`@deprecated`); removal is scheduled for 6.0. + ## Development Setup A `docker-compose.yml` file with a PHP 8.3+ image is included in this project. diff --git a/src/Exception/BadUrlException.php b/src/Exception/BadUrlException.php index 909ab38..ee00e77 100644 --- a/src/Exception/BadUrlException.php +++ b/src/Exception/BadUrlException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class BadUrlException extends ItkOpenIdConnectException +class BadUrlException extends \LogicException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CacheException.php b/src/Exception/CacheException.php index 72a6bff..89e6b45 100644 --- a/src/Exception/CacheException.php +++ b/src/Exception/CacheException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class CacheException extends ItkOpenIdConnectException +class CacheException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ClaimsException.php b/src/Exception/ClaimsException.php index ce5fadd..86ec72f 100644 --- a/src/Exception/ClaimsException.php +++ b/src/Exception/ClaimsException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class ClaimsException extends ItkOpenIdConnectException +class ClaimsException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/CodeException.php b/src/Exception/CodeException.php index 424c7cf..82d7186 100644 --- a/src/Exception/CodeException.php +++ b/src/Exception/CodeException.php @@ -2,6 +2,6 @@ namespace ItkDev\OpenIdConnect\Exception; -class CodeException extends ItkOpenIdConnectException +class CodeException extends \RuntimeException implements OpenIdConnectExceptionInterface { } diff --git a/src/Exception/ConfigurationException.php b/src/Exception/ConfigurationException.php new file mode 100644 index 0000000..45279cd --- /dev/null +++ b/src/Exception/ConfigurationException.php @@ -0,0 +1,13 @@ +setCacheItemPool($options['cacheItemPool']); @@ -84,7 +85,7 @@ public function __construct(array $options = [], array $collaborators = []) } if (!array_key_exists('openIDConnectMetadataUrl', $options)) { - throw new \InvalidArgumentException('Required options not defined: openIDConnectMetadataUrl'); + throw new ConfigurationException('Required options not defined: openIDConnectMetadataUrl'); } if (empty($collaborators['jwt'])) { @@ -114,7 +115,7 @@ public function getBaseAuthorizationUrl(): string } /** - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ public function getAuthorizationUrl(array $options = []): string { @@ -246,7 +247,7 @@ public function validateIdToken(string $idToken, string $nonce): object * @return string * The ID token * - * @throws CodeException Wraps any \Exception thrown by token-endpoint HTTP, JSON parsing, or `getConfiguration()` (with `previous` chained) + * @throws OpenIdConnectExceptionInterface */ public function getIdToken(string $code): string { @@ -265,7 +266,11 @@ public function getIdToken(string $code): string $payload = json_decode((string) $response->getBody(), true, 512, JSON_THROW_ON_ERROR); return $payload['id_token']; - } catch (\Exception $e) { + } catch (IdentityProviderException|ClientExceptionInterface|\JsonException $e) { + // Narrow boundary: IdentityProviderException from league's checkResponse, + // ClientExceptionInterface from Guzzle, \JsonException from json_decode. + // Other failures (e.g. CacheException from getConfiguration) propagate + // as their own concrete OpenIdConnectExceptionInterface subtypes. throw new CodeException('Get ID token failed: '.$e->getMessage(), 0, $e); } } @@ -346,7 +351,7 @@ protected function createResourceOwner(array $response, AccessToken $token): Res * @return array * Array of keys * - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ private function getJwtVerificationKeys(): array { @@ -538,7 +543,7 @@ private function setAllowHttp(bool $allowHttp): void /** * Set the OpenID Connect Metadata Url. * - * @throws ItkOpenIdConnectException + * @throws OpenIdConnectExceptionInterface */ private function setOpenIDConnectMetadataUrl(string $url): void { diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php new file mode 100644 index 0000000..b065542 --- /dev/null +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -0,0 +1,131 @@ +, class-string<\Throwable>}> + */ + public static function concreteProvider(): iterable + { + // Programmer / config errors → \LogicException + yield 'BadUrlException' => [BadUrlException::class, \LogicException::class]; + yield 'IllegalSchemeException' => [IllegalSchemeException::class, \LogicException::class]; + yield 'MissingParameterException' => [MissingParameterException::class, \LogicException::class]; + + // Invalid input to a public method / constructor → \InvalidArgumentException + yield 'ConfigurationException' => [ConfigurationException::class, \InvalidArgumentException::class]; + yield 'NegativeCacheDurationException' => [NegativeCacheDurationException::class, \InvalidArgumentException::class]; + yield 'NegativeLeewayException' => [NegativeLeewayException::class, \InvalidArgumentException::class]; + + // Runtime conditions → \RuntimeException + yield 'CacheException' => [CacheException::class, \RuntimeException::class]; + 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 'CodeException' => [CodeException::class, \RuntimeException::class]; + yield 'ValidationException' => [ValidationException::class, \RuntimeException::class]; + yield 'ClaimsException' => [ClaimsException::class, \RuntimeException::class]; + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $expectedSplParent + */ + #[DataProvider('concreteProvider')] + public function testConcreteImplementsMarker(string $concrete, string $expectedSplParent): void + { + $instance = new $concrete('test'); + $this->assertInstanceOf(OpenIdConnectExceptionInterface::class, $instance); + $this->assertInstanceOf(\Throwable::class, $instance); + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $expectedSplParent + */ + #[DataProvider('concreteProvider')] + public function testConcreteExtendsExpectedSplParent(string $concrete, string $expectedSplParent): void + { + $instance = new $concrete('test'); + $this->assertInstanceOf($expectedSplParent, $instance); + } + + /** + * @param class-string<\Throwable> $concrete + * @param class-string<\Throwable> $expectedSplParent + */ + #[DataProvider('concreteProvider')] + public function testCatchByMarkerMatchesEveryConcrete(string $concrete, string $expectedSplParent): void + { + try { + throw new $concrete('test'); + } catch (OpenIdConnectExceptionInterface $caught) { + $this->assertInstanceOf($concrete, $caught); + + return; + } + // @phpstan-ignore deadCode.unreachable + $this->fail('Catch on OpenIdConnectExceptionInterface should have matched '.$concrete); + } + + public function testHttpExceptionAlsoImplementsPsr18ClientInterface(): void + { + // HttpException is part of the public contract for two markers — the + // OIDC marker and PSR-18's ClientExceptionInterface — so a PSR-18-savvy + // consumer can catch HTTP failures via the standard PSR interface. + $instance = new HttpException('test'); + $this->assertInstanceOf(ClientExceptionInterface::class, $instance); + } + + public function testAbstractBaseImplementsMarker(): void + { + // The deprecated abstract `ItkOpenIdConnectException` is kept for + // backward compatibility through 5.x and still implements the marker. + // Catch sites that wrote `catch (ItkOpenIdConnectException $e)` should + // migrate to the marker interface; this assertion guards the marker + // implementation while the deprecation window is open. + $this->assertTrue( + is_subclass_of( + \ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException::class, + OpenIdConnectExceptionInterface::class, + ), + ); + } +} diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 2aaa02d..a463691 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -12,8 +12,8 @@ use ItkDev\OpenIdConnect\Exception\CodeException; use ItkDev\OpenIdConnect\Exception\HttpException; use ItkDev\OpenIdConnect\Exception\IllegalSchemeException; -use ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException; use ItkDev\OpenIdConnect\Exception\KeyException; +use ItkDev\OpenIdConnect\Exception\MissingParameterException; use ItkDev\OpenIdConnect\Exception\NegativeCacheDurationException; use ItkDev\OpenIdConnect\Exception\NegativeLeewayException; use ItkDev\OpenIdConnect\Exception\ValidationException; @@ -179,7 +179,7 @@ public function testGetAuthorizationUrl(): void public function testGetAuthorizationUrlStateException(): void { - $this->expectException(ItkOpenIdConnectException::class); + $this->expectException(MissingParameterException::class); $this->expectExceptionMessage('Required parameter "state" missing'); $authUrl = $this->provider->getAuthorizationUrl(['nonce' => 'abcd']); @@ -187,7 +187,7 @@ public function testGetAuthorizationUrlStateException(): void public function testGetAuthorizationUrlNonceException(): void { - $this->expectException(ItkOpenIdConnectException::class); + $this->expectException(MissingParameterException::class); $this->expectExceptionMessage('Required parameter "nonce" missing'); $authUrl = $this->provider->getAuthorizationUrl(['state' => 'abcd']); @@ -512,7 +512,9 @@ public function testGetIdTokenFailure(): void $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; $mockHttpClient = $this->createStub(ClientInterface::class); - $mockHttpClient->method('request')->willThrowException(new \RuntimeException('Connection failed')); + $mockHttpClient->method('request')->willThrowException( + new class('Connection failed') extends \RuntimeException implements ClientExceptionInterface {} + ); $mockCacheItem = $this->createStub(CacheItemInterface::class); $mockCacheItem->method('isHit')->willReturn(false); From 4df22f607d403a669ef315192d2b83b02accf3a4 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 10:33:20 +0200 Subject: [PATCH 02/23] Justify @phpstan-ignore and PSR-18 transport stub in tests `@phpstan-ignore deadCode.unreachable` now carries a parenthesized reason (PHPStan's documented format for ignore justifications) so a reviewer can tell at a glance why the suppression exists. The suppressed line is intentionally unreachable in the happy path; it serves as a fail-safe if a future regression breaks the catch-by-marker contract this test verifies. The anonymous `ClientExceptionInterface` implementer in `testGetIdTokenFailure` now has a one-line comment explaining the choice. Guzzle's real exception constructors require a RequestInterface we don't have in a unit test, and any PSR-18 implementer satisfies the contract `getIdToken`'s narrowed catch actually targets. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/Exception/ExceptionHierarchyTest.php | 2 +- tests/Security/OpenIdConfigurationProviderTest.php | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index b065542..06fdd92 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -101,7 +101,7 @@ public function testCatchByMarkerMatchesEveryConcrete(string $concrete, string $ return; } - // @phpstan-ignore deadCode.unreachable + // @phpstan-ignore deadCode.unreachable (safety net if a future regression breaks the catch-by-marker contract) $this->fail('Catch on OpenIdConnectExceptionInterface should have matched '.$concrete); } diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index a463691..c3ef46b 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -512,6 +512,8 @@ public function testGetIdTokenFailure(): void $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; $mockHttpClient = $this->createStub(ClientInterface::class); + // PSR-18 transport stub — Guzzle's real exceptions need a RequestInterface + // we don't have here, and any ClientExceptionInterface satisfies getIdToken's catch. $mockHttpClient->method('request')->willThrowException( new class('Connection failed') extends \RuntimeException implements ClientExceptionInterface {} ); From fcbb5b3b7a0b4580e4ebfeeca0e7ab7b84452edc Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 10:34:48 +0200 Subject: [PATCH 03/23] Enforce @phpstan-ignore-must-have-comment via PHPStan built-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds PHPStan's `reportIgnoresWithoutComments: true` setting (available in `phpstan/phpstan` 2.1.41+, so bump the require-dev constraint). With this setting active, any `@phpstan-ignore` directive without a parenthesized justification fails CI — turning the documented convention into a hard rule that won't quietly drift back. To make the setting actually fire on the test where the original violation surfaced, `tests/` is added to PHPStan's `paths`. That surfaces 46 pre-existing static-analysis issues unrelated to the contract migration (mostly `string|false` flow into `json_decode`, Mockery stub method typing, and dynamic property access on `\stdClass` claims). Generating `phpstan-baseline.neon` grandfathers those so they don't block this PR while still letting future regressions in test code surface as fresh errors. The baseline is deliberately a temporary measure: it should shrink over time as the test-code static-analysis debt is paid down in follow-up PRs. The alternative (keeping `tests/` out of PHPStan scope) would leave the new ignore-comments rule unenforced in exactly the location where the original reviewer complaint landed — which defeats the point of adding the rule. Co-Authored-By: Claude Opus 4.7 (1M context) --- composer.json | 2 +- phpstan-baseline.neon | 139 ++++++++++++++++++++++++++++++++++++++++++ phpstan.neon | 5 ++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 phpstan-baseline.neon diff --git a/composer.json b/composer.json index 196637c..1bf1ecb 100644 --- a/composer.json +++ b/composer.json @@ -31,7 +31,7 @@ "ergebnis/composer-normalize": "^2.50", "friendsofphp/php-cs-fixer": "^3.75", "mockery/mockery": "^1.6.12", - "phpstan/phpstan": "^2.1.40", + "phpstan/phpstan": "^2.1.41", "phpunit/php-code-coverage": "^12", "phpunit/phpunit": "^12" }, diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon new file mode 100644 index 0000000..9d14590 --- /dev/null +++ b/phpstan-baseline.neon @@ -0,0 +1,139 @@ +parameters: + ignoreErrors: + - + message: '#^Call to function is_subclass_of\(\) with ''ItkDev\\\\OpenIdConnect\\\\Exception\\\\ItkOpenIdConnectException'' and ''ItkDev\\\\OpenIdConnect\\\\Exception\\\\OpenIdConnectExceptionInterface'' will always evaluate to true\.$#' + identifier: function.alreadyNarrowedType + count: 1 + path: tests/Exception/ExceptionHierarchyTest.php + + - + message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' + identifier: method.alreadyNarrowedType + count: 1 + path: tests/Exception/ExceptionHierarchyTest.php + + - + message: '#^Property Tests\\Security\\MockJWT\:\:\$leeway has no type specified\.$#' + identifier: missingType.property + count: 1 + path: tests/Security/MockJWT.php + + - + message: '#^Access to an undefined property object\:\:\$aud\.$#' + identifier: property.notFound + count: 2 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Access to an undefined property object\:\:\$nonce\.$#' + identifier: property.notFound + count: 3 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:andReturn\(\)\.$#' + identifier: method.notFound + count: 6 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:andThrow\(\)\.$#' + identifier: method.notFound + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:with\(\)\.$#' + identifier: method.notFound + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' + identifier: method.alreadyNarrowedType + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method generateNonce\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method generateState\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getAuthorizationUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 3 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getBaseAccessTokenUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getBaseAuthorizationUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getDefaultScopes\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getEndSessionUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 6 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getGuarded\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getResourceOwnerDetailsUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method getState\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Cannot call method validateIdToken\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' + identifier: method.nonObject + count: 7 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Parameter \#1 \$json of function json_decode expects string, string\|false given\.$#' + identifier: argument.type + count: 3 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Parameter \#1 \$string of function parse_str expects string, string\|false\|null given\.$#' + identifier: argument.type + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php + + - + message: '#^Property Tests\\Security\\OpenIdConfigurationProviderTest\:\:\$provider \(ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\) is never assigned null so it can be removed from the property type\.$#' + identifier: property.unusedType + count: 1 + path: tests/Security/OpenIdConfigurationProviderTest.php diff --git a/phpstan.neon b/phpstan.neon index 500b620..b0a1a59 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,7 +1,12 @@ +includes: + - phpstan-baseline.neon + parameters: level: 8 paths: - src + - tests + reportIgnoresWithoutComments: true ignoreErrors: - identifier: missingType.iterableValue From baab00a10ae98c6db561c30df9c24353278b9f2b Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 10:43:45 +0200 Subject: [PATCH 04/23] Drop nullable from $provider test property MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `$this->provider` is initialized in `setUp()` and never assigned null, so the nullable type was misleading. Dropping the `?` removes 25 of the 46 grandfathered errors in `phpstan-baseline.neon` — every `Cannot call method X() on …|null` was a downstream consequence of that one type. Co-Authored-By: Claude Opus 4.7 (1M context) --- phpstan-baseline.neon | 72 ------------------- .../OpenIdConfigurationProviderTest.php | 2 +- 2 files changed, 1 insertion(+), 73 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 9d14590..4a89a8d 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -54,72 +54,6 @@ parameters: count: 1 path: tests/Security/OpenIdConfigurationProviderTest.php - - - message: '#^Cannot call method generateNonce\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method generateState\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getAuthorizationUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 3 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getBaseAccessTokenUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getBaseAuthorizationUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getDefaultScopes\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getEndSessionUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 6 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getGuarded\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getResourceOwnerDetailsUrl\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method getState\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Cannot call method validateIdToken\(\) on ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\.$#' - identifier: method.nonObject - count: 7 - path: tests/Security/OpenIdConfigurationProviderTest.php - - message: '#^Parameter \#1 \$json of function json_decode expects string, string\|false given\.$#' identifier: argument.type @@ -131,9 +65,3 @@ parameters: identifier: argument.type count: 1 path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Property Tests\\Security\\OpenIdConfigurationProviderTest\:\:\$provider \(ItkDev\\OpenIdConnect\\Security\\OpenIdConfigurationProvider\|null\) is never assigned null so it can be removed from the property type\.$#' - identifier: property.unusedType - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index c3ef46b..808dac9 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -37,7 +37,7 @@ class OpenIdConfigurationProviderTest extends TestCase private const REDIRECT_URI = 'https://redirect.url'; private const NONCE = '12345678'; - private ?OpenIdConfigurationProvider $provider; + private OpenIdConfigurationProvider $provider; public function setUp(): void { From 02d877972754b29f029ff0f40066ff4dd2fb2f75 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 10:48:44 +0200 Subject: [PATCH 05/23] Install phpstan/phpstan-mockery for Mockery type stubs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `shouldReceive(...)->andReturn(...)` is the Mockery fluent API. The return type of `shouldReceive()` is declared as `Mockery\ExpectationInterface|Mockery\HigherOrderMessage`, and the chained methods only exist on `ExpectationInterface` — PHPStan can't tell the call sites are safe without type stubs. `phpstan/phpstan-mockery` is the official PHPStan extension shipping those stubs. Adding it removes 8 entries from `phpstan-baseline.neon`. Co-Authored-By: Claude Opus 4.7 (1M context) --- composer.json | 3 ++- phpstan-baseline.neon | 18 ------------------ phpstan.neon | 1 + 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/composer.json b/composer.json index 1bf1ecb..4c60f08 100644 --- a/composer.json +++ b/composer.json @@ -33,7 +33,8 @@ "mockery/mockery": "^1.6.12", "phpstan/phpstan": "^2.1.41", "phpunit/php-code-coverage": "^12", - "phpunit/phpunit": "^12" + "phpunit/phpunit": "^12", + "phpstan/phpstan-mockery": "^2.0" }, "autoload": { "psr-4": { diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 4a89a8d..6c77134 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -30,24 +30,6 @@ parameters: count: 3 path: tests/Security/OpenIdConfigurationProviderTest.php - - - message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:andReturn\(\)\.$#' - identifier: method.notFound - count: 6 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:andThrow\(\)\.$#' - identifier: method.notFound - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Call to an undefined method Mockery\\ExpectationInterface\|Mockery\\HigherOrderMessage\:\:with\(\)\.$#' - identifier: method.notFound - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php - - message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' identifier: method.alreadyNarrowedType diff --git a/phpstan.neon b/phpstan.neon index b0a1a59..b1a3869 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,5 +1,6 @@ includes: - phpstan-baseline.neon + - vendor/phpstan/phpstan-mockery/extension.neon parameters: level: 8 From f47f708007c8e7880f6ded3f97d3fb8a5eaa65a1 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 10:51:46 +0200 Subject: [PATCH 06/23] Narrow validateIdToken claim accesses via @var MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validateIdToken()` is declared `: object` in the library — PHPStan can't see the dynamic `nonce` / `aud` properties of the decoded JWT payload. Annotate each call site's local `$claims` with an `object{nonce, aud}` shape so the subsequent property reads type- check. Removes 5 entries from `phpstan-baseline.neon`. Co-Authored-By: Claude Opus 4.7 (1M context) --- phpstan-baseline.neon | 12 ------------ tests/Security/OpenIdConfigurationProviderTest.php | 3 +++ 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 6c77134..32e1130 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -18,18 +18,6 @@ parameters: count: 1 path: tests/Security/MockJWT.php - - - message: '#^Access to an undefined property object\:\:\$aud\.$#' - identifier: property.notFound - count: 2 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Access to an undefined property object\:\:\$nonce\.$#' - identifier: property.notFound - count: 3 - path: tests/Security/OpenIdConfigurationProviderTest.php - - message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' identifier: method.alreadyNarrowedType diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 808dac9..026f72f 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -238,6 +238,7 @@ public function testValidateIdTokenSuccess(): void ) )->andReturn($mockClaims); + /** @var object{nonce: string, aud: string|list} $claims */ $claims = $this->provider->validateIdToken('token', self::NONCE); $this->assertEquals(self::NONCE, $claims->nonce); @@ -436,6 +437,7 @@ public function testValidateIdTokenArrayAudience(): void $mockJWT->shouldReceive('decode')->andReturn($mockClaims); + /** @var object{nonce: string, aud: string|list} $claims */ $claims = $this->provider->validateIdToken('token', self::NONCE); $this->assertEquals(self::NONCE, $claims->nonce); @@ -770,6 +772,7 @@ public function testGetJwtVerificationKeysCacheHit(): void $mockClaims = $this->getMockClaims(); $mockJWT->shouldReceive('decode')->andReturn($mockClaims); + /** @var object{nonce: string} $claims */ $claims = $provider->validateIdToken('token', self::NONCE); $this->assertEquals(self::NONCE, $claims->nonce); } From eda1395ec2074f9e48880e0df6fc18fe678dfdda Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 10:57:59 +0200 Subject: [PATCH 07/23] Fail loudly on missing fixtures and malformed authorization URLs `(string) file_get_contents(...)` silently coerces the `false` returned when a fixture is missing into an empty string, which then flows into `json_decode` and produces `null`. Tests downstream of that report confusing assertion failures instead of "the fixture isn't there". Similarly, `(string) parse_url(...)` masks malformed-URL failures. Replaces the three duplicate fixture loads with a single `loadMockFixture()` helper that uses `assertNotFalse` and `assertIsArray` to fail the test at the actual error boundary. The `parse_url` call adds an `assertIsString` guard for the same reason. Removes 4 entries from `phpstan-baseline.neon`. Co-Authored-By: Claude Opus 4.7 (1M context) --- phpstan-baseline.neon | 12 ------ .../OpenIdConfigurationProviderTest.php | 38 ++++++++++++------- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 32e1130..040ec00 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -23,15 +23,3 @@ parameters: identifier: method.alreadyNarrowedType count: 1 path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Parameter \#1 \$json of function json_decode expects string, string\|false given\.$#' - identifier: argument.type - count: 3 - path: tests/Security/OpenIdConfigurationProviderTest.php - - - - message: '#^Parameter \#1 \$string of function parse_str expects string, string\|false\|null given\.$#' - identifier: argument.type - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 026f72f..fed9b23 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -168,7 +168,9 @@ public function testGetAuthorizationUrl(): void $authUrl = $this->provider->getAuthorizationUrl(['state' => $state, 'nonce' => $nonce]); $query = []; - parse_str(parse_url($authUrl, PHP_URL_QUERY), $query); + $queryString = parse_url($authUrl, PHP_URL_QUERY); + $this->assertIsString($queryString, 'Generated authorization URL must have a query string'); + parse_str($queryString, $query); $this->assertSame('openid', $query['scope']); $this->assertSame('id_token', $query['response_type']); @@ -544,10 +546,7 @@ public function testGetIdTokenFailure(): void public function testGetConfigurationCacheHit(): void { - $configuration = json_decode( - file_get_contents(__DIR__.'/../MockData/mockOpenIDConfiguration.json'), - true - ); + $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); $mockCacheItem = $this->createStub(CacheItemInterface::class); $mockCacheItem->method('isHit')->willReturn(true); @@ -732,10 +731,7 @@ public function testGetJwtVerificationKeysCacheHit(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; - $configuration = json_decode( - file_get_contents(__DIR__.'/../MockData/mockOpenIDConfiguration.json'), - true - ); + $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); $cachedKeys = ['key1' => new Key('public-key-data', 'RS256')]; @@ -807,10 +803,7 @@ public function testGetConfigurationCacheInvalidArgument(): void public function testGetJwtVerificationKeysCacheInvalidArgument(): void { $openIDConnectMetadataUrl = 'https://some.url/openid-configuration'; - $configuration = json_decode( - file_get_contents(__DIR__.'/../MockData/mockOpenIDConfiguration.json'), - true - ); + $configuration = $this->loadMockFixture('mockOpenIDConfiguration.json'); $configCacheItem = $this->createStub(CacheItemInterface::class); $configCacheItem->method('isHit')->willReturn(true); @@ -901,6 +894,25 @@ public function testBase64urlDecodeFailure(): void * @return ResponseInterface * A success ("200") response with mock body data */ + /** + * Load a JSON fixture from tests/MockData and decode it as an associative + * array. Fails the test with an explicit message if the file is missing / + * unreadable / not valid JSON, rather than letting `false` or `null` flow + * silently into the assertion under test. + * + * @return array + */ + private function loadMockFixture(string $filename): array + { + $path = __DIR__.'/../MockData/'.$filename; + $contents = file_get_contents($path); + $this->assertNotFalse($contents, sprintf('Mock fixture not readable: %s', $path)); + $decoded = json_decode($contents, true); + $this->assertIsArray($decoded, sprintf('Mock fixture is not valid JSON: %s', $path)); + + return $decoded; + } + private function getMockHttpSuccessResponse(string $mockResponseDataPath): ResponseInterface { $mockResponseData = file_get_contents(__DIR__.$mockResponseDataPath); From 32aa3e6d53ad195ba50acb41a7b6dd5dc9f6ec70 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:00:05 +0200 Subject: [PATCH 08/23] Clear remaining level-8 test issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small cleanups, each removing one entry from the baseline: - `testCheckResponseSuccess` swaps `assertTrue(true)` (which PHPStan correctly flags as a tautology) for `expectNotToPerformAssertions()`, the PHPUnit idiom for "this test verifies the call doesn't throw". - `testAbstractBaseImplementsMarker` was using a constant-folded `is_subclass_of(...)` call wrapped in `assertTrue`. Replaced with a runtime `ReflectionClass::getInterfaceNames()` check + `assertContains` — PHPStan can't fold the reflection result, and the assertion stays semantically meaningful (catches a future regression that removes the marker from the deprecated abstract). - `MockJWT::$leeway` declares a type (`?int`) so the property satisfies PHPStan's missingType.property rule. Baseline now empty — see follow-up commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- phpstan-baseline.neon | 25 +------------------ tests/Exception/ExceptionHierarchyTest.php | 14 +++++++---- tests/Security/MockJWT.php | 2 +- .../OpenIdConfigurationProviderTest.php | 3 ++- 4 files changed, 13 insertions(+), 31 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 040ec00..aab4991 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -1,25 +1,2 @@ parameters: - ignoreErrors: - - - message: '#^Call to function is_subclass_of\(\) with ''ItkDev\\\\OpenIdConnect\\\\Exception\\\\ItkOpenIdConnectException'' and ''ItkDev\\\\OpenIdConnect\\\\Exception\\\\OpenIdConnectExceptionInterface'' will always evaluate to true\.$#' - identifier: function.alreadyNarrowedType - count: 1 - path: tests/Exception/ExceptionHierarchyTest.php - - - - message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' - identifier: method.alreadyNarrowedType - count: 1 - path: tests/Exception/ExceptionHierarchyTest.php - - - - message: '#^Property Tests\\Security\\MockJWT\:\:\$leeway has no type specified\.$#' - identifier: missingType.property - count: 1 - path: tests/Security/MockJWT.php - - - - message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertTrue\(\) with true will always evaluate to true\.$#' - identifier: method.alreadyNarrowedType - count: 1 - path: tests/Security/OpenIdConfigurationProviderTest.php + ignoreErrors: [] diff --git a/tests/Exception/ExceptionHierarchyTest.php b/tests/Exception/ExceptionHierarchyTest.php index 06fdd92..658cd65 100644 --- a/tests/Exception/ExceptionHierarchyTest.php +++ b/tests/Exception/ExceptionHierarchyTest.php @@ -121,11 +121,15 @@ public function testAbstractBaseImplementsMarker(): void // Catch sites that wrote `catch (ItkOpenIdConnectException $e)` should // migrate to the marker interface; this assertion guards the marker // implementation while the deprecation window is open. - $this->assertTrue( - is_subclass_of( - \ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException::class, - OpenIdConnectExceptionInterface::class, - ), + // + // ReflectionClass keeps the check at runtime so PHPStan can't fold it + // into a constant tautology — the value of the test is catching a + // *future* regression that removes the marker from the abstract. + $reflection = new \ReflectionClass(\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException::class); + $this->assertContains( + OpenIdConnectExceptionInterface::class, + $reflection->getInterfaceNames(), + 'Deprecated abstract must still implement the marker for 5.x BC.', ); } } diff --git a/tests/Security/MockJWT.php b/tests/Security/MockJWT.php index 95a8cde..cdd40e7 100644 --- a/tests/Security/MockJWT.php +++ b/tests/Security/MockJWT.php @@ -4,5 +4,5 @@ class MockJWT { - public static $leeway; + public static ?int $leeway = null; } diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index fed9b23..8bd4338 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -373,6 +373,8 @@ public function testGetResourceOwnerDetailsUrl(): void public function testCheckResponseSuccess(): void { + $this->expectNotToPerformAssertions(); + $response = $this->createStub(ResponseInterface::class); $response->method('getStatusCode')->willReturn(200); @@ -380,7 +382,6 @@ public function testCheckResponseSuccess(): void // Should not throw $method->invoke($this->provider, $response, ['data' => 'value']); - $this->assertTrue(true); } public function testCheckResponseWithErrorString(): void From c865421803f9b6d8c7999c13bcd753bd1710a46e Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:01:03 +0200 Subject: [PATCH 09/23] Delete now-empty phpstan-baseline.neon The four predecessor commits cleared the 46 grandfathered errors that `phpstan-baseline.neon` was absorbing. With the baseline at zero entries the file serves no purpose, so drop it (and its include line from `phpstan.neon`) rather than keep an empty alias around. PHPStan still runs at level 8 across `src` and `tests` and remains clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- phpstan-baseline.neon | 2 -- phpstan.neon | 1 - 2 files changed, 3 deletions(-) delete mode 100644 phpstan-baseline.neon diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon deleted file mode 100644 index aab4991..0000000 --- a/phpstan-baseline.neon +++ /dev/null @@ -1,2 +0,0 @@ -parameters: - ignoreErrors: [] diff --git a/phpstan.neon b/phpstan.neon index b1a3869..6e3ce04 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -1,5 +1,4 @@ includes: - - phpstan-baseline.neon - vendor/phpstan/phpstan-mockery/extension.neon parameters: From 54be58b319d0e4d9721c7329340f41196abf6546 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:25:55 +0200 Subject: [PATCH 10/23] Fix CI failures: redundant PHPDoc, composer order, CHANGELOG - `php-cs-fixer` (@Symfony ruleset) flagged a redundant `@param` PHPDoc on a typed parameter in `getMockHttpSuccessResponse`. Auto-fixed. - `composer normalize` reorders `phpstan-mockery` alphabetically within `require-dev` (it landed at the bottom after the manual `composer require`, between `phpunit/phpunit` and nothing). - The `changelog` CI step requires every PR to touch `CHANGELOG.md`. Added a "Tooling" subsection to `[Unreleased]` summarising what this PR's six commits did (PHPStan scope extension, mockery extension install, baseline cleanup). Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 18 ++++++++++++++++++ composer.json | 4 ++-- .../OpenIdConfigurationProviderTest.php | 3 --- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 470efa0..13d2d49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 migration. Also fixed the `validateIdToken` example to catch the marker interface instead of the now-deprecated abstract. +### Tooling + +- PHPStan now scans `tests/` in addition to `src/` at level 8, with + `reportIgnoresWithoutComments: true` so unexplained + `@phpstan-ignore` directives fail CI. +- Added `phpstan/phpstan-mockery` to `require-dev` for stubs covering + Mockery's fluent `shouldReceive(...)->andReturn(...)` API. +- Cleaned the 46 pre-existing level-8 issues in `tests/`: dropped the + unused nullable from `$this->provider`, narrowed `validateIdToken` + claim accesses with `object{nonce, aud}` `@var` shapes, replaced + silent `(string)` coercion of `file_get_contents` / `parse_url` + failures with `assertNotFalse` / `assertIsString` boundary guards, + swapped `assertTrue(true)` tautologies for + `expectNotToPerformAssertions`, and replaced the constant-folded + `is_subclass_of` marker check with a `ReflectionClass` lookup so + PHPStan can't fold it into a tautology. `phpstan-baseline.neon` + consequently shrinks to zero and is deleted. + ## [4.1.2] - 2026-05-11 - Chained `previous` consistently in `OpenIdConfigurationProvider` catch diff --git a/composer.json b/composer.json index 4c60f08..bb3321b 100644 --- a/composer.json +++ b/composer.json @@ -32,9 +32,9 @@ "friendsofphp/php-cs-fixer": "^3.75", "mockery/mockery": "^1.6.12", "phpstan/phpstan": "^2.1.41", + "phpstan/phpstan-mockery": "^2.0", "phpunit/php-code-coverage": "^12", - "phpunit/phpunit": "^12", - "phpstan/phpstan-mockery": "^2.0" + "phpunit/phpunit": "^12" }, "autoload": { "psr-4": { diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 8bd4338..7c91d1e 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -889,9 +889,6 @@ public function testBase64urlDecodeFailure(): void /** * Get a mock success response with mock date. * - * @param string $mockResponseDataPath - * Path to the file containing the mock response data - * * @return ResponseInterface * A success ("200") response with mock body data */ From 471620cb523450ecd471ce0ae12b36f150232300 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:12:44 +0200 Subject: [PATCH 11/23] 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 12/23] 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 13/23] 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'); From c715359af0c78d0e4b9f3955900e725868fe5bf3 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:48:14 +0200 Subject: [PATCH 14/23] Type constructor $options / $collaborators array shapes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `OpenIdConfigurationProvider::__construct(array $options, array $collaborators)` previously took untyped arrays — every `$options['cacheItemPool']`, `$options['cacheDuration']`, `$collaborators['jwt']`, `$options['openIDConnectMetadataUrl']` read flowed as `mixed` into the typed setters (`setCacheItemPool(CacheItemPoolInterface)`, `setCacheDuration(int)`, `setRequestFactory(RequestFactory)`, `setOpenIDConnectMetadataUrl(string)`), so PHPStan at `level: max` produced 4 `argument.type` errors. Add PHPDoc array shapes that declare the keys we read and their types. Keys are marked optional (`?:`) because the runtime `array_key_exists` + `ConfigurationException` checks still gate them — but their type is narrowed when present so the setter calls type-check. The `...` trailing entry lets the league/oauth2-client extra options (`clientId`, `clientSecret`, `redirectUri`, …) and Guzzle's forwarded options (`timeout`, `proxy`, `verify`) pass through without flagging. Drops a now-redundant `is_int($options['leeway'])` runtime check that became a `function.alreadyNarrowedType` tautology once the shape declared `leeway?: int`. Under `declare(strict_types=1)`, PHP itself enforces the type via `setLeeway(int $leeway)`. Behaviour unchanged — purely a static-analysis tightening at the constructor boundary. Removes 4 errors when PHPStan is run at `level: max`. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 12 ++++++++++ src/Security/OpenIdConfigurationProvider.php | 24 +++++++++++++++++++- 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13d2d49..d79b778 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Tooling +- `OpenIdConfigurationProvider::__construct` now declares an + `array{cacheItemPool?: CacheItemPoolInterface, + openIDConnectMetadataUrl?: string, cacheDuration?: int, leeway?: int, + allowHttp?: bool, ...}` PHPDoc shape for the `$options` parameter + (plus a corresponding shape on `$collaborators` for the `jwt` / + `httpClient` keys). PHPStan can now narrow each setter argument to + the expected type instead of seeing `mixed`. Behaviour unchanged — + removes 4 errors when running PHPStan at `level: max`. Also drops a + now-redundant `is_int($options['leeway'])` runtime check that + became a `function.alreadyNarrowedType` tautology once the shape + was in place (PHP itself enforces the `int` via the `setLeeway` + signature under `declare(strict_types=1)`). - PHPStan now scans `tests/` in addition to `src/` at level 8, with `reportIgnoresWithoutComments: true` so unexplained `@phpstan-ignore` directives fail CI. diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index 89ddbe1..e5a470a 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -65,6 +65,28 @@ class OpenIdConfigurationProvider extends AbstractProvider /** * OpenIdConfigurationProvider constructor. * + * The two well-typed keys (`cacheItemPool`, `openIDConnectMetadataUrl`) + * are marked optional in the array shape because the runtime check + * throws `ConfigurationException` if they are missing — but their type + * is narrowed when present so the downstream setters keep their typed + * arguments. Extra keys (league/oauth2-client's `clientId`, + * `clientSecret`, `redirectUri`, … and Guzzle's `timeout` / `proxy` / + * `verify`) are accepted via `...`. + * + * @param array{ + * cacheItemPool?: CacheItemPoolInterface, + * openIDConnectMetadataUrl?: string, + * cacheDuration?: int, + * leeway?: int, + * allowHttp?: bool, + * ... + * } $options + * @param array{ + * jwt?: \League\OAuth2\Client\Tool\RequestFactory, + * httpClient?: \GuzzleHttp\ClientInterface, + * ... + * } $collaborators + * * @throws OpenIdConnectExceptionInterface */ public function __construct(array $options = [], array $collaborators = []) @@ -80,7 +102,7 @@ public function __construct(array $options = [], array $collaborators = []) $this->setCacheDuration($options['cacheDuration']); } - if (array_key_exists('leeway', $options) && is_int($options['leeway'])) { + if (array_key_exists('leeway', $options)) { $this->setLeeway($options['leeway']); } From 38391f9711c296c1fb60fd049d573a42e1f307fb Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 11:56:58 +0200 Subject: [PATCH 15/23] 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 16/23] 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); From 6515c73ab29e7abd045defae3e7f93b6c50809b9 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 12:22:07 +0200 Subject: [PATCH 17/23] Narrow validateIdToken claims and assorted test types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small narrowings to clear the remaining `mixed` flow that PHPStan flags at `level: max`: - `getJwtVerificationKeys()` declares `@return array` matching the shape it actually builds. The cache-hit branch's `(array) $item->get()` is annotated with the same shape via inline `@var` — we only ever store this structure, so the trust is consistent with the method's contract. - `validateIdToken()`'s `$claims` is annotated with a `\stdClass&object{aud: string|array, iss: string, nonce: string}` shape after `JWT::decode()`. The fields are required string-typed claims per the OIDC spec; `firebase/php-jwt` already enforces JWT validity before this point. No runtime change. - `testCreateResourceOwner` adds an `assertInstanceOf(...)` guard so the `$owner->getId()` call type-checks (the value comes back from `ReflectionMethod::invoke()` which PHPStan sees as `mixed`). - `loadMockFixture()`'s `@return` is relaxed from `array` to `array` — `json_decode($content, true)` doesn't statically guarantee string keys, and the callers cast/narrow as needed for their specific fixture. - 11 `$mockJWT = \Mockery::mock('overload:...')` sites in the test file get an inline `@var \Mockery\MockInterface` annotation. The `overload:` mock-syntax isn't recognised by `phpstan/phpstan-mockery` (overload mocks are special-cased to never instantiate the target class, so the extension's class-resolution doesn't fire); the annotation tells PHPStan the value is a `MockInterface` so the chained `shouldReceive()->...` calls type-check. After this PR rebases onto develop (post-PR #43 / PR A merge), all max-level errors are gone. The `phpstan.neon` `level: 8` → `level: max` bump is left as a follow-up one-line PR so the bump and its prerequisites land independently. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 12 ++++++++++++ src/Security/OpenIdConfigurationProvider.php | 6 ++++-- tests/Security/OpenIdConfigurationProviderTest.php | 14 +++++++++++++- 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e405cdb..2e6e7fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -75,6 +75,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. +- `OpenIdConfigurationProvider::getJwtVerificationKeys` declares its + return type as `array` (was just `array`), matching + the actual shape the method builds. Lets `validateIdToken` pass + the cached keys to `JWT::decode` without a `mixed` flow at + `level: max`. +- `OpenIdConfigurationProvider::validateIdToken` narrows its + `$claims` local via inline `@var \stdClass&object{aud, iss, + nonce}` so the spec-required claim accesses + (`$claims->aud` / `$claims->iss` / `$claims->nonce`) type-check at + `level: max`. No runtime change — these values are guaranteed + present and string-typed by the OIDC spec and `firebase/php-jwt` + already enforces JWT validity. ### Documentation diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index 8e89bf6..b57ad06 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -222,6 +222,7 @@ public function validateIdToken(string $idToken, string $nonce): object // 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; + /** @var \stdClass&object{aud: string|array, iss: string, nonce: string} $claims */ $claims = JWT::decode($idToken, $keys); // "aud" may be an array of strings or a single string // (cf. https://openid.net/specs/openid-connect-core-1_0.html#IDToken). @@ -356,8 +357,8 @@ protected function createResourceOwner(array $response, AccessToken $token): Res /** * Get JWT verification keys from Azure Active Directory. * - * @return array - * Array of keys + * @return array + * Array of keys indexed by JWK `kid` * * @throws OpenIdConnectExceptionInterface */ @@ -372,6 +373,7 @@ private function getJwtVerificationKeys(): array $item = $this->cacheItemPool->getItem($cacheKey); if ($item->isHit()) { + /** @var array $keys (we only ever store this shape) */ $keys = (array) $item->get(); } else { $keysUri = $this->getConfiguration('jwks_uri'); diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index d3be35f..30541fb 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -226,6 +226,7 @@ public function testGetBaseAccessTokenUrl(): void public function testValidateIdTokenSuccess(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); @@ -249,6 +250,7 @@ public function testValidateIdTokenSuccess(): void public function testValidateIdTokenFailure(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockJWT->shouldReceive('decode')->andThrow(SignatureInvalidException::class, 'Signature verification failed'); @@ -260,6 +262,7 @@ public function testValidateIdTokenFailure(): void public function testValidateIdTokenAudience(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->aud = 'incorrect aud'; @@ -274,6 +277,7 @@ public function testValidateIdTokenAudience(): void public function testValidateIdTokenIssuer(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->iss = 'incorrect iss'; @@ -288,6 +292,7 @@ public function testValidateIdTokenIssuer(): void public function testValidateIdTokenNonce(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->nonce = 'incorrect nonce'; @@ -429,11 +434,13 @@ public function testCreateResourceOwner(): void $method = new \ReflectionMethod(OpenIdConfigurationProvider::class, 'createResourceOwner'); $owner = $method->invoke($this->provider, ['id' => '123', 'name' => 'Test'], $token); + $this->assertInstanceOf(\League\OAuth2\Client\Provider\ResourceOwnerInterface::class, $owner); $this->assertSame('123', $owner->getId()); } public function testValidateIdTokenArrayAudience(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->aud = [self::CLIENT_ID, 'other_client']; @@ -449,6 +456,7 @@ public function testValidateIdTokenArrayAudience(): void public function testValidateIdTokenArrayAudienceInvalid(): void { + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockClaims->aud = ['wrong_client_1', 'wrong_client_2']; @@ -847,6 +855,7 @@ public function testGetJwtVerificationKeysRejectsNonStringKid(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $this->expectException(JwksException::class); @@ -892,6 +901,7 @@ public function testGetJwtVerificationKeysUnsupportedKeyType(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $this->expectException(JwksException::class); @@ -937,6 +947,7 @@ public function testGetJwtVerificationKeysCacheHit(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $mockClaims = $this->getMockClaims(); $mockJWT->shouldReceive('decode')->andReturn($mockClaims); @@ -1050,6 +1061,7 @@ public function testBase64urlDecodeFailure(): void 'httpClient' => $mockHttpClient, ]); + /** @var \Mockery\MockInterface $mockJWT */ $mockJWT = \Mockery::mock('overload:Firebase\JWT\JWT', MockJWT::class); $this->expectException(\ItkDev\OpenIdConnect\Exception\DecodeException::class); @@ -1070,7 +1082,7 @@ public function testBase64urlDecodeFailure(): void * unreadable / not valid JSON, rather than letting `false` or `null` flow * silently into the assertion under test. * - * @return array + * @return array top-level decoded JSON; callers cast / narrow as needed */ private function loadMockFixture(string $filename): array { From 9ef18603fe0dbb34774db1a4feed22c1d2de21c4 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 12:49:22 +0200 Subject: [PATCH 18/23] Fix markdown-lint MD024: merge duplicate Documentation sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[Unreleased]` had two `### Documentation` subsections — one added in PR #44 for the README "Exception handling" section, one added later for the per-class PHPDoc audit. markdownlint flags duplicate sibling headings (MD024) and the CI step was failing. Merge the README bullet into the per-class section so there's a single `### Documentation` heading. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e6e7fd..768e63b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,6 +90,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Documentation +- Added a new "Exception handling" section to `README.md` describing the + marker interface, the SPL parents of each concrete, the PSR-18 + co-implementation on `HttpException`, and the 4.x → 5.0 catch-block + migration. Also fixed the `validateIdToken` example to catch the + marker interface instead of the now-deprecated abstract. - 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 @@ -135,14 +140,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 class (catch `OpenIdConnectExceptionInterface` instead). Kept through 5.x; removal scheduled for 6.0. -### Documentation - -- Added a new "Exception handling" section to `README.md` describing the - marker interface, the SPL parents of each concrete, the PSR-18 - co-implementation on `HttpException`, and the 4.x → 5.0 catch-block - migration. Also fixed the `validateIdToken` example to catch the - marker interface instead of the now-deprecated abstract. - ### Tooling - PHPStan now scans `tests/` in addition to `src/` at level 8, with From 716bcfaf168decc252f7c700762ba031999126d9 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 12:42:49 +0200 Subject: [PATCH 19/23] Collapse multi-line @param/@return descriptions onto single lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A few docblocks in `OpenIdConfigurationProvider` and the test suite had their descriptions wrapped onto a continuation `*` line: * @param int $length * Length of the random string to be generated * * @return string * The generated state The `phpdoc_align: vertical` rule from the @Symfony preset doesn't create those wraps — it just aligns whatever multi-line structure exists in the source. Flattening each description back onto its `@param` / `@return` line lets the rule pad the columns into a tidy table instead: * @param int $length Length of the random string to be generated * @return string The generated state No `.php-cs-fixer.dist.php` change: the @Symfony default produces the clean form once the source isn't pre-wrapped. Also consolidates two `### Documentation` subsections under `[Unreleased]` (left over from PR #44) into one — markdownlint's MD024 was flagging the duplicate heading. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 14 +++++ src/Security/OpenIdConfigurationProvider.php | 60 +++++++------------ .../OpenIdConfigurationProviderTest.php | 3 +- 3 files changed, 35 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5435cb..7243863 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 became a `function.alreadyNarrowedType` tautology once the shape was in place (PHP itself enforces the `int` via the `setLeeway` signature under `declare(strict_types=1)`). +- Collapsed multi-line `@param` / `@return` descriptions in + `OpenIdConfigurationProvider` and the test suite onto single + lines. `phpdoc_align: vertical` (the @Symfony preset default) + doesn't *create* the wraps — it just aligns whatever multi-line + structure already exists in the source, so a one-time manual + flatten gives the cleaner format and Symfony's vertical + alignment then pads description columns into a tidy table: + + * @param string|null $postLogoutRedirectUri The URL … + * @param string|null $state If a state … + * @param string|null $idToken The id token + + Future docblocks added with everything on one line stay that + way under the same alignment rule. - PHPStan now scans `tests/` in addition to `src/` at level 8, with `reportIgnoresWithoutComments: true` so unexplained `@phpstan-ignore` directives fail CI. diff --git a/src/Security/OpenIdConfigurationProvider.php b/src/Security/OpenIdConfigurationProvider.php index 62adf40..a777a82 100644 --- a/src/Security/OpenIdConfigurationProvider.php +++ b/src/Security/OpenIdConfigurationProvider.php @@ -169,15 +169,11 @@ public function getAuthorizationUrl(array $options = []): string * @see https://docs.microsoft.com/en-us/azure/active-directory-b2c/openid-connect#send-a-sign-out-request * @see https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout * - * @param string|null $postLogoutRedirectUri - * The URL that the user should be redirected to after successful sign out - * @param string|null $state - * If a state parameter is included in the request, the same value should appear in the response. The application should verify that the state values in the request and response are identical. - * @param string|null $idToken - * The id token + * @param string|null $postLogoutRedirectUri The URL that the user should be redirected to after successful sign out + * @param string|null $state If a state parameter is included in the request, the same value should appear in the response. The application should verify that the state values in the request and response are identical. + * @param string|null $idToken The id token * - * @return string - * The Url to redirect the client to for session logout + * @return string The Url to redirect the client to for session logout * * @throws CacheException * @throws HttpException @@ -220,13 +216,10 @@ public function getEndSessionUrl(?string $postLogoutRedirectUri = null, ?string * processes), the leeway value set by the last provider to call validateIdToken() * will apply globally until overwritten. * - * @param string $idToken - * Raw id token - * @param string $nonce - * Nonce + * @param string $idToken Raw id token + * @param string $nonce Nonce * - * @return object - * The JWT's payload as a PHP object + * @return object The JWT's payload as a PHP object * * @throws CacheException * @throws ClaimsException @@ -268,11 +261,9 @@ public function validateIdToken(string $idToken, string $nonce): object /** * Get id token from code. * - * @param string $code - * The code + * @param string $code The code * - * @return string - * The ID token + * @return string The ID token * * @throws OpenIdConnectExceptionInterface */ @@ -310,11 +301,9 @@ public function getIdToken(string $code): string * Generates a new random string to use as the state parameter in an * authorization flow. * - * @param int $length - * Length of the random string to be generated + * @param int $length Length of the random string to be generated * - * @return string - * The generated state + * @return string The generated state */ public function generateState(int $length = 32): string { @@ -327,11 +316,9 @@ public function generateState(int $length = 32): string * Generates a new random string to use as the nonce parameter in an * authorization flow. * - * @param int $length - * Length of the random string to be generated + * @param int $length Length of the random string to be generated * - * @return string - * The generated nonce + * @return string The generated nonce */ public function generateNonce(int $length = 32): string { @@ -379,8 +366,7 @@ protected function createResourceOwner(array $response, AccessToken $token): Res /** * Get JWT verification keys from Azure Active Directory. * - * @return array - * Array of keys indexed by JWK `kid` + * @return array Array of keys indexed by JWK `kid` * * @throws OpenIdConnectExceptionInterface */ @@ -459,8 +445,7 @@ private static function base64urlDecode(string $input): string /** * Fetch remote json resource. * - * @return array - * Json decoded to array + * @return array Json decoded to array * * @throws HttpException * @throws JsonException @@ -490,11 +475,9 @@ private function fetchJsonResource(string $resourceUrl): array /** * Get Configuration option for key. * - * @param string $key - * The configuration key + * @param string $key The configuration key * - * @return string - * The configuration value for the given key + * @return string The configuration value for the given key * * @throws CacheException * @throws HttpException @@ -550,8 +533,7 @@ private function setCacheItemPool(CacheItemPoolInterface $cacheItemPool): void /** * Set the provider cache duration. * - * @param int $cacheDuration - * The cache duration in seconds + * @param int $cacheDuration The cache duration in seconds * * @throws NegativeCacheDurationException */ @@ -566,8 +548,7 @@ private function setCacheDuration(int $cacheDuration): void /** * Set the leeway to allow for clock skew between hosting server and provider. * - * @param int $leeway - * The leeway in seconds. Must be positive + * @param int $leeway The leeway in seconds. Must be positive * * @throws NegativeLeewayException */ @@ -582,8 +563,7 @@ private function setLeeway(int $leeway): void /** * Set allow HTTP. * - * @param bool $allowHttp - * Whether to allow HTTP + * @param bool $allowHttp Whether to allow HTTP */ private function setAllowHttp(bool $allowHttp): void { diff --git a/tests/Security/OpenIdConfigurationProviderTest.php b/tests/Security/OpenIdConfigurationProviderTest.php index 30541fb..9bffc5d 100644 --- a/tests/Security/OpenIdConfigurationProviderTest.php +++ b/tests/Security/OpenIdConfigurationProviderTest.php @@ -1073,8 +1073,7 @@ public function testBase64urlDecodeFailure(): void /** * Get a mock success response with mock date. * - * @return ResponseInterface - * A success ("200") response with mock body data + * @return ResponseInterface A success ("200") response with mock body data */ /** * Load a JSON fixture from tests/MockData and decode it as an associative From ebfc625818b2b914551ee28b578f37d5cf17b74c Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 12:58:36 +0200 Subject: [PATCH 20/23] Bump PHPStan to level: max The 5.0 series of PRs cleared every max-level error in turn: - #43 typed the constructor `$options` / `$collaborators` array shapes (4 errors). - #42 + #44 narrowed JSON payload accesses and introduced `MetadataException` / `JwksException` for malformed IdP payloads (~9 errors). - #45 declared `getJwtVerificationKeys` as `array`, annotated `validateIdToken`'s `$claims` with a `\stdClass & object{aud, iss, nonce}` shape, and added Mockery overload `@var` annotations on every test mock (the remaining ~7 errors). With zero max-level errors remaining, this commit flips the one line in `phpstan.neon`. Future contributions are now analyzed at PHPStan's strictest level; any regression that reintroduces a `mixed` flow will fail CI. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 7 +++++++ phpstan.neon | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7243863..07fd9be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -142,6 +142,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Tooling +- Bumped PHPStan from `level: 8` to `level: max` in `phpstan.neon`. + The preceding PRs in the 5.0 series (constructor option shapes, + JSON-payload narrowing, claim shape on `validateIdToken`, JWKS + validation, test-side Mockery / fixture / claim narrowings) cleared + every max-level error; the bump is the final one-line config + change that locks in the strictest analysis level for future + contributions. - `OpenIdConfigurationProvider::__construct` now declares an `array{cacheItemPool?: CacheItemPoolInterface, openIDConnectMetadataUrl?: string, cacheDuration?: int, leeway?: int, diff --git a/phpstan.neon b/phpstan.neon index 6e3ce04..c39bc0d 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -2,7 +2,7 @@ includes: - vendor/phpstan/phpstan-mockery/extension.neon parameters: - level: 8 + level: max paths: - src - tests From 4076c79b67e3663c6d9d517abc3a44a5bb4ad4ae Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 12 May 2026 13:18:57 +0200 Subject: [PATCH 21/23] Cut 5.0.0 release entry and add UPGRADE-5.0.md migration guide MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roll the Unreleased section into a tagged [5.0.0] entry dated 2026-05-12 and add `UPGRADE-5.0.md` covering the consumer-visible migration: catch the new `OpenIdConnectExceptionInterface` marker instead of the deprecated `ItkOpenIdConnectException` abstract, the constructor's switch to typed `ConfigurationException` (still under `\InvalidArgumentException` — existing SPL catches keep matching), the new typed throws on malformed JWKS / discovery / token payloads, `getIdToken`'s narrowed boundary catch, and the catch-on-SPL semantic change. Tightened the changelog to the net 4.1.2 → 5.0.0 deltas. Items that were ephemeral inside the 5.0 PR series — the `phpstan-baseline.neon` introduced and removed within the cycle, and the `phpdoc_align: vertical` source-style flatten with no config change — are omitted since they aren't visible from the release boundary. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 225 ++++++++++++------------------------------------- UPGRADE-5.0.md | 149 ++++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+), 172 deletions(-) create mode 100644 UPGRADE-5.0.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 07fd9be..cf6cdf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,189 +7,69 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Changed (BREAKING) +## [5.0.0] - 2026-05-12 -- **Exception hierarchy reworked.** Every exception thrown from a public - method now implements - `\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` (new - 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`, `JwksException`, - `CodeException`, `ValidationException`, `ClaimsException`), - `\LogicException` (programmer/config bug — `BadUrlException`, - `IllegalSchemeException`, `MissingParameterException`), - `\InvalidArgumentException` (invalid input — `ConfigurationException`, - `NegativeCacheDurationException`, `NegativeLeewayException`). - Consumers catching `ItkOpenIdConnectException` should migrate to - `OpenIdConnectExceptionInterface`; the abstract class is kept as a - `@deprecated` alias and still implements the marker, but **concrete - exceptions no longer extend it**, so existing `catch - (ItkOpenIdConnectException $e)` blocks will not match anything thrown - by 5.0+ code. -- `OpenIdConfigurationProvider::__construct` now throws - `ConfigurationException` (new, `\InvalidArgumentException`-typed) - instead of a raw `\InvalidArgumentException` when a required option - is missing. The new type implements the marker; existing - `catch (\InvalidArgumentException $e)` blocks continue to match. -- `OpenIdConfigurationProvider::getIdToken` narrowed its boundary - `catch` from `\Exception` to - `IdentityProviderException|ClientExceptionInterface|\JsonException`. - Cache failures during `getConfiguration` (called for the token - endpoint lookup) now propagate as `CacheException` rather than being - 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 `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 - 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`. -- `OpenIdConfigurationProvider::getJwtVerificationKeys` now validates - the JWKS payload at each level before reading values: the top-level - `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 - (`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 - 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. -- 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. -- `OpenIdConfigurationProvider::getJwtVerificationKeys` declares its - return type as `array` (was just `array`), matching - the actual shape the method builds. Lets `validateIdToken` pass - the cached keys to `JWT::decode` without a `mixed` flow at - `level: max`. -- `OpenIdConfigurationProvider::validateIdToken` narrows its - `$claims` local via inline `@var \stdClass&object{aud, iss, - nonce}` so the spec-required claim accesses - (`$claims->aud` / `$claims->iss` / `$claims->nonce`) type-check at - `level: max`. No runtime change — these values are guaranteed - present and string-typed by the OIDC spec and `firebase/php-jwt` - already enforces JWT validity. +Reworked exception hierarchy and tightened IdP-payload validations. The runtime +behaviour is unchanged for spec-compliant IdPs — see [UPGRADE-5.0.md](UPGRADE-5.0.md) +for the consumer migration guide. -### Documentation +### Changed (BREAKING) -- Added a new "Exception handling" section to `README.md` describing the - marker interface, the SPL parents of each concrete, the PSR-18 - co-implementation on `HttpException`, and the 4.x → 5.0 catch-block - migration. Also fixed the `validateIdToken` example to catch the - marker interface instead of the now-deprecated abstract. -- 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). +- Reworked exception hierarchy around the new + `OpenIdConnectExceptionInterface` marker. Concrete exception classes now extend the + SPL type that best describes the failure category (`\RuntimeException`, + `\LogicException`, `\InvalidArgumentException`) instead of the abstract + `ItkOpenIdConnectException`. Existing `catch (ItkOpenIdConnectException $e)` blocks + will not match anything thrown by 5.0+ code — catch the marker, or scope to a more + specific concrete / SPL parent +- Renamed `KeyException` → `JwksException` for symmetry with the new + `MetadataException` and to better describe its scope (the type fires for both + JWKS-document-level and JWK-entry-level errors) +- `OpenIdConfigurationProvider::__construct` now throws the typed + `ConfigurationException` (still extending `\InvalidArgumentException`) instead of + a raw `\InvalidArgumentException` for missing required options +- New typed throws replace 4.x silent coercions: malformed JWKS payload + (missing `keys` array, non-object JWK entry, missing/non-string `kid` / + `kty` / RSA `e` / `n`, unsupported `kty`) → `JwksException`; malformed + OIDC discovery document → `MetadataException`; token endpoint response + missing string `id_token` → `CodeException` +- `OpenIdConfigurationProvider::getIdToken` narrowed its boundary `catch` from + `\Exception` to the three actually-thrown families + (`IdentityProviderException|ClientExceptionInterface|\JsonException`). + Exceptions from the upstream `getConfiguration('token_endpoint')` call + (`CacheException`, `HttpException`, `MetadataException`, library + `JsonException`) now propagate as themselves rather than being re-wrapped + as `CodeException` ### Added -- `ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface` - 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)` - block. Failing this test class is the early warning that the public - contract has drifted. +- Marker interface `OpenIdConnectExceptionInterface` (extends `\Throwable`) +- Concrete exceptions `ConfigurationException` and `MetadataException` +- `tests/Exception/ExceptionHierarchyTest.php` locks the contract: every concrete + implements the marker, extends the correct SPL parent, and is caught by a single + `catch (OpenIdConnectExceptionInterface $e)` block ### Deprecated -- `ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException` abstract - class (catch `OpenIdConnectExceptionInterface` instead). Kept through - 5.x; removal scheduled for 6.0. +- Abstract `ItkOpenIdConnectException` — catch `OpenIdConnectExceptionInterface` + instead. Kept through 5.x as a documented alias that still implements the marker; + removal scheduled for 6.0 + +### Documentation + +- Added an "Exception handling" section to `README.md` covering the marker + interface, SPL parents, PSR-18 co-implementation on `HttpException`, and the + 4.x → 5.0 catch-block migration +- Class-level PHPDoc on every concrete exception describing its trigger sites and + the boundary against related types ### Tooling -- Bumped PHPStan from `level: 8` to `level: max` in `phpstan.neon`. - The preceding PRs in the 5.0 series (constructor option shapes, - JSON-payload narrowing, claim shape on `validateIdToken`, JWKS - validation, test-side Mockery / fixture / claim narrowings) cleared - every max-level error; the bump is the final one-line config - change that locks in the strictest analysis level for future - contributions. -- `OpenIdConfigurationProvider::__construct` now declares an - `array{cacheItemPool?: CacheItemPoolInterface, - openIDConnectMetadataUrl?: string, cacheDuration?: int, leeway?: int, - allowHttp?: bool, ...}` PHPDoc shape for the `$options` parameter - (plus a corresponding shape on `$collaborators` for the `jwt` / - `httpClient` keys). PHPStan can now narrow each setter argument to - the expected type instead of seeing `mixed`. Behaviour unchanged — - removes 4 errors when running PHPStan at `level: max`. Also drops a - now-redundant `is_int($options['leeway'])` runtime check that - became a `function.alreadyNarrowedType` tautology once the shape - was in place (PHP itself enforces the `int` via the `setLeeway` - signature under `declare(strict_types=1)`). -- Collapsed multi-line `@param` / `@return` descriptions in - `OpenIdConfigurationProvider` and the test suite onto single - lines. `phpdoc_align: vertical` (the @Symfony preset default) - doesn't *create* the wraps — it just aligns whatever multi-line - structure already exists in the source, so a one-time manual - flatten gives the cleaner format and Symfony's vertical - alignment then pads description columns into a tidy table: - - * @param string|null $postLogoutRedirectUri The URL … - * @param string|null $state If a state … - * @param string|null $idToken The id token - - Future docblocks added with everything on one line stay that - way under the same alignment rule. -- PHPStan now scans `tests/` in addition to `src/` at level 8, with - `reportIgnoresWithoutComments: true` so unexplained - `@phpstan-ignore` directives fail CI. -- Added `phpstan/phpstan-mockery` to `require-dev` for stubs covering - Mockery's fluent `shouldReceive(...)->andReturn(...)` API. -- Cleaned the 46 pre-existing level-8 issues in `tests/`: dropped the - unused nullable from `$this->provider`, narrowed `validateIdToken` - claim accesses with `object{nonce, aud}` `@var` shapes, replaced - silent `(string)` coercion of `file_get_contents` / `parse_url` - failures with `assertNotFalse` / `assertIsString` boundary guards, - swapped `assertTrue(true)` tautologies for - `expectNotToPerformAssertions`, and replaced the constant-folded - `is_subclass_of` marker check with a `ReflectionClass` lookup so - PHPStan can't fold it into a tautology. `phpstan-baseline.neon` - consequently shrinks to zero and is deleted. +- PHPStan bumped to `level: max` (was 8). Scans `src/` + `tests/` +- `reportIgnoresWithoutComments: true` so unexplained `@phpstan-ignore` directives + fail CI +- Added `phpstan/phpstan-mockery` to `require-dev` for stubs covering Mockery's + fluent `shouldReceive(...)->andReturn(...)` API ## [4.1.2] - 2026-05-11 @@ -347,7 +227,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - This CHANGELOG file to hopefully serve as an evolving example of a standardized open source project CHANGELOG. -[Unreleased]: https://github.com/itk-dev/openid-connect/compare/4.1.2...HEAD +[Unreleased]: https://github.com/itk-dev/openid-connect/compare/5.0.0...HEAD +[5.0.0]: https://github.com/itk-dev/openid-connect/compare/4.1.2...5.0.0 [4.1.2]: https://github.com/itk-dev/openid-connect/compare/4.1.1...4.1.2 [4.1.1]: https://github.com/itk-dev/openid-connect/compare/4.1.0...4.1.1 [4.1.0]: https://github.com/itk-dev/openid-connect/compare/4.0.3...4.1.0 diff --git a/UPGRADE-5.0.md b/UPGRADE-5.0.md new file mode 100644 index 0000000..2ced64e --- /dev/null +++ b/UPGRADE-5.0.md @@ -0,0 +1,149 @@ +# Upgrading from 4.x to 5.0 + +5.0 reworks the exception hierarchy and tightens several IdP-payload +validations. The runtime behaviour is unchanged for spec-compliant IdPs +— this document covers the consumer-visible API changes you'll need to +adjust catch blocks for. + +## Catch the marker interface, not the abstract + +Concrete exception classes no longer extend +`\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException`. Existing +catches against the abstract will not match anything thrown by 5.0+ +code: + +```diff +- } catch (\ItkDev\OpenIdConnect\Exception\ItkOpenIdConnectException $e) { ++ } catch (\ItkDev\OpenIdConnect\Exception\OpenIdConnectExceptionInterface $e) { +``` + +The abstract class is kept through the 5.x line as a `@deprecated` +alias that still implements the marker; removal is scheduled for 6.0. + +A consumer that needs to discriminate by failure category can scope to +the concrete's SPL parent instead — catching `\RuntimeException` +matches every transient/data failure (network, cache, token +validation, claims mismatch); catching `\LogicException` matches the +programmer-error variants (`BadUrlException`, +`IllegalSchemeException`, `MissingParameterException`); catching +`\InvalidArgumentException` matches the invalid-constructor-input +variants (`ConfigurationException`, `NegativeCacheDurationException`, +`NegativeLeewayException`). + +## Catch on `\InvalidArgumentException` still works for constructor errors + +`OpenIdConfigurationProvider::__construct` previously threw raw +`\InvalidArgumentException` for missing required options. It now +throws the typed `ConfigurationException`, which still **extends** +`\InvalidArgumentException`. Existing catches at the SPL level +continue to match without any change: + +```php +try { + new OpenIdConfigurationProvider([]); +} catch (\InvalidArgumentException $e) { // still catches in 5.0 + // ... +} +``` + +## New typed throws where 4.x silently coerced + +Three sites that previously cast malformed IdP-returned values to +strings now throw a typed exception: + +- **Malformed JWKS payload (`keys` array missing, JWK entry without + string `kid` / `kty` / `e` / `n`)** → `JwksException`. 4.x silently + built a degraded `Key` from whatever the (string) cast produced, or + tripped a downstream type error in `XMLSecurityKey::convertRSA`. +- **OIDC discovery doc with missing / non-string required key** → + `MetadataException` (was bubbled as `CacheException`, semantically + misleading — the failure is the IdP-returned payload not + conforming to the OIDC Discovery spec, not the cache layer + misbehaving). +- **Token endpoint response missing string `id_token`** → + `CodeException` (was a return of `mixed` that produced confusing + errors at the call site). + +If you've been catching `CacheException` specifically for the +missing-config-key case, you'll need to widen to the marker interface +or to `MetadataException`: + +```diff + try { + $provider->getBaseAuthorizationUrl(); +- } catch (\ItkDev\OpenIdConnect\Exception\CacheException $e) { ++ } catch (\ItkDev\OpenIdConnect\Exception\MetadataException $e) { + // Discovery document is malformed + } +``` + +`CacheException` still fires for PSR-6 cache-layer failures (its +strictly-cache-only meaning in 5.0). + +## `getIdToken`'s `@throws` no longer advertises `ClientExceptionInterface` + +The previous `@throws ClientExceptionInterface` declaration on +`getIdToken` documented a dead-code path — the body's catch-all +wrapped the transport exception into `CodeException` before it could +surface. The declaration was removed in 4.1.2 already; the 5.0 boundary +catch is now narrowed to +`IdentityProviderException|ClientExceptionInterface|\JsonException` +explicitly, but the public type returned to the caller remains +`CodeException` (with the cause chained via `$previous`). + +If you were catching `ClientExceptionInterface` after a `getIdToken` +call to handle transport failures specifically, switch to catching +`CodeException` and inspect `$e->getPrevious()` for the original +PSR-18 exception: + +```diff + try { + $idToken = $provider->getIdToken($code); +- } catch (\Psr\Http\Client\ClientExceptionInterface $e) { +- // transport failure ++ } catch (\ItkDev\OpenIdConnect\Exception\CodeException $e) { ++ $transport = $e->getPrevious(); ++ if ($transport instanceof \Psr\Http\Client\ClientExceptionInterface) { ++ // transport failure ++ } + } +``` + +(`HttpException` — fired by the *discovery* / JWKS fetches, not the +token-exchange POST — still implements `ClientExceptionInterface` as +part of its public contract.) + +## Catch-on-SPL semantic change + +The re-parenting onto SPL types means catches on `\RuntimeException`, +`\LogicException`, or `\InvalidArgumentException` now match OIDC +failures where they previously did not. Concretely: a generic retry +decorator wrapping `validateIdToken` with `catch (\RuntimeException +$e)` will now retry on `CacheException`, `HttpException`, +`ValidationException`, `ClaimsException`, etc. + +This is the intended semantic — `\RuntimeException` represents +"transient or data-shape failure," which is exactly what those +concrete types now signal. If your wrapping logic relies on the old +behaviour, narrow the catch to a more specific bundle / library +exception. + +## Per-class summary + +| Concrete | Parent | When it fires | +| --- | --- | --- | +| `BadUrlException` | `\LogicException` | `openIDConnectMetadataUrl` syntax invalid | +| `IllegalSchemeException` | `\LogicException` | http scheme without `allowHttp: true` | +| `MissingParameterException` | `\LogicException` | `getAuthorizationUrl()` missing required `state` / `nonce` | +| `ConfigurationException` | `\InvalidArgumentException` | Constructor missing `cacheItemPool` / `openIDConnectMetadataUrl` | +| `NegativeCacheDurationException` | `\InvalidArgumentException` | `cacheDuration` < 0 | +| `NegativeLeewayException` | `\InvalidArgumentException` | `leeway` < 0 | +| `CacheException` | `\RuntimeException` | PSR-6 cache layer failed | +| `HttpException` (+ `ClientExceptionInterface`) | `\RuntimeException` | HTTP transport failure for discovery / JWKS | +| `JsonException` | `\RuntimeException` | `json_decode` failed on an IdP response body | +| `DecodeException` | `\RuntimeException` | JWK base64 decode failed | +| `JwksException` | `\RuntimeException` | JWKS payload doesn't conform to RFC 7517 | +| `CodeException` | `\RuntimeException` | Token endpoint exchange failed | +| `ValidationException` | `\RuntimeException` | JWT signature / decode failed | +| `ClaimsException` | `\RuntimeException` | JWT claim values wrong (aud / iss / nonce) | +| `MetadataException` | `\RuntimeException` | OIDC discovery document doesn't conform to the spec | From e9f50e6931f9de7203ddbcd9ea700c32ed8bf6d6 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 2 Jun 2026 10:57:06 +0200 Subject: [PATCH 22/23] Set 5.0.0 release date to 2026-06-02 Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf6cdf1..6b50b6e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -## [5.0.0] - 2026-05-12 +## [5.0.0] - 2026-06-02 Reworked exception hierarchy and tightened IdP-payload validations. The runtime behaviour is unchanged for spec-compliant IdPs — see [UPGRADE-5.0.md](UPGRADE-5.0.md) From 043734ec10a0a5567908b35f89cfff1c233ad5b0 Mon Sep 17 00:00:00 2001 From: turegjorup Date: Tue, 2 Jun 2026 11:00:26 +0200 Subject: [PATCH 23/23] Install dependencies before composer audit in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `composer audit` runs in a fresh `docker compose run --rm phpfpm` container with no `vendor/` present, and this is a library so `composer.lock` isn't committed. A newer Composer in the phpfpm image now errors ("No installed packages found …") instead of silently passing, so the audit job started failing. Run `composer install` first, mirroring the composer-normalized job. This diverges from the shared devops_itkdev-docker template; the same fix should be upstreamed there. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/composer.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/composer.yaml b/.github/workflows/composer.yaml index f807f83..e2a1411 100644 --- a/.github/workflows/composer.yaml +++ b/.github/workflows/composer.yaml @@ -77,4 +77,5 @@ jobs: docker network create frontend - run: | + docker compose run --rm phpfpm composer install docker compose run --rm phpfpm composer audit