From 63cefc87639904d122422ccabaa2a606e5c3584a Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Mon, 27 Jul 2026 10:54:42 -0400 Subject: [PATCH 1/2] feat(pkce): default clientId to the client's configured client ID PKCEHelper's four flows (AuthKit/SSO authorization URL and code exchange) required callers to re-pass a clientId the WorkOS client already carries (constructor arg / WORKOS_CLIENT_ID). Make the parameter optional with a fallback to requireClientId(), aligning PHP with the other backend SDKs' override-with-fallback pattern. Explicit arguments still win; an unconfigured client now throws ConfigurationException instead of a TypeError. Co-Authored-By: Claude Fable 5 --- lib/PKCEHelper.php | 22 ++++++++++++------- tests/PKCEHelperTest.php | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/lib/PKCEHelper.php b/lib/PKCEHelper.php index 8ccfab10..c087c3d9 100644 --- a/lib/PKCEHelper.php +++ b/lib/PKCEHelper.php @@ -75,7 +75,7 @@ public static function generate(): array * Generate an AuthKit authorization URL with auto-generated PKCE parameters and state. * * @param string $redirectUri The redirect URI. - * @param string $clientId The WorkOS client ID. + * @param string|null $clientId The WorkOS client ID. Defaults to the client's configured client ID. * @param string|null $state Optional state parameter. Auto-generated if null. * @param string|null $provider Optional auth provider. * @param string|null $connectionId Optional connection ID. @@ -87,7 +87,7 @@ public static function generate(): array */ public function getAuthKitAuthorizationUrl( string $redirectUri, - string $clientId, + ?string $clientId = null, ?string $state = null, ?string $provider = null, ?string $connectionId = null, @@ -96,6 +96,7 @@ public function getAuthKitAuthorizationUrl( ?string $loginHint = null, ?string $screenHint = null, ): array { + $clientId ??= $this->client->requireClientId(); $pkce = self::generate(); $state ??= bin2hex(random_bytes(16)); @@ -134,14 +135,16 @@ public function getAuthKitAuthorizationUrl( * * @param string $code The authorization code. * @param string $codeVerifier The PKCE code verifier. - * @param string $clientId The WorkOS client ID. + * @param string|null $clientId The WorkOS client ID. Defaults to the client's configured client ID. * @return array The authentication response. */ public function authKitCodeExchange( string $code, string $codeVerifier, - string $clientId, + ?string $clientId = null, ): array { + $clientId ??= $this->client->requireClientId(); + return $this->client->request( method: 'POST', path: 'user_management/authenticate', @@ -160,7 +163,7 @@ public function authKitCodeExchange( * Generate an SSO authorization URL with auto-generated PKCE parameters and state. * * @param string $redirectUri The redirect URI. - * @param string $clientId The WorkOS client ID. + * @param string|null $clientId The WorkOS client ID. Defaults to the client's configured client ID. * @param string|null $state Optional state parameter. Auto-generated if null. * @param string|null $domain Optional SSO domain. * @param string|null $provider Optional SSO provider. @@ -172,7 +175,7 @@ public function authKitCodeExchange( */ public function getSsoAuthorizationUrl( string $redirectUri, - string $clientId, + ?string $clientId = null, ?string $state = null, ?string $domain = null, ?string $provider = null, @@ -181,6 +184,7 @@ public function getSsoAuthorizationUrl( ?string $domainHint = null, ?string $loginHint = null, ): array { + $clientId ??= $this->client->requireClientId(); $pkce = self::generate(); $state ??= bin2hex(random_bytes(16)); @@ -219,14 +223,16 @@ public function getSsoAuthorizationUrl( * * @param string $code The authorization code. * @param string $codeVerifier The PKCE code verifier. - * @param string $clientId The WorkOS client ID. + * @param string|null $clientId The WorkOS client ID. Defaults to the client's configured client ID. * @return array The SSO token response. */ public function ssoCodeExchange( string $code, string $codeVerifier, - string $clientId, + ?string $clientId = null, ): array { + $clientId ??= $this->client->requireClientId(); + return $this->client->request( method: 'POST', path: 'sso/token', diff --git a/tests/PKCEHelperTest.php b/tests/PKCEHelperTest.php index df2e19de..0d7bd743 100644 --- a/tests/PKCEHelperTest.php +++ b/tests/PKCEHelperTest.php @@ -86,6 +86,41 @@ public function testGetAuthKitAuthorizationUrl(): void $this->assertSame('S256', $query['code_challenge_method']); } + public function testGetAuthKitAuthorizationUrlFallsBackToConfiguredClientId(): void + { + $client = $this->createMockClient([['status' => 200, 'body' => ['url' => 'https://auth.workos.com/...']]]); + $client->pkce()->getAuthKitAuthorizationUrl( + redirectUri: 'https://example.com/callback', + ); + $query = []; + parse_str($this->getLastRequest()->getUri()->getQuery(), $query); + $this->assertSame('test_client_id', $query['client_id']); + } + + public function testGetAuthKitAuthorizationUrlExplicitClientIdWins(): void + { + $client = $this->createMockClient([['status' => 200, 'body' => ['url' => 'https://auth.workos.com/...']]]); + $client->pkce()->getAuthKitAuthorizationUrl( + redirectUri: 'https://example.com/callback', + clientId: 'client_override', + ); + $query = []; + parse_str($this->getLastRequest()->getUri()->getQuery(), $query); + $this->assertSame('client_override', $query['client_id']); + } + + public function testGetAuthKitAuthorizationUrlThrowsWithoutAnyClientId(): void + { + $client = $this->createMockClient( + [['status' => 200, 'body' => []]], + clientId: null, + ); + $this->expectException(\WorkOS\Exception\ConfigurationException::class); + $client->pkce()->getAuthKitAuthorizationUrl( + redirectUri: 'https://example.com/callback', + ); + } + // -- H11: AuthKit PKCE code exchange -- public function testAuthKitCodeExchange(): void @@ -105,6 +140,17 @@ public function testAuthKitCodeExchange(): void $this->assertSame('verifier_123', $body['code_verifier']); } + public function testAuthKitCodeExchangeFallsBackToConfiguredClientId(): void + { + $client = $this->createMockClient([['status' => 200, 'body' => ['access_token' => 'at_123']]]); + $client->pkce()->authKitCodeExchange( + code: 'auth_code_123', + codeVerifier: 'verifier_123', + ); + $body = json_decode((string) $this->getLastRequest()->getBody(), true); + $this->assertSame('test_client_id', $body['client_id']); + } + // -- H15: SSO PKCE authorization URL -- public function testGetSsoAuthorizationUrl(): void From 3dcbd9f6e689eb2156866c01120c2e5257605c6b Mon Sep 17 00:00:00 2001 From: "Garen J. Torikian" Date: Thu, 10 Sep 2026 16:47:25 -0400 Subject: [PATCH 2/2] fix(session): require valid JWT expiration Signed session tokens without a numeric expiration could bypass expiry validation indefinitely. Fail closed while preserving numeric-string compatibility, and reject tokens at the expiration boundary. Addresses VULN-1270. --- lib/SessionManager.php | 13 +++-- tests/Fixtures/session_expiration_clock.php | 11 ++++ tests/SessionManagerTest.php | 59 +++++++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) create mode 100644 tests/Fixtures/session_expiration_clock.php diff --git a/lib/SessionManager.php b/lib/SessionManager.php index 131d67e3..b8677ec0 100644 --- a/lib/SessionManager.php +++ b/lib/SessionManager.php @@ -361,8 +361,9 @@ private function getCachedJwks(string $clientId, bool $forceRefresh = false): ar * Decode and validate an access token JWT. * * Verifies the JWS signature against the JWKS published for `$clientId`, - * enforces an algorithm allow-list, and rejects expired tokens. This is - * the only path used by {@see authenticate()}; callers must not bypass it. + * enforces an algorithm allow-list, and requires a numeric, unexpired exp + * claim. This is the only path used by {@see authenticate()}; callers must + * not bypass it. * * @param string $accessToken The JWT access token. * @param string $clientId The WorkOS client ID (used to fetch JWKS). @@ -436,8 +437,12 @@ private function decodeAccessToken( throw new \InvalidArgumentException('JWT signature verification failed'); } - // Expiration check (after signature verification). - if (isset($decoded['exp']) && is_numeric($decoded['exp']) && (int) $decoded['exp'] < time()) { + // Require expiration after signature verification; missing or malformed + // claims must not bypass the expiry check. + if (!isset($decoded['exp']) || !is_numeric($decoded['exp'])) { + throw new \InvalidArgumentException('JWT exp claim is missing or invalid'); + } + if ((int) $decoded['exp'] <= time()) { throw new \InvalidArgumentException('JWT has expired'); } diff --git a/tests/Fixtures/session_expiration_clock.php b/tests/Fixtures/session_expiration_clock.php new file mode 100644 index 00000000..1dbd54ae --- /dev/null +++ b/tests/Fixtures/session_expiration_clock.php @@ -0,0 +1,11 @@ +assertSame('org_test', $result['organization_id']); } + /** + * @return array, 1: bool}> + */ + public static function expirationClaimsProvider(): array + { + // The isolated test clock is fixed at 1700000000. + return [ + 'missing' => [[], false], + 'null' => [['exp' => null], false], + 'non-numeric string' => [['exp' => 'never'], false], + 'empty string' => [['exp' => ''], false], + 'true' => [['exp' => true], false], + 'false' => [['exp' => false], false], + 'array' => [['exp' => [1700003600]], false], + 'object' => [['exp' => (object) ['value' => 1700003600]], false], + 'expired' => [['exp' => 1699999999], false], + 'expired numeric string' => [['exp' => '1699999999'], false], + 'exactly now' => [['exp' => 1700000000], false], + 'exactly now numeric string' => [['exp' => '1700000000'], false], + 'future' => [['exp' => 1700000001], true], + 'future numeric string' => [['exp' => '1700000001'], true], + 'future float' => [['exp' => 1700000001.5], true], + ]; + } + + #[DataProvider('expirationClaimsProvider')] + #[RunInSeparateProcess] + #[PreserveGlobalState(false)] + public function testAuthenticateRequiresUnexpiredNumericExp(array $claims, bool $authenticated): void + { + // Keep the exact expiry boundary deterministic without changing the + // production clock or leaking the clock override into other tests. + require __DIR__ . '/Fixtures/session_expiration_clock.php'; + + [$jwks, $jwt] = $this->buildSignedJwt(['sid' => 'session_test'] + $claims); + $sealed = SessionManager::sealSessionFromAuthResponse( + accessToken: $jwt, + refreshToken: 'ref_test', + cookiePassword: $this->cookiePassword, + ); + + $client = $this->createMockClient([['status' => 200, 'body' => $jwks]]); + $result = $client->sessionManager()->authenticate( + sessionData: $sealed, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + + $this->assertSame($authenticated, $result['authenticated']); + if ($authenticated) { + $this->assertSame('session_test', $result['session_id']); + } else { + $this->assertSame('invalid_jwt', $result['reason']); + } + } + public function testAuthenticateRejectsTamperedSignature(): void { [$jwks, $jwt] = $this->buildSignedJwt([