Skip to content

Commit 4ca8bb1

Browse files
committed
fix(mcp): exchange OAuth codes on the redirecting transport
finishAuth was running on the pre-failure HTTP transport after a probe client built the authorize URL, so the code exchange could use a different resource than the redirect. A wiped session is replaced before the retry.
1 parent 931a497 commit 4ca8bb1

5 files changed

Lines changed: 266 additions & 42 deletions

File tree

‎docs/MCP.md‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -141,8 +141,10 @@ than hanging or dispatching against a dead transport.
141141

142142
An HTTP SSE drop does not start that second dial. The Streamable HTTP
143143
transport resumes the stream itself (`reconnectionOptions`: 1s start, doubling,
144-
30s cap). The row stays connected and tools stay live; only a real stdio death
145-
uses the fail-fast stubs and attempt count above.
144+
30s cap). Those options have no random factor and no exhaustion callback, so
145+
the delay is not jittered and a stream that stops retrying stays on the
146+
connected row instead of flipping to failed. Tools stay live; only a real
147+
stdio death uses the fail-fast stubs and attempt count above.
146148

147149
Pressing Enter on a failed or reconnecting row retries now with a single dial
148150
instead of waiting for backoff, reusing the saved settings row — it never

‎src/mcp/client-auth-reauth-cap.test.ts‎

