fix(client): treat HTTP 401/403 on the negotiation probe as auth failures, not legacy evidence (#2564)
This commit is contained in:
parent
a9835b4149
commit
faa7e2b470
3 changed files with 909 additions and 91 deletions
|
|
@ -2047,6 +2047,13 @@ export const REQUIREMENTS: Record<string, Requirement> = {
|
|||
transports: ['streamableHttp'],
|
||||
note: 'This exercises the HTTP hosting/auth layer and OAuth client; the matrix transport arg is ignored, so it runs as a single streamableHttp-labelled cell to avoid duplicate runs.'
|
||||
},
|
||||
'client-auth:negotiation:auth-before-era': {
|
||||
source: 'sdk',
|
||||
behavior:
|
||||
"An OAuth-protected legacy server is reachable under versionNegotiation mode 'auto': the connect-time probe's 401 propagates the auth challenge (UnauthorizedError) without deciding the era — auth settles first, era second — and after finishAuth the reconnect re-probes with the token, takes the legacy server's real server/discover rejection as the era evidence, completes the legacy initialize, and serves tools/call.",
|
||||
transports: ['streamableHttp'],
|
||||
note: "Wire-order pin: exactly two server/discover POSTs — the pre-auth one 401'd by the auth wall, the post-auth one answered by the legacy stack — then a single initialize, only after the second probe. Same single-cell setup as the rest of the client-auth family (self-contained body; the matrix transport arg is ignored)."
|
||||
},
|
||||
'client-auth:403-scope-upgrade': {
|
||||
source: 'https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#step-up-authorization-flow',
|
||||
behavior: 'A 403 with WWW-Authenticate triggers a scope-upgrade authorization attempt; repeated 403s do not loop.',
|
||||
|
|
|
|||
|
|
@ -348,6 +348,70 @@ verifies('client-auth:401-triggers-flow', async (_args: TestArgs) => {
|
|||
}
|
||||
});
|
||||
|
||||
verifies('client-auth:negotiation:auth-before-era', async (_args: TestArgs) => {
|
||||
const validToken = 'negotiated-token';
|
||||
const as = createMockAuthorizationServer({ tokenResponses: [{ access_token: validToken, token_type: 'Bearer' }] });
|
||||
const provider = new RecordingOAuthClientProvider();
|
||||
// The auth wall lives in createCombinedFetch (401 challenge without the
|
||||
// token); behind it a plain legacy stack serves the tool.
|
||||
const mcpHost = hostPerSession(() => {
|
||||
const s = new McpServer({ name: 's', version: '0' });
|
||||
s.registerTool('probe', { inputSchema: z.object({}) }, () => ({ content: [{ type: 'text', text: 'ok' }] }));
|
||||
return s;
|
||||
});
|
||||
const baseFetch = createCombinedFetch({ as, mcpHost, validToken });
|
||||
|
||||
// Wire recorder for MCP-origin POSTs: JSON-RPC method, response status, auth presence.
|
||||
const mcpPosts: Array<{ method: string; status: number; hasAuth: boolean }> = [];
|
||||
const combinedFetch = async (url: URL | string, init?: RequestInit): Promise<Response> => {
|
||||
const urlObj = typeof url === 'string' ? new URL(url) : url;
|
||||
const isMcp = urlObj.origin !== ISSUER && !urlObj.pathname.includes('/.well-known/');
|
||||
const response = await baseFetch(url, init);
|
||||
if (isMcp && init?.method === 'POST') {
|
||||
const body = typeof init.body === 'string' ? (JSON.parse(init.body) as { method?: string }) : {};
|
||||
mcpPosts.push({ method: body.method ?? '?', status: response.status, hasAuth: new Headers(init.headers).has('authorization') });
|
||||
}
|
||||
return response;
|
||||
};
|
||||
|
||||
const client = new Client({ name: 'c', version: '0' }, { versionNegotiation: { mode: 'auto' } });
|
||||
const first = new StreamableHTTPClientTransport(new URL(MCP_URL), { authProvider: provider, fetch: combinedFetch });
|
||||
|
||||
try {
|
||||
// Probe -> 401: the auth challenge propagates. The 401 never decides the
|
||||
// era -- the auth wall answered before the MCP layer saw server/discover.
|
||||
await expect(client.connect(first)).rejects.toThrow(UnauthorizedError);
|
||||
expect(mcpPosts).toEqual([{ method: 'server/discover', status: 401, hasAuth: false }]);
|
||||
expect(provider.redirectedTo).toHaveLength(1);
|
||||
|
||||
// Complete the flow (the mock AS exchanges any code), then reconnect on
|
||||
// a FRESH transport -- a started transport cannot be restarted.
|
||||
await first.finishAuth('e2e-auth-code');
|
||||
expect(provider.saved.tokens?.access_token).toBe(validToken);
|
||||
|
||||
const second = new StreamableHTTPClientTransport(new URL(MCP_URL), { authProvider: provider, fetch: combinedFetch });
|
||||
await client.connect(second);
|
||||
|
||||
// The post-auth re-probe supplied the era evidence: two probes total --
|
||||
// the pre-auth one 401'd, the post-auth one answered by the legacy
|
||||
// stack -- and initialize ran only after the second.
|
||||
const probes = mcpPosts.filter(p => p.method === 'server/discover');
|
||||
expect(probes).toHaveLength(2);
|
||||
expect(probes[1]).toMatchObject({ hasAuth: true });
|
||||
expect(probes[1]!.status).not.toBe(401);
|
||||
const kinds = mcpPosts.map(p => p.method);
|
||||
expect(kinds.filter(k => k === 'initialize')).toHaveLength(1);
|
||||
expect(kinds.indexOf('initialize')).toBeGreaterThan(kinds.lastIndexOf('server/discover'));
|
||||
expect(client.getNegotiatedProtocolVersion()).toBe('2025-11-25');
|
||||
|
||||
const result = await client.callTool({ name: 'probe', arguments: {} });
|
||||
expect(result.content).toEqual([{ type: 'text', text: 'ok' }]);
|
||||
} finally {
|
||||
await client.close();
|
||||
await mcpHost.close();
|
||||
}
|
||||
});
|
||||
|
||||
verifies('client-auth:401-after-auth-throws', async (_args: TestArgs) => {
|
||||
const as = createMockAuthorizationServer({
|
||||
tokenResponses: [{ access_token: 'refreshed-access-token', token_type: 'Bearer' }]
|
||||
|
|
|
|||
|
|
@ -22,9 +22,9 @@ import { createServer } from 'node:http';
|
|||
import { tmpdir } from 'node:os';
|
||||
import path from 'node:path';
|
||||
|
||||
import { Client, StreamableHTTPClientTransport } from '@modelcontextprotocol/client';
|
||||
import { Client, StreamableHTTPClientTransport, UnauthorizedError } from '@modelcontextprotocol/client';
|
||||
import { StdioClientTransport } from '@modelcontextprotocol/client/stdio';
|
||||
import { SdkError, SdkErrorCode } from '@modelcontextprotocol/core-internal';
|
||||
import { SdkError, SdkErrorCode, SdkHttpError } from '@modelcontextprotocol/core-internal';
|
||||
import { NodeStreamableHTTPServerTransport } from '@modelcontextprotocol/node';
|
||||
import { McpServer } from '@modelcontextprotocol/server';
|
||||
import { listenOnRandomPort } from '@modelcontextprotocol/test-helpers';
|
||||
|
|
@ -230,6 +230,117 @@ describe('typed connect errors (Q12) over real sockets', () => {
|
|||
}, 15_000);
|
||||
});
|
||||
|
||||
describe('auth-protected server (HTTP 401/403): typed auth failure, never a legacy verdict (#2561)', () => {
|
||||
const cleanups: Array<() => Promise<void> | void> = [];
|
||||
afterEach(async () => {
|
||||
while (cleanups.length > 0) await cleanups.pop()!();
|
||||
});
|
||||
|
||||
/** An OAuth-protected deployment shape: the auth layer rejects every tokenless request before any MCP handler runs. */
|
||||
async function startProtected(status: 401 | 403) {
|
||||
const server = createServer((_req, res) => {
|
||||
res.writeHead(status, {
|
||||
'WWW-Authenticate': 'Bearer realm="mcp", error="invalid_token"',
|
||||
'Content-Type': 'application/json'
|
||||
});
|
||||
res.end('{"error":"invalid_token"}');
|
||||
});
|
||||
const url = await listenOnRandomPort(server);
|
||||
cleanups.push(() => new Promise<void>(resolve => server.close(() => resolve())));
|
||||
return url;
|
||||
}
|
||||
|
||||
it('auto mode, no authProvider: typed auth failure carrying the 401 — no initialize ever sent', async () => {
|
||||
const url = await startProtected(401);
|
||||
const { calls, fetchFn } = recordingFetch();
|
||||
const client = new Client({ name: 'neg-client', version: '1.0.0' }, { versionNegotiation: { mode: 'auto' } });
|
||||
const transport = new StreamableHTTPClientTransport(url, { fetch: fetchFn });
|
||||
|
||||
await expect(client.connect(transport)).rejects.toSatisfy(
|
||||
error =>
|
||||
error instanceof SdkHttpError &&
|
||||
// The auth code, never EraNegotiationFailed: era-recovery flows
|
||||
// keyed on that code must not consume auth walls.
|
||||
error.code === SdkErrorCode.ClientHttpAuthentication &&
|
||||
error.status === 401 &&
|
||||
error.message.includes('401')
|
||||
);
|
||||
|
||||
// The probe POST is the only wire traffic — the auth wall is not a
|
||||
// legacy verdict, so no initialize follows it.
|
||||
const posts = calls.filter(c => c.method === 'POST');
|
||||
expect(posts.length).toBeGreaterThan(0);
|
||||
expect(posts.every(c => (c.body ?? '').includes('server/discover'))).toBe(true);
|
||||
expect(calls.some(c => (c.body ?? '').includes('"initialize"'))).toBe(false);
|
||||
});
|
||||
|
||||
it('auto mode, no authProvider: a 403 denial is a typed failure carrying the 403, not era evidence', async () => {
|
||||
const url = await startProtected(403);
|
||||
const { calls, fetchFn } = recordingFetch();
|
||||
const client = new Client({ name: 'neg-client', version: '1.0.0' }, { versionNegotiation: { mode: 'auto' } });
|
||||
|
||||
await expect(client.connect(new StreamableHTTPClientTransport(url, { fetch: fetchFn }))).rejects.toSatisfy(
|
||||
error => error instanceof SdkHttpError && error.code === SdkErrorCode.ClientHttpForbidden && error.status === 403
|
||||
);
|
||||
expect(calls.some(c => (c.body ?? '').includes('"initialize"'))).toBe(false);
|
||||
});
|
||||
|
||||
it('pin mode: the rejection names the auth status, never the did-not-offer-pinned-version text', async () => {
|
||||
const url = await startProtected(401);
|
||||
const client = new Client({ name: 'neg-client', version: '1.0.0' }, { versionNegotiation: { mode: { pin: '2026-07-28' } } });
|
||||
|
||||
await expect(client.connect(new StreamableHTTPClientTransport(url))).rejects.toSatisfy(
|
||||
error =>
|
||||
error instanceof SdkError &&
|
||||
error.message.includes('401') &&
|
||||
!error.message.includes('did not offer pinned protocol version')
|
||||
);
|
||||
});
|
||||
|
||||
it('with an authProvider the auth flow is unchanged: UnauthorizedError propagates for finishAuth, no fallback runs', async () => {
|
||||
const url = await startProtected(401);
|
||||
const { calls, fetchFn } = recordingFetch();
|
||||
const client = new Client({ name: 'neg-client', version: '1.0.0' }, { versionNegotiation: { mode: 'auto' } });
|
||||
const transport = new StreamableHTTPClientTransport(url, {
|
||||
fetch: fetchFn,
|
||||
// Token-only provider (no onUnauthorized) whose token the server
|
||||
// rejects: the transport's 401 handling throws UnauthorizedError,
|
||||
// and the probe propagates it unchanged — same as before this fix.
|
||||
authProvider: { token: async () => 'rejected-token' }
|
||||
});
|
||||
|
||||
await expect(client.connect(transport)).rejects.toSatisfy(error => error instanceof UnauthorizedError);
|
||||
expect(calls.some(c => (c.body ?? '').includes('"initialize"'))).toBe(false);
|
||||
});
|
||||
|
||||
it("onUnauthorized re-auth that does not help: the transport's typed 401-after-re-authentication failure propagates unchanged", async () => {
|
||||
const url = await startProtected(401);
|
||||
const { calls, fetchFn } = recordingFetch();
|
||||
const client = new Client({ name: 'neg-client', version: '1.0.0' }, { versionNegotiation: { mode: 'auto' } });
|
||||
let reauthRuns = 0;
|
||||
const transport = new StreamableHTTPClientTransport(url, {
|
||||
fetch: fetchFn,
|
||||
authProvider: {
|
||||
token: async () => 'still-rejected',
|
||||
onUnauthorized: async () => {
|
||||
reauthRuns++;
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
// First 401 runs onUnauthorized, the retry 401s again: the transport's
|
||||
// own diagnostic must reach the caller, not an era-negotiation rewrap.
|
||||
await expect(client.connect(transport)).rejects.toSatisfy(
|
||||
error =>
|
||||
error instanceof SdkHttpError &&
|
||||
error.code === SdkErrorCode.ClientHttpAuthentication &&
|
||||
error.message.includes('after re-authentication')
|
||||
);
|
||||
expect(reauthRuns).toBe(1);
|
||||
expect(calls.some(c => (c.body ?? '').includes('"initialize"'))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('stdio: silent legacy server (probe timeout fallback)', () => {
|
||||
// The stdio transport's backward-compatibility rule: a probe that gets no
|
||||
// response within a reasonable timeout indicates a legacy server — some
|
||||
|
|
|
|||
Loading…
Reference in a new issue