diff --git a/.changeset/bounded-auth-network.md b/.changeset/bounded-auth-network.md new file mode 100644 index 00000000..b69bc04a --- /dev/null +++ b/.changeset/bounded-auth-network.md @@ -0,0 +1,5 @@ +--- +"openwiki": patch +--- + +fix: bound OAuth and ngrok discovery requests with timeouts diff --git a/src/agent/openai-chatgpt-oauth.ts b/src/agent/openai-chatgpt-oauth.ts index cdfeabec..c1021e64 100644 --- a/src/agent/openai-chatgpt-oauth.ts +++ b/src/agent/openai-chatgpt-oauth.ts @@ -8,6 +8,7 @@ import { OPENAI_CHATGPT_PLAN_ENV_KEY, OPENAI_CHATGPT_REFRESH_TOKEN_ENV_KEY, } from "../constants.js"; +import { fetchWithAuthTimeout } from "../auth/http.js"; /** * ChatGPT/Codex OAuth client. @@ -315,11 +316,15 @@ function isCodexResponsesRequest(input: Parameters[0]): boolean { } async function exchangeToken(body: URLSearchParams): Promise { - const res = await fetch(TOKEN_URL, { - method: "POST", - headers: { "Content-Type": "application/x-www-form-urlencoded" }, - body, - }); + const res = await fetchWithAuthTimeout( + TOKEN_URL, + { + method: "POST", + headers: { "Content-Type": "application/x-www-form-urlencoded" }, + body, + }, + { operation: "ChatGPT token exchange" }, + ); if (!res.ok) { throw new Error( diff --git a/src/auth/http.ts b/src/auth/http.ts new file mode 100644 index 00000000..6d81e3ee --- /dev/null +++ b/src/auth/http.ts @@ -0,0 +1,71 @@ +const DEFAULT_AUTH_FETCH_TIMEOUT_MS = 15_000; + +export type AuthFetchOptions = { + timeoutMs?: number; + operation?: string; +}; + +/** Run one authentication request with a hard deadline; exchanges are not retried. */ +export async function fetchWithAuthTimeout( + input: Parameters[0], + init: Parameters[1] = {}, + options: AuthFetchOptions = {}, +): Promise { + const timeoutMs = options.timeoutMs ?? DEFAULT_AUTH_FETCH_TIMEOUT_MS; + const operation = options.operation ?? "Authentication request"; + const timeoutController = new AbortController(); + const callerSignal = init.signal; + const signal = callerSignal + ? AbortSignal.any([callerSignal, timeoutController.signal]) + : timeoutController.signal; + + let rejectTimeout!: (error: Error) => void; + const timeoutError = new Error( + `${operation} timed out after ${timeoutMs}ms. Check your network connection and try again.`, + ); + timeoutError.name = "AuthFetchTimeoutError"; + const timeoutPromise = new Promise((_, reject) => { + rejectTimeout = reject; + }); + let removeCallerAbortListener: (() => void) | undefined; + const callerAbortPromise = callerSignal + ? new Promise((_, reject) => { + const rejectCaller = () => { + const reason: unknown = callerSignal.reason as unknown; + reject( + reason instanceof Error + ? reason + : new Error("Request cancelled by caller"), + ); + }; + + if (callerSignal.aborted) { + rejectCaller(); + return; + } + + callerSignal.addEventListener("abort", rejectCaller, { once: true }); + removeCallerAbortListener = () => + callerSignal.removeEventListener("abort", rejectCaller); + }) + : null; + + const timeoutId = setTimeout(() => { + timeoutController.abort(timeoutError); + rejectTimeout(timeoutError); + }, timeoutMs); + + try { + const request = fetch(input, { ...init, signal }); + return await Promise.race( + callerAbortPromise + ? [request, timeoutPromise, callerAbortPromise] + : [request, timeoutPromise], + ); + } finally { + clearTimeout(timeoutId); + removeCallerAbortListener?.(); + } +} + +export const AUTH_FETCH_TIMEOUT_MS = DEFAULT_AUTH_FETCH_TIMEOUT_MS; diff --git a/src/auth/ngrok.ts b/src/auth/ngrok.ts index 99a712d2..317974eb 100644 --- a/src/auth/ngrok.ts +++ b/src/auth/ngrok.ts @@ -1,6 +1,7 @@ import { spawn } from "node:child_process"; import type { ChildProcess } from "node:child_process"; import { saveOpenWikiEnv } from "../env.js"; +import { fetchWithAuthTimeout } from "./http.js"; const DEFAULT_CALLBACK_PORT = 53682; const OAUTH_CALLBACK_PORT_ENV_KEY = "OPENWIKI_OAUTH_CALLBACK_PORT"; @@ -8,6 +9,7 @@ const HTTPS_OAUTH_REDIRECT_URI_ENV_KEY = "OPENWIKI_HTTPS_OAUTH_REDIRECT_URI"; const NGROK_API_URL = "http://127.0.0.1:4040/api/tunnels"; const NGROK_DISCOVERY_TIMEOUT_MS = 15_000; const NGROK_DISCOVERY_POLL_MS = 500; +const NGROK_FETCH_TIMEOUT_MS = 2_000; export type NgrokStartOptions = { port?: number; @@ -255,22 +257,40 @@ async function waitForRandomNgrokRedirectUri( port: number, ): Promise { const startedAt = Date.now(); + const deadline = startedAt + NGROK_DISCOVERY_TIMEOUT_MS; - while (Date.now() - startedAt < NGROK_DISCOVERY_TIMEOUT_MS) { - const redirectUri = await fetchNgrokRedirectUri(port); + while (Date.now() < deadline) { + const remainingMs = deadline - Date.now(); + const redirectUri = await fetchNgrokRedirectUri( + port, + Math.min(NGROK_FETCH_TIMEOUT_MS, remainingMs), + ); if (redirectUri) { return redirectUri; } - await sleep(NGROK_DISCOVERY_POLL_MS); + const delayMs = Math.min(NGROK_DISCOVERY_POLL_MS, deadline - Date.now()); + if (delayMs > 0) { + await sleep(delayMs); + } } return null; } -async function fetchNgrokRedirectUri(port: number): Promise { +async function fetchNgrokRedirectUri( + port: number, + timeoutMs: number, +): Promise { try { - const response = await fetch(NGROK_API_URL); + const response = await fetchWithAuthTimeout( + NGROK_API_URL, + {}, + { + operation: "ngrok tunnel discovery", + timeoutMs, + }, + ); if (!response.ok) { return null; } diff --git a/src/auth/oauth-discovery.ts b/src/auth/oauth-discovery.ts index 9dfa6abc..4fedcbd6 100644 --- a/src/auth/oauth-discovery.ts +++ b/src/auth/oauth-discovery.ts @@ -1,4 +1,5 @@ import { isIP } from "node:net"; +import { fetchWithAuthTimeout } from "./http.js"; export type OAuthMetadata = { authorization_endpoint?: string; @@ -30,13 +31,14 @@ export async function discoverProtectedResourceMetadata( ]; for (const candidate of candidates) { - const response = await fetch( + const response = await fetchWithAuthTimeout( validateOAuthEndpointUrl( candidate, "MCP protected resource metadata", options, ), { redirect: "manual" }, + { operation: "OAuth protected-resource discovery" }, ); if (response.ok) { return (await response.json()) as ProtectedResourceMetadata; @@ -63,13 +65,14 @@ export async function discoverAuthorizationServerMetadata( ]; for (const candidate of candidates) { - const response = await fetch( + const response = await fetchWithAuthTimeout( validateOAuthEndpointUrl( candidate, "OAuth authorization server metadata", options, ), { redirect: "manual" }, + { operation: "OAuth authorization-server discovery" }, ); if (response.ok) { return (await response.json()) as OAuthMetadata; diff --git a/src/auth/oauth.ts b/src/auth/oauth.ts index 1f069652..b9c372ab 100644 --- a/src/auth/oauth.ts +++ b/src/auth/oauth.ts @@ -8,6 +8,7 @@ import { validateOAuthEndpointUrl, } from "./oauth-discovery.js"; import { getAuthProvider } from "./providers.js"; +import { fetchWithAuthTimeout } from "./http.js"; import type { AuthProviderId, OAuthClientRegistration, @@ -203,20 +204,24 @@ async function registerMcpOAuthClient( validationOptions, ).toString(); - const registrationResponse = await fetch(registrationEndpoint, { - body: JSON.stringify({ - client_name: "OpenWiki", - grant_types: ["authorization_code", "refresh_token"], - redirect_uris: [redirectUri], - response_types: ["code"], - token_endpoint_auth_method: "none", - }), - headers: { - "Content-Type": "application/json", + const registrationResponse = await fetchWithAuthTimeout( + registrationEndpoint, + { + body: JSON.stringify({ + client_name: "OpenWiki", + grant_types: ["authorization_code", "refresh_token"], + redirect_uris: [redirectUri], + response_types: ["code"], + token_endpoint_auth_method: "none", + }), + headers: { + "Content-Type": "application/json", + }, + method: "POST", + redirect: "manual", }, - method: "POST", - redirect: "manual", - }); + { operation: `${provider.displayName} client registration` }, + ); if (!registrationResponse.ok) { throw new Error( @@ -306,7 +311,7 @@ async function exchangeAuthorizationCode({ body.set("resource", provider.mcpResourceUrl); } - const response = await fetch( + const response = await fetchWithAuthTimeout( validateOAuthEndpointUrl( registration.tokenUrl, `${provider.displayName} token endpoint`, @@ -320,6 +325,7 @@ async function exchangeAuthorizationCode({ method: "POST", redirect: "manual", }, + { operation: `${provider.displayName} token exchange` }, ); if (!response.ok) { diff --git a/src/auth/tokens.ts b/src/auth/tokens.ts index 57ecf4bc..58efc076 100644 --- a/src/auth/tokens.ts +++ b/src/auth/tokens.ts @@ -1,4 +1,5 @@ import { loadOpenWikiEnv, saveOpenWikiEnv } from "../env.js"; +import { fetchWithAuthTimeout } from "./http.js"; import { discoverAuthorizationServerMetadata, discoverProtectedResourceMetadata, @@ -92,7 +93,7 @@ export async function refreshOAuthAccessToken( body.set("resource", provider.mcpResourceUrl); } - const response = await fetch( + const response = await fetchWithAuthTimeout( validateOAuthEndpointUrl( tokenUrl, `${provider.displayName} token endpoint`, @@ -107,6 +108,7 @@ export async function refreshOAuthAccessToken( method: "POST", redirect: "manual", }, + { operation: `${provider.displayName} token refresh` }, ); if (!response.ok) { diff --git a/test/auth-http.test.ts b/test/auth-http.test.ts new file mode 100644 index 00000000..78124e15 --- /dev/null +++ b/test/auth-http.test.ts @@ -0,0 +1,63 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { fetchWithAuthTimeout } from "../src/auth/http.ts"; + +describe("fetchWithAuthTimeout", () => { + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + test("rejects a fetch that never settles and aborts its signal", async () => { + vi.useFakeTimers(); + let requestSignal: AbortSignal | undefined; + vi.stubGlobal( + "fetch", + vi.fn((_input: unknown, init?: { signal?: AbortSignal }) => { + requestSignal = init?.signal; + return new Promise(() => {}); + }), + ); + + const pending = fetchWithAuthTimeout( + "https://auth.example.test/token", + {}, + { operation: "Token exchange", timeoutMs: 1000 }, + ); + const rejection = expect(pending).rejects.toThrow( + "Token exchange timed out after 1000ms", + ); + await vi.advanceTimersByTimeAsync(1000); + + await rejection; + expect(requestSignal?.aborted).toBe(true); + }); + + test("propagates caller cancellation instead of converting it to a timeout", async () => { + const controller = new AbortController(); + vi.stubGlobal( + "fetch", + vi.fn( + (_input: unknown, init?: { signal?: AbortSignal }) => + new Promise((_, reject) => { + init?.signal?.addEventListener("abort", () => + reject( + init.signal?.reason instanceof Error + ? init.signal.reason + : new Error("cancelled"), + ), + ); + }), + ), + ); + + const pending = fetchWithAuthTimeout( + "https://auth.example.test/token", + { signal: controller.signal }, + { timeoutMs: 60_000 }, + ); + const reason = new Error("cancelled by caller"); + controller.abort(reason); + + await expect(pending).rejects.toBe(reason); + }); +}); diff --git a/test/oauth-url-validation.test.ts b/test/oauth-url-validation.test.ts index 23cf30c6..53fc98a3 100644 --- a/test/oauth-url-validation.test.ts +++ b/test/oauth-url-validation.test.ts @@ -41,6 +41,7 @@ describe("validateOAuthEndpointUrl", () => { describe("OAuth discovery fetches", () => { afterEach(() => { + vi.useRealTimers(); vi.unstubAllGlobals(); }); @@ -63,4 +64,23 @@ describe("OAuth discovery fetches", () => { expect(call[1]).toMatchObject({ redirect: "manual" }); } }); + + test("times out a stalled metadata response", async () => { + vi.useFakeTimers(); + vi.stubGlobal( + "fetch", + vi.fn(() => new Promise(() => {})), + ); + + const pending = discoverAuthorizationServerMetadata( + "https://auth.notion.com/oauth", + { allowedHosts: ["notion.com"] }, + ); + const rejection = expect(pending).rejects.toThrow( + "OAuth authorization-server discovery timed out", + ); + await vi.advanceTimersByTimeAsync(15_000); + + await rejection; + }); });