Lines changed: 149 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,27 @@ import {
1919
interface MockState {
2020
finishAuthCalls: number;
2121
finishAuthError: Error | undefined;
22+
finishAuthInstanceId: number | undefined;
23+
finishAuthResourceMetadataUrl: string | undefined;
24+
finishAuthScope: string | undefined;
25+
redirectingInstanceId: number | undefined;
26+
preFailureResourceMetadataUrl: string | undefined;
27+
preFailureScope: string | undefined;
28+
challengeResourceMetadataUrl: string | undefined;
29+
challengeScope: string | undefined;
30+
boundTransport:
31+
| {
32+
instanceId: number;
33+
resourceMetadataUrl?: string;
34+
scope?: string;
35+
sessionId?: string;
36+
}
37+
| undefined;
38+
stampSessionId: string | undefined;
39+
sessionStamped: boolean;
40+
callSessionIds: (string | undefined)[];
41+
callOAuthError: Error | undefined;
42+
callbackCloseThrows: boolean;
2243
connectFailuresLeft: number;
2344
connectCalls: number;
2445
/** Thrown once, before any redirect, then cleared. */
@@ -61,6 +82,20 @@ function freshMockState(): MockState {
6182
return {
6283
finishAuthCalls: 0,
6384
finishAuthError: new Error("finishAuth exploded"),
85+
finishAuthInstanceId: undefined,
86+
finishAuthResourceMetadataUrl: undefined,
87+
finishAuthScope: undefined,
88+
redirectingInstanceId: undefined,
89+
preFailureResourceMetadataUrl: undefined,
90+
preFailureScope: undefined,
91+
challengeResourceMetadataUrl: undefined,
92+
challengeScope: undefined,
93+
boundTransport: undefined,
94+
stampSessionId: undefined,
95+
sessionStamped: false,
96+
callSessionIds: [],
97+
callOAuthError: undefined,
98+
callbackCloseThrows: false,
6499
connectFailuresLeft: 0,
65100
connectCalls: 0,
66101
connectOAuthError: undefined,
@@ -193,18 +228,44 @@ function waitForGate(signal: AbortSignal | undefined): Promise<void> {
193228
}
194229

195230
await mockMcpClientModule(mock, {
196-
connect: async (provider) => {
231+
connect: async (provider, transport) => {
197232
mock.connectCalls += 1;
233+
if (transport !== undefined) mock.boundTransport = transport;
198234
if (mock.connectOAuthError !== undefined) {
235+
if (
236+
transport !== undefined &&
237+
mock.preFailureResourceMetadataUrl !== undefined
238+
) {
239+
transport.resourceMetadataUrl = mock.preFailureResourceMetadataUrl;
240+
if (mock.preFailureScope !== undefined)
241+
transport.scope = mock.preFailureScope;
242+
}
199243
const error = mock.connectOAuthError;
200244
mock.connectOAuthError = undefined;
201245
throw error;
202246
}
203247
if (mock.connectFailuresLeft > 0) {
204248
mock.connectFailuresLeft -= 1;
249+
if (
250+
transport !== undefined &&
251+
mock.challengeResourceMetadataUrl !== undefined
252+
) {
253+
transport.resourceMetadataUrl = mock.challengeResourceMetadataUrl;
254+
if (mock.challengeScope !== undefined)
255+
transport.scope = mock.challengeScope;
256+
mock.redirectingInstanceId = transport.instanceId;
257+
}
205258
await emitRedirects(provider);
206259
throw new UnauthorizedError("authorization required");
207260
}
261+
if (
262+
transport !== undefined &&
263+
mock.stampSessionId !== undefined &&
264+
!mock.sessionStamped
265+
) {
266+
transport.sessionId = mock.stampSessionId;
267+
mock.sessionStamped = true;
268+
}
208269
},
209270
listTools: async () => {
210271
if (mock.listFailuresLeft > 0) {
@@ -218,8 +279,23 @@ await mockMcpClientModule(mock, {
218279
}
219280
return { tools: [] };
220281
},
221-
callTool: async () => {
282+
callTool: async (sessionId) => {
222283
mock.callToolCalls += 1;
284+
mock.callSessionIds.push(sessionId);
285+
if (mock.callOAuthError !== undefined) {
286+
if (
287+
mock.boundTransport !== undefined &&
288+
mock.preFailureResourceMetadataUrl !== undefined
289+
) {
290+
mock.boundTransport.resourceMetadataUrl =
291+
mock.preFailureResourceMetadataUrl;
292+
if (mock.preFailureScope !== undefined)
293+
mock.boundTransport.scope = mock.preFailureScope;
294+
}
295+
const error = mock.callOAuthError;
296+
mock.callOAuthError = undefined;
297+
throw error;
298+
}
223299
if (mock.callFailuresLeft > 0) {
224300
mock.callFailuresLeft -= 1;
225301
if (mock.callRedirectsLeft > 0) {
@@ -237,6 +313,9 @@ await mockMcpClientModule(mock, {
237313
await mockMcpTransportModule(mock, {
238314
finishAuth: async (self) => {
239315
mock.finishAuthCalls += 1;
316+
mock.finishAuthInstanceId = self.instanceId;
317+
mock.finishAuthResourceMetadataUrl = self.resourceMetadataUrl;
318+
mock.finishAuthScope = self.scope;
240319
mock.exchangedCodeVerifier = self.provider?.codeVerifier?.();
241320
if (mock.finishAuthError !== undefined) throw mock.finishAuthError;
242321
},
@@ -248,7 +327,9 @@ await mockMcpCallbackServerModule({
248327
await waitForGate(signal);
249328
return "code";
250329
},
251-
close: () => undefined,
330+
close: () => {
331+
if (mock.callbackCloseThrows) throw new Error("callback close threw");
332+
},
252333
});
253334

254335
await mockMcpOAuthProviderModule(
@@ -734,6 +815,71 @@ describe("HTTP MCP re-auth loop prevention", () => {
734815
expect(mock.finishAuthCalls).toBe(0);
735816
});
736817

818+
const defaultDiscovery =
819+
"https://mcp.linear.app/.well-known/oauth-protected-resource";
820+
const challengeMetadata =
821+
"https://resource.linear.app/.well-known/oauth-protected-resource/mcp";
822+
823+
test("finishAuth uses the redirecting transport's 401 resource metadata", async () => {
824+
mock.finishAuthError = undefined;
825+
mock.preFailureResourceMetadataUrl = defaultDiscovery;
826+
mock.preFailureScope = "legacy";
827+
mock.challengeResourceMetadataUrl = challengeMetadata;
828+
mock.challengeScope = "mcp:read";
829+
mock.connectOAuthError = new InvalidScopeError("scope not allowed");
830+
mock.connectFailuresLeft = 1;
831+
832+
const result = await connectWithAuthPrompt();
833+
834+
expect(result.ok).toBe(true);
835+
expect(mock.finishAuthCalls).toBe(1);
836+
expect(mock.redirectingInstanceId).toBeDefined();
837+
expect(mock.finishAuthInstanceId).toBe(mock.redirectingInstanceId);
838+
expect(mock.finishAuthResourceMetadataUrl).toBe(challengeMetadata);
839+
expect(mock.finishAuthScope).toBe("mcp:read");
840+
expect(mock.finishAuthResourceMetadataUrl).not.toBe(defaultDiscovery);
841+
});
842+
843+
test("open-session code exchange does not retry the wiped mcp-session-id", async () => {
844+
mock.finishAuthError = undefined;
845+
mock.stampSessionId = "sess-wiped";
846+
mock.preFailureResourceMetadataUrl = defaultDiscovery;
847+
mock.preFailureScope = "legacy";
848+
mock.challengeResourceMetadataUrl = challengeMetadata;
849+
mock.challengeScope = "mcp:read";
850+
const connected = await connectMCPServer(config, {
851+
onAuthURL: () => {
852+
mock.authURLCount += 1;
853+
},
854+
});
855+
expect(connected.ok).toBe(true);
856+
if (!connected.ok) return;
857+
858+
mock.callOAuthError = new InvalidScopeError("scope not allowed");
859+
mock.connectFailuresLeft = 1;
860+
861+
await expect(
862+
callText(connected.client, "ping", {}, new AbortController().signal),
863+
).resolves.toBe("");
864+
865+
expect(mock.finishAuthInstanceId).toBe(mock.redirectingInstanceId);
866+
expect(mock.finishAuthResourceMetadataUrl).toBe(challengeMetadata);
867+
expect(mock.finishAuthScope).toBe("mcp:read");
868+
expect(mock.callSessionIds[0]).toBe("sess-wiped");
869+
expect(mock.callSessionIds.at(-1)).toBeUndefined();
870+
});
871+
872+
test("a sync callback close throw stays a failed connect result", async () => {
873+
mock.callbackCloseThrows = true;
874+
mock.connectOAuthError = new Error("dial failed");
875+
876+
const result = await connectMCPServer(config, {});
877+
878+
expect(result.ok).toBe(false);
879+
if (result.ok) return;
880+
expect(result.error).toBe("dial failed");
881+
});
882+
737883
test("reconnect during in-flight waitForCode does not emit a second prompt", async () => {
738884
mock.connectFailuresLeft = Number.POSITIVE_INFINITY;
739885
mock.callbackGate = new Promise(() => undefined);

‎src/mcp/client.ts‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,9 @@ interface HTTPAuthContext {
137137
coordinator: SdkAuthFinish;
138138
interactive: boolean;
139139
serverName: string;
140+
client: Client;
141+
/** Builds the next HTTP transport and records it as the live session. */
142+
openTransport: () => Transport;
140143
onAuthorized?: (serverName: string) => void;
141144
}
142145

@@ -156,7 +159,11 @@ interface SdkAuthFinish {
156159
notified?: boolean;
157160
recoveryGeneration?: number;
158161
waiters?: number;
159-
/** Transport the SDK already dialed. finishAuth reuses it; no second client. */
162+
/**
163+
* Transport that finishAuth must use. A probe dial replaces the pre-failure
164+
* transport with the instance that called redirectToAuthorization, so the
165+
* code exchange sends that instance's resource metadata and scope.
166+
*/
160167
transport?: StreamableHTTPClientTransport;
161168
}
162169

@@ -456,16 +463,21 @@ async function dialForFreshAuthorization(
456463
coordinator.lifecycle.signal,
457464
),
458465
);
466+
let redirected = false;
459467
try {
460468
await probeClient.connect(transport as unknown as Transport, {
461469
signal: coordinator.lifecycle.signal,
462470
});
463471
} catch (probeErr) {
464472
if (!isRecoverableAuthError(probeErr)) throw probeErr;
465473
if (coordinator.browserFlow === undefined) throw probeErr;
474+
redirected = true;
466475
} finally {
467476
await closeQuietly(() => probeClient.close());
468477
}
478+
// The probe, not the pre-failure transport, holds the resource metadata and
479+
// scope the authorize URL was built from. finishAuth has to run on it.
480+
if (redirected) coordinator.transport = transport;
469481
}
470482

471483
async function driveRecovery(
@@ -510,6 +522,19 @@ async function driveRecovery(
510522
if (transport === undefined)
511523
throw new Error("no live HTTP transport to finish authorization");
512524
await transport.finishAuth(code);
525+
// The probe is a second client. After its close, the live session still
526+
// carries the mcp-session-id from before resetAuthorization wiped tokens.
527+
const liveSession = context.client.transport;
528+
if (
529+
liveSession !== undefined &&
530+
liveSession !== transport &&
531+
liveSession.sessionId !== undefined
532+
) {
533+
await context.client.close();
534+
await context.client.connect(context.openTransport(), {
535+
signal: coordinator.lifecycle.signal,
536+
});
537+
}
513538
}
514539

515540
function completeVerifiedRecovery(
@@ -619,7 +644,7 @@ async function finishClient(
619644
authContext?: HTTPAuthContext,
620645
signal?: AbortSignal,
621646
closeLifecycle?: () => void,
622-
liveTransport?: { onclose?: () => void },
647+
live?: { current?: { onclose?: () => void } },
623648
): Promise<MCPClient> {
624649
const result = await withHTTPAuthorizationRecovery(authContext, () =>
625650
signal === undefined
@@ -668,7 +693,7 @@ async function finishClient(
668693
},
669694
async close() {
670695
// Disarm first: the owner's hook reports unintentional death only.
671-
if (liveTransport !== undefined) liveTransport.onclose = () => undefined;
696+
if (live?.current !== undefined) live.current.onclose = () => undefined;
672697
closeLifecycle?.();
673698
authContext?.callback.close();
674699
await closeQuietly(() => client.close());
@@ -735,6 +760,7 @@ async function connectStdio(
735760
options.onDisconnect?.();
736761
};
737762
}
763+
const live = { current: transport };
738764
return {
739765
ok: true,
740766
client: await finishClient(
@@ -743,7 +769,7 @@ async function connectStdio(
743769
undefined,
744770
options.signal,
745771
undefined,
746-
transport,
772+
live,
747773
),
748774
};
749775
} catch (err) {
@@ -781,13 +807,22 @@ async function connectHttp(
781807
const normalizedURL = normalizeMCPServerURL(config.url);
782808
const url = new URL(normalizedURL);
783809
let authContext: HTTPAuthContext | undefined;
810+
const live: { current?: { onclose?: () => void } } = {};
811+
const connectedClient = new Client({
812+
name: MCP_CLIENT_NAME,
813+
version: "1.0.0",
814+
});
815+
client = connectedClient;
784816
let makeTransport: () => Transport;
785817
if (config.oauth === false) {
786-
makeTransport = () =>
787-
new StreamableHTTPClientTransport(
818+
makeTransport = () => {
819+
const transport = new StreamableHTTPClientTransport(
788820
url,
789821
streamableHTTPTransportOptions(undefined, options.signal),
790-
) as unknown as Transport;
822+
);
823+
live.current = transport;
824+
return transport as unknown as Transport;
825+
};
791826
} else {
792827
callback = await startCallbackServer(config.name);
793828
const authProvider = await createOAuthProvider({
@@ -807,6 +842,7 @@ async function connectHttp(
807842
streamableHTTPTransportOptions(authProvider, lifecycle.signal),
808843
);
809844
coordinator.transport = transport;
845+
live.current = transport;
810846
return transport as unknown as Transport;
811847
};
812848
authContext = {
@@ -816,24 +852,19 @@ async function connectHttp(
816852
coordinator,
817853
interactive: options.onAuthURL !== undefined,
818854
serverName: config.name,
855+
client: connectedClient,
856+
openTransport: () => makeTransport(),
819857
...(options.onAuthorized !== undefined
820858
? { onAuthorized: options.onAuthorized }
821859
: {}),
822860
};
823861
gateRedirectToAuthorization(authContext);
824862
}
825-
const connectedClient = new Client({
826-
name: MCP_CLIENT_NAME,
827-
version: "1.0.0",
828-
});
829-
client = connectedClient;
830863
// Recovery may dial again after finishAuth; the last transport wins.
831-
let liveTransport: Transport | undefined;
832864
await withHTTPAuthorizationRecovery(
833865
authContext,
834866
() => {
835867
const transport = makeTransport();
836-
liveTransport = transport;
837868
return connectedClient.connect(transport, {
838869
signal: lifecycle.signal,
839870
});
@@ -848,7 +879,7 @@ async function connectHttp(
848879
authContext,
849880
options.signal,
850881
closeLifecycle,
851-
liveTransport,
882+
live,
852883
),
853884
};
854885
} catch (err) {

0 commit comments

Comments
 (0)