fix(security): consume and actor-bind OAuth state on a denied callback

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) <noreply@anthropic.com>

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<unknown>[];
+      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);

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<ConnectToolAppResult> {
-    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;