From 943f828af519767a057e52ed04893f58984a96a0 Mon Sep 17 00:00:00 2001 From: Joe Huss Date: Sun, 28 Jun 2026 12:24:04 -0400 Subject: [PATCH] JwtClaims: strict-aud variant + verification/round-trip docs (S4+B4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit S4 — audience-default hardening + verification docs: - Add `fromPayloadStrict(array): self` that THROWS when `aud` is absent, instead of defaulting to AUD_SERVER. `fromPayload()` keeps the v0.10.x legacy default (existing tests/behaviour unchanged) — BC-safe. - Add a prominent class docblock stating JwtClaims performs NO signature verification: it is a typed view over an already-decoded/verified payload; signature verification is the caller's responsibility (server/hub JwtHandler). B4 — round-trip symmetry note (no behaviour change): - Document the deliberate `toPayload()` null/empty-optional omission asymmetry (wire-compat for legacy decoders) and why the object round-trip stays lossless. - Add round-trip object-equality tests asserting fromPayload(toPayload($claims)) == $claims for both the all-fields and minimal-claims cases. Refactor fromPayload/fromPayloadStrict to share a private build() helper. CHANGELOG [Unreleased] updated. No Version bump (batch-final release step). Gate: PHPUnit 349 tests / 2838 assertions OK; PHPStan L9 (no baseline) no errors; phpcs PSR-12 clean; Psalm no errors. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 18 +++++++ src/Auth/JwtClaims.php | 99 ++++++++++++++++++++++++++++++++---- tests/Auth/JwtClaimsTest.php | 86 +++++++++++++++++++++++++++++++ 3 files changed, 192 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bcc6944..4e54398 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,24 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm unchanged. **Consumer follow-up (phlix-server `RelayConsumer::buildRequest()`):** stop trusting `x-phlix-relay-user` from the envelope and strip forbidden headers via `isForbiddenHeader()` — that paired PR is the actual auth-bypass fix. +- **`Auth\JwtClaims::fromPayloadStrict(array): self`** (finding S4) — strict variant + of `fromPayload()` that **throws** `InvalidArgumentException` when the `aud` claim is + absent, instead of silently defaulting it to `AUD_SERVER`. `fromPayload()` keeps the + v0.10.x backward-compat default for legacy tokens (existing behaviour unchanged); + consumers can migrate to the strict variant once every issuer emits `aud`. Additive, + BC-safe. + +### Documentation +- **`Auth\JwtClaims` security & round-trip docs** (findings S4/B4): + - Class docblock now prominently states `JwtClaims` performs **no signature + verification** — it is a typed view over an already-decoded/verified payload; + verifying the JWT signature (and rejecting `alg: none`) is the caller's + responsibility (server/hub `JwtHandler`). + - Documents the deliberate `toPayload()` asymmetry: null/empty optionals + (`nbf`/`jti`/`scope`/`serverId`) are omitted from the array for legacy-decoder + wire-compat, yet `fromPayload(toPayload($claims)) == $claims` remains lossless + because `fromPayload()` re-applies the defaults. New round-trip object-equality + tests cover both the all-fields and minimal-claims cases. No behaviour change. ## [0.10.1] - 2026-06-23 diff --git a/src/Auth/JwtClaims.php b/src/Auth/JwtClaims.php index 0c5b20c..3c46c25 100644 --- a/src/Auth/JwtClaims.php +++ b/src/Auth/JwtClaims.php @@ -17,6 +17,38 @@ * decoded payload into this DTO so server and hub share one definition * of "what's in a Phlix JWT". * + * ## SECURITY: this class performs NO signature verification. + * + * `JwtClaims` is purely a typed *view* over an array payload that has + * **already been decoded and cryptographically verified** by the caller. + * Neither {@see self::fromPayload()} nor {@see self::fromPayloadStrict()} + * checks the JWT signature, the `alg` header, key identity, or any HMAC — + * they only validate field presence and PHP types. Verifying the token + * signature (and rejecting `alg: none`) is the **caller's responsibility**: + * server and hub do this in their respective `JwtHandler` before passing + * the decoded payload here. Constructing a `JwtClaims` from an unverified + * payload conveys **no** authenticity guarantee. + * + * ## Audience (`aud`) handling + * + * {@see self::fromPayload()} defaults a missing `aud` to {@see self::AUD_SERVER} + * as a deliberate v0.10.x backward-compat shim for legacy tokens minted by + * `JwtHandler` before the field existed. Once every issuer emits `aud`, + * consumers should migrate to {@see self::fromPayloadStrict()}, which throws + * on a missing `aud` instead of defaulting. + * + * ## Round-trip / `toPayload()` asymmetry (deliberate) + * + * {@see self::toPayload()} intentionally **omits** null/empty optional claims + * (`nbf`, `jti`, `scope`, `serverId`) from its output so that legacy decoders + * predating those fields never see unexpected keys — this is a wire-compat + * requirement and must NOT change. The serialization is therefore + * asymmetric at the array level (a minimal claim set produces fewer keys than + * the full constructor), yet `fromPayload(toPayload($claims)) == $claims` + * still holds: `fromPayload()` re-applies the same null/empty defaults for the + * omitted keys, so the object round-trip is lossless. See the round-trip tests + * in `JwtClaimsTest` for both the all-fields and minimal-claims cases. + * * @package Phlix\Shared\Auth * @since 0.2.0 */ @@ -60,8 +92,12 @@ public function __construct( * Build from the array shape `JwtHandler::validateToken()` returns today. * * Tolerant of missing optional fields (`nbf`, `jti`, `scope`, - * `serverId`). Throws {@see InvalidArgumentException} when any of - * the RFC 7519 required fields is missing or has the wrong type. + * `serverId`) and of a missing `aud` (defaulted to {@see self::AUD_SERVER} + * for legacy tokens — see the class docblock). Throws + * {@see InvalidArgumentException} when any of the RFC 7519 required fields + * is missing or has the wrong type. + * + * Performs **no** signature verification — see the class docblock. * * @param array $payload Decoded token payload. * @@ -69,16 +105,11 @@ public function __construct( */ public static function fromPayload(array $payload): self { - $iss = self::requireString($payload, 'iss'); - $sub = self::requireString($payload, 'sub'); - $iat = self::requireInt($payload, 'iat'); - $exp = self::requireInt($payload, 'exp'); - $type = self::requireString($payload, 'type'); - // `aud` is required per the shared shape; default to AUD_SERVER // for tokens emitted by the legacy `JwtHandler` that pre-date // the field. This keeps `fromPayload()` tolerant of v0.10.x - // payloads but still surfaces wrong-typed values. + // payloads but still surfaces wrong-typed values. Migrate to + // fromPayloadStrict() once every issuer emits `aud`. $aud = self::AUD_SERVER; if (array_key_exists('aud', $payload)) { if (!is_string($payload['aud'])) { @@ -87,6 +118,47 @@ public static function fromPayload(array $payload): self $aud = $payload['aud']; } + return self::build($payload, $aud); + } + + /** + * Strict variant of {@see self::fromPayload()} that requires an explicit + * `aud` claim and throws when it is absent, instead of defaulting it to + * {@see self::AUD_SERVER}. + * + * Use this once all token issuers (server and hub `JwtHandler`) emit `aud` + * so a missing audience is treated as a malformed token rather than + * silently accepted as a legacy server-audience token. + * + * Performs **no** signature verification — see the class docblock. + * + * @param array $payload Decoded token payload. + * + * @throws InvalidArgumentException When a required field (including `aud`) + * is missing or wrong-typed. + */ + public static function fromPayloadStrict(array $payload): self + { + $aud = self::requireString($payload, 'aud'); + + return self::build($payload, $aud); + } + + /** + * Build the DTO from a payload using a pre-resolved `aud`. + * + * @param array $payload Decoded token payload. + * + * @throws InvalidArgumentException When a required field is missing or wrong-typed. + */ + private static function build(array $payload, string $aud): self + { + $iss = self::requireString($payload, 'iss'); + $sub = self::requireString($payload, 'sub'); + $iat = self::requireInt($payload, 'iat'); + $exp = self::requireInt($payload, 'exp'); + $type = self::requireString($payload, 'type'); + $nbf = null; if (array_key_exists('nbf', $payload) && $payload['nbf'] !== null) { if (!is_int($payload['nbf'])) { @@ -141,8 +213,13 @@ public static function fromPayload(array $payload): self /** * Serialize to the array shape `JwtHandler::encode()` expects today. * - * Optional fields are omitted when null/empty so v0.10.x decoders - * that pre-date them don't see unexpected keys. + * Optional fields (`nbf`, `jti`, `scope`, `serverId`) are **deliberately + * omitted** when null/empty so v0.10.x decoders that pre-date them don't + * see unexpected keys (wire-compat — must not change). This makes the + * array output asymmetric versus the constructor, but the object + * round-trip remains lossless: `fromPayload(toPayload($claims)) == $claims` + * because `fromPayload()` re-applies the same defaults. See the class + * docblock and the round-trip tests. * * @return array */ diff --git a/tests/Auth/JwtClaimsTest.php b/tests/Auth/JwtClaimsTest.php index 46c2599..bf0a2a2 100644 --- a/tests/Auth/JwtClaimsTest.php +++ b/tests/Auth/JwtClaimsTest.php @@ -156,4 +156,90 @@ public function test_toPayload_omits_optional_null_fields(): void $this->assertArrayNotHasKey('scope', $payload); $this->assertArrayNotHasKey('serverId', $payload); } + + // --- S4: fromPayloadStrict() audience hardening ----------------------- + + public function test_fromPayloadStrict_with_explicit_aud_succeeds(): void + { + $claims = JwtClaims::fromPayloadStrict([ + 'iss' => JwtClaims::ISS_PHLIX_HUB, + 'aud' => JwtClaims::AUD_CLIENT, + 'sub' => 'user-uuid', + 'iat' => 1700000000, + 'exp' => 1700003600, + 'type' => JwtClaims::TYPE_ACCESS, + ]); + + $this->assertSame(JwtClaims::AUD_CLIENT, $claims->aud); + $this->assertSame(JwtClaims::ISS_PHLIX_HUB, $claims->iss); + } + + public function test_fromPayloadStrict_missing_aud_throws(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('JWT claim "aud" is required.'); + JwtClaims::fromPayloadStrict([ + 'iss' => 'phlix', 'sub' => 'u', 'iat' => 1, 'exp' => 2, 'type' => 'access', + ]); + } + + public function test_fromPayloadStrict_wrong_type_aud_throws(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('JWT claim "aud" must be a string.'); + JwtClaims::fromPayloadStrict([ + 'iss' => 'phlix', 'aud' => 42, 'sub' => 'u', 'iat' => 1, 'exp' => 2, 'type' => 'access', + ]); + } + + public function test_fromPayloadStrict_still_validates_other_required_fields(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('JWT claim "iss" is required.'); + JwtClaims::fromPayloadStrict([ + 'aud' => JwtClaims::AUD_SERVER, 'sub' => 'u', 'iat' => 1, 'exp' => 2, 'type' => 'access', + ]); + } + + public function test_fromPayload_still_defaults_missing_aud_for_legacy_tokens(): void + { + // BC: the lenient variant must keep defaulting a missing `aud`. + $claims = JwtClaims::fromPayload([ + 'iss' => 'phlix', 'sub' => 'u', 'iat' => 1, 'exp' => 2, 'type' => 'access', + ]); + + $this->assertSame(JwtClaims::AUD_SERVER, $claims->aud); + } + + // --- B4: object round-trip symmetry ----------------------------------- + + public function test_roundtrip_object_equality_full_claims(): void + { + $claims = JwtClaims::fromPayload([ + 'iss' => JwtClaims::ISS_PHLIX_HUB, + 'aud' => JwtClaims::AUD_CLIENT, + 'sub' => 'user-uuid', + 'iat' => 1700000000, + 'exp' => 1700003600, + 'type' => JwtClaims::TYPE_ACCESS, + 'nbf' => 1700000000, + 'jti' => 'token-id', + 'scope' => ['library:read', 'playback:write'], + 'serverId' => 'server-uuid', + ]); + + $this->assertEquals($claims, JwtClaims::fromPayload($claims->toPayload())); + } + + public function test_roundtrip_object_equality_minimal_claims(): void + { + // toPayload() omits the null/empty optionals; fromPayload() re-defaults + // them, so the reconstructed object must equal the original despite the + // asymmetric array serialization. + $claims = JwtClaims::fromPayload([ + 'iss' => 'phlix', 'sub' => 'u', 'iat' => 1, 'exp' => 2, 'type' => 'access', + ]); + + $this->assertEquals($claims, JwtClaims::fromPayload($claims->toPayload())); + } }