diff --git a/packages/connections/src/github-connect.test.ts b/packages/connections/src/github-connect.test.ts index 9b4ce47ab..f7f7f9b73 100644 --- a/packages/connections/src/github-connect.test.ts +++ b/packages/connections/src/github-connect.test.ts @@ -119,4 +119,31 @@ describe("exchangeCodeForGithubToken", () => { expect(result.message).toContain("getaddrinfo ENOTFOUND"); } }); + + // CL-7235: a GitHub token endpoint that never answers used to leave + // this exchange awaiting `doFetch` forever. It now carries a bounded + // `AbortSignal`, so a stalled provider is caught the same way any + // other network failure already is instead of hanging the `/callback` + // request indefinitely. + test("wires a bounded AbortSignal into the exchange fetch so a stalled provider can't hang the exchange", async () => { + let capturedSignal: AbortSignal | undefined; + const fetchImpl: ExchangeFetch = (_url, init) => { + capturedSignal = init.signal; + return new Promise(() => { + // never resolves -- a provider that never answers. + }); + }; + + void exchangeCodeForGithubToken({ + code: "auth_code_1", + redirectUri: "https://hub.example.test/callback", + clientId: "client_1", + clientSecret: "secret_1", + fetchImpl, + }); + await Promise.resolve(); + + expect(capturedSignal).toBeInstanceOf(AbortSignal); + expect(capturedSignal?.aborted).toBe(false); + }); }); diff --git a/packages/connections/src/github-connect.ts b/packages/connections/src/github-connect.ts index 0ce570c0f..9992106be 100644 --- a/packages/connections/src/github-connect.ts +++ b/packages/connections/src/github-connect.ts @@ -13,6 +13,11 @@ import { type } from "arktype"; +import { + postExchangeRequest, + type OAuthExchangeFetch, +} from "./oauth-exchange-fetch"; + export const GITHUB_AUTHORIZE_URL = "https://github.com/login/oauth/authorize"; export const GITHUB_TOKEN_EXCHANGE_URL = "https://github.com/login/oauth/access_token"; @@ -27,14 +32,7 @@ export type ExchangeResult = | { readonly ok: true; readonly key: string } | { readonly ok: false; readonly message: string }; -export type ExchangeFetch = ( - url: string, - init: { - method: "POST"; - headers: Record; - body: string; - }, -) => Promise; +export type ExchangeFetch = OAuthExchangeFetch; export type ExchangeCodeForGithubTokenArgs = { readonly code: string; @@ -55,7 +53,7 @@ export async function exchangeCodeForGithubToken( const doFetch = args.fetchImpl ?? fetch; let response: Response; try { - response = await doFetch(GITHUB_TOKEN_EXCHANGE_URL, { + response = await postExchangeRequest(doFetch, GITHUB_TOKEN_EXCHANGE_URL, { method: "POST", headers: { "content-type": "application/json", diff --git a/packages/connections/src/gmail-connect.test.ts b/packages/connections/src/gmail-connect.test.ts index ca59f32bb..e2b95968a 100644 --- a/packages/connections/src/gmail-connect.test.ts +++ b/packages/connections/src/gmail-connect.test.ts @@ -7,6 +7,7 @@ import { expect, test } from "bun:test"; import { exchangeCodeForGoogleToken, GOOGLE_TOKEN_EXCHANGE_URL, + type ExchangeFetch, } from "./gmail-connect"; function stubFetch( @@ -89,3 +90,30 @@ test("a Google error response maps to an honest failure that never echoes token expect(result.message).toContain("invalid_grant"); expect(result.message).not.toContain("secret-1"); }); + +// CL-7235: a Google token endpoint that never answers used to leave this +// exchange awaiting `doFetch` forever. It now carries a bounded +// `AbortSignal`, so a stalled provider is caught the same way any other +// network failure already is instead of hanging the `/callback` request +// indefinitely. +test("wires a bounded AbortSignal into the exchange fetch so a stalled provider can't hang the exchange", async () => { + let capturedSignal: AbortSignal | undefined; + const fetchImpl: ExchangeFetch = (_url, init) => { + capturedSignal = init.signal; + return new Promise(() => { + // never resolves -- a provider that never answers. + }); + }; + + void exchangeCodeForGoogleToken({ + code: "auth-code-1", + redirectUri: "https://bench.example.com/callback", + clientId: "client-1", + clientSecret: "secret-1", + fetchImpl, + }); + await Promise.resolve(); + + expect(capturedSignal).toBeInstanceOf(AbortSignal); + expect(capturedSignal?.aborted).toBe(false); +}); diff --git a/packages/connections/src/gmail-connect.ts b/packages/connections/src/gmail-connect.ts index b337fbf19..4adbe66b0 100644 --- a/packages/connections/src/gmail-connect.ts +++ b/packages/connections/src/gmail-connect.ts @@ -12,6 +12,11 @@ import { type } from "arktype"; +import { + postExchangeRequest, + type OAuthExchangeFetch, +} from "./oauth-exchange-fetch"; + export const GOOGLE_AUTHORIZE_URL = "https://accounts.google.com/o/oauth2/v2/auth"; export const GOOGLE_TOKEN_EXCHANGE_URL = "https://oauth2.googleapis.com/token"; @@ -38,20 +43,17 @@ export type GoogleExchangeResult = } | { readonly ok: false; readonly message: string }; +/** Re-exported for symmetry with the other three connect modules and so + * tests can type a stub `fetchImpl` explicitly. */ +export type ExchangeFetch = OAuthExchangeFetch; + export type ExchangeCodeForGoogleTokenArgs = { readonly code: string; readonly codeVerifier?: string; readonly redirectUri: string; readonly clientId: string; readonly clientSecret: string; - readonly fetchImpl?: ( - url: string, - init: { - method: "POST"; - headers: Record; - body: string; - }, - ) => Promise; + readonly fetchImpl?: ExchangeFetch; }; /** @@ -77,7 +79,7 @@ export async function exchangeCodeForGoogleToken( let response: Response; try { - response = await doFetch(GOOGLE_TOKEN_EXCHANGE_URL, { + response = await postExchangeRequest(doFetch, GOOGLE_TOKEN_EXCHANGE_URL, { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, body: params.toString(), diff --git a/packages/connections/src/huggingface-connect.test.ts b/packages/connections/src/huggingface-connect.test.ts new file mode 100644 index 000000000..cbde44b49 --- /dev/null +++ b/packages/connections/src/huggingface-connect.test.ts @@ -0,0 +1,86 @@ +// Unit tests for the Hugging Face connector's code-for-token exchange, +// driven entirely against a stubbed fetch -- no Hugging Face credentials +// involved. +import { expect, test } from "bun:test"; + +import { + exchangeCodeForToken, + HUGGINGFACE_TOKEN_URL, + type ExchangeFetch, +} from "./huggingface-connect"; + +test("exchanges the code and verifier for an access token and expiry", async () => { + const requests: { url: string; body: string }[] = []; + const fetchImpl: ExchangeFetch = async (url, init) => { + requests.push({ url, body: init.body }); + return new Response( + JSON.stringify({ access_token: "hf_minted_token", expires_in: 3600 }), + { status: 200 }, + ); + }; + + const result = await exchangeCodeForToken({ + code: "auth_code_1", + codeVerifier: "verifier_1", + redirectUri: "https://hub.example.test/callback", + clientId: "client_1", + fetchImpl, + now: () => 1_000_000, + }); + + expect(result).toEqual({ + ok: true, + accessToken: "hf_minted_token", + expiresAt: new Date(1_000_000 + 3600 * 1000).toISOString(), + }); + expect(requests[0]?.url).toBe(HUGGINGFACE_TOKEN_URL); + const params = new URLSearchParams(requests[0]?.body ?? ""); + expect(params.get("code")).toBe("auth_code_1"); + expect(params.get("code_verifier")).toBe("verifier_1"); +}); + +test("a transport failure is reported honestly, never as a token", async () => { + const fetchImpl: ExchangeFetch = async () => { + throw new Error("getaddrinfo ENOTFOUND"); + }; + + const result = await exchangeCodeForToken({ + code: "auth_code_1", + codeVerifier: "verifier_1", + redirectUri: "https://hub.example.test/callback", + clientId: "client_1", + fetchImpl, + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.message).toContain("Could not reach Hugging Face"); + } +}); + +// CL-7235: a Hugging Face token endpoint that never answers used to +// leave this exchange awaiting `doFetch` forever. It now carries a +// bounded `AbortSignal`, so a stalled provider is caught the same way +// any other network failure already is instead of hanging the +// `/callback` request indefinitely. +test("wires a bounded AbortSignal into the exchange fetch so a stalled provider can't hang the exchange", async () => { + let capturedSignal: AbortSignal | undefined; + const fetchImpl: ExchangeFetch = (_url, init) => { + capturedSignal = init.signal; + return new Promise(() => { + // never resolves -- a provider that never answers. + }); + }; + + void exchangeCodeForToken({ + code: "auth_code_1", + codeVerifier: "verifier_1", + redirectUri: "https://hub.example.test/callback", + clientId: "client_1", + fetchImpl, + }); + await Promise.resolve(); + + expect(capturedSignal).toBeInstanceOf(AbortSignal); + expect(capturedSignal?.aborted).toBe(false); +}); diff --git a/packages/connections/src/huggingface-connect.ts b/packages/connections/src/huggingface-connect.ts index 977d0b1df..45d3bbcac 100644 --- a/packages/connections/src/huggingface-connect.ts +++ b/packages/connections/src/huggingface-connect.ts @@ -15,6 +15,11 @@ import { type } from "arktype"; +import { + postExchangeRequest, + type OAuthExchangeFetch, +} from "./oauth-exchange-fetch"; + export const HUGGINGFACE_AUTHORIZE_URL = "https://huggingface.co/oauth/authorize"; export const HUGGINGFACE_TOKEN_URL = "https://huggingface.co/oauth/token"; @@ -44,14 +49,7 @@ export type ExchangeResult = } | { readonly ok: false; readonly message: string }; -export type ExchangeFetch = ( - url: string, - init: { - method: "POST"; - headers: Record; - body: string; - }, -) => Promise; +export type ExchangeFetch = OAuthExchangeFetch; export type ExchangeCodeForTokenArgs = { readonly code: string; @@ -84,7 +82,7 @@ export async function exchangeCodeForToken( let response: Response; try { - response = await doFetch(HUGGINGFACE_TOKEN_URL, { + response = await postExchangeRequest(doFetch, HUGGINGFACE_TOKEN_URL, { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, body: body.toString(), diff --git a/packages/connections/src/oauth-exchange-fetch.test.ts b/packages/connections/src/oauth-exchange-fetch.test.ts new file mode 100644 index 000000000..fbf6fefe2 --- /dev/null +++ b/packages/connections/src/oauth-exchange-fetch.test.ts @@ -0,0 +1,87 @@ +// Every OAuth connect module (github-connect.ts, gmail-connect.ts, +// huggingface-connect.ts, openrouter-connect.ts) routes its outbound +// token/key exchange through `postExchangeRequest`. These tests exercise +// that shared mechanism directly, standing in for a per-provider hang +// test: proving the timeout cancels an in-flight request (aborts the +// fetch a real implementation is waiting on) rather than racing a timer +// while the request keeps running unobserved. +import { expect, test } from "bun:test"; + +import { + OAUTH_EXCHANGE_TIMEOUT_MS, + postExchangeRequest, + type OAuthExchangeFetch, +} from "./oauth-exchange-fetch"; + +// The two real-timer tests below use a small but not razor-thin +// `timeoutMs` (100ms) and a generous per-test timeout (20s) so they +// prove the abort actually fires without false-failing on a busy CI +// runner -- the assertion is "clearly bounded, not that it happened in +// under a handful of milliseconds." +test("a provider that never answers is aborted once the timeout fires, not left hanging", async () => { + let sawAbort = false; + const fetchImpl: OAuthExchangeFetch = (_url, init) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener("abort", () => { + sawAbort = true; + reject(init.signal?.reason); + }); + }); + + await expect( + postExchangeRequest( + fetchImpl, + "https://provider.example.test/token", + { method: "POST", headers: {}, body: "" }, + 100, + ), + ).rejects.toBeDefined(); + expect(sawAbort).toBe(true); +}, 20_000); + +test("passes the caller's timeoutMs through to the abort signal, not the default", async () => { + const start = performance.now(); + const fetchImpl: OAuthExchangeFetch = (_url, init) => + new Promise((_resolve, reject) => { + init.signal?.addEventListener("abort", () => reject(init.signal?.reason)); + }); + + await expect( + postExchangeRequest( + fetchImpl, + "https://provider.example.test/token", + { method: "POST", headers: {}, body: "" }, + 100, + ), + ).rejects.toBeDefined(); + + expect(performance.now() - start).toBeLessThan(OAUTH_EXCHANGE_TIMEOUT_MS); +}, 20_000); + +test("a provider that answers before the timeout resolves normally", async () => { + const fetchImpl: OAuthExchangeFetch = async () => + new Response(JSON.stringify({ ok: true }), { status: 200 }); + + const response = await postExchangeRequest( + fetchImpl, + "https://provider.example.test/token", + { method: "POST", headers: {}, body: "" }, + 5, + ); + + expect(response.status).toBe(200); +}); + +test("defaults to OAUTH_EXCHANGE_TIMEOUT_MS when no timeoutMs is given", async () => { + const fetchImpl: OAuthExchangeFetch = (_url, init) => { + expect(init.signal).toBeInstanceOf(AbortSignal); + expect(init.signal?.aborted).toBe(false); + return Promise.resolve(new Response(null, { status: 200 })); + }; + + await postExchangeRequest(fetchImpl, "https://provider.example.test/token", { + method: "POST", + headers: {}, + body: "", + }); +}); diff --git a/packages/connections/src/oauth-exchange-fetch.ts b/packages/connections/src/oauth-exchange-fetch.ts new file mode 100644 index 000000000..7c84cd9fb --- /dev/null +++ b/packages/connections/src/oauth-exchange-fetch.ts @@ -0,0 +1,36 @@ +// Shared code-for-token/key exchange fetch for every OAuth connect +// module (github-connect.ts, gmail-connect.ts, huggingface-connect.ts, +// openrouter-connect.ts). Each posts once to its provider's token +// endpoint and must not hang forever if that provider never answers -- +// this mirrors the `AbortSignal.timeout(...)` already applied to every +// outbound call in `./probes.ts`, so one shared call site replaces four +// separate copies of the same timeout wiring. The signal aborts the +// underlying request the instant the timer fires; it never races a +// timer while leaving the fetch itself running unobserved. + +export const OAUTH_EXCHANGE_TIMEOUT_MS = 10_000; + +export type OAuthExchangeFetch = ( + url: string, + init: { + method: "POST"; + headers: Record; + body: string; + signal?: AbortSignal; + }, +) => Promise; + +/** + * Posts to an OAuth token/key exchange endpoint bounded by `timeoutMs` + * (default `OAUTH_EXCHANGE_TIMEOUT_MS`). A timeout rejects the same way + * any other network failure does, so callers keep their existing + * catch-and-convert-to-`{ ok: false, message }` handling unchanged. + */ +export async function postExchangeRequest( + fetchImpl: OAuthExchangeFetch, + url: string, + init: { method: "POST"; headers: Record; body: string }, + timeoutMs: number = OAUTH_EXCHANGE_TIMEOUT_MS, +): Promise { + return fetchImpl(url, { ...init, signal: AbortSignal.timeout(timeoutMs) }); +} diff --git a/packages/connections/src/openrouter-connect.test.ts b/packages/connections/src/openrouter-connect.test.ts new file mode 100644 index 000000000..679b9bf96 --- /dev/null +++ b/packages/connections/src/openrouter-connect.test.ts @@ -0,0 +1,76 @@ +// Unit tests for the OpenRouter connector's code-for-key exchange, +// driven entirely against a stubbed fetch -- no OpenRouter credentials +// involved. +import { expect, test } from "bun:test"; + +import { + exchangeCodeForKey, + OPENROUTER_KEY_EXCHANGE_URL, + type ExchangeFetch, +} from "./openrouter-connect"; + +test("exchanges the code and verifier for the user-scoped API key", async () => { + const requests: { url: string; body: unknown }[] = []; + const fetchImpl: ExchangeFetch = async (url, init) => { + requests.push({ url, body: JSON.parse(init.body) }); + return new Response(JSON.stringify({ key: "sk-or-minted" }), { + status: 200, + }); + }; + + const result = await exchangeCodeForKey({ + code: "auth_code_1", + codeVerifier: "verifier_1", + fetchImpl, + }); + + expect(result).toEqual({ ok: true, key: "sk-or-minted" }); + expect(requests[0]?.url).toBe(OPENROUTER_KEY_EXCHANGE_URL); + expect(requests[0]?.body).toEqual({ + code: "auth_code_1", + code_verifier: "verifier_1", + code_challenge_method: "S256", + }); +}); + +test("a transport failure is reported honestly, never as a key", async () => { + const fetchImpl: ExchangeFetch = async () => { + throw new Error("getaddrinfo ENOTFOUND"); + }; + + const result = await exchangeCodeForKey({ + code: "auth_code_1", + codeVerifier: "verifier_1", + fetchImpl, + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.message).toContain("Could not reach OpenRouter"); + } +}); + +// CL-7235: an OpenRouter key endpoint that never answers used to leave +// this exchange awaiting `doFetch` forever. It now carries a bounded +// `AbortSignal`, so a stalled provider is caught the same way any other +// network failure already is instead of hanging the `/callback` request +// indefinitely. +test("wires a bounded AbortSignal into the exchange fetch so a stalled provider can't hang the exchange", async () => { + let capturedSignal: AbortSignal | undefined; + const fetchImpl: ExchangeFetch = (_url, init) => { + capturedSignal = init.signal; + return new Promise(() => { + // never resolves -- a provider that never answers. + }); + }; + + void exchangeCodeForKey({ + code: "auth_code_1", + codeVerifier: "verifier_1", + fetchImpl, + }); + await Promise.resolve(); + + expect(capturedSignal).toBeInstanceOf(AbortSignal); + expect(capturedSignal?.aborted).toBe(false); +}); diff --git a/packages/connections/src/openrouter-connect.ts b/packages/connections/src/openrouter-connect.ts index cd432557d..e87b09126 100644 --- a/packages/connections/src/openrouter-connect.ts +++ b/packages/connections/src/openrouter-connect.ts @@ -8,6 +8,11 @@ import { type } from "arktype"; +import { + postExchangeRequest, + type OAuthExchangeFetch, +} from "./oauth-exchange-fetch"; + export const OPENROUTER_AUTH_URL = "https://openrouter.ai/auth"; export const OPENROUTER_KEY_EXCHANGE_URL = "https://openrouter.ai/api/v1/auth/keys"; @@ -22,14 +27,7 @@ export type ExchangeResult = | { readonly ok: true; readonly key: string } | { readonly ok: false; readonly message: string }; -export type ExchangeFetch = ( - url: string, - init: { - method: "POST"; - headers: Record; - body: string; - }, -) => Promise; +export type ExchangeFetch = OAuthExchangeFetch; export type ExchangeCodeForKeyArgs = { readonly code: string; @@ -49,7 +47,7 @@ export async function exchangeCodeForKey( const doFetch = args.fetchImpl ?? fetch; let response: Response; try { - response = await doFetch(OPENROUTER_KEY_EXCHANGE_URL, { + response = await postExchangeRequest(doFetch, OPENROUTER_KEY_EXCHANGE_URL, { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify({