From f7023b205c2e40a917ed410e4b8fdf7d919179bd Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 11 Aug 2026 21:05:30 +0000 Subject: [PATCH] fix(security): consume and actor-bind OAuth state on a denied callback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A denial or a cancel is a final answer to an authorization request, but `completeOAuthCallback` treated it as merely a failed callback: the row in `tool_oauth_states` survived, so the `state` the user just refused stayed completable for the rest of its 10-minute TTL by anyone who could produce a code. Any linked `request_confirmation` also stayed `pending`, so the board kept offering an authorization link for a flow the user had already declined and the requesting agent never learned the answer. Route every terminal callback — success, denial, cancel — through one `consumeOAuthState` helper. Two properties pull in opposite directions and both matter: - a stranger's callback must not *consume* the request, so the row is loaded and bound to the caller (expiry, actor type/id, session, or the subject user) before anything is deleted, and a failed binding check leaves the victim's flow live and completable; - a replayed callback must not *complete* the request, so the delete is the single statement that decides ownership — `DELETE … RETURNING` hands the row to exactly one of two concurrent callbacks and the loser is told the state is spent rather than exchanging a code against it. A denial then resolves any linked interaction as `rejected`, scoped to a still-`pending` row so a board user who answered the card directly keeps their own answer, with a Paperclip-authored reason rather than the provider's. The message and its `providerError` label are PAP-17108's, unchanged. Five regression tests, both negative-controlled: a code obtained before a denial can no longer be exchanged; an unrecognized cancel code behaves the same; another session's or another user's denial is refused without spending the request, which the owner then completes; the board's prompt ends `rejected`; and two simultaneous callbacks produce exactly one token exchange. That last one needs a second database handle — this driver pipelines everything issued through one handle, which serializes the two callbacks and makes a non-atomic delete pass — and it has to connect before racing, or the first callback wins uncontested. Co-Authored-By: Claude Opus 5 (1M context) --- .../__tests__/generic-mcp-connection.test.ts | 192 ++++++++++++++++++ server/src/services/tool-access.ts | 106 ++++++++-- 2 files changed, 278 insertions(+), 20 deletions(-) diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index b7c76878ad3..8d1eeb60bba 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -12,6 +12,8 @@ import { companySecrets, companySecretVersions, createDb, + issueThreadInteractions, + issues, principalPermissionGrants, secretAccessEvents, toolAccessAuditEvents, @@ -315,6 +317,8 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { await db.delete(toolCatalogEntries); await db.delete(toolConnections); await db.delete(toolApplications); + await db.delete(issueThreadInteractions); + await db.delete(issues); await db.delete(principalPermissionGrants); await db.delete(companyMemberships); await db.delete(agents); @@ -834,6 +838,194 @@ describeEmbeddedPostgres("generic remote MCP connections", () => { }); }); + /** + * PAP-17109 — a denial or a cancel is a final answer to an authorization + * request, so it has to end the request rather than just fail the callback. + * Left live, the `state` the user refused stays completable for the rest of its + * TTL by anyone who can produce a code, and the board keeps showing a prompt + * for a flow the user already declined. + */ + describe("denied and cancelled callbacks", () => { + it("stops a later code from completing a flow the user refused", async () => { + const fixture = installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + const actor = { actorType: "user" as const, actorId: "board-user" }; + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture denied" }); + const start = await service.startOAuth(company.id, connected.connectionId, { redirectUri: REDIRECT_URI, actor }); + const state = new URL(start.authorizationUrl).searchParams.get("state")!; + // Held before the denial on purpose: this is the whole attack. A code that + // shows up after "no" must not be exchangeable. + const code = fixture.issueAuthorizationCode(start.authorizationUrl); + + await expect(service.completeOAuthCallback({ state, error: "access_denied", redirectUri: REDIRECT_URI, actor })) + .rejects.toMatchObject({ status: 400, details: { code: "oauth_authorization_denied" } }); + + await expect(service.completeOAuthCallback({ state, code, iss: ISSUER, redirectUri: REDIRECT_URI, actor })) + .rejects.toMatchObject({ status: 400, message: "OAuth state was not found or has already been used" }); + + expect(fixture.requestsTo("/token")).toHaveLength(0); + const [connection] = await db.select().from(toolConnections).where(eq(toolConnections.id, connected.connectionId)); + // Still a draft: nothing about the refused attempt made it a live connection. + expect(connection).toMatchObject({ status: "draft" }); + expect(connection!.credentialSecretRefs.some((ref) => ref.configPath === "oauth.access_token")).toBe(false); + }); + + it("treats a cancel the same as a denial even when the provider names it its own way", async () => { + const fixture = installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + const actor = { actorType: "user" as const, actorId: "board-user" }; + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture cancelled" }); + const start = await service.startOAuth(company.id, connected.connectionId, { redirectUri: REDIRECT_URI, actor }); + const state = new URL(start.authorizationUrl).searchParams.get("state")!; + const code = fixture.issueAuthorizationCode(start.authorizationUrl); + + // Real providers invent their own cancel codes. Whether or not Paperclip + // recognizes the label, the request is over. + const thrown = await service.completeOAuthCallback({ + state, + error: "user_cancelled_authorize", + redirectUri: REDIRECT_URI, + actor, + }).then(() => null, (error: unknown) => error); + expect(thrown).toMatchObject({ status: 400, details: { code: "oauth_authorization_denied" } }); + expect((thrown as Error).message).not.toContain("user_cancelled_authorize"); + + await expect(db.select().from(toolOauthStates)).resolves.toHaveLength(0); + await expect(service.completeOAuthCallback({ state, code, iss: ISSUER, redirectUri: REDIRECT_URI, actor })) + .rejects.toMatchObject({ status: 400 }); + expect(fixture.requestsTo("/token")).toHaveLength(0); + }); + + it("will not let another session's denial spend a pending request", async () => { + const fixture = installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + const owner = { actorType: "user" as const, actorId: "board-user", sessionId: "owner-session" }; + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture victim" }); + const start = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: owner, + }); + const state = new URL(start.authorizationUrl).searchParams.get("state")!; + + // Same user, different browser session. + await expect(service.completeOAuthCallback({ + state, + error: "access_denied", + redirectUri: REDIRECT_URI, + actor: { ...owner, sessionId: "other-session" }, + })).rejects.toMatchObject({ status: 403 }); + + // A different user — which is also how a different company arrives, since a + // state is bound to the actor id that started it and the callback route + // authorizes against the state row's own company. + await expect(service.completeOAuthCallback({ + state, + error: "access_denied", + redirectUri: REDIRECT_URI, + actor: { ...owner, actorId: "someone-else" }, + })).rejects.toMatchObject({ status: 403 }); + + // Neither refusal cost the owner anything: the request is still live and + // still completable. + await expect(db.select().from(toolOauthStates).where(eq(toolOauthStates.state, state))).resolves.toHaveLength(1); + const code = fixture.issueAuthorizationCode(start.authorizationUrl); + await expect(service.completeOAuthCallback({ state, code, iss: ISSUER, redirectUri: REDIRECT_URI, actor: owner })) + .resolves.toMatchObject({ connectionId: connected.connectionId }); + }); + + it("resolves the board's pending authorization prompt as rejected", async () => { + installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + const [agent] = await db.insert(agents).values({ + companyId: company.id, + name: `Connector ${randomUUID()}`, + role: "engineer", + status: "idle", + adapterType: "process", + adapterConfig: {}, + runtimeConfig: {}, + }).returning(); + const [issue] = await db.insert(issues).values({ + companyId: company.id, + title: "Connect the analytics app", + }).returning(); + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture prompt" }); + + // The agent-driven shape: the agent asks, the board user answers in the + // provider's window, so the prompt's fate is decided by the callback. + const start = await service.startOAuth(company.id, connected.connectionId, { + redirectUri: REDIRECT_URI, + actor: { actorType: "agent", actorId: agent!.id }, + subjectUserId: "board-user", + issueId: issue!.id, + }); + const [pending] = await db.select().from(issueThreadInteractions); + expect(pending).toMatchObject({ kind: "request_confirmation", status: "pending" }); + + await expect(service.completeOAuthCallback({ + state: new URL(start.authorizationUrl).searchParams.get("state")!, + error: "access_denied", + redirectUri: REDIRECT_URI, + actor: { actorType: "user", actorId: "board-user" }, + })).rejects.toMatchObject({ status: 400, details: { code: "oauth_authorization_denied" } }); + + const [resolved] = await db + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, pending!.id)); + expect(resolved).toMatchObject({ status: "rejected", resolvedByUserId: "board-user" }); + expect(resolved!.result).toMatchObject({ outcome: "rejected" }); + expect(resolved!.resolvedAt).not.toBeNull(); + // The prompt's reason is Paperclip's own copy, never the provider's. + expect(JSON.stringify(resolved!.result)).not.toContain("access_denied"); + await expect(db.select().from(toolOauthStates)).resolves.toHaveLength(0); + }); + + it("lets only one of two simultaneous callbacks exchange a code", async () => { + const fixture = installMcpOAuthFixture({ auth: "oauth" }); + const company = await createCompany(db); + const service = toolAccessService(db); + const actor = { actorType: "user" as const, actorId: "board-user" }; + const connected = await service.connectGalleryApp(company.id, { link: MCP_URL, name: "Fixture replay" }); + const start = await service.startOAuth(company.id, connected.connectionId, { redirectUri: REDIRECT_URI, actor }); + const state = new URL(start.authorizationUrl).searchParams.get("state")!; + const code = fixture.issueAuthorizationCode(start.authorizationUrl); + + // A second database handle, because a race needs two connections: this + // driver pipelines everything issued through one handle, which would + // serialize the two callbacks and prove nothing. With two connections both + // callbacks can read a live state row before either deletes it, so the + // atomic `DELETE … RETURNING` is what picks the winner. + const otherDb = createDb(tempDb!.connectionString); + const otherService = toolAccessService(otherDb); + let settled: PromiseSettledResult[]; + try { + // Connect before racing: paying TCP and startup latency inside the race + // would just hand the first callback an uncontested head start. + await otherDb.select().from(companies).limit(1); + settled = await Promise.allSettled([ + service.completeOAuthCallback({ state, code, iss: ISSUER, redirectUri: REDIRECT_URI, actor }), + otherService.completeOAuthCallback({ state, code, iss: ISSUER, redirectUri: REDIRECT_URI, actor }), + ]); + } finally { + await otherDb.$client.end({ timeout: 5 }); + } + + expect(settled.filter((entry) => entry.status === "fulfilled")).toHaveLength(1); + // The loser never reached the token endpoint at all. + expect(fixture.requestsTo("/token")).toHaveLength(1); + const rejection = settled.find((entry) => entry.status === "rejected") as PromiseRejectedResult; + expect(rejection.reason).toMatchObject({ + status: 400, + message: "OAuth state was not found or has already been used", + }); + }); + }); + it("accepts an iss that differs only by a trailing slash", async () => { const fixture = installMcpOAuthFixture({ auth: "oauth" }); const company = await createCompany(db); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 2898d8fd6a1..29cd107945f 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -6595,6 +6595,82 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return row ?? null; } + /** + * Answer a pending authorization request exactly once (PAP-17109). + * + * Every terminal callback — success, denial, cancel — comes through here, so + * the row that authorizes a token exchange stops existing the moment the flow + * reaches an outcome. Two properties matter and they pull in opposite + * directions: + * + * - A stranger's callback must not *consume* the request. So the row is loaded + * and bound to the caller before anything is deleted; a failed binding check + * leaves the victim's flow live and completable. + * - A replayed callback must not *complete* the request. So the delete is the + * single statement that decides ownership: `RETURNING` hands the row to + * exactly one of two concurrent callbacks, and the loser is told the state is + * spent instead of exchanging a code against it. + */ + async function consumeOAuthState(state: string, actor: ActorInfo | undefined) { + const [stateRow] = await db + .select() + .from(toolOauthStates) + .where(eq(toolOauthStates.state, state)) + .limit(1); + if (!stateRow) throw badRequest("OAuth state was not found or has already been used"); + if (stateRow.expiresAt.getTime() <= Date.now()) throw badRequest("OAuth state has expired"); + if (stateRow.subjectUserId) { + if (actor?.actorType !== "user" || actor.actorId !== stateRow.subjectUserId) { + throw forbidden("OAuth callback user does not match the requested subject"); + } + } else { + assertSameOAuthActor(stateRow, actor); + } + const [consumed] = await db + .delete(toolOauthStates) + .where(eq(toolOauthStates.state, state)) + .returning(); + if (!consumed) throw badRequest("OAuth state was not found or has already been used"); + return consumed; + } + + /** + * End the board's "Connect your account" prompt when the user declines in the + * provider's window (PAP-17109). Without this the card stays `pending`, so the + * board keeps offering an authorization link for a flow the user just refused + * and the requesting agent never learns the answer. + * + * Scoped to a still-`pending` row: a board user who already answered the card + * directly keeps their own answer. + */ + async function rejectPendingOAuthInteraction( + stateRow: typeof toolOauthStates.$inferSelect, + actor: ActorInfo | undefined, + ) { + if (!stateRow.interactionId) return; + const now = new Date(); + await db + .update(issueThreadInteractions) + .set({ + status: "rejected", + result: { + version: 1, + outcome: "rejected", + // Paperclip's own words: the provider's explanation is untrusted and + // this reason is rendered in the thread (PAP-17108). + reason: "Authorization was declined or cancelled in the provider's window", + }, + resolvedByUserId: actor?.actorType === "user" ? actor.actorId : null, + resolvedAt: now, + updatedAt: now, + }) + .where(and( + eq(issueThreadInteractions.id, stateRow.interactionId), + eq(issueThreadInteractions.companyId, stateRow.companyId), + eq(issueThreadInteractions.status, "pending"), + )); + } + async function completeOAuthCallback(input: { state: string; code?: string | null; @@ -6610,34 +6686,24 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} iss?: string | null; actor?: ActorInfo; }): Promise { - const [stateRow] = await db - .select() - .from(toolOauthStates) - .where(eq(toolOauthStates.state, input.state)) - .limit(1); - if (!stateRow) throw badRequest("OAuth state was not found or has already been used"); - if (stateRow.expiresAt.getTime() <= Date.now()) throw badRequest("OAuth state has expired"); - if (stateRow.subjectUserId) { - if (input.actor?.actorType !== "user" || input.actor.actorId !== stateRow.subjectUserId) { - throw forbidden("OAuth callback user does not match the requested subject"); - } - } else { - assertSameOAuthActor(stateRow, input.actor); - } - // The provider's report of a failure is only acted on once the callback is - // bound to a state Paperclip issued and to the actor that started the flow, - // so an unsolicited callback cannot drive this path at all. A denial is - // terminal for the attempt, so the state is consumed either way. + // Binding first, outcome second: the provider's report of a failure is only + // acted on once the callback is bound to a state Paperclip issued and to the + // actor that started the flow, so an unsolicited callback cannot drive any + // path here. Consuming the state up front is what makes a denial terminal — + // a refused request must not stay completable by a later code (PAP-17109). + const stateRow = await consumeOAuthState(input.state, input.actor); if (input.error) { - await db.delete(toolOauthStates).where(eq(toolOauthStates.state, input.state)); + await rejectPendingOAuthInteraction(stateRow, input.actor); const providerError = normalizeOAuthProviderError(input.error); throw new HttpError(400, oauthProviderErrorMessage(providerError, "The authorization server denied the request."), { code: "oauth_authorization_denied", providerError, }); } + // Neither a code nor an error is not a usable answer either. It still spends + // the request: the recovery is a fresh authorization, not a state left live + // waiting for a better callback. if (!input.code) throw badRequest("OAuth callback is missing a code"); - await db.delete(toolOauthStates).where(eq(toolOauthStates.state, input.state)); let connection = await getConnectionRow(stateRow.connectionId, stateRow.companyId); const sourceTemplateKey = typeof connection.config.sourceTemplateKey === "string" ? connection.config.sourceTemplateKey : null;