diff --git a/lib/HttpClient.php b/lib/HttpClient.php index 9da6100e..e343ced1 100644 --- a/lib/HttpClient.php +++ b/lib/HttpClient.php @@ -238,7 +238,53 @@ private function resolveUrl(string $path, ?RequestOptions $options): string $baseUrl = $options !== null && $options->baseUrl !== null ? $options->baseUrl : $this->baseUrl; $baseUrl = rtrim($baseUrl, '/'); - return $baseUrl . '/' . ltrim($path, '/'); + return $baseUrl . '/' . self::encodePathSegments(ltrim($path, '/')); + } + + /** + * RFC 3986 path-segment encoding for an entire path string. + * + * Splits `$path` on `/`, percent-encodes any unsafe characters in each + * segment, and reassembles with `/` separators. Existing valid + * percent-encoded triplets (`%XX`) are preserved verbatim, so generated + * services that already call `rawurlencode($id)` are not double-encoded. + * + * Defense-in-depth: when a service forgets to encode an interpolated ID + * like `om_xyz?foo`, non-slash unsafe characters (`?`, `#`, etc.) inside + * the segment are percent-encoded instead of opening a new query or + * fragment. A raw `/` inside an unencoded ID still acts as a path + * separator — callers are responsible for `rawurlencode`-ing IDs that can + * contain `/`. + */ + private static function encodePathSegments(string $path): string + { + if ($path === '') { + return ''; + } + $segments = explode('/', $path); + return implode('/', array_map([self::class, 'encodePathSegment'], $segments)); + } + + private static function encodePathSegment(string $segment): string + { + if ($segment === '') { + return ''; + } + // Re-encode each character except RFC 3986 pchar safe characters and + // already-formed `%XX` triplets. Keeps idempotency for callers that + // already rawurlencoded their input. + return preg_replace_callback( + '/%[0-9A-Fa-f]{2}|[^A-Za-z0-9\-._~!$&\'()*+,;=:@]/', + static function (array $match): string { + $value = $match[0]; + // Preserve existing percent-encoded triplets. + if (strlen($value) === 3 && $value[0] === '%') { + return $value; + } + return rawurlencode($value); + }, + $segment, + ) ?? $segment; } private function resolveTimeout(?RequestOptions $options): int diff --git a/lib/SessionManager.php b/lib/SessionManager.php index 9bdde3fe..131d67e3 100644 --- a/lib/SessionManager.php +++ b/lib/SessionManager.php @@ -13,6 +13,23 @@ class SessionManager { + /** + * In-memory JWKS cache, keyed by client ID. Values are + * `['keys' => array, 'fetched_at' => int]`. Cache lives for the + * lifetime of the SessionManager instance and is bypassed when a + * token's `kid` isn't found, so key rotation still resolves quickly. + * + * @var array + */ + private array $jwksCache = []; + + /** + * JWKS cache TTL in seconds. WorkOS rotates signing keys on the order + * of weeks, so a few minutes is plenty to absorb traffic spikes + * without making session checks dependent on a live JWKS round-trip. + */ + private const JWKS_CACHE_TTL_SECONDS = 300; + public function __construct( private readonly HttpClient $client, ) { @@ -147,7 +164,7 @@ public function authenticate( } try { - $decoded = self::decodeAccessToken($session['access_token'], $clientId, $baseUrl); + $decoded = $this->decodeAccessToken($session['access_token'], $clientId); } catch (\Exception $e) { return [ 'authenticated' => false, @@ -296,50 +313,255 @@ public static function getJwksUrl( */ public function fetchJwks(string $clientId): array { - return $this->client->request( + $response = $this->client->request( method: 'GET', path: "sso/jwks/{$clientId}", ); + if ($response === null) { + throw new \RuntimeException('Failed to fetch JWKS: empty response'); + } + return $response; } + /** + * Return the JWKS for `$clientId`, served from an in-memory cache + * with a {@see JWKS_CACHE_TTL_SECONDS}-second TTL. Set + * `$forceRefresh` to bypass the cache after a `kid` miss, which + * lets newly-rotated keys be discovered without waiting for TTL + * expiry. + * + * @return array + */ + private function getCachedJwks(string $clientId, bool $forceRefresh = false): array + { + $now = time(); + $entry = $this->jwksCache[$clientId] ?? null; + if ( + !$forceRefresh + && $entry !== null + && ($now - $entry['fetched_at']) < self::JWKS_CACHE_TTL_SECONDS + ) { + return $entry['keys']; + } + + $keys = $this->fetchJwks($clientId); + $this->jwksCache[$clientId] = ['keys' => $keys, 'fetched_at' => $now]; + + return $keys; + } + + /** + * Algorithms permitted on the JWS header. WorkOS access tokens are signed + * with RS256; no other algorithm is accepted, in particular `none` is + * always rejected. + */ + private const ALLOWED_JWS_ALGORITHMS = ['RS256']; + /** * Decode and validate an access token JWT. * - * This is a basic JWT decode. For production use, fetch JWKS and validate - * the signature properly. This helper decodes without signature verification - * for extracting claims when the token has already been validated upstream. + * 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. * * @param string $accessToken The JWT access token. - * @param string $clientId The WorkOS client ID (unused in basic decode). - * @param string $baseUrl The WorkOS API base URL (unused in basic decode). + * @param string $clientId The WorkOS client ID (used to fetch JWKS). * @return array The decoded JWT claims. - * @throws \InvalidArgumentException If the token cannot be decoded. + * @throws \InvalidArgumentException If the token cannot be decoded or fails verification. */ - private static function decodeAccessToken( + private function decodeAccessToken( string $accessToken, string $clientId, - string $baseUrl, ): array { $parts = explode('.', $accessToken); if (count($parts) !== 3) { throw new \InvalidArgumentException('Invalid JWT format'); } - $payload = base64_decode(strtr($parts[1], '-_', '+/'), true); - if ($payload === false) { - throw new \InvalidArgumentException('Invalid JWT payload encoding'); + [$headerB64, $payloadB64, $signatureB64] = $parts; + + $headerJson = self::base64UrlDecode($headerB64); + if ($headerJson === false) { + throw new \InvalidArgumentException('Invalid JWT header encoding'); + } + $header = json_decode($headerJson, true); + if (!is_array($header)) { + throw new \InvalidArgumentException('Invalid JWT header JSON'); + } + + $alg = $header['alg'] ?? null; + if (!is_string($alg) || !in_array($alg, self::ALLOWED_JWS_ALGORITHMS, true)) { + throw new \InvalidArgumentException('Unsupported JWT algorithm'); } - $decoded = json_decode($payload, true); - if ($decoded === null) { + $payloadJson = self::base64UrlDecode($payloadB64); + if ($payloadJson === false) { + throw new \InvalidArgumentException('Invalid JWT payload encoding'); + } + $decoded = json_decode($payloadJson, true); + if (!is_array($decoded)) { throw new \InvalidArgumentException('Invalid JWT payload JSON'); } - // Check expiration - if (isset($decoded['exp']) && $decoded['exp'] < time()) { + $signature = self::base64UrlDecode($signatureB64); + if ($signature === false || $signature === '') { + throw new \InvalidArgumentException('Invalid JWT signature encoding'); + } + + // Resolve a JWK matching the header `kid`. Without a `kid` we won't + // guess — refuse rather than try every key, which would mask key + // rotation bugs. + $kid = $header['kid'] ?? null; + if (!is_string($kid) || $kid === '') { + throw new \InvalidArgumentException('JWT header missing kid'); + } + + // Try the cached JWKS first; if the `kid` isn't present, force a + // refresh once to handle key rotation, then fail if still unknown. + $jwks = $this->getCachedJwks($clientId); + $jwk = self::findJwkByKid($jwks, $kid); + if ($jwk === null) { + $jwks = $this->getCachedJwks($clientId, forceRefresh: true); + $jwk = self::findJwkByKid($jwks, $kid); + } + if ($jwk === null) { + throw new \InvalidArgumentException('No JWKS key matches JWT kid'); + } + + $publicKeyPem = self::jwkToRsaPublicKeyPem($jwk); + $signingInput = $headerB64 . '.' . $payloadB64; + + $verified = openssl_verify($signingInput, $signature, $publicKeyPem, OPENSSL_ALGO_SHA256); + if ($verified !== 1) { + throw new \InvalidArgumentException('JWT signature verification failed'); + } + + // Expiration check (after signature verification). + if (isset($decoded['exp']) && is_numeric($decoded['exp']) && (int) $decoded['exp'] < time()) { throw new \InvalidArgumentException('JWT has expired'); } + // TODO(security-fix-plan.md, finding #60): enforce documented WorkOS + // `iss` and `aud` values once empirically confirmed. The other WorkOS + // SDKs (Ruby, Python) currently skip `aud` verification, so the + // canonical values are not authoritatively documented in this repo. + // Track resolution under "Open questions / follow-ups" in the plan. + return $decoded; } + + /** + * Decode a base64url-encoded segment, tolerating missing padding. + * + * @return string|false The decoded bytes, or false on malformed input. + */ + private static function base64UrlDecode(string $segment): string|false + { + $remainder = strlen($segment) % 4; + if ($remainder !== 0) { + $segment .= str_repeat('=', 4 - $remainder); + } + + return base64_decode(strtr($segment, '-_', '+/'), true); + } + + /** + * Locate a JWK in the JWKS response by `kid`. + * + * @param array $jwks + * @return array|null + */ + private static function findJwkByKid(array $jwks, string $kid): ?array + { + $keys = $jwks['keys'] ?? null; + if (!is_array($keys)) { + return null; + } + foreach ($keys as $jwk) { + if (is_array($jwk) && ($jwk['kid'] ?? null) === $kid) { + return $jwk; + } + } + return null; + } + + /** + * Convert an RSA JWK (`kty=RSA`, base64url `n`/`e`) to a PEM-encoded + * public key suitable for {@see openssl_verify()}. + * + * @param array $jwk + */ + private static function jwkToRsaPublicKeyPem(array $jwk): string + { + if (($jwk['kty'] ?? null) !== 'RSA') { + throw new \InvalidArgumentException('Unsupported JWK key type'); + } + $n = $jwk['n'] ?? null; + $e = $jwk['e'] ?? null; + if (!is_string($n) || !is_string($e)) { + throw new \InvalidArgumentException('Malformed RSA JWK'); + } + + $modulus = self::base64UrlDecode($n); + $exponent = self::base64UrlDecode($e); + if ($modulus === false || $exponent === false) { + throw new \InvalidArgumentException('Malformed RSA JWK encoding'); + } + + // Build a DER-encoded SubjectPublicKeyInfo for an RSA public key, then + // wrap it as a PEM document. Avoids a hard dependency on a JWT library. + $modulusDer = self::derEncodeUnsignedInteger($modulus); + $exponentDer = self::derEncodeUnsignedInteger($exponent); + $rsaPublicKey = self::derEncodeSequence($modulusDer . $exponentDer); + $bitString = self::derEncodeBitString($rsaPublicKey); + + // AlgorithmIdentifier: SEQUENCE { OID 1.2.840.113549.1.1.1, NULL }. + $rsaOid = "\x06\x09\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01"; + $algorithmIdentifier = self::derEncodeSequence($rsaOid . "\x05\x00"); + $spki = self::derEncodeSequence($algorithmIdentifier . $bitString); + + $pem = "-----BEGIN PUBLIC KEY-----\n" + . chunk_split(base64_encode($spki), 64, "\n") + . "-----END PUBLIC KEY-----\n"; + + return $pem; + } + + private static function derEncodeLength(int $length): string + { + if ($length < 0x80) { + return chr($length); + } + $bytes = ''; + while ($length > 0) { + $bytes = chr($length & 0xff) . $bytes; + $length >>= 8; + } + return chr(0x80 | strlen($bytes)) . $bytes; + } + + private static function derEncodeSequence(string $contents): string + { + return "\x30" . self::derEncodeLength(strlen($contents)) . $contents; + } + + private static function derEncodeUnsignedInteger(string $bytes): string + { + // Strip leading zero bytes, then re-prepend a single 0x00 if the + // high bit of the first byte is set so the value remains positive. + $bytes = ltrim($bytes, "\x00"); + if ($bytes === '') { + $bytes = "\x00"; + } elseif ((ord($bytes[0]) & 0x80) !== 0) { + $bytes = "\x00" . $bytes; + } + return "\x02" . self::derEncodeLength(strlen($bytes)) . $bytes; + } + + private static function derEncodeBitString(string $bytes): string + { + // 0x00 = number of unused bits in the final octet (always zero here). + $contents = "\x00" . $bytes; + return "\x03" . self::derEncodeLength(strlen($contents)) . $contents; + } } diff --git a/tests/HttpClientTest.php b/tests/HttpClientTest.php index 1e949913..be9037a2 100644 --- a/tests/HttpClientTest.php +++ b/tests/HttpClientTest.php @@ -252,6 +252,78 @@ public function testEmptyErrorBodySetsNullCodeAndError(): void } } + public function testResolveUrlPreservesEncodedIdAsSingleSegment(): void + { + // security-fix-plan.md finding #61: an untrusted ID like `om_xyz?/foo` + // must remain a single percent-encoded path segment instead of + // opening a new path/query when interpolated into a path template. + // Generated services already call `rawurlencode($id)`; the centralized + // fix must preserve that encoding (no double-encoding) and not re-split + // the encoded slash. + $mock = new MockHandler([ + new Response(200, ['Content-Type' => 'application/json'], '{}'), + ]); + $history = []; + $handler = HandlerStack::create($mock); + $handler->push(\GuzzleHttp\Middleware::history($history)); + + $client = new HttpClient( + apiKey: 'test_key', + clientId: null, + baseUrl: 'https://api.workos.com', + timeout: 10, + maxRetries: 0, + handler: $handler, + ); + + $id = 'om_xyz?/foo'; + $client->request('GET', 'organizations/' . rawurlencode($id)); + + $request = $history[array_key_last($history)]['request']; + $uri = $request->getUri(); + + // No query string — `?` inside the ID stayed percent-encoded. + $this->assertSame('', $uri->getQuery()); + // The whole ID (including its `/`) stayed inside a single segment. + $this->assertSame('/organizations/om_xyz%3F%2Ffoo', $uri->getPath()); + + // Defense-in-depth: a path with a raw `?` in an interpolated ID has + // the `?` encoded so no stray query string opens. Note that a raw `/` + // inside an unencoded ID remains a path separator — callers must + // rawurlencode IDs that can contain `/` (covered by the first case + // above). Here we use `om_xyz?foo` (no slash) to assert exactly that + // boundary: `?` is encoded, segment count is preserved. + $client2 = new HttpClient( + apiKey: 'test_key', + clientId: null, + baseUrl: 'https://api.workos.com', + timeout: 10, + maxRetries: 0, + handler: $handler, + ); + $mock->append(new Response(200, ['Content-Type' => 'application/json'], '{}')); + $client2->request('GET', 'organizations/om_xyz?foo'); + $rawRequest = $history[array_key_last($history)]['request']; + $this->assertSame('', $rawRequest->getUri()->getQuery()); + $this->assertSame('/organizations/om_xyz%3Ffoo', $rawRequest->getUri()->getPath()); + + // And confirm the documented caveat: a raw `/` in an unencoded ID is + // NOT contained — it splits into a new segment. This pins the current + // behaviour so a future refactor that silently changes it is caught. + $client3 = new HttpClient( + apiKey: 'test_key', + clientId: null, + baseUrl: 'https://api.workos.com', + timeout: 10, + maxRetries: 0, + handler: $handler, + ); + $mock->append(new Response(200, ['Content-Type' => 'application/json'], '{}')); + $client3->request('GET', 'organizations/om_xyz/foo'); + $rawSlashRequest = $history[array_key_last($history)]['request']; + $this->assertSame('/organizations/om_xyz/foo', $rawSlashRequest->getUri()->getPath()); + } + public function testNonStringCodeFieldIsIgnored(): void { $body = json_encode([ diff --git a/tests/SessionManagerTest.php b/tests/SessionManagerTest.php index 713f0238..36fa1c7d 100644 --- a/tests/SessionManagerTest.php +++ b/tests/SessionManagerTest.php @@ -137,4 +137,240 @@ public function testSessionManagerAccessibleFromClient(): void $client = $this->createMockClient([]); $this->assertInstanceOf(SessionManager::class, $client->sessionManager()); } + + // -- security-fix-plan.md finding #60: JWS verification -- + + /** + * Build a JWKS dict + signed JWT for verification tests. + * + * @param array $claims + * @param string $alg The `alg` value to advertise in the JWT header. + * @return array{0: array, 1: string} JWKS, signed JWT. + */ + private function buildSignedJwt(array $claims, string $alg = 'RS256', string $kid = 'kid_test'): array + { + $key = openssl_pkey_new([ + 'private_key_bits' => 2048, + 'private_key_type' => OPENSSL_KEYTYPE_RSA, + ]); + $details = openssl_pkey_get_details($key); + + $b64u = static fn (string $bytes): string => rtrim(strtr(base64_encode($bytes), '+/', '-_'), '='); + + $header = $b64u(json_encode(['alg' => $alg, 'typ' => 'JWT', 'kid' => $kid])); + $payload = $b64u(json_encode($claims)); + $signingInput = $header . '.' . $payload; + + openssl_sign($signingInput, $signature, $key, OPENSSL_ALGO_SHA256); + $jwt = $signingInput . '.' . $b64u($signature); + + $jwk = [ + 'kty' => 'RSA', + 'kid' => $kid, + 'alg' => 'RS256', + 'use' => 'sig', + 'n' => $b64u($details['rsa']['n']), + 'e' => $b64u($details['rsa']['e']), + ]; + + return [['keys' => [$jwk]], $jwt]; + } + + public function testAuthenticateValidatesSignedJwt(): void + { + [$jwks, $jwt] = $this->buildSignedJwt([ + 'sid' => 'session_test', + 'org_id' => 'org_test', + 'exp' => time() + 3600, + ]); + + $sealed = SessionManager::sealSessionFromAuthResponse( + accessToken: $jwt, + refreshToken: 'ref_test', + cookiePassword: $this->cookiePassword, + user: ['id' => 'usr_test'], + ); + + $client = $this->createMockClient([['status' => 200, 'body' => $jwks]]); + $result = $client->sessionManager()->authenticate( + sessionData: $sealed, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + + $this->assertTrue($result['authenticated']); + $this->assertSame('session_test', $result['session_id']); + $this->assertSame('org_test', $result['organization_id']); + } + + public function testAuthenticateRejectsTamperedSignature(): void + { + [$jwks, $jwt] = $this->buildSignedJwt([ + 'sid' => 'session_test', + 'exp' => time() + 3600, + ]); + + // Flip a byte in the middle of the signature segment so the base64 + // decoder produces a clearly different signature. Avoids the trailing + // padding bits that base64url can canonicalise away. + $parts = explode('.', $jwt); + $sig = $parts[2]; + $mid = intdiv(strlen($sig), 2); + $parts[2] = substr($sig, 0, $mid) . ($sig[$mid] === 'A' ? 'B' : 'A') . substr($sig, $mid + 1); + $tampered = implode('.', $parts); + + $sealed = SessionManager::sealSessionFromAuthResponse( + accessToken: $tampered, + 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->assertFalse($result['authenticated']); + $this->assertSame('invalid_jwt', $result['reason']); + } + + public function testAuthenticateRejectsAlgNone(): void + { + // Forge a `none`-algorithm token; signature verification must refuse + // even before fetching JWKS. + $b64u = static fn (string $bytes): string => rtrim(strtr(base64_encode($bytes), '+/', '-_'), '='); + $header = $b64u(json_encode(['alg' => 'none', 'typ' => 'JWT', 'kid' => 'kid_test'])); + $payload = $b64u(json_encode(['sid' => 'session_test', 'exp' => time() + 3600])); + $jwt = $header . '.' . $payload . '.'; + + $sealed = SessionManager::sealSessionFromAuthResponse( + accessToken: $jwt, + refreshToken: 'ref_test', + cookiePassword: $this->cookiePassword, + ); + + $client = $this->createMockClient([]); + $result = $client->sessionManager()->authenticate( + sessionData: $sealed, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + + $this->assertFalse($result['authenticated']); + $this->assertSame('invalid_jwt', $result['reason']); + } + + public function testAuthenticateRejectsUnknownKid(): void + { + [, $jwt] = $this->buildSignedJwt( + ['sid' => 'session_test', 'exp' => time() + 3600], + 'RS256', + 'kid_signed_with', + ); + + // JWKS advertises a different kid than the token claims. + $otherJwks = ['keys' => [['kty' => 'RSA', 'kid' => 'kid_other', 'n' => 'AQ', 'e' => 'AQAB']]]; + + $sealed = SessionManager::sealSessionFromAuthResponse( + accessToken: $jwt, + refreshToken: 'ref_test', + cookiePassword: $this->cookiePassword, + ); + + // Queue TWO identical responses: the first satisfies the cached lookup, + // the second satisfies the forced refresh triggered by the kid miss. + // This exercises the "No JWKS key matches JWT kid" guard in + // decodeAccessToken rather than falsely passing on a MockHandler queue + // exhaustion (OutOfBoundsException caught by authenticate()). + $client = $this->createMockClient([ + ['status' => 200, 'body' => $otherJwks], + ['status' => 200, 'body' => $otherJwks], + ]); + $result = $client->sessionManager()->authenticate( + sessionData: $sealed, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + + $this->assertFalse($result['authenticated']); + $this->assertSame('invalid_jwt', $result['reason']); + } + + public function testAuthenticateCachesJwksAcrossCalls(): void + { + [$jwks, $jwt] = $this->buildSignedJwt([ + 'sid' => 'session_test', + 'exp' => time() + 3600, + ]); + + $sealed = SessionManager::sealSessionFromAuthResponse( + accessToken: $jwt, + refreshToken: 'ref_test', + cookiePassword: $this->cookiePassword, + ); + + // Only ONE JWKS response is queued; a second fetch would make + // the MockHandler throw, so successful back-to-back authenticate + // calls prove the cache served the second one. + $client = $this->createMockClient([['status' => 200, 'body' => $jwks]]); + $sessionManager = $client->sessionManager(); + + $first = $sessionManager->authenticate( + sessionData: $sealed, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + $second = $sessionManager->authenticate( + sessionData: $sealed, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + + $this->assertTrue($first['authenticated']); + $this->assertTrue($second['authenticated']); + $this->assertCount(1, $this->requestHistory); + } + + public function testAuthenticateRefreshesJwksOnUnknownKid(): void + { + // Seed the cache with a JWKS that only knows `kid_old`, then + // present a token signed with `kid_new`. The kid-miss path + // should force a refresh and find `kid_new` in the second + // JWKS response. + [$oldJwks] = $this->buildSignedJwt( + ['sid' => 'session_old', 'exp' => time() + 3600], + 'RS256', + 'kid_old', + ); + + [$newJwks, $newJwt] = $this->buildSignedJwt( + ['sid' => 'session_new', 'exp' => time() + 3600], + 'RS256', + 'kid_new', + ); + + $sealedNew = SessionManager::sealSessionFromAuthResponse( + accessToken: $newJwt, + refreshToken: 'ref_test', + cookiePassword: $this->cookiePassword, + ); + + $client = $this->createMockClient([ + ['status' => 200, 'body' => $oldJwks], + ['status' => 200, 'body' => $newJwks], + ]); + $sessionManager = $client->sessionManager(); + + $result = $sessionManager->authenticate( + sessionData: $sealedNew, + cookiePassword: $this->cookiePassword, + clientId: 'client_123', + ); + + $this->assertTrue($result['authenticated']); + $this->assertSame('session_new', $result['session_id']); + $this->assertCount(2, $this->requestHistory); + } }