diff --git a/clients/cli/README.md b/clients/cli/README.md index 807b9881e..96039eaad 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -524,7 +524,7 @@ The CLI runs the same loopback callback server as the TUI (`http://127.0.0.1:627 1. Starts the callback listener on `--callback-url` (or `MCP_OAUTH_CALLBACK_URL`) 2. Prints the authorization URL to stderr (OSC 8 hyperlink when stderr is a TTY) and **opens the default browser** when allowed. Default: open on a **stderr** TTY (so `2>&1 | tee` prints a plain URL into the log but does not auto-open); plain URL only when stderr is non-TTY. `MCP_AUTO_OPEN_ENABLED=false` never opens; `=true` forces open even on a non-TTY (same as the web launcher). -3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC +3. Waits for the browser redirect, exchanges the code, and retries connect or the failed RPC. Only a redirect echoing this flow's OAuth `state` is acted on — any other request to the listener, an `?error=` response included, gets a 400 and is ignored, so another local process or a web page cannot cancel the sign-in or choose the error it reports; the wait continues until the matching redirect or the 15-minute timeout Interactive OAuth (connect-time or mid-RPC) requires a TTY on **stdin or stderr** (so `2>&1 | tee` still works — stdin stays a TTY), or `MCP_AUTO_OPEN_ENABLED=true`. When neither stdin nor stderr is a TTY and that env is unset (typical CI), the CLI fails fast with `auth_required` instead of waiting up to 15 minutes on the loopback callback. Use **`--stored-auth-only`** for non-interactive runs that should only consume the shared store. diff --git a/clients/cli/__tests__/oauth-interactive.test.ts b/clients/cli/__tests__/oauth-interactive.test.ts index b73a725ce..18e9afec7 100644 --- a/clients/cli/__tests__/oauth-interactive.test.ts +++ b/clients/cli/__tests__/oauth-interactive.test.ts @@ -39,7 +39,7 @@ const oauthTestStatePath = join( async function completeOAuthAuthorization( authorizationUrl: URL, -): Promise<{ code: string; iss?: string }> { +): Promise<{ code: string; iss?: string; state?: string }> { let response = await fetch(authorizationUrl.toString(), { redirect: "manual", }); @@ -73,14 +73,17 @@ async function completeOAuthAuthorization( throw new Error("Missing authorization code"); } const iss = redirect.searchParams.get("iss") ?? undefined; - return iss ? { code, iss } : { code }; + // The loopback listener only acts on a callback echoing the flow's state + // (#2847), exactly as a real browser redirect does. + const state = redirect.searchParams.get("state") ?? undefined; + return { code, ...(iss ? { iss } : {}), ...(state ? { state } : {}) }; } function createAutoCompleteNavigation( redirectUrlProvider: MutableRedirectUrlProvider, ) { return new CallbackNavigation(async (url) => { - const { code, iss } = await completeOAuthAuthorization(url); + const { code, iss, state } = await completeOAuthAuthorization(url); const redirect = redirectUrlProvider.redirectUrl; if (!redirect) { throw new Error("redirectUrl not set"); @@ -88,6 +91,7 @@ function createAutoCompleteNavigation( const callback = new URL(redirect); callback.searchParams.set("code", code); if (iss) callback.searchParams.set("iss", iss); + if (state) callback.searchParams.set("state", state); await fetch(callback.href); }); } diff --git a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts index a0cdaa267..52a032416 100644 --- a/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts +++ b/clients/web/src/test/core/auth/runner-interactive-oauth.test.ts @@ -37,6 +37,7 @@ function mockClient( interface MockCallbackHandlers { onCallback?: OAuthCallbackServerStartOptions["onCallback"]; onError?: OAuthCallbackServerStartOptions["onError"]; + expectedState?: OAuthCallbackServerStartOptions["expectedState"]; } function createMockCallbackServer(handlers: { @@ -47,6 +48,7 @@ function createMockCallbackServer(handlers: { handlers.current = { onCallback: opts.onCallback, onError: opts.onError, + expectedState: opts.expectedState, }; return { port: 6276, @@ -336,6 +338,7 @@ describe("runRunnerInteractiveOAuth", () => { handlers.current.onError?.({ error: "access_denied", error_description: "user cancelled", + source: "redirect", }); return new URL("https://as.example/authorize"); }), @@ -352,7 +355,177 @@ describe("runRunnerInteractiveOAuth", () => { }, createCallbackServer: () => createMockCallbackServer(handlers), }), - ).rejects.toThrow("user cancelled"); + ).rejects.toThrow( + "OAuth authorization failed. The authorization server said: user cancelled (access_denied)", + ); + }); + + it("hands the listener the state of the authorize URL authenticate() returns (#2847)", async () => { + const client = mockClient({ + authenticate: vi.fn(async () => { + await simulateCallback(handlers.current); + return new URL("https://as.example/authorize?state=connect-state"); + }), + }); + + await runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }); + + await expect(handlers.current.expectedState).resolves.toBe("connect-state"); + }); + + it("hands the listener the step-up authorize URL's state before navigating (#2847)", async () => { + let stateAtNavigation: string | undefined; + const client = mockClient({ + beginInteractiveAuthorization: vi.fn(async () => { + stateAtNavigation = await handlers.current.expectedState; + await simulateCallback(handlers.current, "step-up-code"); + }), + }); + + await runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + authorizationUrl: new URL("https://as.example/authorize?state=step-up"), + createCallbackServer: () => createMockCallbackServer(handlers), + }); + + expect(stateAtNavigation).toBe("step-up"); + }); + + it("resolves an undefined expected state for an authorize URL without one", async () => { + const client = mockClient({ + authenticate: vi.fn(async () => { + await simulateCallback(handlers.current); + return new URL("https://as.example/authorize"); + }), + }); + + await runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }); + + await expect(handlers.current.expectedState).resolves.toBeUndefined(); + }); + + it("leaves a listener-reported callback_error unlabeled — it is our failure, not the server's", async () => { + const client = mockClient({ + authenticate: vi.fn(async () => { + handlers.current.onError?.({ + error: "callback_error", + error_description: "exchange blew up", + source: "callback", + }); + return new URL("https://as.example/authorize"); + }), + }); + + const run = runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }); + await expect(run).rejects.toThrow("exchange blew up"); + await expect(run).rejects.not.toThrow("authorization server said"); + }); + + it("labels a redirect error even when it spells its code callback_error", async () => { + const client = mockClient({ + authenticate: vi.fn(async () => { + handlers.current.onError?.({ + error: "callback_error", + error_description: "Server-supplied diagnosis", + source: "redirect", + }); + return new URL("https://as.example/authorize"); + }), + }); + + await expect( + runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }), + ).rejects.toThrow( + "The authorization server said: Server-supplied diagnosis (callback_error)", + ); + }); + + it("reports a bare 'OAuth error' when the listener reports no error text at all", async () => { + const client = mockClient({ + authenticate: vi.fn(async () => { + handlers.current.onError?.({ error: "", source: "redirect" }); + return new URL("https://as.example/authorize"); + }), + }); + + const run = runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }); + await expect(run).rejects.toThrow(/^OAuth error$/); + }); + + it("falls back to a generic message for a callback_error without a description", async () => { + const client = mockClient({ + authenticate: vi.fn(async () => { + handlers.current.onError?.({ + error: "callback_error", + source: "callback", + }); + return new URL("https://as.example/authorize"); + }), + }); + + await expect( + runRunnerInteractiveOAuth({ + client, + redirectUrlProvider: { redirectUrl: "" }, + callbackListen: { + hostname: "127.0.0.1", + port: 6276, + pathname: "/oauth/callback", + }, + createCallbackServer: () => createMockCallbackServer(handlers), + }), + ).rejects.toThrow("OAuth callback failed"); }); it("times out when the browser callback never arrives", async () => { @@ -443,7 +616,10 @@ describe("runRunnerInteractiveOAuth", () => { const redirectUrlProvider = { redirectUrl: "" }; const client = mockClient({ authenticate: vi.fn(async () => { - handlers.current.onError?.({ error: "access_denied" }); + handlers.current.onError?.({ + error: "access_denied", + source: "redirect", + }); return new URL("https://as.example/authorize"); }), }); @@ -459,7 +635,9 @@ describe("runRunnerInteractiveOAuth", () => { }, createCallbackServer: () => createMockCallbackServer(handlers), }), - ).rejects.toThrow("access_denied"); + ).rejects.toThrow( + "OAuth authorization failed. The authorization server said: access_denied", + ); }); it("defaults to createOAuthCallbackServer when none is provided", async () => { diff --git a/clients/web/src/test/integration/auth/node/oauth-callback-server.test.ts b/clients/web/src/test/integration/auth/node/oauth-callback-server.test.ts index 63ba05d5c..e3c3858eb 100644 --- a/clients/web/src/test/integration/auth/node/oauth-callback-server.test.ts +++ b/clients/web/src/test/integration/auth/node/oauth-callback-server.test.ts @@ -1,9 +1,10 @@ -import { describe, it, expect, afterEach } from "vitest"; +import { describe, it, expect, afterEach, vi } from "vitest"; import { connect } from "node:net"; import { createOAuthCallbackServer, type OAuthCallbackServer, } from "@inspector/core/auth/node/oauth-callback-server.js"; +import { runRunnerInteractiveOAuth } from "@inspector/core/auth/node/runner-interactive-oauth.js"; /** * Send a raw HTTP request line over a socket so we can use request targets @@ -25,6 +26,21 @@ function rawRequest(port: number, requestLine: string): Promise { }); } +/** + * Resolves when the listener has received its next HTTP request — the + * server-side signal that a request has actually arrived, which a client-side + * sleep cannot establish under load. Element access reads the private + * `server` field (TypeScript permits it for exactly this kind of test seam); + * the class deliberately exposes no request hook of its own. + */ +function nextRequest(listener: OAuthCallbackServer): Promise { + const http = listener["server"]; + if (!http) throw new Error("listener not started"); + return new Promise((resolve) => { + http.once("request", () => resolve()); + }); +} + describe("OAuthCallbackServer", () => { let server: OAuthCallbackServer; @@ -108,6 +124,9 @@ describe("OAuthCallbackServer", () => { ); expect(res.status).toBe(404); + const html = await res.text(); + expect(html).toContain("not the OAuth callback"); + expect(html).not.toContain("OAuth complete"); }); it("GET /oauth/callback?error=access_denied returns 400 and invokes onError", async () => { @@ -128,7 +147,10 @@ describe("OAuthCallbackServer", () => { expect(res.status).toBe(400); const html = await res.text(); expect(html).toContain("OAuth failed"); - expect(html).toContain("access_denied"); + // #2847: the query-string text is labeled as the server's words. + expect(html).toContain( + "The authorization server said: Error: access_denied.", + ); expect(errors).toHaveLength(1); expect(errors[0]!.error).toBe("access_denied"); expect(errors[0]!.error_description).toBe("User denied"); @@ -154,9 +176,10 @@ describe("OAuthCallbackServer", () => { const res = await fetch(`http://localhost:${result.port}/other`); expect(res.status).toBe(404); + expect(await res.text()).toContain("not the OAuth callback"); }); - it("POST /oauth/callback returns 405", async () => { + it("POST /oauth/callback returns 405 with the not-the-callback page", async () => { server = createOAuthCallbackServer(); const result = await server.start({ port: 0 }); @@ -166,6 +189,9 @@ describe("OAuthCallbackServer", () => { ); expect(res.status).toBe(405); + const html = await res.text(); + expect(html).toContain("not the OAuth callback"); + expect(html).not.toContain("OAuth complete"); }); it("stops server after first successful callback so second request fails", async () => { @@ -235,6 +261,9 @@ describe("OAuthCallbackServer", () => { ); const second = await secondP; expect(second.status).toBe(409); + const html = await second.text(); + expect(html).toContain("already received its callback"); + expect(html).not.toContain("OAuth complete"); release(); const first = await firstP; expect(first.status).toBe(200); @@ -248,7 +277,9 @@ describe("OAuthCallbackServer", () => { // exercising the try/catch 400 branch in handleRequest. const raw = await rawRequest(result.port, "GET // HTTP/1.1"); expect(raw).toContain("400 Bad Request"); - expect(raw).toContain("OAuth complete"); + // #2843: a request that is not the callback must not claim success. + expect(raw).toContain("not the OAuth callback"); + expect(raw).not.toContain("OAuth complete"); }); it("returns 400 JSON for an unparseable URL when Accept: application/json", async () => { @@ -389,4 +420,205 @@ describe("OAuthCallbackServer", () => { `http://[::1]:${result.port}/oauth/callback`, ); }); + describe("expectedState (#2847)", () => { + const STATE = "flow-state-123"; + + it("ignores a success callback with the wrong state and keeps listening", async () => { + server = createOAuthCallbackServer(); + const codes: string[] = []; + const result = await server.start({ + port: 0, + expectedState: STATE, + onCallback: async (p) => { + codes.push(p.code); + }, + }); + const base = `http://localhost:${result.port}/oauth/callback`; + + const wrong = await fetch(`${base}?code=forged&state=other`); + expect(wrong.status).toBe(400); + const html = await wrong.text(); + expect(html).toContain("does not belong to the sign-in in progress"); + expect(html).not.toContain("OAuth complete"); + + const missing = await fetch(`${base}?code=forged`); + expect(missing.status).toBe(400); + expect(codes).toEqual([]); + + const right = await fetch(`${base}?code=real&state=${STATE}`); + expect(right.status).toBe(200); + expect(codes).toEqual(["real"]); + }); + + it("ignores an error response with the wrong (or no) state, so it cannot end the flow", async () => { + server = createOAuthCallbackServer(); + const errors: string[] = []; + const result = await server.start({ + port: 0, + expectedState: STATE, + onError: (p) => errors.push(p.error), + }); + const base = `http://localhost:${result.port}/oauth/callback`; + + const injected = await fetch( + `${base}?error=access_denied&error_description=injected+by+another+process`, + ); + expect(injected.status).toBe(400); + const html = await injected.text(); + expect(html).not.toContain("injected by another process"); + expect(html).toContain("was ignored"); + expect(errors).toEqual([]); + + const genuine = await fetch(`${base}?error=access_denied&state=${STATE}`); + expect(genuine.status).toBe(400); + expect(await genuine.text()).toContain("The authorization server said"); + expect(errors).toEqual(["access_denied"]); + }); + + it("answers a state mismatch with JSON when Accept: application/json", async () => { + server = createOAuthCallbackServer(); + const result = await server.start({ port: 0, expectedState: STATE }); + + const res = await fetch( + `http://localhost:${result.port}/oauth/callback?code=x&state=nope`, + { headers: { Accept: "application/json" } }, + ); + expect(res.status).toBe(400); + expect(await res.json()).toEqual({ error: "State mismatch" }); + }); + + it("holds a callback that arrives before the state is known, then matches it", async () => { + let resolveState!: (s: string | undefined) => void; + const expectedState = new Promise((r) => { + resolveState = r; + }); + server = createOAuthCallbackServer(); + const codes: string[] = []; + const result = await server.start({ + port: 0, + expectedState, + onCallback: async (p) => { + codes.push(p.code); + }, + }); + + const arrived = nextRequest(server); + const pending = fetch( + `http://localhost:${result.port}/oauth/callback?code=early&state=${STATE}`, + ); + await arrived; + expect(codes).toEqual([]); + resolveState(STATE); + expect((await pending).status).toBe(200); + expect(codes).toEqual(["early"]); + }); + + it("matches only a stateless callback when the flow carries no state", async () => { + server = createOAuthCallbackServer(); + const codes: string[] = []; + const result = await server.start({ + port: 0, + expectedState: Promise.resolve(undefined), + onCallback: async (p) => { + codes.push(p.code); + }, + }); + const base = `http://localhost:${result.port}/oauth/callback`; + + expect((await fetch(`${base}?code=a&state=x`)).status).toBe(400); + expect((await fetch(`${base}?code=b`)).status).toBe(200); + expect(codes).toEqual(["b"]); + }); + + it("answers a held callback with 503 when the listener stops first", async () => { + server = createOAuthCallbackServer(); + const result = await server.start({ + port: 0, + expectedState: new Promise(() => {}), + }); + + const arrived = nextRequest(server); + const pending = fetch( + `http://localhost:${result.port}/oauth/callback?code=x&state=${STATE}`, + ); + await arrived; + await server.stop(); + const res = await pending; + expect(res.status).toBe(503); + const html = await res.text(); + expect(html).toContain("no longer waiting"); + expect(html).not.toContain("OAuth complete"); + }); + + it("answers 503 JSON when expectedState rejects", async () => { + server = createOAuthCallbackServer(); + const result = await server.start({ + port: 0, + expectedState: Promise.reject(new Error("no flow")), + }); + + const res = await fetch( + `http://localhost:${result.port}/oauth/callback?code=x&state=${STATE}`, + { headers: { Accept: "application/json" } }, + ); + expect(res.status).toBe(503); + expect(await res.json()).toEqual({ + error: "Not waiting for a callback", + }); + }); + }); + + it("runRunnerInteractiveOAuth survives an injected error callback and completes on the genuine one (#2847)", async () => { + const STATE = "genuine-flow-state"; + const redirectUrlProvider = { redirectUrl: "" }; + const requests: Promise[] = []; + const completeOAuthFlow = vi.fn(async () => {}); + const listener = createOAuthCallbackServer(); + server = listener; + + const result = await runRunnerInteractiveOAuth({ + client: { + authenticate: async () => { + const base = redirectUrlProvider.redirectUrl; + // Another local process races the browser with a stateless error, + // then the genuine redirect lands. Neither is awaited here — a + // real navigation is not — and both arrive before the runner has + // learned the flow's state, so the listener holds them until then. + const forgedArrived = nextRequest(listener); + requests.push( + fetch( + `${base}?error=access_denied&error_description=injected+by+another+process`, + ), + ); + await forgedArrived; + const genuineArrived = nextRequest(listener); + requests.push(fetch(`${base}?code=real-code&state=${STATE}`)); + await genuineArrived; + return new URL(`https://as.example/authorize?state=${STATE}`); + }, + beginInteractiveAuthorization: async () => {}, + completeOAuthFlow, + checkAuthChallengeSatisfied: async () => true, + }, + redirectUrlProvider, + callbackListen: { + hostname: "127.0.0.1", + port: 0, + pathname: "/oauth/callback", + }, + callbackTimeoutMs: 10_000, + createCallbackServer: () => listener, + }); + + expect(result).toEqual({ kind: "success" }); + const [forged, genuine] = await Promise.all(requests); + expect(forged!.status).toBe(400); + expect(await forged!.text()).not.toContain("injected by another process"); + expect(genuine!.status).toBe(200); + expect(completeOAuthFlow).toHaveBeenCalledWith( + "real-code", + undefined, + STATE, + ); + }); }); diff --git a/core/auth/node/oauth-callback-server.ts b/core/auth/node/oauth-callback-server.ts index fa17b59b3..57b3e08ef 100644 --- a/core/auth/node/oauth-callback-server.ts +++ b/core/auth/node/oauth-callback-server.ts @@ -6,17 +6,70 @@ import { generateOAuthErrorDescription } from "../utils.js"; const DEFAULT_HOSTNAME = "127.0.0.1"; const DEFAULT_CALLBACK_PATH = "/oauth/callback"; -const SUCCESS_HTML = ` - -OAuth complete -

OAuth complete. You can close this window.

-`; +const SUCCESS_HTML = page( + "OAuth complete", + "OAuth complete. You can close this window.", +); + +/** + * Body for every request that is not the OAuth callback at all — a wrong + * path, a non-GET method, an unparseable target (#2843). These used to send + * {@link SUCCESS_HTML} under a 404/405/400 status, telling a user who opened + * the wrong URL that sign-in had finished while the CLI/TUI was still + * waiting for the real redirect. + */ +const NOT_CALLBACK_HTML = page( + "Not the OAuth callback", + "This address is not the OAuth callback the Inspector is waiting for. " + + "Sign-in has not finished; return to the Inspector to continue.", +); + +/** + * Body for a callback whose `state` is not the flow in progress (#2847). Such + * a request is ignored rather than allowed to end the flow, so this page must + * not claim sign-in finished — or failed — either. + */ +const STATE_MISMATCH_HTML = page( + "OAuth callback ignored", + "This callback does not belong to the sign-in in progress and was ignored. " + + "Sign-in has not finished; the Inspector is still waiting for the " + + "authorization server.", +); + +const ALREADY_HANDLED_HTML = page( + "OAuth callback already received", + "This sign-in has already received its callback. You can close this window.", +); + +const STOPPED_HTML = page( + "OAuth callback not expected", + "The Inspector is no longer waiting for this sign-in. " + + "Start sign-in again from the Inspector.", +); -function errorHtml(message: string): string { +/** + * Error page for an authorization-server error response. The text after the + * label comes from the redirect's query string, i.e. from whoever produced + * the request, so it is labeled as the server's words rather than presented + * as the Inspector's own diagnosis (#2847). + */ +function errorHtml(serverText: string): string { + return page( + "OAuth error", + `OAuth failed. The authorization server said: ${serverText}`, + ); +} + +/** Failure of the Inspector's own token exchange after a matching callback. */ +function exchangeErrorHtml(message: string): string { + return page("OAuth error", `OAuth failed: ${message}`); +} + +function page(title: string, text: string): string { return ` -OAuth error -

OAuth failed: ${escapeHtml(message)}

+${escapeHtml(title)} +

${escapeHtml(text)}

`; } @@ -38,6 +91,15 @@ export type OAuthCallbackHandler = (params: { export type OAuthErrorHandler = (params: { error: string; error_description?: string | null; + /** + * Where the error came from (#2847). `"redirect"`: an `?error=` response, + * whose `error` and `error_description` are query-string text the + * Inspector did not write. `"callback"`: the host's own `onCallback` + * rejected (the token exchange failed) — the Inspector's own message. + * Both strings are copied from the request in the first case, so neither + * can tell the two apart; this field is the only reliable discriminant. + */ + source: "redirect" | "callback"; }) => void; export interface OAuthCallbackServerStartOptions { @@ -46,6 +108,25 @@ export interface OAuthCallbackServerStartOptions { path?: string; onCallback?: OAuthCallbackHandler; onError?: OAuthErrorHandler; + /** + * The OAuth `state` of the flow this listener serves (#2847). Only a + * callback carrying exactly this `state` — success **or** error response — + * is acted on; any other gets a short 400 and the listener keeps waiting, + * so a local process or a web page that can make the browser hit the + * fixed loopback port can neither cancel the sign-in nor choose the text + * it reports. + * + * A promise, because the listener has to be bound (its URL is the + * `redirect_uri`) before the authorize URL — and so the `state` — exists. + * A callback that arrives before it settles is held until it does, which + * is what keeps an authorization server that redirects immediately from + * racing the caller. Resolving `undefined` means the flow carries no + * `state`, so only a callback without one matches. + * + * Omitted, no `state` check is made and the first callback ends the flow; + * that is only appropriate for a host that verifies `state` itself. + */ + expectedState?: string | Promise; } export interface OAuthCallbackServerStartResult { @@ -53,6 +134,23 @@ export interface OAuthCallbackServerStartResult { redirectUrl: string; } +/** No `expectedState` was given: accept a callback whatever its `state`. */ +const ANY_STATE = Symbol("any-state"); +/** The listener stopped while a callback waited on `expectedState`. */ +const STOPPED = Symbol("stopped"); + +/** A promise that settles to {@link STOPPED} once `fire()` is called. */ +function createStopSignal(): { + stopped: Promise; + fire: () => void; +} { + let fire!: () => void; + const stopped = new Promise((resolve) => { + fire = () => resolve(STOPPED); + }); + return { stopped, fire }; +} + /** * Minimal HTTP server that receives OAuth 2.1 redirects at GET /oauth/callback. * Used by TUI/CLI to complete the authorization code flow. @@ -66,6 +164,16 @@ export class OAuthCallbackServer { private handled = false; private onCallback?: OAuthCallbackHandler; private onError?: OAuthErrorHandler; + /** + * The verdict every callback waits on: the flow's `state` once known, or + * {@link STOPPED} if the listener stops first. Raced once per `start()`, + * not per request, so an ignored request leaves no reaction on the stop + * signal behind it for the rest of a wait that can last 15 minutes. + */ + private expectedState: Promise< + string | undefined | typeof ANY_STATE | typeof STOPPED + > = Promise.resolve(ANY_STATE); + private stopSignal = createStopSignal(); /** * Start the server. Port defaults to 0 (random) when omitted; TUI/CLI callers @@ -91,6 +199,18 @@ export class OAuthCallbackServer { this.onError = onError; this.handled = false; this.callbackPath = path; + // A rejected expectedState names no flow, so it is read as "stopped": + // nothing can match it, and the waiting callback must still be answered. + this.stopSignal = createStopSignal(); + this.expectedState = + "expectedState" in options + ? Promise.race([ + Promise.resolve(options.expectedState).catch( + (): typeof STOPPED => STOPPED, + ), + this.stopSignal.stopped, + ]) + : Promise.resolve(ANY_STATE); return new Promise((resolve, reject) => { this.server = createServer((req, res) => this.handleRequest(req, res)); @@ -115,10 +235,13 @@ export class OAuthCallbackServer { } /** - * Stop the server. Idempotent. + * Stop the server. Idempotent. A callback still waiting on + * `expectedState` is answered first, since `close()` waits for every open + * request to finish. */ async stop(): Promise { if (!this.server) return; + this.stopSignal.fire(); await new Promise((resolve) => { this.server!.close(() => resolve()); }); @@ -138,12 +261,21 @@ export class OAuthCallbackServer { body: string, contentType = "text/html; charset=utf-8", ) => { - res.writeHead(status, { "Content-Type": contentType }); + // A one-shot loopback listener gains nothing from keep-alive, and a + // kept-alive socket holds `stop()`'s `close()` open after the last + // response — including the 503 a held callback gets on stop. + res.writeHead(status, { + "Content-Type": contentType, + Connection: "close", + }); res.end(body); }; if (req.method !== "GET") { - send(405, needJson ? '{"error":"Method Not Allowed"}' : SUCCESS_HTML); + send( + 405, + needJson ? '{"error":"Method Not Allowed"}' : NOT_CALLBACK_HTML, + ); return; } @@ -161,19 +293,51 @@ export class OAuthCallbackServer { search = u.search; state = u.searchParams.get("state") ?? undefined; } catch { - send(400, needJson ? '{"error":"Bad Request"}' : SUCCESS_HTML); + send(400, needJson ? '{"error":"Bad Request"}' : NOT_CALLBACK_HTML); return; } if (pathname !== this.callbackPath) { - send(404, needJson ? '{"error":"Not Found"}' : SUCCESS_HTML); + send(404, needJson ? '{"error":"Not Found"}' : NOT_CALLBACK_HTML); + return; + } + + // void: the promise owns its outcome — every branch of handleCallback + // answers the request, and it has no rejection path to surface. + void this.expectedState.then((expected) => + this.handleCallback(expected, state, search, needJson, send), + ); + } + + private handleCallback( + expected: string | undefined | typeof ANY_STATE | typeof STOPPED, + state: string | undefined, + search: string, + needJson: boolean | undefined, + send: (status: number, body: string) => void, + ): void { + if (expected === STOPPED) { + send( + 503, + needJson ? '{"error":"Not waiting for a callback"}' : STOPPED_HTML, + ); + return; + } + + // #2847: a callback that is not for this flow — success or error + // response alike — is ignored, and the listener keeps waiting for the + // one that is (or for the caller's timeout). + if (expected !== ANY_STATE && state !== expected) { + send(400, needJson ? '{"error":"State mismatch"}' : STATE_MISMATCH_HTML); return; } if (this.handled) { send( 409, - needJson ? '{"error":"Callback already handled"}' : SUCCESS_HTML, + needJson + ? '{"error":"Callback already handled"}' + : ALREADY_HANDLED_HTML, ); return; } @@ -191,8 +355,12 @@ export class OAuthCallbackServer { }) .catch((err) => { const msg = err instanceof Error ? err.message : String(err); - this.onError?.({ error: "callback_error", error_description: msg }); - send(500, errorHtml(msg)); + this.onError?.({ + error: "callback_error", + error_description: msg, + source: "callback", + }); + send(500, exchangeErrorHtml(msg)); void this.stop(); }); } else { @@ -207,6 +375,7 @@ export class OAuthCallbackServer { this.onError?.({ error: params.error, error_description: params.error_description ?? undefined, + source: "redirect", }); send(400, errorHtml(msg)); } diff --git a/core/auth/node/runner-interactive-oauth.ts b/core/auth/node/runner-interactive-oauth.ts index 3191b5f75..59a9bac34 100644 --- a/core/auth/node/runner-interactive-oauth.ts +++ b/core/auth/node/runner-interactive-oauth.ts @@ -7,6 +7,7 @@ import { issuerBindingFailureCopy } from "../oauthUx.js"; import { createOAuthCallbackServer, type OAuthCallbackServer, + type OAuthErrorHandler, } from "./oauth-callback-server.js"; import type { RunnerOAuthCallbackConfig } from "./runner-oauth-callback.js"; @@ -78,6 +79,38 @@ function toRunnerOAuthError(err: unknown): Error { return err instanceof Error ? err : new Error(String(err)); } +/** The `state` an authorize URL carries — what its callback must echo. */ +function stateOf(authorizationUrl: URL): string | undefined { + return authorizationUrl.searchParams.get("state") ?? undefined; +} + +/** + * The flow's failure message for an error the listener reported. + * + * A redirect error (`?error=…`) is text from the redirect's query string, so + * it is labeled as the authorization server's words rather than passed off + * as the Inspector's own diagnosis (#2847). Only the listener's `source` + * marks a failure as ours — `error` itself is copied from the request, so a + * redirect can carry any value there, `callback_error` included. + */ +function callbackErrorMessage( + params: Parameters[0], +): string { + if (params.source === "callback") { + return params.error_description ?? "OAuth callback failed"; + } + // Nothing the server said to label — `error` is required by the protocol, + // but a listener reporting an empty one must still produce a readable + // failure rather than "said: ". + if (!params.error && !params.error_description) { + return "OAuth error"; + } + const said = params.error_description + ? `${params.error_description} (${params.error})` + : params.error; + return `OAuth authorization failed. The authorization server said: ${said}`; +} + /** * Run interactive OAuth for Node runners (TUI / CLI): loopback callback server, * browser redirect, authorization-code exchange via {@link completeOAuthFlow}. @@ -144,12 +177,22 @@ export async function runRunnerInteractiveOAuth( let timeoutId: ReturnType | undefined; + // The listener only acts on a callback carrying this flow's `state` + // (#2847). It has to be bound before the authorize URL exists, so the + // state is handed over once known; a callback that arrives first is held + // by the listener until then. + let resolveExpectedState!: (state: string | undefined) => void; + const expectedState = new Promise((resolve) => { + resolveExpectedState = resolve; + }); + try { const { redirectUrl } = await racingSignals( server.start({ hostname: options.callbackListen.hostname, port: options.callbackListen.port, path: options.callbackListen.pathname, + expectedState, onCallback: async (params) => { try { await options.client.completeOAuthFlow( @@ -163,12 +206,7 @@ export async function runRunnerInteractiveOAuth( } }, onError: (params) => { - flowReject( - new Error( - /* v8 ignore next -- params.error is a required non-null string, so the "OAuth error" fallback is unreachable */ - params.error_description ?? params.error ?? "OAuth error", - ), - ); + flowReject(new Error(callbackErrorMessage(params))); }, }), ); @@ -203,6 +241,7 @@ export async function runRunnerInteractiveOAuth( void waitForCallback.catch(() => {}); if (options.authorizationUrl) { + resolveExpectedState(stateOf(options.authorizationUrl)); await racingSignals( options.client.beginInteractiveAuthorization(options.authorizationUrl), ); @@ -210,6 +249,7 @@ export async function runRunnerInteractiveOAuth( } else { const authUrl = await racingSignals(options.client.authenticate()); if (authUrl !== undefined) { + resolveExpectedState(stateOf(authUrl)); await waitForCallback; } else { return { kind: "already_authorized" };