From b65b1ae145adaa7dd6acda06e6e5d053ae89afa5 Mon Sep 17 00:00:00 2001 From: Samuel Mbabhazi Date: Sun, 1 Nov 2026 21:14:58 +0200 Subject: [PATCH] fix(auth): reject non-loopback http and fragment redirect URIs at registration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The registration handler validated redirect_uris only with SafeUrlSchema, so it registered http://attacker.example.com/callback and URIs with a fragment. The MCP authorization spec requires HTTPS or a loopback host, and RFC 6749 §3.1.2 forbids a fragment. Both now get 400 invalid_client_metadata, which is what the hosting:auth:as:redirect-uri-scheme requirement already expected, so its known failure is removed and the scenario also covers a fragment. The loopback hosts are the ones the authorize handler already relaxes ports for (localhost, 127.0.0.1, [::1]). Private-use schemes stay accepted. Fixes #3142 --- .../register-redirect-uri-validation.md | 7 +++ src/server/auth/handlers/authorize.ts | 2 +- src/server/auth/handlers/register.ts | 13 ++++++ test/e2e/requirements.ts | 10 ++--- test/e2e/scenarios/hosting-express.test.ts | 4 ++ test/server/auth/handlers/register.test.ts | 44 +++++++++++++++++++ 6 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 .changeset/register-redirect-uri-validation.md diff --git a/.changeset/register-redirect-uri-validation.md b/.changeset/register-redirect-uri-validation.md new file mode 100644 index 0000000000..9002de7453 --- /dev/null +++ b/.changeset/register-redirect-uri-validation.md @@ -0,0 +1,7 @@ +--- +'@modelcontextprotocol/sdk': patch +--- + +The client registration handler of `mcpAuthRouter` now rejects a redirect URI that uses plain `http:` on a host other than `localhost`, `127.0.0.1` or `[::1]`, and a redirect URI that includes a fragment, with `400 invalid_client_metadata`. The MCP authorization specification +requires redirect URIs to use HTTPS or a loopback host, and RFC 6749 §3.1.2 forbids a fragment. A client that registered such a URI before keeps its registration, but registering it again now fails; HTTPS URIs, loopback `http:` URIs on any port and private-use schemes such as +`com.example.app:/callback` are accepted as before. diff --git a/src/server/auth/handlers/authorize.ts b/src/server/auth/handlers/authorize.ts index 4b9f3b327f..0267f7f015 100644 --- a/src/server/auth/handlers/authorize.ts +++ b/src/server/auth/handlers/authorize.ts @@ -15,7 +15,7 @@ export type AuthorizationHandlerOptions = { rateLimit?: Partial | false; }; -const LOOPBACK_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]']); +export const LOOPBACK_HOSTS = new Set(['localhost', '127.0.0.1', '[::1]']); /** * Validates a requested redirect_uri against a registered one. diff --git a/src/server/auth/handlers/register.ts b/src/server/auth/handlers/register.ts index 1830619b4e..c32ba23d0f 100644 --- a/src/server/auth/handlers/register.ts +++ b/src/server/auth/handlers/register.ts @@ -6,6 +6,7 @@ import { OAuthRegisteredClientsStore } from '../clients.js'; import { rateLimit, Options as RateLimitOptions } from 'express-rate-limit'; import { allowedMethods } from '../middleware/allowedMethods.js'; import { InvalidClientMetadataError, ServerError, TooManyRequestsError, OAuthError } from '../errors.js'; +import { LOOPBACK_HOSTS } from './authorize.js'; export type ClientRegistrationHandlerOptions = { /** @@ -37,6 +38,12 @@ export type ClientRegistrationHandlerOptions = { const DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS = 30 * 24 * 60 * 60; // 30 days +// MCP requires HTTPS or a loopback host for redirect URIs, and RFC 6749 §3.1.2 forbids a fragment. +function isAllowedRedirectUri(uri: string): boolean { + const url = new URL(uri); + return !uri.includes('#') && (url.protocol !== 'http:' || LOOPBACK_HOSTS.has(url.hostname)); +} + export function clientRegistrationHandler({ clientsStore, clientSecretExpirySeconds = DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS, @@ -80,6 +87,12 @@ export function clientRegistrationHandler({ } const clientMetadata = parseResult.data; + const invalidRedirectUri = clientMetadata.redirect_uris.find(uri => !isAllowedRedirectUri(uri)); + if (invalidRedirectUri !== undefined) { + throw new InvalidClientMetadataError( + `Invalid redirect_uris: ${invalidRedirectUri} must use HTTPS or a loopback host and must not include a fragment` + ); + } const isPublicClient = clientMetadata.token_endpoint_auth_method === 'none'; // Generate client credentials diff --git a/test/e2e/requirements.ts b/test/e2e/requirements.ts index 101215f40a..5059ffed63 100644 --- a/test/e2e/requirements.ts +++ b/test/e2e/requirements.ts @@ -1849,14 +1849,10 @@ export const REQUIREMENTS: Record = { }, 'hosting:auth:as:redirect-uri-scheme': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#communication-security', - behavior: 'The bundled registration endpoint accepts only redirect URIs that use HTTPS or target a loopback host.', + behavior: + 'The bundled registration endpoint accepts only redirect URIs that use HTTPS or target a loopback host, and rejects redirect URIs that include a fragment.', transports: ['streamableHttp'], - note: 'These exercise the HTTP hosting/auth layer (mostly over real Express); the matrix transport arg is ignored, so they run as a single streamableHttp-labelled cell to avoid duplicate runs.', - knownFailures: [ - { - note: 'The bundled registration endpoint validates redirect_uris only with SafeUrlSchema, so a non-HTTPS, non-loopback URI like http://attacker.example.com/callback is registered with 201 instead of being rejected with 400 invalid_client_metadata.' - } - ] + note: 'These exercise the HTTP hosting/auth layer (mostly over real Express); the matrix transport arg is ignored, so they run as a single streamableHttp-labelled cell to avoid duplicate runs.' }, 'hosting:auth:as:redirect-uri-binding': { source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#open-redirection', diff --git a/test/e2e/scenarios/hosting-express.test.ts b/test/e2e/scenarios/hosting-express.test.ts index bb072a7f56..6b43e016e1 100644 --- a/test/e2e/scenarios/hosting-express.test.ts +++ b/test/e2e/scenarios/hosting-express.test.ts @@ -626,6 +626,10 @@ verifies('hosting:auth:as:redirect-uri-scheme', async (_args: TestArgs) => { expect(nonLoopbackHttp.status).toBe(400); const body = (await nonLoopbackHttp.json()) as { error?: string }; expect(body.error).toBe('invalid_client_metadata'); + + const withFragment = await postRegistration(host.baseUrl, ['https://client.example.com/callback#fragment']); + expect(withFragment.status).toBe(400); + expect(((await withFragment.json()) as { error?: string }).error).toBe('invalid_client_metadata'); }); verifies('hosting:auth:as:redirect-uri-binding', async (_args: TestArgs) => { diff --git a/test/server/auth/handlers/register.test.ts b/test/server/auth/handlers/register.test.ts index 03fde46d23..65a69c7c55 100644 --- a/test/server/auth/handlers/register.test.ts +++ b/test/server/auth/handlers/register.test.ts @@ -106,6 +106,50 @@ describe('Client Registration Handler', () => { expect(spyRegisterClient).not.toHaveBeenCalled(); }); + it('rejects plain-HTTP redirect URIs on non-loopback hosts', async () => { + const response = await supertest(app) + .post('/register') + .send({ + redirect_uris: ['https://client.example.com/callback', 'http://attacker.example.com/callback'] + }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_client_metadata'); + expect(response.body.error_description).toContain('http://attacker.example.com/callback'); + expect(spyRegisterClient).not.toHaveBeenCalled(); + }); + + it('accepts plain-HTTP redirect URIs on loopback hosts', async () => { + const redirectUris = ['http://localhost:3000/callback', 'http://127.0.0.1:49152/callback', 'http://[::1]/callback']; + + const response = await supertest(app).post('/register').send({ redirect_uris: redirectUris }); + + expect(response.status).toBe(201); + expect(response.body.redirect_uris).toEqual(redirectUris); + }); + + it('rejects redirect URIs with a fragment', async () => { + const response = await supertest(app) + .post('/register') + .send({ + redirect_uris: ['https://client.example.com/callback#fragment'] + }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_client_metadata'); + expect(spyRegisterClient).not.toHaveBeenCalled(); + }); + + it('accepts private-use URI scheme redirect URIs', async () => { + const response = await supertest(app) + .post('/register') + .send({ + redirect_uris: ['com.example.app:/oauth/callback'] + }); + + expect(response.status).toBe(201); + }); + it('successfully registers client with minimal metadata', async () => { const clientMetadata: OAuthClientMetadata = { redirect_uris: ['https://example.com/callback']