Release 5.1.0 - #65
Merged
Merged
Conversation
…d reporting Baseline run: 198 mutants, 141 killed, Covered Code MSI 71% against 100% line coverage; full run completes in ~14 seconds. The minimum score (minCoveredMsi: 68) lives in infection.json5 only, shared by task test:mutation, pr:actions and the CI job. CI annotates escaped mutants inline on PRs via --logger-github and publishes develop results to the Stryker dashboard, feeding the README badge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test: add Infection mutation testing with enforced score and dashboard reporting
Mutation testing showed the exception contract was only half-verified: expectExceptionMessage() matched static prefixes, so every dynamic message part (offending URL, audience list, issuer, nonce, input) could be dropped or reordered undetected, and the 0 code passed at each wrap boundary was never asserted. Wrap-boundary tests now check the full message, code 0 and the chained previous cause, and a new test covers invalid JSON from the token endpoint. Kills 33 of 57 escaped mutants (71% -> 87% covered MSI). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mutation testing showed the cache write path was executed but never verified: the set/expiresAfter/save calls for both the discovery document and the JWKS key map could be removed without a failing test (silently disabling caching entirely), the namespaced cache key was unasserted, and a multi-key JWKS could be truncated to its first entry unnoticed. Kills 8 escaped mutants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mutation testing showed the constructor's setRequestFactory call could be removed (silently falling back to the parent's default factory) and the < 0 guards on cacheDuration/leeway could become <= 0 (rejecting the valid boundary value 0) without a failing test. Kills 3 escaped mutants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ption-assertions # Conflicts: # CHANGELOG.md
test: assert full exception messages, codes and previous-chains
…e-behavior # Conflicts: # CHANGELOG.md
test: assert discovery document and JWKS caching behavior
…tructor-boundaries # Conflicts: # CHANGELOG.md
test: pin request factory wiring and zero-boundary option values
provider.example.org for IdP-side URLs (metadata), app.example.org for application-side URLs (redirect URI); some.url and redirect.url are not reserved names. The post-logout URL already uses the reserved .test TLD and the Azure B2C fixture endpoints are left as realistic shape examples. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The suite kills 185 of 198 mutants (93% covered MSI); the 13 survivors are equivalent or contrived. The bootstrap threshold of 68 no longer defends anything; 90 keeps headroom for run-to-run variance. The Unreleased changelog entries from the mutation-testing rollout are condensed to the final outcome, matching the tone of earlier releases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test: use RFC 2606 reserved domains in fixtures and README example
…ed-msi # Conflicts: # CHANGELOG.md
ci: raise minimum mutation score to 90 and condense changelog entries
`allowHttp` was enforced only in `setOpenIDConnectMetadataUrl()`. The `authorization_endpoint`, `token_endpoint`, `userinfo_endpoint`, `end_session_endpoint` and `jwks_uri` read from the discovery document were used verbatim, so a tampered or misconfigured document announcing `http://…/token` got the client secret posted in plaintext during the code exchange. Route every endpoint read through a new private `getSecureEndpoint()`, and share the scheme policy with the metadata URL check via `assertSecureUrl()`. Scheme comparison is now case-insensitive per RFC 3986 §3.1, so `HTTPS://` is accepted where it was previously rejected. `issuer` keeps the plain accessor — it is compared as a string, never dereferenced as a URL.
feat: enforce https on discovered OIDC endpoints
Infection reported 13 escaped mutants. Because --logger-github annotates every escaped mutant in src/ with no notion of what a PR changed, all 13 surfaced as annotations on every pull request, and the 90 threshold against a 93% score left room for ~7 new escapees to land unnoticed. Six are gone through real changes: - generateState()/generateNonce() are now tested without an explicit argument, covering the 32-character default that sets state and nonce entropy. Every existing test passed 32 explicitly, so the default was never exercised. - The (array) casts on cached discovery and JWKS payloads are now covered by tests feeding an object, which is what a PSR-6 pool can return after a round-trip. Without the cast the discovery path raises "Cannot use object of type stdClass as array" and the JWKS path a TypeError — bare \Error values that are not part of the library's exception contract. - getIdToken() no longer catches IdentityProviderException. The method issues the token request directly instead of through league's getParsedResponse(), so checkResponse() is never on this path and the arm was unreachable. - getAuthorizationUrl() no longer sets a 'scope' default, which league's getAuthorizationParameters() already backfills from getDefaultScopes() with the same value. The remaining seven are equivalent — the mutated code cannot produce an observable difference — and are excluded per mutator in infection.json5 with the reason stated: json_decode()'s depth argument (511 and 513 need a payload nested exactly 512 deep), the increment on the random-state length (getRandomState halves it for bin2hex, so 33 yields the same 32 characters as 32), and the (string) cast on a status code that Exception coerces anyway. Score is now 100% with the threshold at 100, so the gate is binding. The infection/infection constraint moves to ^0.35.2 in the same change, so the threshold is verified against the current mutator set rather than one two minors behind; 0.35.2 generates the same mutants and needs no config changes.
test: kill escaped mutants and make the mutation gate binding
PHPStan ran on one PHP version against one dependency set, so it spoke for neither end of what composer.json declares. - phpstan.neon pins the analysis to the declared php ^8.3 range (8.3-8.5) instead of assuming whichever PHP the job runs on. Analysing on 8.3 said nothing about 8.5, and analysing on 8.5 would accept syntax that breaks 8.3 consumers. - The phpstan job moves to the phpfpm85 service, so the ceiling analysis resolves the newest installable dependency set. league/oauth2-client declares a PHP upper bound, so which of its releases Composer can install depends on the runtime version. - A phpstan-lowest job, and task analyze:php:lowest, analyse the dependency floor. Only the packages `require` names are lowered: a plain --prefer-lowest drags PHPUnit and phpstan-mockery down too, and then most of what is reported is an artefact of that rather than a statement about the runtime dependencies. The task restores the ceiling on a deferred step, so it leaves the container as it found it. Unit tests already covered both dependency sets across 8.3/8.4/8.5, so this closes the analysis axis only. Running the floor showed league/oauth2-client resolving to 2.6.0, which predates PKCE (2.7.0) and league's own Guzzle security floor. Two floors go up as a result: - league/oauth2-client to ^2.8.1. PKCE arrived in 2.7.0, and 2.8.1 raises league's Guzzle constraint to ^6.5.8 || ^7.4.5 for the advisories affecting earlier releases. - robrichards/xmlseclibs to ^4.0. It requires php >= 8.0, which php ^8.3 satisfies, and swaps its ext-openssl requirement for phpseclib/phpseclib ^3.0. XMLSecurityKey::convertRSA() — the only API used here — is unchanged. Analysis and tests are clean at both floors and the ceiling.
xmlseclibs 4.0 answers an empty modulus or exponent with a bare \Exception, which escapes validateIdToken() without implementing OpenIdConnectExceptionInterface — the contract violation CLAUDE.md rules out. The existing is_string() guard does not stop it: the value is a string, and base64_decode() accepts it. The check therefore sits after the decode rather than before it, because "", " " and "\n" all base64-decode to zero bytes; guarding the raw value would catch only the first. On 3.1.5 the same input threw nothing and silently built a key from an empty modulus, which then failed signature verification further along. So this is not a regression the version bump introduced so much as one it made visible, and the fix is an improvement over both versions.
…floors ci: analyse the PHP range and the dependency floor, and raise the floors
XMLSecurityKey::convertRSA() was the only thing this library used from robrichards/xmlseclibs, and firebase/php-jwt — already a direct dependency, and the library that verifies the signature — does the same conversion in JWK::parseKey(). Dropping xmlseclibs also drops the phpseclib subtree its 4.0 release introduced. JWK::parseKeySet() was the obvious candidate but is laxer than the validation 5.0.0 introduced: it accepts a non-string, empty or undecodable exponent, coerces a non-string kid, treats an unsupported kty as a key to skip, and raises a bare \TypeError on a non-object entry. Adopting it would have undone that work, so the existing guards stay in front and only the key construction is delegated. The cache changes shape as a consequence. JWK::parseKey() returns keys wrapping an OpenSSLAsymmetricKey, which PHP refuses to serialize, so the built keys cannot go into a PSR-6 pool. getJwksDocument() caches the fetched JWKS document instead and parsing happens per call — microseconds, and the network fetch is still cached. The cache key changes from `…||jwks` to `…||jwks-document` so entries written by 5.0, which hold serialized Key objects, are never read back as a document. Two test-visible consequences: - The suite overload-mocks Firebase\JWT\JWT to stub decode(), which also took out JWT::urlsafeB64Decode() — called by JWK::parseKey() in production. A single overloadJwt() helper now sets up both, replacing twenty hand-rolled overloads. - A JWKS entry carrying a private key clears every guard and is refused by parseKey() instead, so there is now a test for the wrap-and-chain behaviour on that path.
refactor: build JWKS keys with firebase/php-jwt, drop xmlseclibs
Four changes to validateIdToken(), all in the same seam. Require "exp" and "iat" (OIDC Core §2 makes both REQUIRED). firebase/php-jwt validates "exp" only when it is present, so a token omitting it never expired. Forged tokens still die at the signature check, so exploiting this needs a misbehaving IdP — but the deadline should not be optional. Compare the nonce with hash_equals(). It is the only claim checked against a value the caller holds, so a timing signal there leaks that secret rather than a public identifier. Compare the audience strictly. PHP's loose comparison treats numeric strings as equal by value, so an audience of "1e2" satisfied a client id of "100". Non-string audience entries are filtered out: they cannot match a string client id, and interpolating them into the exception message rendered an "Array to string conversion" instead of naming the audiences. Require "iss" and "nonce" to be non-empty strings. Both were interpolated into exception messages unchecked, so a signed token carrying an array in either claim turned a claims mismatch into a bare \Error that does not implement OpenIdConnectExceptionInterface. Two new private helpers, requireNumericClaim() and requireStringClaim(), hold the checks; the latter returns the narrowed value so callers cannot re-read the untyped property. The last two go beyond the planned exp/iat and hash_equals work, but they are the same defect class in the same function: a claim whose type was assumed rather than checked.
feat: harden ID token claim validation
Both documents were decoded and cached with no size limit, so a hostile or misconfigured endpoint could hand over an unbounded body. Both are a few kilobytes in practice, so a mebibyte is generous. The declared body size is checked first where the response reports one, and the retrieved content unconditionally, because a chunked response reports no size. Both resources go through fetchJsonResource(), so one cap covers them. The cap bounds what gets decoded and written to the cache, not peak memory: Guzzle has already buffered the body by the time it is visible here. Bounding the transfer itself would need a streaming read against a `stream => true` request, which is a larger change than the exposure warrants for a document fetched from a configured host over TLS. That limitation is stated in the method docblock rather than left implied. The boundary is tested from both sides — exactly at the limit is accepted, one byte over is refused — for the declared size and for the content, which is also what pins the constant against a mutation off by one.
feat: cap the discovery document and JWKS at 1 MiB
firebase/php-jwt exposes leeway only as JWT::$leeway, a process-global static with no per-call alternative. The write already sat immediately before the decode, but only a comment said it had to, and nothing stopped a future edit from putting a cache read or an HTTP call between them. decodeWithLeeway() now does nothing but set the static and decode, so the invariant has a single home and a stated rationale: no suspension point may come between the two, which is why the verification keys are resolved by the caller before it is entered. Under PHP-FPM that ordering is a formality; under cooperative concurrency it is what stops a fibre decoding with a sibling provider's leeway, and under a preemptive model it would not be enough at all. Behaviour is unchanged. The test suite already carried MockJWT::$leeway for this and never asserted it, so there is now a test pinning the configured leeway to what reaches the decode. The extraction also needed the return type spelled as \stdClass rather than object: `object` erased the narrowing JWT::decode() provides and PHPStan lost $claims->aud.
Setting the process-global is unavoidable — firebase/php-jwt offers no per-call leeway — but leaving it set is a choice. Under PHP-FPM the mutated static dies with the request. In a worker process it persists for the life of the process and silently applies to every other firebase/php-jwt consumer in it that never sets its own leeway, which makes this library a bad neighbour in exactly the runtime it otherwise suits. The restore is in a finally block, so a rejected token cannot leave our leeway applied either. Both paths are tested: the value in effect during the decode, and the value left behind after a success and after a SignatureInvalidException.
refactor: contain the JWT::$leeway static, and restore it after each decode
generateState() writes league's $this->state, so the inherited getState() returns it. Where the provider is built per request that is redundant but harmless. On an instance shared between requests — a long-running worker, or a container that memoizes the service — the property holds whichever request wrote it last, which may belong to a different user. The remedy cannot be code here: getAuthorizationUrl() writes the same property via league's getAuthorizationParameters() whether or not generateState() is called, and getState() is inherited public API. So the guidance is to never read the state back off the provider, and the contract worth naming is the one the shipped flow already follows — the caller persists state and nonce in the session and compares against its own copy. Documented in README.md next to the code that does it, and on both generator methods. generateNonce() gets the contrast: it stores nothing, which is the shape the state should be treated as having.
docs: name the session-carried contract for state and nonce
getAuthorizationUrl() defaults to response_type=id_token with response_mode=query — the OIDC implicit flow, with the ID token delivered in the query string. Callers should pass response_type=code and exchange via getIdToken(), which is what openid-connect-bundle already does. Two accurate reasons, since the obvious citation does not apply. OIDC Core §3.2.2.5 returns implicit-flow parameters in the fragment, which never reaches the server — so the query response mode exists to make the token readable server-side, relying on a provider extension rather than anything the spec describes. And it puts a credential in the query string, where web server access logs and browser history keep it. RFC 9700 §2.1.2 recommends code over response types that return tokens in the authorization response, but its normative sentence names access tokens and offers `code id_token` as acceptable, so it does not literally cover a bare id_token response. The README says so rather than overstating it. Documentation only. The default becomes code in 6.0; no runtime deprecation notice is emitted, because it would fire on this library's own default path and consumers could only silence it by making the change the notice asks for — which the changelog and README ask for directly instead.
…type docs: deprecate the id_token response_type default
PKCE is opt-in with no configuration flag: passing a code_challenge to getAuthorizationUrl() turns it on, omitting it changes nothing. That keeps existing consumers untouched — a default-on switch would have broken them, since RFC 7636 §4.6 requires code_verifier at the token endpoint once a challenge has been accepted, and no released consumer stores one yet. code_challenge_method=S256 is filled in whenever a challenge is present. Omitting it makes the server assume "plain" (RFC 7636 §4.3), which would quietly turn an S256 challenge into a secret sent in the clear, so it is not left to the caller to remember. Deliberately not built on league's PKCE support. Overriding getPkceMethod() is what makes league generate the verifier and hold it on $this->pkceCode; on a provider instance shared between requests — a long-running worker, or a container that memoizes the service — that lets one request's verifier be sent for another request's token exchange. This is the same hazard class as reading getState() back, and the fix is the same: the caller carries the value. generatePkceVerifier() mirrors generateNonce() in storing nothing, getPkceChallenge() is a pure function of its argument, and getIdToken() takes the verifier as an explicit parameter. The verifier length is not configurable. RFC 7636 §4.1 caps it at 128 characters and requires servers to accept the full range, so there is no reason to ask for less entropy — and 96 random bytes base64url-encode to exactly 128 characters, so no truncation is involved. Tested against the RFC 7636 Appendix B verifier/challenge vector, plus that the verifier never appears in the authorization URL, that an explicit challenge method is respected, and that code_verifier is sent only when the caller supplies one. The README gains the first code-flow example it has had; it previously documented only the implicit flow.
feat: add S256 PKCE, carried by the caller rather than the provider
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 71 93 +22
===========================================
Files 1 1
Lines 185 235 +50
===========================================
+ Hits 185 235 +50
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Roll the accumulated Unreleased entries into a dated 5.1.0 section and add the compare link. The entries are rewritten to state the release as a diff against 5.0.0: what was added, changed, deprecated, removed and fixed, without the intermediate steps that got there. Subsections follow Keep a Changelog's order. Rationale lives in the commit messages and pull requests. No version constant to bump: the library carries none, and composer.json declares no version. No upgrade guide, since nothing here is breaking.
turegjorup
force-pushed
the
release/5.1.0
branch
from
August 26, 2026 11:44
7adde73 to
49c7502
Compare
Verified every task name and code claim against the Taskfile and the source. - `task test` was documented under Unit Testing but reports no executed tests: phpunit.xml.dist declares coverage reports, so the suite needs XDEBUG_MODE=coverage. Documents `task test:coverage` and says why. - `task analyze` does not exist; the task is `analyze:php`. Documents that, plus `analyze:php:lowest` and what the two axes cover. - `task pr:actions` was described as coding standards, static analysis and tests; it also runs composer validation, the floor analysis, the test matrix and mutation testing. - The mutation section named only `minCoveredMsi`; `minMsi` is set too, both at 100, and per-mutator exclusions carry their reasons. - The verification example demonstrated the deprecated implicit flow as the only complete example. It now shows the authorization code flow — getIdToken() then validateIdToken() — with the implicit variant kept below as a two-line note. The state comparison in that example uses hash_equals() and a type guard, matching what the library now does for the nonce. - References led with the Implicit Client Implementer's Guide alone. Adds Core and the Basic (code flow) guide, and marks the implicit one as the current deprecated default. - The configuration example carried `https:/...` with one slash, which the scheme validation would now reject, and passed a string where a CacheItemPoolInterface instance is required. - `firebase/jwt` corrected to `firebase/php-jwt`, and its repository link updated to the googleapis organisation it moved to.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security hardening throughout, with no breaking API changes.
What is in it
allowHttpwas enforced only onopenIDConnectMetadataUrl; theauthorization_endpoint,token_endpoint,userinfo_endpoint,end_session_endpointandjwks_uriwere used verbatim, so a tampered or misconfigured document could get the client secret posted in plaintext during the code exchange.expandiatare now required (OIDC Core §2 makes both mandatory, andfirebase/php-jwtonly validatesexpwhen present — so a token without it never expired). The nonce is compared withhash_equals(), the audience strictly, andiss/noncemust be non-empty strings.code_challengeturns it on. The verifier is carried by the caller in the session, never on the provider.robrichards/xmlseclibsis gone, along with thephpseclibsubtree its 4.0 release introduced. JWKS keys are built byfirebase/php-jwt's ownJWK::parseKey(), which was already a dependency.JWT::$leewayis written in one place and restored afterwards, so this library no longer leaves its leeway applied to otherfirebase/php-jwtconsumers in a long-lived process.Three things to know when upgrading
None of these is an API break, but each can change what a running deployment sees:
allowHttp— a local Keycloak without TLS, for instance. That has always been the documented switch for exactly this; it simply now governs the discovered endpoints too.exporiatis rejected. Spec-compliant providers are unaffected.…||jwksto…||jwks-document, because the cache now holds the JWKS document rather than the parsedKeyobjects. Entries written by 5.0 are left alone and expire on their own; the first request after deploying re-fetches the JWKS.There is no
UPGRADE-5.1.md: nothing here requires a code change from consumers.Also worth noting
league/oauth2-clientfloors at^2.8.1(was^2.6), which is where league raised its own Guzzle constraint to^6.5.8 || ^7.4.5for the advisories affecting earlier releases.CI gained a dependency-floor PHPStan job and now analyses the whole declared PHP range rather than whichever version the job runs on. The mutation-score threshold is 100 and binding, so a newly escaped mutant fails the build rather than being absorbed by headroom.
Verification
143 tests green across PHP 8.3/8.4/8.5 on both stable and lowest dependency resolutions. Coverage 100% (34/34 methods, 202/202 lines). Mutation score 100% with zero escaped mutants. PHPStan max clean at the dependency ceiling and floor; php-cs-fixer, markdownlint, prettier,
composer validate,composer normalizeandcomposer auditall clean.Changelog only in this PR — no source changes.