From b69ca5d0059655d54c0ec372fd107d7a3f430d24 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:43:55 +0100 Subject: [PATCH 01/24] [Client] Don't log a pending authorization as a transport error when a stream resumes --- src/Client/Transport/HttpTransport.php | 4 ++++ .../Client/Transport/HttpTransportTest.php | 21 +++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/Client/Transport/HttpTransport.php b/src/Client/Transport/HttpTransport.php index 5c4c36a2..687b8747 100644 --- a/src/Client/Transport/HttpTransport.php +++ b/src/Client/Transport/HttpTransport.php @@ -956,6 +956,10 @@ private function processSseReconnect(): void try { $response = $this->httpClient->sendRequest($request); + } catch (AuthorizationPendingException $e) { + $this->failSseResumption($e); + + return; } catch (AuthorizationException $e) { $this->handleError($e); $this->failSseResumption($e); diff --git a/tests/Unit/Client/Transport/HttpTransportTest.php b/tests/Unit/Client/Transport/HttpTransportTest.php index f2960eca..294edcfd 100644 --- a/tests/Unit/Client/Transport/HttpTransportTest.php +++ b/tests/Unit/Client/Transport/HttpTransportTest.php @@ -318,6 +318,27 @@ public function testAuthorizationFailureDuringResumptionIsNotRetried(): void $this->assertCount(2, $httpClient->requests); } + #[TestDox('a pending authorization during a stream resumption reaches the request as it is, without logging an error')] + public function testPendingAuthorizationDuringResumptionReachesTheRequest(): void + { + $clock = new SteppingClock(); + $httpClient = RecordingHttpClient::queued([ + new Response(200, ['Content-Type' => 'text/event-stream'], "id: event-0\ndata:\n\n"), + new AuthorizationPendingException('https://auth.example.com/authorize?state=abc'), + ], $clock); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects($this->never())->method('error'); + + try { + $this->runRequest($this->createTransport(httpClient: $httpClient, clock: $clock, logger: $logger), 1); + $this->fail('The request should have failed.'); + } catch (AuthorizationPendingException $e) { + $this->assertSame('https://auth.example.com/authorize?state=abc', $e->authorizationUrl); + } + + $this->assertCount(2, $httpClient->requests); + } + #[TestDox('the server-minted session ID is exposed while connected and cleared on close')] public function testSessionIdIsExposedAndClearedOnClose(): void { From 746940eaad4d9eeef10e81e3cf96b60034fe6297 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:47:28 +0100 Subject: [PATCH 02/24] [Client] Key stored tokens by grant and client, not only by issuer and resource Closes #719 --- docs/client/authorization.md | 2 + .../Auth/CredentialStorageInterface.php | 2 +- src/Client/Auth/OAuthAuthenticator.php | 20 +++- .../Client/Auth/OAuthAuthenticatorTest.php | 93 +++++++++++++------ 4 files changed, 85 insertions(+), 32 deletions(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index edc4fc8f..1159fef9 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -89,6 +89,8 @@ The file contains tokens and client secrets. The SDK creates it with the mode `0 The storage keys everything by the issuer of the authorization server. If a server moves to another authorization server, the client registers again and doesn't send its old client ID to the new one. It also registers again if you change the redirect URI, if it needs scopes that its registration doesn't include, or if the token endpoint answers `invalid_client` for a client that it registered. +Tokens are also stored per resource, grant and client ID. So you can share one storage between a service account and an application for users, or between clients with different client IDs. None of them uses a token of another. + ## Finishing the Authorization in a Later Request A web application can't wait for the browser within one request. It redirects the user to the authorization server, which sends the user back to a callback route of the application. Use `RedirectAuthorizationHandler` for this, with one storage per user session: diff --git a/src/Client/Auth/CredentialStorageInterface.php b/src/Client/Auth/CredentialStorageInterface.php index 0799541c..2b1ee5b6 100644 --- a/src/Client/Auth/CredentialStorageInterface.php +++ b/src/Client/Auth/CredentialStorageInterface.php @@ -27,7 +27,7 @@ public function saveClientRegistration(string $issuer, ClientRegistration $regis /** * @param string $issuer the authorization server's issuer identifier - * @param string $resource the canonical URI of the protected resource the token may be sent to + * @param string $resource the canonical URI of the protected resource the token may be sent to, followed by `#`, the grant and, for a configured client, its client ID */ public function getToken(string $issuer, string $resource): ?AccessToken; diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index 0e1ca2a0..fa77b881 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -102,7 +102,7 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re $this->issuer = $metadata->issuer; $this->resource = $resource; - $stored = $this->storage->getToken($metadata->issuer, $resource); + $stored = $this->storage->getToken($metadata->issuer, $this->tokenKey($resource)); $scopes = $this->selectScopes($challenge, $scopesSupported, $metadata, $stored); // The resource never requires offline_access, which only gets a refresh token. $needed = array_values(array_diff($scopes, ['offline_access'])); @@ -159,7 +159,7 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re $token = $token->withGrantedScopes($scopes); $this->requestedScopes = $scopes; - $this->storage->saveToken($metadata->issuer, $resource, $token); + $this->storage->saveToken($metadata->issuer, $this->tokenKey($resource), $token); } $this->token = $token; @@ -241,7 +241,7 @@ public function finishAuthorization(array $callback): void $this->token = $token->withGrantedScopes($scopes); $this->requestedScopes = $scopes; $this->audience = $pending->resource; - $this->storage->saveToken($metadata->issuer, $pending->resource, $this->token); + $this->storage->saveToken($metadata->issuer, $this->tokenKey($pending->resource), $this->token); } /** @@ -463,7 +463,7 @@ private function refresh(?AccessToken $stored, AuthorizationServerMetadata $meta return $this->storage->synchronize(function () use ($metadata, $client, $scopes, $resource, $request): ?AccessToken { // Another process may have refreshed the token while this one waited. - $current = $this->storage->getToken($metadata->issuer, $resource); + $current = $this->storage->getToken($metadata->issuer, $this->tokenKey($resource)); if (null === $current) { return null; @@ -481,7 +481,7 @@ private function refresh(?AccessToken $stored, AuthorizationServerMetadata $meta $token = $this->spendRefreshToken($current, $current->refreshToken, $metadata, $client, $resource); if (null !== $token) { - $this->storage->saveToken($metadata->issuer, $resource, $token); + $this->storage->saveToken($metadata->issuer, $this->tokenKey($resource), $token); } return $token; @@ -770,6 +770,16 @@ private function resolveClient(AuthorizationServerMetadata $metadata, array $sco }); } + /** + * Where a token is stored: configurations sharing a storage must not take over each other's tokens. + */ + private function tokenKey(string $resource): string + { + $client = $this->configuration->clientId ?? $this->configuration->clientMetadataUrl; + + return $resource.'#'.$this->configuration->grant->value.(null === $client ? '' : ' '.$client); + } + /** * @param array $parameters * diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index 2d1e65e0..ce2c39c4 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -251,7 +251,7 @@ public function testRefreshesATokenWithoutOfflineAccess(): void { $server = new FakeOAuthServer(resourceScopes: ['mcp:read'], serverScopes: ['mcp:read', 'offline_access'], refreshTokens: true); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired-token', 'Bearer', time() - 60, 'refresh-token', ['mcp:read'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired-token', 'Bearer', time() - 60, 'refresh-token', ['mcp:read'])); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); @@ -582,7 +582,7 @@ public function testFinishesAuthorizationInAnotherRequest(): void $this->assertSame($server->authorization['code_challenge'], rtrim(strtr(base64_encode(hash('sha256', $server->token['code_verifier'], true)), '+/', '-_'), '=')); $this->assertSame($server->authorization['redirect_uri'], $server->token['redirect_uri']); $this->assertSame($server->authorization['resource'], $server->token['resource']); - $stored = (new FileCredentialStorage($path))->getToken('https://auth.example.com', self::ENDPOINT); + $stored = (new FileCredentialStorage($path))->getToken('https://auth.example.com', self::tokenKey()); $this->assertSame(['mcp:read'], $stored?->scopes); $this->assertSame([], json_decode((string) file_get_contents($path), true)['pending']); } finally { @@ -605,7 +605,7 @@ public function testRejectsInvalidDeferredCallbacks(array $replacement): void } catch (AuthorizationException $e) { $this->assertNotInstanceOf(AuthorizationPendingException::class, $e); $this->assertNotContains('https://auth.example.com/token', $server->requested); - $this->assertNull($storage->getToken('https://auth.example.com', self::ENDPOINT)); + $this->assertNull($storage->getToken('https://auth.example.com', self::tokenKey())); } } @@ -720,7 +720,7 @@ public function testSynchronousAuthorizationSupportsExistingCredentialStorage(): $server = new FakeOAuthServer(); $storage = $this->createMock(CredentialStorageInterface::class); $storage->method('synchronize')->willReturnCallback(static fn (callable $operation): mixed => $operation()); - $storage->expects($this->once())->method('saveToken')->with('https://auth.example.com', self::ENDPOINT, $this->isInstanceOf(AccessToken::class)); + $storage->expects($this->once())->method('saveToken')->with('https://auth.example.com', self::tokenKey(), $this->isInstanceOf(AccessToken::class)); $authenticator = $this->authenticator($server, storage: $storage); $this->assertTrue($authenticator->handleChallenge($this->request(), $server->challenge())); @@ -1140,12 +1140,12 @@ public function testRefreshKeepsTheStoredScopes(): void { $server = new FakeOAuthServer(refreshTokens: true); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token', ['mcp:read', 'mcp:write'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token', ['mcp:read', 'mcp:write'])); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge(['mcp:read'])); $this->assertSame('refresh_token', $server->token['grant_type'] ?? null); - $this->assertSame(['mcp:read', 'mcp:write'], $storage->getToken('https://auth.example.com', self::ENDPOINT)?->scopes); + $this->assertSame(['mcp:read', 'mcp:write'], $storage->getToken('https://auth.example.com', self::tokenKey())?->scopes); } #[TestDox('a refresh does not record scopes the authorization server did not grant')] @@ -1153,12 +1153,12 @@ public function testRefreshDoesNotInventScopes(): void { $server = new FakeOAuthServer(refreshTokens: true); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge(['admin'])); $this->assertSame('refresh_token', $server->token['grant_type'] ?? null); - $this->assertSame([], $storage->getToken('https://auth.example.com', self::ENDPOINT)?->scopes); + $this->assertSame([], $storage->getToken('https://auth.example.com', self::tokenKey())?->scopes); } #[TestDox('a new redirect URI gets a new registration, as the old one would be refused')] @@ -1194,7 +1194,7 @@ public function testRegistersAgainWhenTheClientIsUnknown(): void $server->unknownClient = 'forgotten-client'; $storage = new InMemoryCredentialStorage(); $storage->saveClientRegistration('https://auth.example.com', new ClientRegistration('forgotten-client', dynamic: true)); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge()); @@ -1236,7 +1236,7 @@ public function testRefreshKeepsAClientRegisteredByAnotherWorker(): void $server = new FakeOAuthServer(refreshTokens: true); $server->unknownClient = 'forgotten-client'; $storage = $this->storageWithAnotherWorkerRegistering(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); @@ -1285,7 +1285,7 @@ public function testConfiguredClientRefusedOnRefreshFailsFast(): void $server = new FakeOAuthServer(refreshTokens: true); $server->unknownClient = 'known-client'; $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'refresh')); + $storage->saveToken('https://auth.example.com', self::tokenKey(clientId: 'known-client'), new AccessToken('expired', 'Bearer', time() - 60, 'refresh')); $configuration = OAuth::forApplication('test')->setClientCredentials('known-client')->setExpectedIssuer('https://auth.example.com'); try { @@ -1297,7 +1297,7 @@ public function testConfiguredClientRefusedOnRefreshFailsFast(): void } $this->assertSame([], $server->authorization); - $this->assertNotNull($storage->getToken('https://auth.example.com', self::ENDPOINT)); + $this->assertNotNull($storage->getToken('https://auth.example.com', self::tokenKey(clientId: 'known-client'))); } #[TestDox('an error that only mentions a refused scope in its text does not send the user back')] @@ -1391,7 +1391,7 @@ public function testUsesARefreshTokenUnlessTheGrantIsNotListed(?array $grantType $server = new FakeOAuthServer(refreshTokens: true); $server->grantTypes = $grantTypes; $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'a-refresh-token')); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); @@ -1415,19 +1415,52 @@ public function testRefreshesAStoredTokenThatIsAboutToExpire(): void { $server = new FakeOAuthServer(refreshTokens: true); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expiring', 'Bearer', time() + 10, 'a-refresh-token')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expiring', 'Bearer', time() + 10, 'a-refresh-token')); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); $this->assertSame('refresh_token', $server->token['grant_type'] ?? null); } + #[TestDox('a token stored by another client configuration in the same storage is not taken over')] + public function testDoesNotTakeOverATokenOfAnotherClient(): void + { + $server = new FakeOAuthServer(); + $storage = new InMemoryCredentialStorage(); + $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge(['mcp:read'])); + $this->assertSame('registered-client', $server->authorization['client_id'] ?? null); + + $configured = $this->authenticator($server, storage: $storage, configuration: OAuth::forApplication('test') + ->setClientCredentials('known-client') + ->setExpectedIssuer('https://auth.example.com')); + $configured->handleChallenge($this->request(), $server->challenge(['mcp:read'])); + + $this->assertSame('known-client', $server->authorization['client_id'] ?? null); + $this->assertNotNull($storage->getToken('https://auth.example.com', self::tokenKey())); + $this->assertNotNull($storage->getToken('https://auth.example.com', self::tokenKey(clientId: 'known-client'))); + } + + #[TestDox('a service account does not take over a token stored for a user of the same client')] + public function testServiceAccountDoesNotTakeOverAUserToken(): void + { + $server = new FakeOAuthServer(); + $storage = new InMemoryCredentialStorage(); + $storage->saveToken('https://auth.example.com', self::tokenKey(clientId: 'known-client'), new AccessToken('user-token', 'Bearer', time() + 3600, null, ['mcp:read'])); + + $serviceAccount = $this->authenticator($server, storage: $storage, configuration: OAuth::forServiceAccount('test', 'known-client', 'secret') + ->setExpectedIssuer('https://auth.example.com')); + $serviceAccount->handleChallenge($this->request(), $server->challenge(['mcp:read'])); + + $this->assertSame('client_credentials', $server->token['grant_type'] ?? null); + $this->assertNotSame('Bearer user-token', $serviceAccount->authenticate($this->request())->getHeaderLine('Authorization')); + } + #[TestDox('a step-up keeps the scopes of a stored token')] public function testStepUpKeepsTheScopesOfAStoredToken(): void { $server = new FakeOAuthServer(); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('stored', 'Bearer', time() + 3600, null, ['mcp:read'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('stored', 'Bearer', time() + 3600, null, ['mcp:read'])); $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge(['mcp:read'])); @@ -1442,7 +1475,7 @@ public function testStoredOfflineAccessIsNotRequestedWhenTurnedOff(): void { $server = new FakeOAuthServer(serverScopes: ['offline_access']); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('stored', 'Bearer', time() + 3600, null, ['mcp:read', 'offline_access'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('stored', 'Bearer', time() + 3600, null, ['mcp:read', 'offline_access'])); $authenticator = $this->authenticator($server, storage: $storage, configuration: OAuth::forApplication('test')->setOfflineAccess(false)); $authenticator->handleChallenge((new Psr17Factory())->createRequest('POST', self::ENDPOINT)->withHeader('Authorization', 'Bearer stored'), $server->challenge(['mcp:write'], 403, 'insufficient_scope')); @@ -1494,7 +1527,7 @@ public function testOnlyAStepUpKeepsTheScopesOfAStoredToken(): void { $server = new FakeOAuthServer(); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, null, ['openid', 'mcp:read'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, null, ['openid', 'mcp:read'])); $this->authenticator($server, storage: $storage, configuration: OAuth::forApplication('test')->setScopes('openid'))->handleChallenge($this->request(), $server->challenge()); @@ -1615,7 +1648,7 @@ public function testAnOutageDoesNotStartANewAuthorization(int $status, string $b $server = new FakeOAuthServer(refreshTokens: true); $server->refreshAnswer = new Response($status, [], $body); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'still-good')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'still-good')); try { $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); @@ -1625,7 +1658,7 @@ public function testAnOutageDoesNotStartANewAuthorization(int $status, string $b } $this->assertSame([], $server->authorization); - $this->assertSame('still-good', $storage->getToken('https://auth.example.com', self::ENDPOINT)?->refreshToken); + $this->assertSame('still-good', $storage->getToken('https://auth.example.com', self::tokenKey())?->refreshToken); } #[TestWith(['invalid_grant'])] @@ -1636,7 +1669,7 @@ public function testARefusedRefreshTokenStartsANewAuthorization(string $error): $server = new FakeOAuthServer(refreshTokens: true); $server->refreshAnswer = new Response(400, [], (string) json_encode(['error' => $error])); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'revoked')); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'revoked')); $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); @@ -1860,7 +1893,7 @@ public function testClientMetadataUrlRefusedOnRefreshNamesTheDocument(): void $server = new FakeOAuthServer(refreshTokens: true, clientMetadataDocuments: true); $server->unknownClient = 'https://app.example.com/client.json'; $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'refresh')); + $storage->saveToken('https://auth.example.com', self::tokenKey(clientId: 'https://app.example.com/client.json'), new AccessToken('expired', 'Bearer', time() - 60, 'refresh')); $configuration = OAuth::forApplication('test')->setClientMetadataUrl('https://app.example.com/client.json'); try { @@ -1936,7 +1969,7 @@ public function testServiceAccountIgnoresABroaderStoredToken(array $storedScopes { $server = new FakeOAuthServer(); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('broader', 'Bearer', time() + 3600, null, $storedScopes)); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('broader', 'Bearer', time() + 3600, null, $storedScopes)); $authenticator = $this->authenticator($server, storage: $storage, configuration: OAuth::forServiceAccount('job', 'job-client', 'job-secret') ->setExpectedIssuer('https://auth.example.com') ->setScopes('mcp:read')); @@ -1952,7 +1985,7 @@ public function testServiceAccountDoesNotRefreshABroaderStoredToken(): void { $server = new FakeOAuthServer(refreshTokens: true); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('broader', 'Bearer', time() - 60, 'broader-refresh', ['mcp:read', 'admin'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('broader', 'Bearer', time() - 60, 'broader-refresh', ['mcp:read', 'admin'])); $authenticator = $this->authenticator($server, storage: $storage, configuration: OAuth::forServiceAccount('job', 'job-client', 'job-secret') ->setExpectedIssuer('https://auth.example.com') ->setScopes('mcp:read')); @@ -1998,7 +2031,7 @@ public function testServiceAccountStepUpIgnoresStoredScopes(int $expiresIn): voi ->setScopes('mcp:read')); $authenticator->handleChallenge($this->request(), $server->challenge(['mcp:read'])); $current = $authenticator->authenticate($this->request()); - $storage->saveToken('https://auth.example.com', self::ENDPOINT, new AccessToken('broader', 'Bearer', time() + $expiresIn, null, ['mcp:read', 'admin'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('broader', 'Bearer', time() + $expiresIn, null, ['mcp:read', 'admin'])); $server->token = []; $retry = $authenticator->handleChallenge($current, $server->challenge(['admin'], 403, 'insufficient_scope')); @@ -2252,6 +2285,14 @@ public function synchronize(callable $operation): mixed }; } + /** + * The key the authenticator stores a token for ENDPOINT under. + */ + public static function tokenKey(string $grant = 'authorization_code', ?string $clientId = null): string + { + return self::ENDPOINT.'#'.$grant.(null === $clientId ? '' : ' '.$clientId); + } + private function request(): RequestInterface { return (new Psr17Factory())->createRequest('POST', self::ENDPOINT); @@ -2273,7 +2314,7 @@ private function storageWithAnotherProcess(AccessToken $tokenOfOtherProcess): Cr public function __construct(private readonly AccessToken $tokenOfOtherProcess) { $this->storage = new InMemoryCredentialStorage(); - $this->storage->saveToken('https://auth.example.com', OAuthAuthenticatorTest::ENDPOINT, new AccessToken('expired', 'Bearer', time() - 60, 'original')); + $this->storage->saveToken('https://auth.example.com', OAuthAuthenticatorTest::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'original')); } public function getClientRegistration(string $issuer): ?ClientRegistration @@ -2304,7 +2345,7 @@ public function forget(?string $issuer = null): void public function synchronize(callable $operation): mixed { - $this->storage->saveToken('https://auth.example.com', OAuthAuthenticatorTest::ENDPOINT, $this->tokenOfOtherProcess); + $this->storage->saveToken('https://auth.example.com', OAuthAuthenticatorTest::tokenKey(), $this->tokenOfOtherProcess); $this->synchronized = true; try { From e767e5d80843f9bf3b9ad84301f80217e272da1b Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:49:15 +0100 Subject: [PATCH 03/24] [Docs] Name the HTTP clients that follow redirects through PSR-18 --- docs/client/authorization.md | 2 +- docs/client/transports.md | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index 1159fef9..28e759f1 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -291,7 +291,7 @@ The client treats the MCP server and its metadata as untrusted input. It stops w These checks only look at how a URL writes its host. A DNS name that points at loopback or a private network still gets through. If you run the client on a server, block requests to private networks in your HTTP client or proxy (e.g. with Symfony's `NoPrivateNetworkHttpClient`) and pass that client to `setHttpClient()`. Pass it to `HeadlessAuthorizationHandler` as well, which sends its own request to the authorization endpoint. !!! warning - A client that you pass to `setHttpClient()` *must not* follow redirects. The client only turns them off on a `Psr18Client` of `symfony/http-client` 6.2 or later. With redirects on, an authorization server can send the authorization code and the client credentials to any address. + A client that you pass to `setHttpClient()` *must not* follow redirects. The client only turns them off on a `Psr18Client` of `symfony/http-client` 6.2 or later. Guzzle doesn't follow redirects through PSR-18, but an HTTPlug adapter around a Guzzle client with the default handler stack, HTTPlug's `RedirectPlugin` and Symfony's `HttplugClient` do. With redirects on, an authorization server can send the authorization code and the client credentials to any address. The client ignores a `resource_metadata` URL on another origin than the MCP server and looks at the well-known URLs instead. It only sends a token to the resource it was issued for. diff --git a/docs/client/transports.md b/docs/client/transports.md index fd38da2d..9938a529 100644 --- a/docs/client/transports.md +++ b/docs/client/transports.md @@ -149,6 +149,10 @@ The transport turns off redirects on a `symfony/http-client` client that it disc it also turns them off on a `symfony/http-client` client that you pass as `httpClient`. Any other client keeps its own configuration. +Guzzle doesn't follow redirects through PSR-18. An HTTPlug adapter around a Guzzle client with the +default handler stack (e.g. `php-http/guzzle7-adapter`), HTTPlug's `RedirectPlugin` and Symfony's +`HttplugClient` do. + Such a client *must not* follow redirects to another origin. If it does, your headers and the request body reach that origin, and the transport accepts its answer as the server's. With `auth`, it *must not* follow any redirect, because a redirect on the same origin also receives the token. From 83b28c7241098bdd2276a7fbe4b2a0fb2354a4cd Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:49:23 +0100 Subject: [PATCH 04/24] [Client] Note in the changelog what a redirect exposed before 0.10.0 --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea51e742..89e90264 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,7 +95,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * Fix the stdio client hanging on Windows while the server stays silent, without enforcing its timeouts: `StdioTransport` now reads only once `stream_select()` reports output. Requires PHP 8.5, as `stream_select()` on Windows reports an idle pipe as readable before that. * [BC Break] Remove JSON-RPC batch support: a JSON array from the server is dropped with a warning. Batching was removed from MCP in `2025-06-18`. * [BC Break] Fix `HttpTransport` reading a response body that is not an SSE stream without a limit, which could exhaust client memory on a large body. It now reads up to the new `maxResponseBodyBytes` argument (default: 8 MiB): a successful response over that limit fails the request, and an error status keeps its body cut at the limit. Raise the limit if your server sends larger results. -* [BC Break] `HttpTransport` turns off redirects on a discovered `symfony/http-client` client and treats every `3xx` answer as an error that names the status and target. An endpoint that is reached through a redirect stops working, so configure its final URL. A client you pass must not follow redirects to another origin. +* [BC Break] `HttpTransport` turns off redirects on a discovered `symfony/http-client` client and treats every `3xx` answer as an error that names the status and target. Before, a redirect to another origin received the configured headers, such as an API key, and on a `307` or `308` the request body. If an endpoint of yours may have redirected to a host you don't control, rotate those credentials. An endpoint that is reached through a redirect stops working, so configure its final URL. A client you pass must not follow redirects to another origin. * Fix `HttpTransport` taking an `Mcp-Session-Id` from any response: it replaced the session mid-way and, on `2026-07-28`, made `getSessionId()` return a value and `close()` send a `DELETE`. Only the answer to `initialize` sets it. * Add `OAuth::forServiceAccount()` for the client credentials grant, `OAuth::setPrivateKeyJwt()` for `private_key_jwt` client authentication and `OAuth::setCrossAppAccess()` to reach a server through an enterprise identity. * Add `OAuth::setClientMetadataUrl()`, to use a client ID metadata document where the authorization server supports it, and `ConsoleAuthorizationHandler` for a shell without a browser. From 466f892a1745403bb57fe9e8e35879c961fa3a7c Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:51:11 +0100 Subject: [PATCH 05/24] [Client] Refuse an authorization server or endpoint in a private network that a remote server names --- docs/client/authorization.md | 3 +- .../Auth/AuthorizationServerMetadata.php | 5 +++ src/Client/Auth/OAuthAuthenticator.php | 4 +++ src/Server/Transport/Http/OAuth/SecureUrl.php | 30 ++++++++++++++++ .../Auth/AuthorizationServerMetadataTest.php | 29 ++++++++++++++++ .../Client/Auth/OAuthAuthenticatorTest.php | 34 +++++++++++++++++++ .../Transport/Http/OAuth/SecureUrlTest.php | 26 ++++++++++++++ 7 files changed, 130 insertions(+), 1 deletion(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index 28e759f1..85ec60c4 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -279,6 +279,7 @@ The client treats the MCP server and its metadata as untrusted input. It stops w | The MCP server URL, an authorization server URL or an endpoint doesn't use `https` on a host that isn't loopback | Credentials would travel without encryption | | One of these URLs names `0.0.0.0`, `::`, an IPv4 address within IPv6 or an IPv4 address in another form than four decimal numbers (e.g. `127.1`), or writes its host with percent-encoding or non-ASCII characters | The request could reach a local service or a private network without looking like it | | A server that isn't on loopback names a loopback authorization server or endpoint, unless `setExpectedIssuer()` allows that issuer | It could reach services that only listen on this machine | +| A server that isn't on loopback or in a private network names an IP address of a private or link-local network (e.g. `10.0.0.5` or `169.254.169.254`) as authorization server or endpoint, unless `setExpectedIssuer()` allows that issuer | It could reach services of the network the client runs in | | The authorization server metadata doesn't list `S256` in `code_challenge_methods_supported` | The client can't use PKCE with this server | | The authorization endpoint is on another origin than the issuer and the metadata doesn't announce `iss`, without `setExpectedIssuer()` | A response could come from another server, and the code would go to this one | | The authorization response has no `state`, or another `state` | It doesn't answer the request of this client | @@ -288,7 +289,7 @@ The client treats the MCP server and its metadata as untrusted input. It stops w | With `setLegacyDiscovery(true)`, a metadata location answers anything but a `404` and gives no usable metadata | The client only uses the fixed endpoints after a `404` | | The server has no protected resource metadata, without `setLegacyDiscovery(true)` | There is nothing to check the authorization server against | -These checks only look at how a URL writes its host. A DNS name that points at loopback or a private network still gets through. If you run the client on a server, block requests to private networks in your HTTP client or proxy (e.g. with Symfony's `NoPrivateNetworkHttpClient`) and pass that client to `setHttpClient()`. Pass it to `HeadlessAuthorizationHandler` as well, which sends its own request to the authorization endpoint. +These checks only look at how a URL writes its host. A DNS name that points at loopback or a private network still gets through, as the client doesn't resolve names. If you run the client on a server, block requests to private networks in your HTTP client or proxy (e.g. with Symfony's `NoPrivateNetworkHttpClient`) and pass that client to `setHttpClient()`. Pass it to `HeadlessAuthorizationHandler` as well, which sends its own request to the authorization endpoint. !!! warning A client that you pass to `setHttpClient()` *must not* follow redirects. The client only turns them off on a `Psr18Client` of `symfony/http-client` 6.2 or later. Guzzle doesn't follow redirects through PSR-18, but an HTTPlug adapter around a Guzzle client with the default handler stack, HTTPlug's `RedirectPlugin` and Symfony's `HttplugClient` do. With redirects on, an authorization server can send the authorization code and the client credentials to any address. diff --git a/src/Client/Auth/AuthorizationServerMetadata.php b/src/Client/Auth/AuthorizationServerMetadata.php index 29561afd..4d7c751f 100644 --- a/src/Client/Auth/AuthorizationServerMetadata.php +++ b/src/Client/Auth/AuthorizationServerMetadata.php @@ -88,6 +88,11 @@ public static function fromArray(array $data): self throw new InvalidArgumentException(\sprintf('The authorization server metadata of "%s" declares "%s" as "%s", which is a loopback address.', $data['issuer'], $endpoint, $data[$endpoint])); } + // Nor may a server outside a private network send the client into one. + if (SecureUrl::isPrivateNetwork($data[$endpoint]) && \is_string($data['issuer'] ?? null) && !SecureUrl::isPrivateNetwork($data['issuer']) && !SecureUrl::isLoopback($data['issuer'])) { + throw new InvalidArgumentException(\sprintf('The authorization server metadata of "%s" declares "%s" as "%s", which is an address of a private network.', $data['issuer'], $endpoint, $data[$endpoint])); + } + $parts = SecureUrl::parse($data[$endpoint], $endpoint); // RFC 6749 sections 3.1 and 3.2 rule out fragments; RFC 9110 section 4.2.4 treats untrusted userinfo as an error. diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index fa77b881..ce89f9b8 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -274,6 +274,10 @@ private function discover(string $endpoint, AuthorizationChallenge $challenge): throw new AuthorizationException(\sprintf('The protected resource metadata for "%s" names "%s" as its authorization server, which is a loopback address. Call OAuth::setExpectedIssuer() if that is intended.', $endpoint, $issuer)); } + if (SecureUrl::isPrivateNetwork($issuer) && !SecureUrl::isPrivateNetwork($endpoint) && !SecureUrl::isLoopback($endpoint) && $issuer !== $this->configuration->expectedIssuer) { + throw new AuthorizationException(\sprintf('The protected resource metadata for "%s" names "%s" as its authorization server, which is an address of a private network. Call OAuth::setExpectedIssuer() if that is intended.', $endpoint, $issuer)); + } + $server = $this->discovery->discoverAuthorizationServer($issuer); if (null === $server) { diff --git a/src/Server/Transport/Http/OAuth/SecureUrl.php b/src/Server/Transport/Http/OAuth/SecureUrl.php index df7b5676..15881669 100644 --- a/src/Server/Transport/Http/OAuth/SecureUrl.php +++ b/src/Server/Transport/Http/OAuth/SecureUrl.php @@ -66,6 +66,36 @@ public static function isLoopback(string $url): bool return \is_string($host) && self::isLoopbackHost($host); } + /** + * Whether a URL names an address of a private or link-local network as an IP literal; a name is not resolved. + */ + public static function isPrivateNetwork(string $url): bool + { + $host = parse_url($url, \PHP_URL_HOST); + $host = \is_string($host) ? trim($host, '[]') : ''; + + if (false === filter_var($host, \FILTER_VALIDATE_IP)) { + return false; + } + + $address = (string) inet_pton($host); + $ranges = 4 === \strlen($address) + ? [['10.0.0.0', 8], ['172.16.0.0', 12], ['192.168.0.0', 16], ['169.254.0.0', 16], ['100.64.0.0', 10]] + : [['fc00::', 7], ['fe80::', 10]]; + + foreach ($ranges as [$network, $bits]) { + $prefix = (string) inet_pton($network); + $bytes = intdiv($bits, 8); + $mask = 0xFF << (8 - $bits % 8) & 0xFF; + + if (0 === substr_compare($address, $prefix, 0, $bytes) && (0 === $bits % 8 || (\ord($address[$bytes]) & $mask) === \ord($prefix[$bytes]))) { + return true; + } + } + + return false; + } + // Each of these can reach this machine or a private network without looking like it. private static function hidesItsAddress(string $host): bool { diff --git a/tests/Unit/Client/Auth/AuthorizationServerMetadataTest.php b/tests/Unit/Client/Auth/AuthorizationServerMetadataTest.php index a8575f31..dfd14a19 100644 --- a/tests/Unit/Client/Auth/AuthorizationServerMetadataTest.php +++ b/tests/Unit/Client/Auth/AuthorizationServerMetadataTest.php @@ -156,6 +156,35 @@ public function testAcceptsLoopbackEndpointsOfALoopbackServer(): void $this->assertSame('http://localhost:9000/token', $metadata->tokenEndpoint); } + #[TestWith(['token_endpoint'])] + #[TestWith(['authorization_endpoint'])] + #[TestWith(['registration_endpoint'])] + #[TestDox('a remote authorization server can not name an endpoint in a private network')] + public function testRejectsAPrivateNetworkEndpointOfARemoteServer(string $endpoint): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('private network'); + + AuthorizationServerMetadata::fromArray([ + 'issuer' => 'https://auth.example.com', + 'authorization_endpoint' => 'https://auth.example.com/authorize', + 'token_endpoint' => 'https://auth.example.com/token', + $endpoint => 'https://169.254.169.254/x', + ]); + } + + #[TestDox('an authorization server in a private network can name endpoints there')] + public function testAcceptsPrivateNetworkEndpointsOfAPrivateServer(): void + { + $metadata = AuthorizationServerMetadata::fromArray([ + 'issuer' => 'https://10.0.0.5', + 'authorization_endpoint' => 'https://10.0.0.5/authorize', + 'token_endpoint' => 'https://10.0.0.6/token', + ]); + + $this->assertSame('https://10.0.0.6/token', $metadata->tokenEndpoint); + } + #[TestDox('an unstated grant list means the authorization code grant, and nothing else')] public function testDefaultGrantTypes(): void { diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index ce2c39c4..85e03892 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -980,6 +980,40 @@ public function testRefusesALoopbackIssuerOfARemoteServer(string $issuer): void $this->assertSame([], array_filter($server->requested, static fn (string $url): bool => str_starts_with($url, $issuer))); } + #[TestWith(['https://169.254.169.254'])] + #[TestWith(['https://10.0.0.5'])] + #[TestWith(['https://[fd00::1]'])] + #[TestDox('a remote server can not make the client request an address of a private network')] + public function testRefusesAPrivateNetworkIssuerOfARemoteServer(string $issuer): void + { + $server = new FakeOAuthServer(authorizationServer: $issuer); + + try { + $this->authenticator($server)->handleChallenge($this->request(), $server->challenge()); + $this->fail('The authorization server in a private network was not refused.'); + } catch (AuthorizationException $e) { + $this->assertStringContainsString('private network', $e->getMessage()); + } + + $this->assertSame([], array_filter($server->requested, static fn (string $url): bool => str_starts_with($url, $issuer))); + } + + #[TestDox('an application can allow an authorization server in a private network with the expected issuer')] + public function testExpectedIssuerAllowsAPrivateNetworkIssuer(): void + { + $server = new FakeOAuthServer(authorizationServer: 'https://10.0.0.5'); + $configuration = OAuth::forApplication('test')->setExpectedIssuer('https://10.0.0.5'); + + try { + $this->authenticator($server, configuration: $configuration)->handleChallenge($this->request(), $server->challenge()); + $this->fail('The fake server has no metadata for this issuer.'); + } catch (AuthorizationException $e) { + $this->assertStringContainsString('No usable metadata', $e->getMessage()); + } + + $this->assertNotSame([], array_filter($server->requested, static fn (string $url): bool => str_contains($url, '10.0.0.5'))); + } + #[TestDox('another type of token from the identity provider is not presented as a grant')] public function testCrossAppAccessRefusesAnotherTokenType(): void { diff --git a/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php b/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php index cccd0c14..ba3c419f 100644 --- a/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php +++ b/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php @@ -129,4 +129,30 @@ public function testDetectsLoopbackUrls(string $url, bool $loopback): void { $this->assertSame($loopback, SecureUrl::isLoopback($url)); } + + /** + * @return iterable + */ + public static function providePrivateNetworkUrls(): iterable + { + yield '10/8' => ['https://10.0.0.5/x', true]; + yield 'upper end of 172.16/12' => ['https://172.31.255.1/x', true]; + yield 'above 172.16/12' => ['https://172.32.0.1/x', false]; + yield '192.168/16' => ['https://192.168.1.1/x', true]; + yield 'link-local metadata service' => ['https://169.254.169.254/x', true]; + yield 'shared address space' => ['https://100.100.100.200/x', true]; + yield 'above the shared address space' => ['https://100.128.0.1/x', false]; + yield 'unique local IPv6' => ['https://[fd00::1]/x', true]; + yield 'link-local IPv6' => ['https://[fe80::1]/x', true]; + yield 'public IPv4' => ['https://8.8.8.8/x', false]; + yield 'public IPv6' => ['https://[2001:db8::1]/x', false]; + yield 'loopback' => ['https://127.0.0.1/x', false]; + yield 'a name, which is not resolved' => ['https://internal.corp/x', false]; + } + + #[DataProvider('providePrivateNetworkUrls')] + public function testDetectsPrivateNetworkUrls(string $url, bool $private): void + { + $this->assertSame($private, SecureUrl::isPrivateNetwork($url)); + } } From 64878b26d3c3b95eac7627aefd09ba2ef40c621d Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:52:08 +0100 Subject: [PATCH 06/24] [Client] Turn off redirects on a Symfony client passed to HttpTransport without auth --- CHANGELOG.md | 2 +- docs/client/transports.md | 16 +++++++-------- src/Client/Transport/HttpTransport.php | 2 +- .../Client/Transport/HttpTransportTest.php | 20 +++++++++++++++++++ 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89e90264..ec77cdb1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -95,7 +95,7 @@ All notable changes to `mcp/sdk` will be documented in this file. * Fix the stdio client hanging on Windows while the server stays silent, without enforcing its timeouts: `StdioTransport` now reads only once `stream_select()` reports output. Requires PHP 8.5, as `stream_select()` on Windows reports an idle pipe as readable before that. * [BC Break] Remove JSON-RPC batch support: a JSON array from the server is dropped with a warning. Batching was removed from MCP in `2025-06-18`. * [BC Break] Fix `HttpTransport` reading a response body that is not an SSE stream without a limit, which could exhaust client memory on a large body. It now reads up to the new `maxResponseBodyBytes` argument (default: 8 MiB): a successful response over that limit fails the request, and an error status keeps its body cut at the limit. Raise the limit if your server sends larger results. -* [BC Break] `HttpTransport` turns off redirects on a discovered `symfony/http-client` client and treats every `3xx` answer as an error that names the status and target. Before, a redirect to another origin received the configured headers, such as an API key, and on a `307` or `308` the request body. If an endpoint of yours may have redirected to a host you don't control, rotate those credentials. An endpoint that is reached through a redirect stops working, so configure its final URL. A client you pass must not follow redirects to another origin. +* [BC Break] `HttpTransport` turns off redirects on a `symfony/http-client` client, discovered or passed, and treats every `3xx` answer as an error that names the status and target. Before, a redirect to another origin received the configured headers, such as an API key, and on a `307` or `308` the request body. If an endpoint of yours may have redirected to a host you don't control, rotate those credentials. An endpoint that is reached through a redirect stops working, so configure its final URL. A client you pass must not follow redirects to another origin. * Fix `HttpTransport` taking an `Mcp-Session-Id` from any response: it replaced the session mid-way and, on `2026-07-28`, made `getSessionId()` return a value and `close()` send a `DELETE`. Only the answer to `initialize` sets it. * Add `OAuth::forServiceAccount()` for the client credentials grant, `OAuth::setPrivateKeyJwt()` for `private_key_jwt` client authentication and `OAuth::setCrossAppAccess()` to reach a server through an enterprise identity. * Add `OAuth::setClientMetadataUrl()`, to use a client ID metadata document where the authorization server supports it, and `ConsoleAuthorizationHandler` for a shell without a browser. diff --git a/docs/client/transports.md b/docs/client/transports.md index 9938a529..14403254 100644 --- a/docs/client/transports.md +++ b/docs/client/transports.md @@ -145,19 +145,19 @@ If the server answers a request with a `3xx` status, the request fails at once w `ConnectionException` at once, with no retry and no fallback to the `initialize` handshake. To reach a server that moved, configure its new URL as `endpoint`. -The transport turns off redirects on a `symfony/http-client` client that it discovers. With `auth`, -it also turns them off on a `symfony/http-client` client that you pass as `httpClient`. Any other -client keeps its own configuration. - -Guzzle doesn't follow redirects through PSR-18. An HTTPlug adapter around a Guzzle client with the -default handler stack (e.g. `php-http/guzzle7-adapter`), HTTPlug's `RedirectPlugin` and Symfony's -`HttplugClient` do. +The transport turns off redirects on a `symfony/http-client` client, whether it discovers the client +or you pass it as `httpClient`. Any other client keeps its own configuration. Such a client *must not* follow redirects to another origin. If it does, your headers and the request body reach that origin, and the transport accepts its answer as the server's. With `auth`, it *must not* follow any redirect, because a redirect on the same origin also receives the token. -To pass a `symfony/http-client` client, create it with `max_redirects` set to `0`: +Guzzle doesn't follow redirects through PSR-18. An HTTPlug adapter around a Guzzle client with the +default handler stack (e.g. `php-http/guzzle7-adapter`), HTTPlug's `RedirectPlugin` and Symfony's +`HttplugClient` do. + +Before `symfony/http-client` 6.2, the transport can't change a client that you pass. Create it with +`max_redirects` set to `0`: ```php use Symfony\Component\HttpClient\HttpClient; diff --git a/src/Client/Transport/HttpTransport.php b/src/Client/Transport/HttpTransport.php index 687b8747..a3d777a1 100644 --- a/src/Client/Transport/HttpTransport.php +++ b/src/Client/Transport/HttpTransport.php @@ -160,7 +160,7 @@ public function __construct( $this->listenParser = new SseParser($maxSseBufferBytes); $this->reconnectPolicy = $reconnectPolicy ?? new ReconnectPolicy(); $this->clock = $clock ?? new MonotonicClock(); - $client = $httpClient ?? NoRedirects::discover(); + $client = null === $httpClient ? NoRedirects::discover() : NoRedirects::apply($httpClient); $this->httpClient = null === $auth ? $client : new AuthenticatingHttpClient($client, $auth); $this->requestFactory = $requestFactory ?? Psr17FactoryDiscovery::findRequestFactory(); $this->streamFactory = $streamFactory ?? Psr17FactoryDiscovery::findStreamFactory(); diff --git a/tests/Unit/Client/Transport/HttpTransportTest.php b/tests/Unit/Client/Transport/HttpTransportTest.php index 294edcfd..96c34458 100644 --- a/tests/Unit/Client/Transport/HttpTransportTest.php +++ b/tests/Unit/Client/Transport/HttpTransportTest.php @@ -743,6 +743,26 @@ public function testReconnectStatusHandling(int $status, int $requests): void $this->assertCount($requests, $httpClient->requests); } + #[TestDox('a Symfony client passed without auth stops following redirects')] + public function testPassedSymfonyClientStopsFollowingRedirects(): void + { + if (!(new \ReflectionClass(Psr18Client::class))->hasMethod('withOptions')) { + $this->markTestSkipped('Psr18Client has no withOptions() before symfony/http-client 6.2.'); + } + + $maxRedirects = null; + $mock = new MockHttpClient(static function (string $method, string $url, array $options) use (&$maxRedirects): MockResponse { + $maxRedirects = $options['max_redirects']; + + return new MockResponse('', ['http_code' => 202]); + }); + + $transport = new HttpTransport('http://localhost/mcp', httpClient: new Psr18Client($mock), requestFactory: $this->factory, streamFactory: $this->factory); + $transport->send('{"jsonrpc":"2.0","method":"notifications/initialized"}'); + + $this->assertSame(0, $maxRedirects); + } + #[TestDox('a discovered Symfony client keeps its configuration and stops following redirects')] public function testDiscoveredClientKeepsItsConfiguration(): void { From 70e869047f19908467b997812745aad9c8df2fe1 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:53:14 +0100 Subject: [PATCH 07/24] [Client] Try the next metadata location when a document describes another resource --- docs/client/authorization.md | 2 +- src/Client/Auth/MetadataDiscovery.php | 20 +++++++++++-- .../Client/Auth/MetadataDiscoveryTest.php | 28 +++++++++++++++++++ 3 files changed, 47 insertions(+), 3 deletions(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index 85ec60c4..e0407d86 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -274,7 +274,7 @@ The client treats the MCP server and its metadata as untrusted input. It stops w | Input | Reason | | --- | --- | -| The protected resource metadata names a `resource` that doesn't cover the endpoint | The token would be issued for another server | +| No location of the protected resource metadata names a `resource` that covers the endpoint | The token would be issued for another server | | The authorization server metadata names another `issuer` than the one it was fetched for | The document describes another server | | The MCP server URL, an authorization server URL or an endpoint doesn't use `https` on a host that isn't loopback | Credentials would travel without encryption | | One of these URLs names `0.0.0.0`, `::`, an IPv4 address within IPv6 or an IPv4 address in another form than four decimal numbers (e.g. `127.1`), or writes its host with percent-encoding or non-ASCII characters | The request could reach a local service or a private network without looking like it | diff --git a/src/Client/Auth/MetadataDiscovery.php b/src/Client/Auth/MetadataDiscovery.php index 9e47f8ba..c39c2cae 100644 --- a/src/Client/Auth/MetadataDiscovery.php +++ b/src/Client/Auth/MetadataDiscovery.php @@ -39,9 +39,13 @@ public function __construct( * * @param string $resourceUrl the MCP endpoint being called * @param ?string $metadataUrl the location the challenge named, which wins over any guess + * + * @return ?ProtectedResourceMetadata the first document that covers the endpoint, else the first for another resource, which the caller must refuse */ public function discoverProtectedResource(string $resourceUrl, ?string $metadataUrl = null): ?ProtectedResourceMetadata { + $unrelated = null; + foreach (null !== $metadataUrl ? [$metadataUrl] : self::protectedResourceCandidates($resourceUrl) as $candidate) { if (!AuthorizationServerMetadata::isTransportSecure($candidate)) { $this->logger->error('Refusing protected resource discovery: the metadata URL is neither an HTTPS URL nor a loopback address.', ['url' => $candidate]); @@ -56,13 +60,25 @@ public function discoverProtectedResource(string $resourceUrl, ?string $metadata } try { - return ProtectedResourceMetadata::fromArray($document); + $metadata = ProtectedResourceMetadata::fromArray($document); } catch (InvalidArgumentException $e) { $this->logger->warning('Ignoring malformed protected resource metadata', ['url' => $candidate, 'exception' => $e]); + + continue; } + + // RFC 9728 section 3.3: the document must not be used, but a later location may describe this resource. + if (!$metadata->covers($resourceUrl)) { + $this->logger->warning('Ignoring protected resource metadata for another resource', ['url' => $candidate, 'resource' => $metadata->resource]); + $unrelated ??= $metadata; + + continue; + } + + return $metadata; } - return null; + return $unrelated; } /** diff --git a/tests/Unit/Client/Auth/MetadataDiscoveryTest.php b/tests/Unit/Client/Auth/MetadataDiscoveryTest.php index a783a274..43f03003 100644 --- a/tests/Unit/Client/Auth/MetadataDiscoveryTest.php +++ b/tests/Unit/Client/Auth/MetadataDiscoveryTest.php @@ -307,6 +307,34 @@ public function testHonoursTheChallengeMetadataUrl(): void $this->assertSame(['https://mcp.example.com/custom/location.json'], $client->requested); } + #[TestDox('a document for another resource does not stop a later location')] + public function testContinuesAfterAnUnrelatedResource(): void + { + $path = 'https://mcp.example.com/.well-known/oauth-protected-resource/mcp'; + $root = 'https://mcp.example.com/.well-known/oauth-protected-resource'; + $client = new RecordingClient([ + $path => self::json(['resource' => 'https://mcp.example.com/other']), + $root => self::json(['resource' => 'https://mcp.example.com']), + ]); + + $metadata = $this->discovery($client)->discoverProtectedResource('https://mcp.example.com/mcp'); + + $this->assertSame('https://mcp.example.com', $metadata?->resource); + $this->assertSame([$path, $root], $client->requested); + } + + #[TestDox('a document for another resource is returned if no location describes the endpoint, for the caller to refuse')] + public function testReturnsTheUnrelatedResourceIfNothingElseIsFound(): void + { + $client = new RecordingClient([ + 'https://mcp.example.com/.well-known/oauth-protected-resource/mcp' => self::json(['resource' => 'https://evil.example.com/mcp']), + ]); + + $metadata = $this->discovery($client)->discoverProtectedResource('https://mcp.example.com/mcp'); + + $this->assertSame('https://evil.example.com/mcp', $metadata?->resource); + } + #[TestDox('an invalid resource identifier does not stop a later location')] public function testContinuesAfterAnInvalidResource(): void { From c40195726e455bf11fffa3ad3b09777a5efe66f6 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:56:11 +0100 Subject: [PATCH 08/24] [Client] Refresh a token before it expires instead of waiting for the server to reject it --- docs/client/authorization.md | 2 +- src/Client/Auth/OAuthAuthenticator.php | 124 +++++++++++++++--- .../Client/Auth/OAuthAuthenticatorTest.php | 48 ++++++- 3 files changed, 151 insertions(+), 23 deletions(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index e0407d86..781556d1 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -248,7 +248,7 @@ OAuth::forApplication('My App')->setScopes('mcp:read', 'mcp:write'); If the server answers with `insufficient_scope`, the client adds the scopes of this challenge and authorizes again. It doesn't authorize again for scopes that it already requested. With any other `403`, the client doesn't authorize again. -If the authorization server supports `offline_access`, the client requests it to get a refresh token. If the server refuses this scope, the client tries again without it. Use `setOfflineAccess(false)` to never request this scope. The server can still issue a refresh token, and the client then uses it. +If the authorization server supports `offline_access`, the client requests it to get a refresh token. If the server refuses this scope, the client tries again without it. Use `setOfflineAccess(false)` to never request this scope. The server can still issue a refresh token, and the client then uses it. It refreshes a token shortly before it expires, so the server doesn't have to reject a request first. ## Connecting to Older Servers diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index ce89f9b8..bb62699d 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -42,6 +42,10 @@ final class OAuthAuthenticator implements AuthenticatorInterface private bool $offlineAccessRefused = false; + /** Where the current token came from, so it can be refreshed before it expires. */ + private ?AuthorizationServerMetadata $metadata = null; + private ?ClientRegistration $client = null; + public function __construct( private readonly OAuthConfiguration $configuration, private readonly CredentialStorageInterface $storage, @@ -55,7 +59,9 @@ public function __construct( public function authenticate(RequestInterface $request): RequestInterface { - if (null === $this->token || $this->token->isExpired()) { + $token = $this->token; + + if (null === $token) { return $request; } @@ -64,7 +70,15 @@ public function authenticate(RequestInterface $request): RequestInterface return $request; } - return $request->withHeader('Authorization', $this->token->getAuthorizationHeader()); + if ($token->isExpiringSoon()) { + $token = $this->refreshBeforeSending($token, $request, $this->audience); + } + + if ($token->isExpired()) { + return $request; + } + + return $request->withHeader('Authorization', $token->getAuthorizationHeader()); } public function handleChallenge(RequestInterface $request, ResponseInterface $response): bool @@ -120,6 +134,8 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re $this->logger->debug('Reusing a stored access token', ['issuer' => $metadata->issuer, 'resource' => $resource]); $this->token = $stored; $this->audience = $resource; + $this->metadata = $metadata; + $this->client = null; return true; } @@ -164,6 +180,8 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re $this->token = $token; $this->audience = $resource; + $this->metadata = $metadata; + $this->client = $client; return true; } @@ -212,6 +230,8 @@ public function finishAuthorization(array $callback): void $this->token = null; $this->audience = null; + $this->metadata = null; + $this->client = null; $this->requestedScopes = []; $this->issuer = $metadata->issuer; $this->resource = $pending->resource; @@ -241,6 +261,8 @@ public function finishAuthorization(array $callback): void $this->token = $token->withGrantedScopes($scopes); $this->requestedScopes = $scopes; $this->audience = $pending->resource; + $this->metadata = $metadata; + $this->client = $client; $this->storage->saveToken($metadata->issuer, $this->tokenKey($pending->resource), $this->token); } @@ -449,6 +471,39 @@ private function scopeLimit(): ?array return $this->configuration->scopes; } + /** + * Saves the round trip of a rejected request; on any failure the server's challenge starts the usual flow. + */ + private function refreshBeforeSending(AccessToken $token, RequestInterface $request, string $resource): AccessToken + { + if (null === $token->refreshToken || null === $this->metadata) { + return $token; + } + + $client = $this->client ?? $this->knownClient($this->metadata); + + if (null === $client) { + return $token; + } + + try { + $refreshed = $this->refresh($token, $this->metadata, $client, array_values(array_diff($token->scopes, ['offline_access'])), $resource, $request); + } catch (AuthorizationException $e) { + $this->logger->info('Refreshing the access token before sending failed; leaving it to the server\'s challenge.', ['exception' => $e]); + + return $token; + } + + if (null === $refreshed) { + return $token; + } + + $this->token = $refreshed; + $this->client = $client; + + return $refreshed; + } + /** * Trade a refresh token for a new access token, one process at a time as a rotated refresh token can only be spent once. * @@ -725,24 +780,10 @@ private function verifyCallback(array $callback, string $state, AuthorizationSer private function resolveClient(AuthorizationServerMetadata $metadata, array $scopes, ?array $scopesSupported): ClientRegistration { $configuration = $this->configuration; + $client = $this->configuredClient($metadata); - if (null !== $configuration->clientId) { - return new ClientRegistration( - $configuration->clientId, - $configuration->clientSecret, - TokenEndpointAuthMethod::negotiate( - $metadata->tokenEndpointAuthMethodsSupported, - null !== $configuration->clientSecret, - null !== $configuration->privateKeyPem, - Grant::ClientCredentials !== $configuration->grant, - ), - ); - } - - if ($metadata->clientIdMetadataDocumentSupported && null !== $configuration->clientMetadataUrl) { - $this->logger->debug('Using the client metadata document URL as the client id', ['client_id' => $configuration->clientMetadataUrl]); - - return new ClientRegistration($configuration->clientMetadataUrl, null, TokenEndpointAuthMethod::negotiate($metadata->tokenEndpointAuthMethodsSupported, false)); + if (null !== $client) { + return $client; } // Another process may register at the same time, and its tokens would not work with this client. @@ -784,6 +825,51 @@ private function tokenKey(string $resource): string return $resource.'#'.$this->configuration->grant->value.(null === $client ? '' : ' '.$client); } + /** + * The client identity that needs no registration: a configured client or a client metadata document. + */ + private function configuredClient(AuthorizationServerMetadata $metadata): ?ClientRegistration + { + $configuration = $this->configuration; + + if (null !== $configuration->clientId) { + return new ClientRegistration( + $configuration->clientId, + $configuration->clientSecret, + TokenEndpointAuthMethod::negotiate( + $metadata->tokenEndpointAuthMethodsSupported, + null !== $configuration->clientSecret, + null !== $configuration->privateKeyPem, + Grant::ClientCredentials !== $configuration->grant, + ), + ); + } + + if ($metadata->clientIdMetadataDocumentSupported && null !== $configuration->clientMetadataUrl) { + $this->logger->debug('Using the client metadata document URL as the client id', ['client_id' => $configuration->clientMetadataUrl]); + + return new ClientRegistration($configuration->clientMetadataUrl, null, TokenEndpointAuthMethod::negotiate($metadata->tokenEndpointAuthMethodsSupported, false)); + } + + return null; + } + + /** + * The client identity in use, without registering one. + */ + private function knownClient(AuthorizationServerMetadata $metadata): ?ClientRegistration + { + $client = $this->configuredClient($metadata); + + if (null !== $client) { + return $client; + } + + $stored = $this->storage->getClientRegistration($metadata->issuer); + + return null !== $stored && !$stored->isExpired() && $this->matchesRedirectUris($stored) ? $stored : null; + } + /** * @param array $parameters * diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index 85e03892..d04e540a 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -1413,9 +1413,51 @@ public function authorize(string $authorizationUrl, string $redirectUri): array $this->assertSame('0', $server->token['code'] ?? null); } - /** - * @param ?string[] $grantTypes - */ + #[TestDox('a token that is about to expire is refreshed before the request is sent')] + public function testRefreshesBeforeSending(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $server->expiresIn = 10; + $authenticator = $this->authenticator($server); + $authenticator->handleChallenge($this->request(), $server->challenge()); + $server->expiresIn = 3600; + + $request = $authenticator->authenticate($this->request()); + + $this->assertSame('refresh_token', $server->token['grant_type'] ?? null); + $this->assertSame('Bearer access-token-2', $request->getHeaderLine('Authorization')); + } + + #[TestDox('a refused refresh before sending keeps the token until it expires, and leaves the rest to the server\'s challenge')] + public function testRefusedRefreshBeforeSendingKeepsTheToken(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $server->expiresIn = 10; + $authenticator = $this->authenticator($server); + $authenticator->handleChallenge($this->request(), $server->challenge()); + $server->refreshAnswer = new Response(400, [], (string) json_encode(['error' => 'invalid_client'])); + + $request = $authenticator->authenticate($this->request()); + + $this->assertSame('refresh_token', $server->token['grant_type'] ?? null); + $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); + } + + #[TestDox('a token without a refresh token is sent until it expires, without a token request')] + public function testNoRefreshBeforeSendingWithoutARefreshToken(): void + { + $server = new FakeOAuthServer(); + $server->expiresIn = 10; + $authenticator = $this->authenticator($server); + $authenticator->handleChallenge($this->request(), $server->challenge()); + $requests = \count($server->requested); + + $request = $authenticator->authenticate($this->request()); + + $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); + $this->assertCount($requests, $server->requested); + } + #[TestWith([null, 'refresh_token'])] #[TestWith([['authorization_code', 'refresh_token'], 'refresh_token'])] #[TestWith([['authorization_code'], 'authorization_code'])] From 0475224b9755a93f4d86eecd714f75a0f30e11b5 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 02:57:42 +0100 Subject: [PATCH 09/24] [Client] Don't retry a request whose body can't tell or seek instead of throwing a stream error --- src/Client/Auth/AuthenticatingHttpClient.php | 23 +++++++++++-- .../Auth/AuthenticatingHttpClientTest.php | 34 +++++++++++++------ 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/src/Client/Auth/AuthenticatingHttpClient.php b/src/Client/Auth/AuthenticatingHttpClient.php index bbba8238..5aadb7be 100644 --- a/src/Client/Auth/AuthenticatingHttpClient.php +++ b/src/Client/Auth/AuthenticatingHttpClient.php @@ -17,6 +17,7 @@ use Psr\Http\Client\ClientInterface; use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\StreamInterface; /** * A PSR-18 client that authenticates every request and retries the ones the @@ -63,7 +64,7 @@ public function sendUnchallenged(RequestInterface $request): ResponseInterface public function sendRequest(RequestInterface $request): ResponseInterface { $body = $request->getBody(); - $position = $body->isSeekable() ? $body->tell() : null; + $position = self::replayPosition($body); $sent = $this->authenticator->authenticate($request); $response = $this->client->sendRequest($sent); @@ -97,8 +98,13 @@ public function sendRequest(RequestInterface $request): ResponseInterface return $response; } + try { + $body->seek($position); + } catch (\RuntimeException) { + return $response; + } + $response->getBody()->close(); - $body->seek($position); $sent = $this->authenticator->authenticate($request); $response = $this->client->sendRequest($sent); @@ -106,4 +112,17 @@ public function sendRequest(RequestInterface $request): ResponseInterface return $response; } + + private static function replayPosition(StreamInterface $body): ?int + { + if (!$body->isSeekable()) { + return null; + } + + try { + return $body->tell(); + } catch (\RuntimeException) { + return null; + } + } } diff --git a/tests/Unit/Client/Auth/AuthenticatingHttpClientTest.php b/tests/Unit/Client/Auth/AuthenticatingHttpClientTest.php index 974081f1..25a0d9f1 100644 --- a/tests/Unit/Client/Auth/AuthenticatingHttpClientTest.php +++ b/tests/Unit/Client/Auth/AuthenticatingHttpClientTest.php @@ -200,8 +200,8 @@ public function testFailedChallengeIsAClientException(): void ->sendRequest((new Psr17Factory())->createRequest('POST', 'https://mcp.example.com/mcp')); } - #[TestDox('a body that fails to seek leaves no rejected response open')] - public function testClosesTheRejectedResponseWhenTheBodyCannotSeek(): void + #[TestDox('a body that fails to seek ends the exchange with the rejected response, inside the PSR-18 contract')] + public function testReturnsTheRejectedResponseWhenTheBodyCannotSeek(): void { $rejected = new Response(401, [], 'body'); $inner = $this->createStub(ClientInterface::class); @@ -212,15 +212,29 @@ public function testClosesTheRejectedResponseWhenTheBodyCannotSeek(): void $body->method('tell')->willReturn(0); $body->method('seek')->willThrowException(new \RuntimeException('Unable to seek.')); - try { - (new AuthenticatingHttpClient($inner, new CountingAuthenticator())) - ->sendRequest((new Psr17Factory())->createRequest('POST', 'https://mcp.example.com/mcp')->withBody($body)); - $this->fail('The seek failure should have reached the caller.'); - } catch (\RuntimeException $e) { - $this->assertSame('Unable to seek.', $e->getMessage()); - } + $response = (new AuthenticatingHttpClient($inner, new CountingAuthenticator())) + ->sendRequest((new Psr17Factory())->createRequest('POST', 'https://mcp.example.com/mcp')->withBody($body)); - $this->assertFalse($rejected->getBody()->isReadable()); + $this->assertSame($rejected, $response); + $this->assertTrue($response->getBody()->isReadable()); + } + + #[TestDox('a body that fails to tell its position is not replayed')] + public function testDoesNotRetryABodyThatCannotTell(): void + { + $body = $this->createStub(StreamInterface::class); + $body->method('isSeekable')->willReturn(true); + $body->method('tell')->willThrowException(new \RuntimeException('Unable to tell.')); + + $inner = new RejectingClient(1); + $authenticator = new CountingAuthenticator(); + + $response = (new AuthenticatingHttpClient($inner, $authenticator)) + ->sendRequest((new Psr17Factory())->createRequest('POST', 'https://mcp.example.com/mcp')->withBody($body)); + + $this->assertSame(401, $response->getStatusCode()); + $this->assertSame(1, $inner->calls); + $this->assertSame(1, $authenticator->challenges); } #[TestDox('a body that cannot be replayed ends the exchange after the challenge')] From 61bbbcc1fbefdef6a5f2c72121af98ed2c391403 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 11:44:44 +0100 Subject: [PATCH 10/24] [Client] Credit bbdu3 as author of the deferred authorization classes --- src/Client/Auth/PendingAuthorization.php | 2 +- src/Client/Auth/PendingAuthorizationStorageInterface.php | 2 +- src/Client/Auth/RedirectAuthorizationHandler.php | 2 +- src/Exception/AuthorizationPendingException.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Client/Auth/PendingAuthorization.php b/src/Client/Auth/PendingAuthorization.php index 78126bc1..c4ee0709 100644 --- a/src/Client/Auth/PendingAuthorization.php +++ b/src/Client/Auth/PendingAuthorization.php @@ -14,7 +14,7 @@ use Mcp\Exception\InvalidArgumentException; /** - * @author PHP MCP SDK contributors + * @author bbdu3 */ final class PendingAuthorization implements \JsonSerializable { diff --git a/src/Client/Auth/PendingAuthorizationStorageInterface.php b/src/Client/Auth/PendingAuthorizationStorageInterface.php index 6e110c1b..047cc2a7 100644 --- a/src/Client/Auth/PendingAuthorizationStorageInterface.php +++ b/src/Client/Auth/PendingAuthorizationStorageInterface.php @@ -14,7 +14,7 @@ /** * Storage must be scoped to the user session that initiated authorization. * - * @author PHP MCP SDK contributors + * @author bbdu3 */ interface PendingAuthorizationStorageInterface extends CredentialStorageInterface { diff --git a/src/Client/Auth/RedirectAuthorizationHandler.php b/src/Client/Auth/RedirectAuthorizationHandler.php index 130cd9e3..bab252db 100644 --- a/src/Client/Auth/RedirectAuthorizationHandler.php +++ b/src/Client/Auth/RedirectAuthorizationHandler.php @@ -14,7 +14,7 @@ use Mcp\Exception\AuthorizationPendingException; /** - * @author PHP MCP SDK contributors + * @author bbdu3 */ final class RedirectAuthorizationHandler implements AuthorizationHandlerInterface { diff --git a/src/Exception/AuthorizationPendingException.php b/src/Exception/AuthorizationPendingException.php index ad3ffd82..42a62ee9 100644 --- a/src/Exception/AuthorizationPendingException.php +++ b/src/Exception/AuthorizationPendingException.php @@ -12,7 +12,7 @@ namespace Mcp\Exception; /** - * @author PHP MCP SDK contributors + * @author bbdu3 */ final class AuthorizationPendingException extends AuthorizationException { From 3f7b8634083981e07be74f52abb68ed2a54aa24b Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 11:51:01 +0100 Subject: [PATCH 11/24] [Client] Keep the token when the storage fails during the refresh before sending --- src/Client/Auth/OAuthAuthenticator.php | 3 +- .../Client/Auth/OAuthAuthenticatorTest.php | 59 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index bb62699d..edf30940 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -14,6 +14,7 @@ use Mcp\Exception\AuthorizationException; use Mcp\Exception\AuthorizationPendingException; use Mcp\Exception\OAuthErrorException; +use Mcp\Exception\RuntimeException; use Mcp\Exception\TokenEndpointUnavailableException; use Mcp\Server\Transport\Http\OAuth\SecureUrl; use Psr\Http\Message\RequestInterface; @@ -488,7 +489,7 @@ private function refreshBeforeSending(AccessToken $token, RequestInterface $requ try { $refreshed = $this->refresh($token, $this->metadata, $client, array_values(array_diff($token->scopes, ['offline_access'])), $resource, $request); - } catch (AuthorizationException $e) { + } catch (AuthorizationException|RuntimeException $e) { $this->logger->info('Refreshing the access token before sending failed; leaving it to the server\'s challenge.', ['exception' => $e]); return $token; diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index d04e540a..76edd633 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -31,6 +31,7 @@ use Mcp\Exception\AuthorizationException; use Mcp\Exception\AuthorizationPendingException; use Mcp\Exception\OAuthErrorException; +use Mcp\Exception\RuntimeException; use Nyholm\Psr7\Factory\Psr17Factory; use Nyholm\Psr7\Response; use PHPUnit\Framework\Attributes\DataProvider; @@ -1443,6 +1444,64 @@ public function testRefusedRefreshBeforeSendingKeepsTheToken(): void $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); } + #[TestDox('a storage failure during the refresh before sending keeps the token until it expires')] + public function testStorageFailureBeforeSendingKeepsTheToken(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $server->expiresIn = 10; + $storage = new class implements CredentialStorageInterface { + public bool $broken = false; + + private readonly InMemoryCredentialStorage $storage; + + public function __construct() + { + $this->storage = new InMemoryCredentialStorage(); + } + + public function getClientRegistration(string $issuer): ?ClientRegistration + { + return $this->storage->getClientRegistration($issuer); + } + + public function saveClientRegistration(string $issuer, ClientRegistration $registration): void + { + $this->storage->saveClientRegistration($issuer, $registration); + } + + public function getToken(string $issuer, string $resource): ?AccessToken + { + return $this->storage->getToken($issuer, $resource); + } + + public function saveToken(string $issuer, string $resource, AccessToken $token): void + { + $this->storage->saveToken($issuer, $resource, $token); + } + + public function forget(?string $issuer = null): void + { + $this->storage->forget($issuer); + } + + public function synchronize(callable $operation): mixed + { + if ($this->broken) { + throw new RuntimeException('Could not open the credential lock file.'); + } + + return $this->storage->synchronize($operation); + } + }; + $authenticator = $this->authenticator($server, storage: $storage); + $authenticator->handleChallenge($this->request(), $server->challenge()); + $storage->broken = true; + + $request = $authenticator->authenticate($this->request()); + + $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); + } + #[TestDox('a token without a refresh token is sent until it expires, without a token request')] public function testNoRefreshBeforeSendingWithoutARefreshToken(): void { From 9c7089364136fbc4dacd0a9afb1662e4a3dbb567 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 11:51:01 +0100 Subject: [PATCH 12/24] [Client] Seed the service account tests under the key the service account reads --- tests/Unit/Client/Auth/OAuthAuthenticatorTest.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index 76edd633..fd8457c3 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -2104,7 +2104,7 @@ public function testServiceAccountIgnoresABroaderStoredToken(array $storedScopes { $server = new FakeOAuthServer(); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('broader', 'Bearer', time() + 3600, null, $storedScopes)); + $storage->saveToken('https://auth.example.com', self::tokenKey('client_credentials', 'job-client'), new AccessToken('broader', 'Bearer', time() + 3600, null, $storedScopes)); $authenticator = $this->authenticator($server, storage: $storage, configuration: OAuth::forServiceAccount('job', 'job-client', 'job-secret') ->setExpectedIssuer('https://auth.example.com') ->setScopes('mcp:read')); @@ -2120,7 +2120,7 @@ public function testServiceAccountDoesNotRefreshABroaderStoredToken(): void { $server = new FakeOAuthServer(refreshTokens: true); $storage = new InMemoryCredentialStorage(); - $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('broader', 'Bearer', time() - 60, 'broader-refresh', ['mcp:read', 'admin'])); + $storage->saveToken('https://auth.example.com', self::tokenKey('client_credentials', 'job-client'), new AccessToken('broader', 'Bearer', time() - 60, 'broader-refresh', ['mcp:read', 'admin'])); $authenticator = $this->authenticator($server, storage: $storage, configuration: OAuth::forServiceAccount('job', 'job-client', 'job-secret') ->setExpectedIssuer('https://auth.example.com') ->setScopes('mcp:read')); @@ -2166,7 +2166,7 @@ public function testServiceAccountStepUpIgnoresStoredScopes(int $expiresIn): voi ->setScopes('mcp:read')); $authenticator->handleChallenge($this->request(), $server->challenge(['mcp:read'])); $current = $authenticator->authenticate($this->request()); - $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('broader', 'Bearer', time() + $expiresIn, null, ['mcp:read', 'admin'])); + $storage->saveToken('https://auth.example.com', self::tokenKey('client_credentials', 'job-client'), new AccessToken('broader', 'Bearer', time() + $expiresIn, null, ['mcp:read', 'admin'])); $server->token = []; $retry = $authenticator->handleChallenge($current, $server->challenge(['admin'], 403, 'insufficient_scope')); From 1fc820093ecad17e94201b112b0122fa05788712 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:05:52 +0100 Subject: [PATCH 13/24] [Client] Keep the token when looking up its client fails during the refresh before sending --- src/Client/Auth/OAuthAuthenticator.php | 10 +-- .../Client/Auth/OAuthAuthenticatorTest.php | 61 +++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index edf30940..fa992782 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -481,13 +481,13 @@ private function refreshBeforeSending(AccessToken $token, RequestInterface $requ return $token; } - $client = $this->client ?? $this->knownClient($this->metadata); + try { + $client = $this->client ?? $this->knownClient($this->metadata); - if (null === $client) { - return $token; - } + if (null === $client) { + return $token; + } - try { $refreshed = $this->refresh($token, $this->metadata, $client, array_values(array_diff($token->scopes, ['offline_access'])), $resource, $request); } catch (AuthorizationException|RuntimeException $e) { $this->logger->info('Refreshing the access token before sending failed; leaving it to the server\'s challenge.', ['exception' => $e]); diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index fd8457c3..1d5e1631 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -1502,6 +1502,67 @@ public function synchronize(callable $operation): mixed $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); } + #[TestDox('a storage failure while looking up the client of a stored token keeps the token until it expires')] + public function testRegistrationLookupFailureBeforeSendingKeepsTheToken(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $storage = new class implements CredentialStorageInterface { + public bool $broken = false; + + private readonly InMemoryCredentialStorage $storage; + + public function __construct() + { + $this->storage = new InMemoryCredentialStorage(); + } + + public function getClientRegistration(string $issuer): ?ClientRegistration + { + if ($this->broken) { + throw new RuntimeException('Could not read the keychain.'); + } + + return $this->storage->getClientRegistration($issuer); + } + + public function saveClientRegistration(string $issuer, ClientRegistration $registration): void + { + $this->storage->saveClientRegistration($issuer, $registration); + } + + public function getToken(string $issuer, string $resource): ?AccessToken + { + return $this->storage->getToken($issuer, $resource); + } + + public function saveToken(string $issuer, string $resource, AccessToken $token): void + { + $this->storage->saveToken($issuer, $resource, $token); + } + + public function forget(?string $issuer = null): void + { + $this->storage->forget($issuer); + } + + public function synchronize(callable $operation): mixed + { + return $this->storage->synchronize($operation); + } + }; + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('stored', 'Bearer', time() + 3600, 'a-refresh-token')); + $authenticator = $this->authenticator($server, storage: $storage); + $authenticator->handleChallenge($this->request(), $server->challenge()); + + // The stored token was reused without resolving a client; let it come close to its expiry. + (new \ReflectionProperty($authenticator, 'token'))->setValue($authenticator, new AccessToken('stored', 'Bearer', time() + 10, 'a-refresh-token')); + $storage->broken = true; + + $request = $authenticator->authenticate($this->request()); + + $this->assertSame('Bearer stored', $request->getHeaderLine('Authorization')); + } + #[TestDox('a token without a refresh token is sent until it expires, without a token request')] public function testNoRefreshBeforeSendingWithoutARefreshToken(): void { From f14f235235272fb97344fe596cb4246a5db9aa1c Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:06:16 +0100 Subject: [PATCH 14/24] [Docs] Say that an expected issuer doesn't allow loopback or private endpoints of a public authorization server --- docs/client/authorization.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index 781556d1..afb99de1 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -278,8 +278,8 @@ The client treats the MCP server and its metadata as untrusted input. It stops w | The authorization server metadata names another `issuer` than the one it was fetched for | The document describes another server | | The MCP server URL, an authorization server URL or an endpoint doesn't use `https` on a host that isn't loopback | Credentials would travel without encryption | | One of these URLs names `0.0.0.0`, `::`, an IPv4 address within IPv6 or an IPv4 address in another form than four decimal numbers (e.g. `127.1`), or writes its host with percent-encoding or non-ASCII characters | The request could reach a local service or a private network without looking like it | -| A server that isn't on loopback names a loopback authorization server or endpoint, unless `setExpectedIssuer()` allows that issuer | It could reach services that only listen on this machine | -| A server that isn't on loopback or in a private network names an IP address of a private or link-local network (e.g. `10.0.0.5` or `169.254.169.254`) as authorization server or endpoint, unless `setExpectedIssuer()` allows that issuer | It could reach services of the network the client runs in | +| A server that isn't on loopback names a loopback authorization server, unless `setExpectedIssuer()` allows that issuer, or an authorization server that isn't on loopback names a loopback endpoint, even with `setExpectedIssuer()` | It could reach services that only listen on this machine | +| A server that isn't on loopback or in a private network names an IP address of a private or link-local network (e.g. `10.0.0.5` or `169.254.169.254`) as authorization server, unless `setExpectedIssuer()` allows that issuer, or an authorization server outside such a network names one as endpoint, even with `setExpectedIssuer()` | It could reach services of the network the client runs in | | The authorization server metadata doesn't list `S256` in `code_challenge_methods_supported` | The client can't use PKCE with this server | | The authorization endpoint is on another origin than the issuer and the metadata doesn't announce `iss`, without `setExpectedIssuer()` | A response could come from another server, and the code would go to this one | | The authorization response has no `state`, or another `state` | It doesn't answer the request of this client | From 55fd0c11e7ca451ee58e2a40c406258a88815f10 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:22:44 +0100 Subject: [PATCH 15/24] [Client] Record the client a token was issued to and never use a token of another --- docs/client/authorization.md | 2 +- src/Client/Auth/AccessToken.php | 19 +++++++- src/Client/Auth/OAuthAuthenticator.php | 12 ++++- tests/Unit/Client/Auth/AccessTokenTest.php | 2 +- .../Client/Auth/OAuthAuthenticatorTest.php | 47 +++++++++++++++++++ 5 files changed, 77 insertions(+), 5 deletions(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index afb99de1..f8108ff9 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -89,7 +89,7 @@ The file contains tokens and client secrets. The SDK creates it with the mode `0 The storage keys everything by the issuer of the authorization server. If a server moves to another authorization server, the client registers again and doesn't send its old client ID to the new one. It also registers again if you change the redirect URI, if it needs scopes that its registration doesn't include, or if the token endpoint answers `invalid_client` for a client that it registered. -Tokens are also stored per resource, grant and client ID. So you can share one storage between a service account and an application for users, or between clients with different client IDs. None of them uses a token of another. +Tokens are also stored per resource, grant and client ID. So you can share one storage between a service account and an application for users, or between clients with different client IDs. None of them uses a token of another. Each token also records the client it was issued to, and a client never uses or refreshes a token of another, e.g. after a dynamic registration was replaced. A storage of your own must keep that client ID, which `AccessToken::jsonSerialize()` includes. ## Finishing the Authorization in a Later Request diff --git a/src/Client/Auth/AccessToken.php b/src/Client/Auth/AccessToken.php index 3fa49270..dec3e2a2 100644 --- a/src/Client/Auth/AccessToken.php +++ b/src/Client/Auth/AccessToken.php @@ -31,6 +31,7 @@ final class AccessToken implements \JsonSerializable * @param ?int $expiresAt unix timestamp the token expires at, null when the server did not say * @param ?string $refreshToken the refresh token, when the grant produced one * @param string[] $scopes the scopes the authorization server actually granted + * @param ?string $clientId the client the token was issued to, null when unknown */ public function __construct( public readonly string $accessToken, @@ -38,6 +39,7 @@ public function __construct( public readonly ?int $expiresAt = null, public readonly ?string $refreshToken = null, public readonly array $scopes = [], + public readonly ?string $clientId = null, ) { if ('' === trim($accessToken)) { throw new InvalidArgumentException('The access token must not be empty.'); @@ -122,7 +124,20 @@ public function withGrantedScopes(array $scopes): self return $this; } - return new self($this->accessToken, $this->tokenType, $this->expiresAt, $this->refreshToken, $scopes); + return new self($this->accessToken, $this->tokenType, $this->expiresAt, $this->refreshToken, $scopes, $this->clientId); + } + + public function withClientId(string $clientId): self + { + return new self($this->accessToken, $this->tokenType, $this->expiresAt, $this->refreshToken, $this->scopes, $clientId); + } + + /** + * Whether the token may be used by this client: a token and its refresh token belong to the client they were issued to (RFC 6749 section 6). + */ + public function isIssuedTo(?string $clientId): bool + { + return null === $this->clientId || $this->clientId === $clientId; } public function getAuthorizationHeader(): string @@ -141,6 +156,7 @@ public function jsonSerialize(): array 'expires_at' => $this->expiresAt, 'refresh_token' => $this->refreshToken, 'scopes' => $this->scopes, + 'client_id' => $this->clientId, ], static fn (mixed $value): bool => null !== $value && [] !== $value); } @@ -155,6 +171,7 @@ public static function fromArray(array $data): self isset($data['expires_at']) && is_numeric($data['expires_at']) ? (int) $data['expires_at'] : null, \is_string($data['refresh_token'] ?? null) ? $data['refresh_token'] : null, array_values(array_filter((array) ($data['scopes'] ?? []), 'is_string')), + \is_string($data['client_id'] ?? null) ? $data['client_id'] : null, ); } diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index fa992782..21d00d53 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -118,6 +118,13 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re $this->resource = $resource; $stored = $this->storage->getToken($metadata->issuer, $this->tokenKey($resource)); + + // A dynamic registration may have been replaced since the token was issued, and its successor can't use it. + if (null !== $stored && !$stored->isIssuedTo($this->knownClient($metadata)?->clientId)) { + $this->logger->info('Ignoring a stored token of another client', ['issuer' => $metadata->issuer, 'resource' => $resource]); + $stored = null; + } + $scopes = $this->selectScopes($challenge, $scopesSupported, $metadata, $stored); // The resource never requires offline_access, which only gets a refresh token. $needed = array_values(array_diff($scopes, ['offline_access'])); @@ -525,7 +532,7 @@ private function refresh(?AccessToken $stored, AuthorizationServerMetadata $meta // Another process may have refreshed the token while this one waited. $current = $this->storage->getToken($metadata->issuer, $this->tokenKey($resource)); - if (null === $current) { + if (null === $current || !$current->isIssuedTo($client->clientId)) { return null; } @@ -574,6 +581,7 @@ private function spendRefreshToken(AccessToken $stored, string $refreshToken, Au $token->expiresAt, $token->refreshToken ?? $refreshToken, [] === $token->scopes ? $stored->scopes : $token->scopes, + $token->clientId, ); } @@ -702,7 +710,7 @@ private function grant(AuthorizationServerMetadata $metadata, ClientRegistration */ private function requestToken(AuthorizationServerMetadata $metadata, ClientRegistration $client, array $parameters): AccessToken { - return $this->tokenEndpoint->request($metadata, $client, $parameters, $this->configuration->privateKeyPem, $this->configuration->signingAlgorithm); + return $this->tokenEndpoint->request($metadata, $client, $parameters, $this->configuration->privateKeyPem, $this->configuration->signingAlgorithm)->withClientId($client->clientId); } /** diff --git a/tests/Unit/Client/Auth/AccessTokenTest.php b/tests/Unit/Client/Auth/AccessTokenTest.php index aa27b540..412171cd 100644 --- a/tests/Unit/Client/Auth/AccessTokenTest.php +++ b/tests/Unit/Client/Auth/AccessTokenTest.php @@ -146,7 +146,7 @@ public function testWithGrantedScopes(): void #[TestDox('a token survives a round trip through storage')] public function testRoundTrip(): void { - $token = new AccessToken('abc', 'Bearer', 4600, 'refresh', ['mcp:read']); + $token = new AccessToken('abc', 'Bearer', 4600, 'refresh', ['mcp:read'], 'client-1'); $restored = AccessToken::fromArray($token->jsonSerialize()); $this->assertEquals($token, $restored); diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index 1d5e1631..ebc17e73 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -809,6 +809,53 @@ public function testDeferredAuthorizationKeepsItsClientAfterAReRegistration(): v $this->assertSame('newer-client', $storage->getClientRegistration('https://auth.example.com')?->clientId); } + #[TestDox('the token of a pending authorization finished after a re-registration is not used by the new client')] + public function testTheNewClientDoesNotUseTheTokenOfTheReplacedOne(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $storage = new InMemoryCredentialStorage(); + $callback = $this->startPending($server, $storage); + $storage->saveClientRegistration('https://auth.example.com', new ClientRegistration('newer-client', 'newer-secret', TokenEndpointAuthMethod::ClientSecretPost, true, scopes: ['mcp:read', 'mcp:write'])); + $this->authenticator($server, storage: $storage)->finishAuthorization($callback); + $this->assertSame('registered-client', $storage->getToken('https://auth.example.com', self::tokenKey())?->clientId); + $server->authorization = []; + + $authenticator = $this->authenticator($server, storage: $storage); + $authenticator->handleChallenge($this->request(), $server->challenge()); + + $this->assertNotSame([], $server->authorization); + $this->assertSame('newer-client', $server->token['client_id'] ?? null); + $this->assertSame('newer-client', $storage->getToken('https://auth.example.com', self::tokenKey())?->clientId); + } + + #[TestDox('a stored refresh token of another client is not spent')] + public function testDoesNotRefreshATokenOfAnotherRegistration(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $storage = new InMemoryCredentialStorage(); + $storage->saveClientRegistration('https://auth.example.com', new ClientRegistration('newer-client', 'newer-secret', TokenEndpointAuthMethod::ClientSecretPost, true, scopes: ['mcp:read', 'mcp:write'])); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('expired', 'Bearer', time() - 60, 'older-refresh-token', [], 'older-client')); + + $this->authenticator($server, storage: $storage)->handleChallenge($this->request(), $server->challenge()); + + $this->assertSame('authorization_code', $server->token['grant_type'] ?? null); + $this->assertNotSame([], $server->authorization); + } + + #[TestDox('a stored token without a client is used as before')] + public function testUsesAStoredTokenOfAnUnknownClient(): void + { + $server = new FakeOAuthServer(); + $storage = new InMemoryCredentialStorage(); + $storage->saveToken('https://auth.example.com', self::tokenKey(), new AccessToken('stored', 'Bearer', time() + 3600)); + + $authenticator = $this->authenticator($server, storage: $storage); + $authenticator->handleChallenge($this->request(), $server->challenge()); + + $this->assertSame([], $server->authorization); + $this->assertSame('Bearer stored', $authenticator->authenticate($this->request())->getHeaderLine('Authorization')); + } + #[TestDox('a refused callback keeps the registration of another flow')] public function testDeferredAuthorizationKeepsANewerRegistrationOnInvalidClient(): void { From b7a1379eb02eecf19e84d7ebccc496213f9fa081 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:24:43 +0100 Subject: [PATCH 16/24] [Client] Use the email address bbdu3 commits with in the author tags --- src/Client/Auth/PendingAuthorization.php | 2 +- src/Client/Auth/PendingAuthorizationStorageInterface.php | 2 +- src/Client/Auth/RedirectAuthorizationHandler.php | 2 +- src/Exception/AuthorizationPendingException.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Client/Auth/PendingAuthorization.php b/src/Client/Auth/PendingAuthorization.php index c4ee0709..3c1e5f82 100644 --- a/src/Client/Auth/PendingAuthorization.php +++ b/src/Client/Auth/PendingAuthorization.php @@ -14,7 +14,7 @@ use Mcp\Exception\InvalidArgumentException; /** - * @author bbdu3 + * @author bbdu3 */ final class PendingAuthorization implements \JsonSerializable { diff --git a/src/Client/Auth/PendingAuthorizationStorageInterface.php b/src/Client/Auth/PendingAuthorizationStorageInterface.php index 047cc2a7..9d6e1585 100644 --- a/src/Client/Auth/PendingAuthorizationStorageInterface.php +++ b/src/Client/Auth/PendingAuthorizationStorageInterface.php @@ -14,7 +14,7 @@ /** * Storage must be scoped to the user session that initiated authorization. * - * @author bbdu3 + * @author bbdu3 */ interface PendingAuthorizationStorageInterface extends CredentialStorageInterface { diff --git a/src/Client/Auth/RedirectAuthorizationHandler.php b/src/Client/Auth/RedirectAuthorizationHandler.php index bab252db..b511d63b 100644 --- a/src/Client/Auth/RedirectAuthorizationHandler.php +++ b/src/Client/Auth/RedirectAuthorizationHandler.php @@ -14,7 +14,7 @@ use Mcp\Exception\AuthorizationPendingException; /** - * @author bbdu3 + * @author bbdu3 */ final class RedirectAuthorizationHandler implements AuthorizationHandlerInterface { diff --git a/src/Exception/AuthorizationPendingException.php b/src/Exception/AuthorizationPendingException.php index 42a62ee9..2c96beaa 100644 --- a/src/Exception/AuthorizationPendingException.php +++ b/src/Exception/AuthorizationPendingException.php @@ -12,7 +12,7 @@ namespace Mcp\Exception; /** - * @author bbdu3 + * @author bbdu3 */ final class AuthorizationPendingException extends AuthorizationException { From b6362c96821b625c0588861d5a861fe51bb82a31 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:25:22 +0100 Subject: [PATCH 17/24] [Client] Don't repeat a failed refresh before sending for the same token --- src/Client/Auth/OAuthAuthenticator.php | 8 +++++++- .../Client/Auth/OAuthAuthenticatorTest.php | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index 21d00d53..271c31dd 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -47,6 +47,9 @@ final class OAuthAuthenticator implements AuthenticatorInterface private ?AuthorizationServerMetadata $metadata = null; private ?ClientRegistration $client = null; + /** A refresh before sending that failed isn't repeated for the same token, as a down token endpoint would delay every request. */ + private ?AccessToken $refreshFailedFor = null; + public function __construct( private readonly OAuthConfiguration $configuration, private readonly CredentialStorageInterface $storage, @@ -484,7 +487,7 @@ private function scopeLimit(): ?array */ private function refreshBeforeSending(AccessToken $token, RequestInterface $request, string $resource): AccessToken { - if (null === $token->refreshToken || null === $this->metadata) { + if (null === $token->refreshToken || null === $this->metadata || $token === $this->refreshFailedFor) { return $token; } @@ -498,11 +501,14 @@ private function refreshBeforeSending(AccessToken $token, RequestInterface $requ $refreshed = $this->refresh($token, $this->metadata, $client, array_values(array_diff($token->scopes, ['offline_access'])), $resource, $request); } catch (AuthorizationException|RuntimeException $e) { $this->logger->info('Refreshing the access token before sending failed; leaving it to the server\'s challenge.', ['exception' => $e]); + $this->refreshFailedFor = $token; return $token; } if (null === $refreshed) { + $this->refreshFailedFor = $token; + return $token; } diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index ebc17e73..90e64641 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -1491,6 +1491,25 @@ public function testRefusedRefreshBeforeSendingKeepsTheToken(): void $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); } + #[TestWith([400, 'invalid_grant'])] + #[TestWith([503, 'temporarily_unavailable'])] + #[TestDox('a failed refresh before sending is not tried again for the same token: $_dataName')] + public function testFailedRefreshBeforeSendingIsNotRepeated(int $status, string $error): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $server->expiresIn = 10; + $authenticator = $this->authenticator($server); + $authenticator->handleChallenge($this->request(), $server->challenge()); + $server->refreshAnswer = new Response($status, [], (string) json_encode(['error' => $error])); + $tokenRequests = \count(array_filter($server->requested, static fn (string $url): bool => str_ends_with($url, '/token'))); + + $authenticator->authenticate($this->request()); + $request = $authenticator->authenticate($this->request()); + + $this->assertCount($tokenRequests + 1, array_filter($server->requested, static fn (string $url): bool => str_ends_with($url, '/token'))); + $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); + } + #[TestDox('a storage failure during the refresh before sending keeps the token until it expires')] public function testStorageFailureBeforeSendingKeepsTheToken(): void { From 90cff68ccfad53b10badfdbf2ff4b029bef6efe8 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:25:31 +0100 Subject: [PATCH 18/24] [Docs] Say that the private network check looks at IP addresses, not at internal names --- docs/client/authorization.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index f8108ff9..2169f602 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -279,7 +279,8 @@ The client treats the MCP server and its metadata as untrusted input. It stops w | The MCP server URL, an authorization server URL or an endpoint doesn't use `https` on a host that isn't loopback | Credentials would travel without encryption | | One of these URLs names `0.0.0.0`, `::`, an IPv4 address within IPv6 or an IPv4 address in another form than four decimal numbers (e.g. `127.1`), or writes its host with percent-encoding or non-ASCII characters | The request could reach a local service or a private network without looking like it | | A server that isn't on loopback names a loopback authorization server, unless `setExpectedIssuer()` allows that issuer, or an authorization server that isn't on loopback names a loopback endpoint, even with `setExpectedIssuer()` | It could reach services that only listen on this machine | -| A server that isn't on loopback or in a private network names an IP address of a private or link-local network (e.g. `10.0.0.5` or `169.254.169.254`) as authorization server, unless `setExpectedIssuer()` allows that issuer, or an authorization server outside such a network names one as endpoint, even with `setExpectedIssuer()` | It could reach services of the network the client runs in | +| An MCP server whose URL isn't a loopback or private-network IP address names an IP address of a private or link-local network (e.g. `10.0.0.5` or `169.254.169.254`) as authorization server, unless `setExpectedIssuer()` allows that issuer. A DNS name counts as public, so an MCP server at `https://mcp.corp.internal` with its authorization server at `https://10.0.0.5` needs `setExpectedIssuer()` | It could reach services of the network the client runs in | +| An authorization server whose URL isn't a loopback or private-network IP address names such an address as endpoint, even with `setExpectedIssuer()` | The same | | The authorization server metadata doesn't list `S256` in `code_challenge_methods_supported` | The client can't use PKCE with this server | | The authorization endpoint is on another origin than the issuer and the metadata doesn't announce `iss`, without `setExpectedIssuer()` | A response could come from another server, and the code would go to this one | | The authorization response has no `state`, or another `state` | It doesn't answer the request of this client | From 7810b72635b30f112d1d2e2530f32566c2f41f77 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:26:46 +0100 Subject: [PATCH 19/24] [Client] Name the issuer checks of the endpoint validation --- src/Client/Auth/AuthorizationServerMetadata.php | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/Client/Auth/AuthorizationServerMetadata.php b/src/Client/Auth/AuthorizationServerMetadata.php index 4d7c751f..0038e91d 100644 --- a/src/Client/Auth/AuthorizationServerMetadata.php +++ b/src/Client/Auth/AuthorizationServerMetadata.php @@ -74,6 +74,11 @@ public static function fromArray(array $data): self } } + /** @var string $issuer */ + $issuer = $data['issuer']; + $issuerOnLoopback = SecureUrl::isLoopback($issuer); + $issuerIsPublic = !$issuerOnLoopback && !SecureUrl::isPrivateNetwork($issuer); + foreach (['authorization_endpoint', 'token_endpoint', 'registration_endpoint'] as $endpoint) { if (!\is_string($data[$endpoint] ?? null)) { continue; @@ -84,13 +89,13 @@ public static function fromArray(array $data): self } // Only a server on this machine may send the client back to this machine. - if (SecureUrl::isLoopback($data[$endpoint]) && \is_string($data['issuer'] ?? null) && !SecureUrl::isLoopback($data['issuer'])) { - throw new InvalidArgumentException(\sprintf('The authorization server metadata of "%s" declares "%s" as "%s", which is a loopback address.', $data['issuer'], $endpoint, $data[$endpoint])); + if (!$issuerOnLoopback && SecureUrl::isLoopback($data[$endpoint])) { + throw new InvalidArgumentException(\sprintf('The authorization server metadata of "%s" declares "%s" as "%s", which is a loopback address.', $issuer, $endpoint, $data[$endpoint])); } // Nor may a server outside a private network send the client into one. - if (SecureUrl::isPrivateNetwork($data[$endpoint]) && \is_string($data['issuer'] ?? null) && !SecureUrl::isPrivateNetwork($data['issuer']) && !SecureUrl::isLoopback($data['issuer'])) { - throw new InvalidArgumentException(\sprintf('The authorization server metadata of "%s" declares "%s" as "%s", which is an address of a private network.', $data['issuer'], $endpoint, $data[$endpoint])); + if ($issuerIsPublic && SecureUrl::isPrivateNetwork($data[$endpoint])) { + throw new InvalidArgumentException(\sprintf('The authorization server metadata of "%s" declares "%s" as "%s", which is an address of a private network.', $issuer, $endpoint, $data[$endpoint])); } $parts = SecureUrl::parse($data[$endpoint], $endpoint); From 51700a03815195dfc4d1c20296eb96f49e8f2afd Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:26:46 +0100 Subject: [PATCH 20/24] [Client] Describe the token key correctly in the scope limit --- src/Client/Auth/OAuthAuthenticator.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index 271c31dd..b037d756 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -459,7 +459,7 @@ private function isUsable(AccessToken $token, array $scopes, RequestInterface $r } /** - * Tokens are stored per server and resource, so a stored one may come from a broader configuration. + * Configurations with the same grant and client share stored tokens, so a stored one may come from a broader configuration. */ private function isWithinScopeLimit(AccessToken $token): bool { From 42454d3bceec239172014a6f62824d15863b83d6 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:26:46 +0100 Subject: [PATCH 21/24] [Client] Refuse metadata for another resource in the discovery, instead of returning it to be refused --- src/Client/Auth/MetadataDiscovery.php | 12 ++++++++++-- src/Client/Auth/OAuthAuthenticator.php | 5 ----- tests/Unit/Client/Auth/MetadataDiscoveryTest.php | 10 ++++++---- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/src/Client/Auth/MetadataDiscovery.php b/src/Client/Auth/MetadataDiscovery.php index c39c2cae..084adeee 100644 --- a/src/Client/Auth/MetadataDiscovery.php +++ b/src/Client/Auth/MetadataDiscovery.php @@ -11,6 +11,7 @@ namespace Mcp\Client\Auth; +use Mcp\Exception\AuthorizationException; use Mcp\Exception\InvalidArgumentException; use Psr\Http\Client\ClientExceptionInterface; use Psr\Http\Client\ClientInterface; @@ -40,7 +41,9 @@ public function __construct( * @param string $resourceUrl the MCP endpoint being called * @param ?string $metadataUrl the location the challenge named, which wins over any guess * - * @return ?ProtectedResourceMetadata the first document that covers the endpoint, else the first for another resource, which the caller must refuse + * @return ?ProtectedResourceMetadata the first document that covers the endpoint, null if there is none + * + * @throws AuthorizationException if documents were found, but only for other resources */ public function discoverProtectedResource(string $resourceUrl, ?string $metadataUrl = null): ?ProtectedResourceMetadata { @@ -78,7 +81,12 @@ public function discoverProtectedResource(string $resourceUrl, ?string $metadata return $metadata; } - return $unrelated; + if (null !== $unrelated) { + // A token for another resource would be sent to this one: a confused deputy. + throw new AuthorizationException(\sprintf('The protected resource metadata for "%s" declares the unrelated resource "%s"; refusing to authorize against it.', $resourceUrl, $unrelated->resource)); + } + + return null; } /** diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index b037d756..b02ade0a 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -290,11 +290,6 @@ private function discover(string $endpoint, AuthorizationChallenge $challenge): return [$endpoint, $this->discoverLegacyServer($endpoint), null]; } - if (!$metadata->covers($endpoint)) { - // A token for another resource would be sent to this one: a confused deputy. - throw new AuthorizationException(\sprintf('The protected resource metadata for "%s" declares the unrelated resource "%s"; refusing to authorize against it.', $endpoint, $metadata->resource)); - } - $issuer = $this->selectIssuer($endpoint, $metadata->authorizationServers); // Checked before the fetch, as even the request is made on the server's behalf. diff --git a/tests/Unit/Client/Auth/MetadataDiscoveryTest.php b/tests/Unit/Client/Auth/MetadataDiscoveryTest.php index 43f03003..1781a578 100644 --- a/tests/Unit/Client/Auth/MetadataDiscoveryTest.php +++ b/tests/Unit/Client/Auth/MetadataDiscoveryTest.php @@ -12,6 +12,7 @@ namespace Mcp\Tests\Unit\Client\Auth; use Mcp\Client\Auth\MetadataDiscovery; +use Mcp\Exception\AuthorizationException; use Nyholm\Psr7\Factory\Psr17Factory; use Nyholm\Psr7\Response; use PHPUnit\Framework\Attributes\DataProvider; @@ -323,16 +324,17 @@ public function testContinuesAfterAnUnrelatedResource(): void $this->assertSame([$path, $root], $client->requested); } - #[TestDox('a document for another resource is returned if no location describes the endpoint, for the caller to refuse')] - public function testReturnsTheUnrelatedResourceIfNothingElseIsFound(): void + #[TestDox('documents only for other resources are refused, rather than taken as no metadata')] + public function testRefusesWhenOnlyUnrelatedResourcesAreFound(): void { $client = new RecordingClient([ 'https://mcp.example.com/.well-known/oauth-protected-resource/mcp' => self::json(['resource' => 'https://evil.example.com/mcp']), ]); - $metadata = $this->discovery($client)->discoverProtectedResource('https://mcp.example.com/mcp'); + $this->expectException(AuthorizationException::class); + $this->expectExceptionMessage('declares the unrelated resource "https://evil.example.com/mcp"'); - $this->assertSame('https://evil.example.com/mcp', $metadata?->resource); + $this->discovery($client)->discoverProtectedResource('https://mcp.example.com/mcp'); } #[TestDox('an invalid resource identifier does not stop a later location')] From 79608ada20191680910b2e96d389add7205cffe8 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:31:39 +0100 Subject: [PATCH 22/24] [Server] Use the email address bbdu3 commits with in the author tags --- src/Server/Fiber/FiberContextInterface.php | 2 +- tests/Unit/Fixtures/RecordingFiberContext.php | 2 +- tests/Unit/Server/Fiber/FiberContextTest.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Server/Fiber/FiberContextInterface.php b/src/Server/Fiber/FiberContextInterface.php index 0b1c884a..6af30500 100644 --- a/src/Server/Fiber/FiberContextInterface.php +++ b/src/Server/Fiber/FiberContextInterface.php @@ -14,7 +14,7 @@ /** * Carries execution context, e.g. the active OpenTelemetry context, into the fibers request handlers run in. * - * @author bbdu3 + * @author bbdu3 */ interface FiberContextInterface { diff --git a/tests/Unit/Fixtures/RecordingFiberContext.php b/tests/Unit/Fixtures/RecordingFiberContext.php index a94fed76..9e2d1fb0 100644 --- a/tests/Unit/Fixtures/RecordingFiberContext.php +++ b/tests/Unit/Fixtures/RecordingFiberContext.php @@ -15,7 +15,7 @@ use Mcp\Server\Fiber\FiberContextInterface; /** - * @author bbdu3 + * @author bbdu3 */ final class RecordingFiberContext implements FiberContextInterface { diff --git a/tests/Unit/Server/Fiber/FiberContextTest.php b/tests/Unit/Server/Fiber/FiberContextTest.php index f1c0bdb7..6307e86e 100644 --- a/tests/Unit/Server/Fiber/FiberContextTest.php +++ b/tests/Unit/Server/Fiber/FiberContextTest.php @@ -29,7 +29,7 @@ use Psr\Http\Message\ResponseInterface; /** - * @author bbdu3 + * @author bbdu3 */ final class FiberContextTest extends TestCase { From 92979d094b12c70cac1b51e299cb3a8226a2a7a3 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 12:36:38 +0100 Subject: [PATCH 23/24] [Client] Also count NAT64, 6to4 and site-local addresses as a private network --- src/Server/Transport/Http/OAuth/SecureUrl.php | 38 +++++++++++++++++-- .../Transport/Http/OAuth/SecureUrlTest.php | 8 ++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/Server/Transport/Http/OAuth/SecureUrl.php b/src/Server/Transport/Http/OAuth/SecureUrl.php index 15881669..bfd7949b 100644 --- a/src/Server/Transport/Http/OAuth/SecureUrl.php +++ b/src/Server/Transport/Http/OAuth/SecureUrl.php @@ -25,6 +25,9 @@ */ final class SecureUrl { + /** @var list */ + private const PRIVATE_IPV4 = [['10.0.0.0', 8], ['172.16.0.0', 12], ['192.168.0.0', 16], ['169.254.0.0', 16], ['100.64.0.0', 10]]; + /** * @return array{scheme: string, host: string, port?: int, path?: string, query?: string, fragment?: string} * @@ -79,12 +82,41 @@ public static function isPrivateNetwork(string $url): bool } $address = (string) inet_pton($host); - $ranges = 4 === \strlen($address) - ? [['10.0.0.0', 8], ['172.16.0.0', 12], ['192.168.0.0', 16], ['169.254.0.0', 16], ['100.64.0.0', 10]] - : [['fc00::', 7], ['fe80::', 10]]; + if (4 === \strlen($address)) { + return self::isWithin($address, self::PRIVATE_IPV4); + } + + if (self::isWithin($address, [['64:ff9b:1::', 48]])) { + return true; + } + + // NAT64 (RFC 6052) and 6to4 (RFC 3056) addresses reach the IPv4 address they carry, loopback included. + $embedded = match (true) { + self::isWithin($address, [['64:ff9b::', 96]]) => substr($address, 12), + self::isWithin($address, [['2002::', 16]]) => substr($address, 2, 4), + default => null, + }; + + if (null !== $embedded) { + return self::isWithin($embedded, [...self::PRIVATE_IPV4, ['127.0.0.0', 8], ['0.0.0.0', 8]]); + } + + return self::isWithin($address, [['fc00::', 7], ['fe80::', 10], ['fec0::', 10]]); + } + + /** + * @param list $ranges + */ + private static function isWithin(string $address, array $ranges): bool + { foreach ($ranges as [$network, $bits]) { $prefix = (string) inet_pton($network); + + if (\strlen($prefix) !== \strlen($address)) { + continue; + } + $bytes = intdiv($bits, 8); $mask = 0xFF << (8 - $bits % 8) & 0xFF; diff --git a/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php b/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php index ba3c419f..b357210d 100644 --- a/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php +++ b/tests/Unit/Server/Transport/Http/OAuth/SecureUrlTest.php @@ -148,6 +148,14 @@ public static function providePrivateNetworkUrls(): iterable yield 'public IPv6' => ['https://[2001:db8::1]/x', false]; yield 'loopback' => ['https://127.0.0.1/x', false]; yield 'a name, which is not resolved' => ['https://internal.corp/x', false]; + yield 'site-local IPv6' => ['https://[fec0::1]/x', true]; + yield 'NAT64 of a private address' => ['https://[64:ff9b::a00:5]/x', true]; + yield 'NAT64 of the metadata service' => ['https://[64:ff9b::a9fe:a9fe]/x', true]; + yield 'NAT64 of loopback' => ['https://[64:ff9b::7f00:1]/x', true]; + yield 'NAT64 of a public address' => ['https://[64:ff9b::808:808]/x', false]; + yield 'local-use NAT64' => ['https://[64:ff9b:1::808:808]/x', true]; + yield '6to4 of a private address' => ['https://[2002:c0a8:101::1]/x', true]; + yield '6to4 of a public address' => ['https://[2002:808:808::1]/x', false]; } #[DataProvider('providePrivateNetworkUrls')] From b6838454991b958d4691ff01b24f2715e60c1e19 Mon Sep 17 00:00:00 2001 From: Christopher Hertel Date: Tue, 10 Nov 2026 13:01:24 +0100 Subject: [PATCH 24/24] [Client] Don't refresh a token before sending that lives shorter than the leeway --- docs/client/authorization.md | 2 +- src/Client/Auth/OAuthAuthenticator.php | 13 +++-- .../Client/Auth/OAuthAuthenticatorTest.php | 53 +++++++++++++++---- 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/docs/client/authorization.md b/docs/client/authorization.md index 2169f602..f3a056c2 100644 --- a/docs/client/authorization.md +++ b/docs/client/authorization.md @@ -248,7 +248,7 @@ OAuth::forApplication('My App')->setScopes('mcp:read', 'mcp:write'); If the server answers with `insufficient_scope`, the client adds the scopes of this challenge and authorizes again. It doesn't authorize again for scopes that it already requested. With any other `403`, the client doesn't authorize again. -If the authorization server supports `offline_access`, the client requests it to get a refresh token. If the server refuses this scope, the client tries again without it. Use `setOfflineAccess(false)` to never request this scope. The server can still issue a refresh token, and the client then uses it. It refreshes a token shortly before it expires, so the server doesn't have to reject a request first. +If the authorization server supports `offline_access`, the client requests it to get a refresh token. If the server refuses this scope, the client tries again without it. Use `setOfflineAccess(false)` to never request this scope. The server can still issue a refresh token, and the client then uses it. It refreshes a token 30 seconds before it expires, so the server doesn't have to reject a request first. A token that lives shorter than that is only refreshed once the server rejects it. ## Connecting to Older Servers diff --git a/src/Client/Auth/OAuthAuthenticator.php b/src/Client/Auth/OAuthAuthenticator.php index b02ade0a..5751292b 100644 --- a/src/Client/Auth/OAuthAuthenticator.php +++ b/src/Client/Auth/OAuthAuthenticator.php @@ -47,8 +47,8 @@ final class OAuthAuthenticator implements AuthenticatorInterface private ?AuthorizationServerMetadata $metadata = null; private ?ClientRegistration $client = null; - /** A refresh before sending that failed isn't repeated for the same token, as a down token endpoint would delay every request. */ - private ?AccessToken $refreshFailedFor = null; + /** Not refreshed before sending: a failed refresh would repeat on every request, and so would a token shorter-lived than the leeway. */ + private ?AccessToken $noRefreshBeforeSending = null; public function __construct( private readonly OAuthConfiguration $configuration, @@ -193,6 +193,7 @@ public function handleChallenge(RequestInterface $request, ResponseInterface $re $this->audience = $resource; $this->metadata = $metadata; $this->client = $client; + $this->noRefreshBeforeSending = $token->isExpiringSoon() ? $token : null; return true; } @@ -274,6 +275,7 @@ public function finishAuthorization(array $callback): void $this->audience = $pending->resource; $this->metadata = $metadata; $this->client = $client; + $this->noRefreshBeforeSending = $this->token->isExpiringSoon() ? $this->token : null; $this->storage->saveToken($metadata->issuer, $this->tokenKey($pending->resource), $this->token); } @@ -482,7 +484,7 @@ private function scopeLimit(): ?array */ private function refreshBeforeSending(AccessToken $token, RequestInterface $request, string $resource): AccessToken { - if (null === $token->refreshToken || null === $this->metadata || $token === $this->refreshFailedFor) { + if (null === $token->refreshToken || null === $this->metadata || $token === $this->noRefreshBeforeSending) { return $token; } @@ -496,19 +498,20 @@ private function refreshBeforeSending(AccessToken $token, RequestInterface $requ $refreshed = $this->refresh($token, $this->metadata, $client, array_values(array_diff($token->scopes, ['offline_access'])), $resource, $request); } catch (AuthorizationException|RuntimeException $e) { $this->logger->info('Refreshing the access token before sending failed; leaving it to the server\'s challenge.', ['exception' => $e]); - $this->refreshFailedFor = $token; + $this->noRefreshBeforeSending = $token; return $token; } if (null === $refreshed) { - $this->refreshFailedFor = $token; + $this->noRefreshBeforeSending = $token; return $token; } $this->token = $refreshed; $this->client = $client; + $this->noRefreshBeforeSending = $refreshed->isExpiringSoon() ? $refreshed : null; return $refreshed; } diff --git a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php index 90e64641..1043f1d2 100644 --- a/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php +++ b/tests/Unit/Client/Auth/OAuthAuthenticatorTest.php @@ -1465,10 +1465,10 @@ public function authorize(string $authorizationUrl, string $redirectUri): array public function testRefreshesBeforeSending(): void { $server = new FakeOAuthServer(refreshTokens: true); - $server->expiresIn = 10; - $authenticator = $this->authenticator($server); + $storage = new InMemoryCredentialStorage(); + $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge()); - $server->expiresIn = 3600; + self::ageToken($authenticator, $storage); $request = $authenticator->authenticate($this->request()); @@ -1480,9 +1480,10 @@ public function testRefreshesBeforeSending(): void public function testRefusedRefreshBeforeSendingKeepsTheToken(): void { $server = new FakeOAuthServer(refreshTokens: true); - $server->expiresIn = 10; - $authenticator = $this->authenticator($server); + $storage = new InMemoryCredentialStorage(); + $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge()); + self::ageToken($authenticator, $storage); $server->refreshAnswer = new Response(400, [], (string) json_encode(['error' => 'invalid_client'])); $request = $authenticator->authenticate($this->request()); @@ -1497,9 +1498,10 @@ public function testRefusedRefreshBeforeSendingKeepsTheToken(): void public function testFailedRefreshBeforeSendingIsNotRepeated(int $status, string $error): void { $server = new FakeOAuthServer(refreshTokens: true); - $server->expiresIn = 10; - $authenticator = $this->authenticator($server); + $storage = new InMemoryCredentialStorage(); + $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge()); + self::ageToken($authenticator, $storage); $server->refreshAnswer = new Response($status, [], (string) json_encode(['error' => $error])); $tokenRequests = \count(array_filter($server->requested, static fn (string $url): bool => str_ends_with($url, '/token'))); @@ -1510,11 +1512,27 @@ public function testFailedRefreshBeforeSendingIsNotRepeated(int $status, string $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); } + #[TestDox('a token that lives shorter than the leeway is refreshed before sending at most once')] + public function testShortLivedTokensAreNotRefreshedBeforeEveryRequest(): void + { + $server = new FakeOAuthServer(refreshTokens: true); + $server->expiresIn = 20; + $authenticator = $this->authenticator($server); + $authenticator->handleChallenge($this->request(), $server->challenge()); + $tokenRequests = \count(array_filter($server->requested, static fn (string $url): bool => str_ends_with($url, '/token'))); + + for ($i = 0; $i < 3; ++$i) { + $request = $authenticator->authenticate($this->request()); + } + + $this->assertCount($tokenRequests, array_filter($server->requested, static fn (string $url): bool => str_ends_with($url, '/token'))); + $this->assertSame('Bearer access-token-1', $request->getHeaderLine('Authorization')); + } + #[TestDox('a storage failure during the refresh before sending keeps the token until it expires')] public function testStorageFailureBeforeSendingKeepsTheToken(): void { $server = new FakeOAuthServer(refreshTokens: true); - $server->expiresIn = 10; $storage = new class implements CredentialStorageInterface { public bool $broken = false; @@ -1561,6 +1579,7 @@ public function synchronize(callable $operation): mixed }; $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge()); + self::ageToken($authenticator, $storage); $storage->broken = true; $request = $authenticator->authenticate($this->request()); @@ -1633,9 +1652,10 @@ public function synchronize(callable $operation): mixed public function testNoRefreshBeforeSendingWithoutARefreshToken(): void { $server = new FakeOAuthServer(); - $server->expiresIn = 10; - $authenticator = $this->authenticator($server); + $storage = new InMemoryCredentialStorage(); + $authenticator = $this->authenticator($server, storage: $storage); $authenticator->handleChallenge($this->request(), $server->challenge()); + self::ageToken($authenticator, $storage); $requests = \count($server->requested); $request = $authenticator->authenticate($this->request()); @@ -2555,6 +2575,19 @@ public static function tokenKey(string $grant = 'authorization_code', ?string $c return self::ENDPOINT.'#'.$grant.(null === $clientId ? '' : ' '.$clientId); } + /** + * Brings the current token close to its expiry, in the authenticator and in the storage. + */ + private static function ageToken(OAuthAuthenticator $authenticator, CredentialStorageInterface $storage): void + { + $property = new \ReflectionProperty($authenticator, 'token'); + $token = $property->getValue($authenticator); + \assert($token instanceof AccessToken); + $aged = new AccessToken($token->accessToken, $token->tokenType, time() + 10, $token->refreshToken, $token->scopes, $token->clientId); + $property->setValue($authenticator, $aged); + $storage->saveToken('https://auth.example.com', self::tokenKey(), $aged); + } + private function request(): RequestInterface { return (new Psr17Factory())->createRequest('POST', self::ENDPOINT);