From dbb5e18e57a7336dc36c24ad540096ee8be407a9 Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:13:51 -0500 Subject: [PATCH 1/2] Send somebody back to Settings when the vendor cannot be reached at all The callback says every failure ends the same way: back at Settings with a word about what happened, and nothing written. One did not. Redeeming the code turns a refusal into null by asking `!response.ok`, and that question needs a response. There is none when the connection is refused, the name does not resolve, TLS will not agree, or the fifteen-second timeout fires, so the rejection went straight through a function whose whole contract is to refuse quietly. Nothing above it catches anything, so somebody who had just consented at the vendor got a bare 500 with no Location instead of the page that would have told them. registerDynamicClient had the same hole against the same promise, one step earlier in the flow, and is fixed with it. Neither route needed changing: both already do the right thing with null, and both were already tested doing it. Caught is not the same as unnoticed, so both log the cause. Until now the framework's own handler printed these on the way to the 500, and buying the redirect by dropping that would have made a vendor outage look exactly like nobody trying to connect. The person sees the ordinary failure, indistinguishable from every other refusal on an endpoint that deliberately tells an unauthenticated caller nothing; the deployment sees which vendor and why. --- server/src/plugins/oauth.ts | 109 +++++++++++++++----- server/tests/plugin-oauth-callback.test.ts | 114 +++++++++++++++++++++ server/tests/plugin-oauth.test.ts | 97 ++++++++++++++++++ 3 files changed, 294 insertions(+), 26 deletions(-) diff --git a/server/src/plugins/oauth.ts b/server/src/plugins/oauth.ts index 5d9c2b70d..0367e5669 100644 --- a/server/src/plugins/oauth.ts +++ b/server/src/plugins/oauth.ts @@ -323,20 +323,57 @@ export async function redeemAuthorizationCode(input: { // A public (DCR) client proves itself with PKCE, and some vendors refuse an unexpected empty field. if (input.clientSecret) params.set("client_secret", input.clientSecret); - const response = await fetch(input.tokenUrl, { - method: "POST", - headers: { "content-type": "application/x-www-form-urlencoded" }, - body: params, + /* + * A vendor that could not be reached at all, which the refusal below cannot see. + * + * `!response.ok` needs a response, and there is none when the connection is refused, the name does + * not resolve, TLS will not agree, or the timeout on this request fires. Unguarded, that rejection + * escaped a function whose whole contract is to refuse quietly, and it escaped at the worst + * available moment: the callback has no failure handler above it, so somebody who had just + * consented at the vendor got a bare 500 with no Location instead of Settings telling them it did + * not work. The same reasoning as the defensive read further down, one step earlier in the request. + * + * Nothing is read off the error. Which of those it was is a fact about the vendor's infrastructure + * on a route that answers an unauthenticated caller, and the person is told the same sentence + * either way. + */ + let response: Response; + try { + response = await fetch(input.tokenUrl, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: params, + /* + * A redirect is a refusal, not a detour to be followed. + * + * `tokenUrl` is pinned in the catalogue because this request carries a client secret and an + * authorization code, and following a 302 would hand both to whatever address the answer named. + * Manual leaves the 3xx as the response, which is not `ok`, so it falls into the refusal below. + */ + redirect: "manual", + signal: AbortSignal.timeout(15_000), + }); + } catch (error) { /* - * A redirect is a refusal, not a detour to be followed. + * Logged, because refusing quietly to the person must not mean refusing quietly to the + * deployment. Until this, an unreachable token endpoint reached Hono's default handler, which + * prints the error before its 500; catching it without a line here would have bought the + * redirect by making a vendor outage look exactly like nobody trying to connect. * - * `tokenUrl` is pinned in the catalogue because this request carries a client secret and an - * authorization code, and following a 302 would hand both to whatever address the answer named. - * Manual leaves the 3xx as the response, which is not `ok`, so it falls into the refusal below. + * The status is what a person sees and this is what an operator sees, and only the second one + * says which vendor and why. Neither the code nor the client secret is in a transport error: + * they are in the request body, which never got sent. */ - redirect: "manual", - signal: AbortSignal.timeout(15_000), - }); + console.error( + JSON.stringify({ + type: "oauth-token-endpoint-unreachable", + tokenUrl: input.tokenUrl, + note: "A person's consent could not be redeemed. They were sent back to Settings with a failure.", + error: String(error), + }), + ); + return null; + } if (!response.ok) return null; @@ -388,21 +425,41 @@ export async function registerDynamicClient(input: { registrationUrl: string; redirectUri: string; }): Promise<{ clientId: string; clientSecret: string } | null> { - const response = await fetch(input.registrationUrl, { - method: "POST", - headers: { "content-type": "application/json" }, - body: JSON.stringify({ - redirect_uris: [input.redirectUri], - grant_types: ["authorization_code", "refresh_token"], - response_types: ["code"], - token_endpoint_auth_method: "none", - client_name: "OpenBot", - }), - // The registration endpoint is pinned in the catalogue, so a redirect is somebody else deciding - // where this deployment introduces itself. Left as the response, which is not `ok`. - redirect: "manual", - signal: AbortSignal.timeout(15_000), - }); + // A vendor that could not be reached at all — see `redeemAuthorizationCode`, which states the same + // gap in full. This one lands on an administrator pressing Connect rather than on somebody + // mid-consent, and null is what turns it into the 502 that route already writes for a vendor that + // would not register us. + let response: Response; + try { + response = await fetch(input.registrationUrl, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + redirect_uris: [input.redirectUri], + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + token_endpoint_auth_method: "none", + client_name: "OpenBot", + }), + // The registration endpoint is pinned in the catalogue, so a redirect is somebody else deciding + // where this deployment introduces itself. Left as the response, which is not `ok`. + redirect: "manual", + signal: AbortSignal.timeout(15_000), + }); + } catch (error) { + // Logged for the same reason the redemption above is: the caller's 502 tells an administrator + // to check the vendor's status, and this is the line that says the vendor could not be reached + // at all rather than that it turned us down. + console.error( + JSON.stringify({ + type: "oauth-registration-endpoint-unreachable", + registrationUrl: input.registrationUrl, + note: "This deployment could not introduce itself to the vendor. Connect answered 502.", + error: String(error), + }), + ); + return null; + } if (!response.ok) return null; // A 200 is not a promise of JSON — see `redeemAuthorizationCode`. A body that will not parse is // the vendor answering with something other than a client, which is this function's null. diff --git a/server/tests/plugin-oauth-callback.test.ts b/server/tests/plugin-oauth-callback.test.ts index 757d84a78..018bf965e 100644 --- a/server/tests/plugin-oauth-callback.test.ts +++ b/server/tests/plugin-oauth-callback.test.ts @@ -229,3 +229,117 @@ describe("a consent this deployment did not start", () => { expect(recorded).toEqual([]); }); }); + +/** + * The vendor answered the consent screen and then could not be reached for the redemption. + * + * Every other refusal here is one of our own checks saying no before the network is touched, which + * is why nothing caught this: the vendor in these tests is either willing or never asked. A vendor + * that is reachable enough to send somebody back and unreachable a moment later is the ordinary + * shape of an outage, and it lands on somebody who has just consented. + */ +describe("a vendor that could not be reached for the redemption", () => { + async function sealed(): Promise { + return await sealConnectState( + { userId: "user-1", serverId: "notion", verifier: "v-1" }, + KEY, + ); + } + + async function withUnreachableVendor( + reject: () => never, + run: () => Promise, + ): Promise { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => reject()) as unknown as typeof fetch; + try { + await run(); + } finally { + globalThis.fetch = realFetch; + } + } + + test("a connection failure ends at Settings, not on a 500", async () => { + const recorded: Recorded[] = []; + const hono = app({ recorded }); + const state = await sealed(); + + await withUnreachableVendor( + () => { + throw new TypeError( + "Unable to connect. Is the computer able to access the url?", + ); + }, + async () => { + const response = await hono.request(callbackUrl(state)); + // Both halves of the promise the handler makes, and the status is the half that was broken: + // a throw out of the redemption left Hono answering 500 with no Location at all. + expect(response.status).toBe(302); + expect(response.headers.get("location")).toBe(FAILED); + }, + ); + expect(recorded).toEqual([]); + }); + + test("a token endpoint that never answers ends the same way", async () => { + const recorded: Recorded[] = []; + const hono = app({ recorded }); + const state = await sealed(); + + await withUnreachableVendor( + () => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }, + async () => { + const response = await hono.request(callbackUrl(state)); + expect(response.status).toBe(302); + expect(response.headers.get("location")).toBe(FAILED); + }, + ); + expect(recorded).toEqual([]); + }); + + /** + * Quiet to the person, not quiet to the deployment. + * + * The redirect above is deliberately the same one every other refusal produces, which is what + * makes this test necessary: from the outside a vendor that is down and a vendor that said no are + * now indistinguishable, so the only place the difference survives is the log. Before the refusal + * was caught at all, the framework's own handler printed it on the way to a 500; catching it + * without putting a line back would have paid for the redirect with the outage nobody can see. + */ + test("the deployment is told, even though the person is only sent back", async () => { + const recorded: Recorded[] = []; + const hono = app({ recorded }); + const state = await sealed(); + const said: string[] = []; + const realError = console.error; + console.error = (...args: unknown[]) => { + said.push(args.map(String).join(" ")); + }; + + try { + await withUnreachableVendor( + () => { + throw new TypeError("Unable to connect."); + }, + async () => { + const response = await hono.request(callbackUrl(state)); + expect(response.status).toBe(302); + expect(response.headers.get("location")).toBe(FAILED); + }, + ); + } finally { + console.error = realError; + } + + const line = said.find((said) => + said.includes("oauth-token-endpoint-unreachable"), + ); + expect(line).toBeDefined(); + // Which vendor, so an operator reading this knows where to look, and the cause. + expect(line).toContain("mcp.notion.com"); + expect(line).toContain("Unable to connect."); + expect(recorded).toEqual([]); + }); +}); diff --git a/server/tests/plugin-oauth.test.ts b/server/tests/plugin-oauth.test.ts index b8487c9d6..d8e7f9ec0 100644 --- a/server/tests/plugin-oauth.test.ts +++ b/server/tests/plugin-oauth.test.ts @@ -558,6 +558,50 @@ describe("registering this deployment as an OAuth client", () => { globalThis.fetch = realFetch; } }); + + /** + * A vendor that cannot be reached at all, which is not the same as one that refused. + * + * `!response.ok` needs a response, and there is none when the connection is refused, the name does + * not resolve, TLS will not agree or the fifteen-second timeout fires first. Unguarded, the fetch + * rejects straight through a function whose documented answer to a vendor that will not register + * us is null, and the caller that reads null to mean "try again later" never runs. + */ + test("a vendor that cannot be reached is a refusal, not a thrown transport error", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => { + throw new TypeError( + "Unable to connect. Is the computer able to access the url?", + ); + }) as unknown as typeof fetch; + try { + expect( + await registerDynamicClient({ + registrationUrl: "https://vendor.example/register", + redirectUri: "https://openbot.example/cb", + }), + ).toBeNull(); + } finally { + globalThis.fetch = realFetch; + } + }); + + test("a registration endpoint that never answers is the same refusal", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }) as unknown as typeof fetch; + try { + expect( + await registerDynamicClient({ + registrationUrl: "https://vendor.example/register", + redirectUri: "https://openbot.example/cb", + }), + ).toBeNull(); + } finally { + globalThis.fetch = realFetch; + } + }); }); describe("redeeming an authorization code", () => { @@ -699,4 +743,57 @@ describe("redeeming an authorization code", () => { globalThis.fetch = realFetch; } }); + + /** + * A token endpoint that cannot be reached at all. + * + * The one failure with no response to inspect, so `!response.ok` never sees it: a refused + * connection, a name that does not resolve, TLS that will not agree, or the fifteen-second timeout + * firing. It is also the failure that lands at the worst moment, on somebody who has just consented + * at the vendor and is being sent back here. Unguarded it escapes the callback as a bare 500 with + * no Location, so instead of Settings saying it did not work, they get an error page. + */ + test("a token endpoint that cannot be reached is a refusal, not a thrown transport error", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => { + throw new TypeError( + "Unable to connect. Is the computer able to access the url?", + ); + }) as unknown as typeof fetch; + try { + expect( + await redeemAuthorizationCode({ + tokenUrl: "https://vendor.example/token", + clientId: "client-id", + clientSecret: "", + code: "code-1", + redirectUri: "https://openbot.example/api/plugins/oauth/callback", + verifier: "verifier-1", + }), + ).toBeNull(); + } finally { + globalThis.fetch = realFetch; + } + }); + + test("a token endpoint that never answers is the same refusal", async () => { + const realFetch = globalThis.fetch; + globalThis.fetch = (async () => { + throw new DOMException("The operation timed out.", "TimeoutError"); + }) as unknown as typeof fetch; + try { + expect( + await redeemAuthorizationCode({ + tokenUrl: "https://vendor.example/token", + clientId: "client-id", + clientSecret: "", + code: "code-1", + redirectUri: "https://openbot.example/api/plugins/oauth/callback", + verifier: "verifier-1", + }), + ).toBeNull(); + } finally { + globalThis.fetch = realFetch; + } + }); }); From ee5eb66467ded8ea34102bd359753d005ea6c37e Mon Sep 17 00:00:00 2001 From: beardthelion <56458543+beardthelion@users.noreply.github.com> Date: Wed, 2 Sep 2026 23:39:39 -0500 Subject: [PATCH 2/2] Answer the rest of the connect flow's failures the way it already answers the others Review of the redemption fix turned up the same shape in three more places on the same flow, all of them reachable and none of them saying so. Writing the grant to the vault was still unguarded, which is the identical failure one step later and on the worst possible step: somebody who has just finished consenting at the vendor, told by a blank error that something went wrong at the end of the one part they did correctly. It ends at Settings now, like everything before it. The refresh token stays out of the log; the row it belonged to was never written. A malformed endpoint was being read as a vendor outage, because `fetch` refuses an unusable URL by throwing the same kind of error a refused connection does. Both are checked before the request is attempted. The person is told the same thing either way, since there is nothing else to tell them, but a catalogue somebody can fix and a vendor nobody can are worth different lines to whoever is reading them. Connect's 502 said the vendor refused this deployment's registration. After the fix a vendor that could not be reached arrives at the same branch, so the sentence says both rather than naming the wrong one confidently. The timeout tests now read the abort signal instead of ignoring it. They would have passed against code that had dropped the deadline entirely, which is the one thing a timeout test is there to notice. And the registration refusal gets a test that runs the real function through the store rather than the stub the rest of that suite injects, so the null the store's branches are written for is one the shipped code produced. --- CHANGELOG.md | 20 +++ server/src/plugins/oauth.ts | 38 ++++++ server/src/plugins/routes.ts | 38 +++++- server/tests/plugin-connect-route.test.ts | 12 +- server/tests/plugin-oauth-callback.test.ts | 126 +++++++++++++++++- server/tests/plugin-oauth.test.ts | 19 ++- server/tests/plugin-store.integration.test.ts | 61 +++++++++ 7 files changed, 298 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d446e0438..92176e9bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,26 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Connecting an account survives a vendor that is down + +Finishing a connection used to end on a blank server error if the vendor could not be reached at the +moment you were sent back, whether that was a refused connection, a name that would not resolve, or +fifteen seconds of silence. It now ends where every other failed connect already ended: back on +Connected accounts with a note, and nothing stored. Pressing Connect for a vendor this deployment +has not introduced itself to yet behaves the same way, answering 502 rather than a server error, and +that message now covers a vendor that could not be reached as well as one that turned the +registration down. + +A connection whose grant cannot be written to the vault ends the same way, rather than on the blank +error it used to give somebody who had just finished consenting. + +Because the person is told the same thing whatever went wrong, the server log is now where the +difference lives. Three lines to look for: `oauth-token-endpoint-unreachable` and +`oauth-registration-endpoint-unreachable` name the vendor and the cause, and +`oauth-connection-not-recorded` says a consent completed and could not be kept. A fourth, +`oauth-token-endpoint-unusable`, means the fault is this deployment's catalogue rather than the +vendor. + ### Coworkers are made in a wizard and managed in a dialog Creating a coworker is now a three-step wizard — who it is, who may see it, then where it runs, diff --git a/server/src/plugins/oauth.ts b/server/src/plugins/oauth.ts index 0367e5669..9ce118333 100644 --- a/server/src/plugins/oauth.ts +++ b/server/src/plugins/oauth.ts @@ -323,6 +323,30 @@ export async function redeemAuthorizationCode(input: { // A public (DCR) client proves itself with PKCE, and some vendors refuse an unexpected empty field. if (input.clientSecret) params.set("client_secret", input.clientSecret); + /* + * Our own catalogue, told apart from their outage before a request is attempted. + * + * `fetch` refuses a malformed URL by throwing the same kind of error a refused connection does, + * so without this the two would reach the same catch and read as the same sentence. They are not + * the same thing: one is a vendor having a bad day and the other is an endpoint in this + * deployment's own catalogue that nobody can ever connect through. The person gets the ordinary + * refusal either way, because there is nothing else to give them, and the log is where the + * difference has to survive. + * + * Checking it here is also what leaves the catch below covering only the transport, rather than + * quietly standing in for a mistake in a frozen literal. + */ + if (!URL.canParse(input.tokenUrl)) { + console.error( + JSON.stringify({ + type: "oauth-token-endpoint-unusable", + tokenUrl: input.tokenUrl, + note: "This vendor's token endpoint is not a usable address, so nobody can connect it until the catalogue is fixed.", + }), + ); + return null; + } + /* * A vendor that could not be reached at all, which the refusal below cannot see. * @@ -425,6 +449,20 @@ export async function registerDynamicClient(input: { registrationUrl: string; redirectUri: string; }): Promise<{ clientId: string; clientSecret: string } | null> { + // The same catalogue check the redemption makes, and for the same reason: a registration endpoint + // that is not an address is this deployment's mistake, not the vendor's outage, and only the log + // can tell them apart. + if (!URL.canParse(input.registrationUrl)) { + console.error( + JSON.stringify({ + type: "oauth-registration-endpoint-unusable", + registrationUrl: input.registrationUrl, + note: "This vendor's registration endpoint is not a usable address, so this deployment can never introduce itself.", + }), + ); + return null; + } + // A vendor that could not be reached at all — see `redeemAuthorizationCode`, which states the same // gap in full. This one lands on an administrator pressing Connect rather than on somebody // mid-consent, and null is what turns it into the 502 that route already writes for a vendor that diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index d03ac556c..8e9e22c89 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -415,7 +415,7 @@ export function createPluginRoutes( if (entry.auth.clientRegistration === "dynamic") { return context.json( { - error: `${entry.title} refused this deployment's registration. Try again, and check the vendor's status if it persists.`, + error: `${entry.title} would not register this deployment, or could not be reached. Try again, and check the vendor's status if it persists.`, }, 502, ); @@ -514,12 +514,36 @@ export function createPluginRoutes( }); if (!grant) return context.redirect(failed); - await store.recordConnection({ - serverId: state.serverId, - userId: state.userId, - refreshToken: grant.refreshToken, - scope: grant.scope, - }); + /* + * The last thing that can fail, answered the same way as everything before it. + * + * A vault that will not take the grant is this deployment's problem, not the person's, and they + * have already done their part at the vendor. Unhandled, this threw past the handler and gave + * them the bare 500 that every other failure on this route was written to avoid, on the one + * path where they had most reason to think it had worked. + * + * Told, because unlike the refusals above this one is nobody's fault but ours, and the person's + * sentence deliberately says nothing about which failure it was. The refresh token is not + * logged: it is the one thing here worth stealing, and the row it belonged to was never written. + */ + try { + await store.recordConnection({ + serverId: state.serverId, + userId: state.userId, + refreshToken: grant.refreshToken, + scope: grant.scope, + }); + } catch (error) { + console.error( + JSON.stringify({ + type: "oauth-connection-not-recorded", + serverId: state.serverId, + note: "A person consented and the grant could not be stored. They were sent back to Settings with a failure and will have to connect again.", + error: String(error), + }), + ); + return context.redirect(failed); + } return context.redirect( connectedAccountsUrlFor( diff --git a/server/tests/plugin-connect-route.test.ts b/server/tests/plugin-connect-route.test.ts index ccdcf46b7..3aa9617fd 100644 --- a/server/tests/plugin-connect-route.test.ts +++ b/server/tests/plugin-connect-route.test.ts @@ -82,7 +82,15 @@ describe("connecting a dynamically registered vendor", () => { expect(url.searchParams.get("client_id")).toBe("dyn-1"); }); - test("a refused registration answers 502, naming the vendor", async () => { + /** + * One answer for two states, because there is one thing to do about either. + * + * A vendor that turned this deployment down and a vendor that could not be reached both leave + * `ensureOAuthClient` with no client, and the person pressing Connect has the same next step + * whichever it was. The sentence says both rather than naming the wrong one confidently; which it + * actually was is in the log, where it is of use to somebody who can act on it. + */ + test("a registration that produced no client answers 502, naming the vendor", async () => { const ensureCalls: { serverId: string; by: string }[] = []; const hono = app({ oauthClientFor: async () => null, @@ -101,7 +109,7 @@ describe("connecting a dynamically registered vendor", () => { expect(ensureCalls.length).toBe(1); const body = (await response.json()) as { error: string }; expect(body.error).toBe( - "Notion refused this deployment's registration. Try again, and check the vendor's status if it persists.", + "Notion would not register this deployment, or could not be reached. Try again, and check the vendor's status if it persists.", ); }); }); diff --git a/server/tests/plugin-oauth-callback.test.ts b/server/tests/plugin-oauth-callback.test.ts index 018bf965e..a3d3f5c81 100644 --- a/server/tests/plugin-oauth-callback.test.ts +++ b/server/tests/plugin-oauth-callback.test.ts @@ -2,7 +2,11 @@ import { describe, expect, test } from "bun:test"; import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; import type { AppVariables } from "../src/auth/guards"; -import { challengeFor, sealConnectState } from "../src/plugins/oauth"; +import { + challengeFor, + redeemAuthorizationCode, + sealConnectState, +} from "../src/plugins/oauth"; import { createPluginRoutes } from "../src/plugins/routes"; /** @@ -48,13 +52,17 @@ function app(input: { recorded: Recorded[]; /** Whether the person named by the state still has access. Present by default. */ personHasAccess?: (userId: string) => Promise; + /** What the vault does with the grant. Records it by default; a test may refuse instead. */ + recordConnection?: (connection: Recorded) => Promise; }) { const store = { oauthClientFor: async () => ({ clientId: "dyn-1", clientSecret: "" }), ensureOAuthClient: async () => ({ clientId: "dyn-1", clientSecret: "" }), - recordConnection: async (connection: Recorded) => { - input.recorded.push(connection); - }, + recordConnection: + input.recordConnection ?? + (async (connection: Recorded) => { + input.recorded.push(connection); + }), }; const routes = createPluginRoutes( store as never, @@ -246,12 +254,26 @@ describe("a vendor that could not be reached for the redemption", () => { ); } + /** + * The signal is inspected rather than ignored, which is the difference between proving the catch + * runs and proving it runs on the failure it was written for. A real timeout rejects because the + * request was already on the wire when `AbortSignal.timeout` fired, so a stub that never looked at + * `init.signal` would pass just as happily against code that had forgotten to pass one. + * + * What is still not exercised is the fifteen seconds themselves: the deadline is a literal in the + * source, and waiting it out is not a test anybody would run. The signal being present and unfired + * at the moment of the call is the part that can be checked here. + */ async function withUnreachableVendor( reject: () => never, run: () => Promise, + seen?: { signals: (AbortSignal | undefined)[] }, ): Promise { const realFetch = globalThis.fetch; - globalThis.fetch = (async () => reject()) as unknown as typeof fetch; + globalThis.fetch = (async (_url: unknown, init?: RequestInit) => { + seen?.signals.push(init?.signal ?? undefined); + return reject(); + }) as unknown as typeof fetch; try { await run(); } finally { @@ -285,6 +307,7 @@ describe("a vendor that could not be reached for the redemption", () => { const recorded: Recorded[] = []; const hono = app({ recorded }); const state = await sealed(); + const seen = { signals: [] as (AbortSignal | undefined)[] }; await withUnreachableVendor( () => { @@ -295,7 +318,13 @@ describe("a vendor that could not be reached for the redemption", () => { expect(response.status).toBe(302); expect(response.headers.get("location")).toBe(FAILED); }, + seen, ); + + // The deadline was armed and had not fired when the request went out, which is what makes this + // a timeout rather than a rejection that happened to arrive first. + expect(seen.signals[0]).toBeInstanceOf(AbortSignal); + expect(seen.signals[0]?.aborted).toBe(false); expect(recorded).toEqual([]); }); @@ -343,3 +372,90 @@ describe("a vendor that could not be reached for the redemption", () => { expect(recorded).toEqual([]); }); }); + +/** + * The last thing that can fail, and the one the redemption fix did not reach. + * + * Everything before this point answers a failure with the same redirect. Writing the grant did not: + * a vault that will not take it threw past the handler, and the person who had just consented got + * the same bare 500 that an unreachable vendor used to give them. It is the identical shape one step + * later, so it gets the identical answer. + */ +describe("a grant the vault would not take", () => { + test("a refused write ends at Settings, and no half-connection is claimed", async () => { + const recorded: Recorded[] = []; + const hono = app({ + recorded, + recordConnection: async () => { + throw new Error("could not reach the database"); + }, + }); + const state = await sealConnectState( + { userId: "user-1", serverId: "notion", verifier: "v-1" }, + KEY, + ); + const said: string[] = []; + const realError = console.error; + console.error = (...args: unknown[]) => { + said.push(args.map(String).join(" ")); + }; + + try { + await withWillingVendor(async () => { + const response = await hono.request(callbackUrl(state)); + expect(response.status).toBe(302); + expect(response.headers.get("location")).toBe(FAILED); + }); + } finally { + console.error = realError; + } + + expect(recorded).toEqual([]); + // Told, for the same reason the unreachable vendor is: this one is the deployment's own fault, + // and the person is being handed the sentence that says nothing about which. + expect( + said.find((line) => line.includes("oauth-connection-not-recorded")), + ).toBeDefined(); + }); +}); + +/** + * A pinned endpoint that is not an address. + * + * `fetch` refuses a malformed URL by throwing the same kind of error a refused connection does, so + * the catch that made the vendor's outage quiet would make this quiet in exactly the same words. + * The person still gets the ordinary refusal, because there is nothing else to give them, but the + * log has to say which of the two it was: one is somebody else's outage and the other is this + * deployment's own catalogue, and only one of them is worth waking up for. + */ +describe("a token endpoint that is not a usable address", () => { + test("is refused like any other, and named as ours in the log", async () => { + const said: string[] = []; + const realError = console.error; + console.error = (...args: unknown[]) => { + said.push(args.map(String).join(" ")); + }; + + try { + expect( + await redeemAuthorizationCode({ + tokenUrl: "not a url", + clientId: "dyn-1", + clientSecret: "", + code: "code-1", + redirectUri: "https://openbot.example/api/plugins/oauth/callback", + verifier: "v-1", + }), + ).toBeNull(); + } finally { + console.error = realError; + } + + expect( + said.find((line) => line.includes("oauth-token-endpoint-unusable")), + ).toBeDefined(); + expect( + said.find((line) => line.includes("oauth-token-endpoint-unreachable")), + ).toBeUndefined(); + }); +}); diff --git a/server/tests/plugin-oauth.test.ts b/server/tests/plugin-oauth.test.ts index d8e7f9ec0..ba13c4a08 100644 --- a/server/tests/plugin-oauth.test.ts +++ b/server/tests/plugin-oauth.test.ts @@ -586,9 +586,17 @@ describe("registering this deployment as an OAuth client", () => { } }); + /** + * The signal is checked, not just the catch. A stub that ignores `init.signal` would pass against + * code that had dropped the deadline entirely, which is the one thing a timeout test exists to + * notice. The fifteen seconds themselves stay unexercised; the literal is in the source and + * waiting it out is not a test. + */ test("a registration endpoint that never answers is the same refusal", async () => { + const seen: (AbortSignal | undefined)[] = []; const realFetch = globalThis.fetch; - globalThis.fetch = (async () => { + globalThis.fetch = (async (_url: unknown, init?: RequestInit) => { + seen.push(init?.signal ?? undefined); throw new DOMException("The operation timed out.", "TimeoutError"); }) as unknown as typeof fetch; try { @@ -601,6 +609,8 @@ describe("registering this deployment as an OAuth client", () => { } finally { globalThis.fetch = realFetch; } + expect(seen[0]).toBeInstanceOf(AbortSignal); + expect(seen[0]?.aborted).toBe(false); }); }); @@ -776,9 +786,12 @@ describe("redeeming an authorization code", () => { } }); + /** Same reasoning as the registration side: the deadline is what is being tested, so it is read. */ test("a token endpoint that never answers is the same refusal", async () => { + const seen: (AbortSignal | undefined)[] = []; const realFetch = globalThis.fetch; - globalThis.fetch = (async () => { + globalThis.fetch = (async (_url: unknown, init?: RequestInit) => { + seen.push(init?.signal ?? undefined); throw new DOMException("The operation timed out.", "TimeoutError"); }) as unknown as typeof fetch; try { @@ -795,5 +808,7 @@ describe("redeeming an authorization code", () => { } finally { globalThis.fetch = realFetch; } + expect(seen[0]).toBeInstanceOf(AbortSignal); + expect(seen[0]?.aborted).toBe(false); }); }); diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index f05914ffc..89e2ef832 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -2018,6 +2018,67 @@ describe("a dynamic client the vendor has evicted", () => { ); }); + /** + * The registration nothing stands in for. + * + * Every other test here injects `registerClient`, which is right for asserting what the store does + * with an answer but means the real function is never the one answering. What it returns when the + * vendor cannot be reached at all is exactly what the store's `null` branches were written for, so + * once that path exists it is worth one test that lets the real code produce the value rather than + * a stub asserting the value the real code is assumed to produce. + */ + const storeWithRealRegistration = createPluginStore({ + database, + auditStore: createAuditStore(database), + credentials: vault, + encryptionKey: DYNAMIC_KEY, + policy: () => policy, + callVendor: async () => ({ + text: "[vendor not reached in tests]", + isError: false, + }), + exchangeRefreshToken: seams.exchangeRefreshToken, + redirectUri: REDIRECT_URI, + }); + + test("an unreachable registration endpoint leaves no client and no trail", async () => { + await clearClient(); + const registeredBefore = await registeredRows(); + const said: string[] = []; + const realError = console.error; + const realFetch = globalThis.fetch; + console.error = (...args: unknown[]) => { + said.push(args.map(String).join(" ")); + }; + globalThis.fetch = (async () => { + throw new TypeError("Unable to connect."); + }) as unknown as typeof fetch; + + try { + expect( + await storeWithRealRegistration.ensureOAuthClient( + dynamicServerId, + "someone@openbot.test", + ), + ).toBeNull(); + } finally { + globalThis.fetch = realFetch; + console.error = realError; + } + + // Nothing kept, and nothing claimed. A trail row here would say this deployment registered + // itself with a vendor that never answered. + expect( + await storeWithRealRegistration.oauthClientFor(dynamicServerId), + ).toBe(null); + expect((await registeredRows()).length).toBe(registeredBefore.length); + expect( + said.find((line) => + line.includes("oauth-registration-endpoint-unreachable"), + ), + ).toBeDefined(); + }); + test("an entry an administrator registers by hand is left alone", async () => { /* * Drive, whose client is pasted in from Google's console. Registering one for it would be