From bfc8fd81afc33e1f180de200b214b006e7ad133b Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 12 Aug 2026 11:54:58 +0000 Subject: [PATCH] fix(oauth): return failed callbacks to setup Co-Authored-By: Paperclip --- .../__tests__/generic-mcp-connection.test.ts | 35 ++++++++++++ server/src/routes/tool-access.ts | 57 +++++++++++++------ server/src/services/tool-access.ts | 5 +- 3 files changed, 80 insertions(+), 17 deletions(-) diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index 6a8f011a33e..082740d633d 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -920,6 +920,41 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { await expect(db.select().from(toolOauthStates).where(eq(toolOauthStates.state, state))).resolves.toHaveLength(0); }); + it("returns browser denials to setup without reflecting provider-authored details", async () => { + installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + const app = createRouteApp(db); + const connected = await service.connectGalleryApp(company.id, { + link: MCP_URL, + name: "Fixture browser denial", + }); + const start = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: { actorType: "user", actorId: "board-user" }, + }); + const state = new URL(start.authorizationUrl).searchParams.get("state")!; + + const res = await request(app) + .get("/api/tools/oauth/callback") + .set("Accept", "text/html") + .query({ + state, + error: "access_denied", + error_description: HOSTILE_ERROR_DESCRIPTION, + error_uri: HOSTILE_ERROR_BODY.error_uri, + }); + + expect(res.status).toBe(303); + const location = new URL(res.headers.location, PUBLIC_BASE_URL); + expect(location.pathname).toBe(`/${company.issuePrefix}/apps/${connected.connectionId}/setup`); + expect(location.searchParams.get("oauth")).toBe("denied"); + expect(location.searchParams.get("code")).toBe("oauth_authorization_denied"); + expect(res.headers.location).not.toContain(PROVIDER_CANARY); + expect(res.headers.location).not.toContain("error_description"); + expect(res.headers.location).not.toContain("error_uri"); + }); + it("validates the callback state before acting on a provider-reported error", async () => { installMcpOAuthFixture({ auth: "oauth" }); const company = await createCompany(db); diff --git a/server/src/routes/tool-access.ts b/server/src/routes/tool-access.ts index d8dcadb3a1d..42e8da566b2 100644 --- a/server/src/routes/tool-access.ts +++ b/server/src/routes/tool-access.ts @@ -120,6 +120,16 @@ export function toolAccessRoutes( } return new URL("/api/tools/oauth/callback", configured).toString(); } + + async function oauthSetupPath(companyId: string, connectionId: string) { + const [company] = await db + .select({ issuePrefix: companies.issuePrefix }) + .from(companies) + .where(eq(companies.id, companyId)) + .limit(1); + if (!company) throw new Error("OAuth callback connection belongs to a missing company"); + return `/${company.issuePrefix}/apps/${connectionId}/setup`; + } const access = accessService(db); async function assertBoardToolPermission(req: Request, companyId: string, permissionKey: PermissionKey) { @@ -370,14 +380,34 @@ export function toolAccessRoutes( throw badRequest("Invalid or expired OAuth state"); } assertToolAppMutationAccess(req, pendingState.companyId); - const result = await svc.completeOAuthCallback({ - state, - code, - error, - iss, - redirectUri: oauthRedirectUri(), - actor: getActorInfo(req), - }); + const acceptsHtml = req.get("accept")?.includes("text/html") === true; + let result: Awaited>; + try { + result = await svc.completeOAuthCallback({ + state, + code, + error, + iss, + redirectUri: oauthRedirectUri(), + actor: getActorInfo(req), + }); + } catch (callbackError) { + if (!acceptsHtml) throw callbackError; + const details = callbackError instanceof HttpError + && callbackError.details + && typeof callbackError.details === "object" + && !Array.isArray(callbackError.details) + ? callbackError.details as Record + : null; + const callbackErrorCode = typeof details?.code === "string" ? details.code : null; + const params = new URLSearchParams({ + oauth: callbackErrorCode === "oauth_authorization_denied" ? "denied" : "failed", + }); + if (callbackErrorCode) params.set("code", callbackErrorCode); + const setupPath = await oauthSetupPath(pendingState.companyId, pendingState.connectionId); + res.redirect(303, `${setupPath}?${params.toString()}`); + return; + } await logActivity(db, { companyId: result.connection.companyId, actorType: "user", @@ -390,14 +420,9 @@ export function toolAccessRoutes( catalogEntryCount: result.catalog.length, }, }); - if (req.get("accept")?.includes("text/html")) { - const [company] = await db - .select({ issuePrefix: companies.issuePrefix }) - .from(companies) - .where(eq(companies.id, result.connection.companyId)) - .limit(1); - if (!company) throw new Error("OAuth callback connection belongs to a missing company"); - res.redirect(303, `/${company.issuePrefix}/apps/${result.connection.id}/setup?oauth=connected`); + if (acceptsHtml) { + const setupPath = await oauthSetupPath(result.connection.companyId, result.connection.id); + res.redirect(303, `${setupPath}?oauth=connected`); return; } res.json(result); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index c10b1257dae..c32eed92574 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -7045,7 +7045,10 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} async function peekOAuthState(state: string) { const [row] = await db - .select({ companyId: toolOauthStates.companyId }) + .select({ + companyId: toolOauthStates.companyId, + connectionId: toolOauthStates.connectionId, + }) .from(toolOauthStates) .where(eq(toolOauthStates.state, state)) .limit(1);