From 1602512820832b43afef6786bedeee4e5294db8e Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 9 Oct 2026 11:43:09 -0700 Subject: [PATCH] fix(sentry): classify expected credential sharing denials (#15714) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - A run can use only an AI credential shared with its responsible user. > - The selection service correctly rejects a credential outside that user's sharing permissions. > - That known configuration rejection still becomes a Sentry error because it has no owned selection marker. > - This pull request marks only that verified sharing rejection and reuses the existing pre-provider reporting gate. > - The task remains blocked with its recovery action, while unexpected authorization and runtime failures remain reportable. ## Linked Issues or Issue Description Refs #15618, which introduced owned provenance for expected AI account selection blockers. The credential-sharing subtype still used a plain HTTP 403. No duplicate open fix was found. **What happened?** An active responsible user selects an AI credential they cannot use. Selection correctly throws `This credential is not shared with the responsible user`. A fresh task remains blocked, but the broad `ConfigurationIncompleteFailure` wrapper is also sent to Sentry. **Expected behavior** Preserve the denial, HTTP 403, task error and owner recovery action. Keep this proven pre-provider configuration rejection out of Sentry without suppressing unrelated permission failures. **Steps to reproduce** Assign a fresh run an explicit personal credential owned by another user, or a shared credential whose human audience excludes its active responsible user. The existing selection check denies access before adapter dispatch. ## What Changed - Add a dedicated sharing-rejection factory that preserves the existing 403, message and absent details while retaining one private bounded reason. - Use it only after active responsible-user membership and the existing credential checks have passed. Missing identity and other authorization checks remain unmarked. - Keep the existing failed/preparing/setup/bootstrap reporting conditions and provider/resume exclusions unchanged. - Add real selection, heartbeat recovery, reporter-boundary and forged-error regressions. Update monitoring documentation. ## Verification - Producer, private-marker and run-failure reporter suites: 310 tests passed. - Actual heartbeat recovery cases: 6 passed; 377 unrelated cases filtered. These verify failed runs, blocked tasks, active owner recovery actions, failed wakeups and no adapter dispatch. - Coverage includes personal and restricted shared credentials, allowed audience changes, missing identity, inactive credential owners, wrong binding kinds, membership in another company, agent permission checks, forged 403 markers, resumed native input and provider work already started. - `pnpm -r typecheck` and `pnpm build` passed. All 53 PR checks passed; two unrelated Storybook jobs were skipped. - The full local aggregate was **not green**: it reported an existing browser-sign-in case and a real-runner damaged-resume case before the broad run was stopped (exit 130). The full affected AI-connections file then passed 111/111 in isolation, including the browser case. The unchanged native-session-resume file reproduced 47 passed / 1 failed on both this commit and an untouched base checkout with the same local dependencies and runner binary: the fixture expected a succeeded terminal result and received a failed one. No broader local-suite pass is claimed; full sharded CI passed. - Independent review passed the same 310 focused tests and six heartbeat cases from a clean checkout. Replacing only the producer with its original implementation fails both new marker assertions while preserving 403; restoring this change passes. - Greptile scored this exact commit 5/5 with no review threads. - Source, tests and documentation were scanned for credentials, personal data, private URLs and generated artifacts; none found. ## Risks - A known sharing rejection no longer emits a Sentry run-failure event only when the existing fresh setup proof is present. It remains visible in the task and recovery UI. - No credential-use permission, HTTP contract, execution or retry behavior changes. - This is not a blanket filter for HTTP 403 or `ConfigurationIncompleteFailure`. Unknown causes and resumed or post-provider failures remain reportable. ## Model Used OpenAI GPT-6 through Codex, with tool execution and parallel independent review. The exact serving variant and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run focused tests locally and they pass; broader local limitations are documented above - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- doc/observability.md | 8 +++- server/src/__tests__/ai-connections.test.ts | 44 +++++++++++++++++-- .../heartbeat-process-recovery.test.ts | 20 ++++++--- .../__tests__/run-failure-report.test.ts | 18 ++++++++ ...i-connection-configuration-failure.test.ts | 14 +++++- .../ai-connection-configuration-failure.ts | 10 ++++- server/src/services/ai-connections.ts | 8 +++- 7 files changed, 106 insertions(+), 16 deletions(-) diff --git a/doc/observability.md b/doc/observability.md index e8d6cbb2151..a7880e31638 100644 --- a/doc/observability.md +++ b/doc/observability.md @@ -445,8 +445,12 @@ recovery, credentials, or execution policy. AI account selection also stays local when the selection service proves a missing responsible user, personal default, or selected connection; an -incompatible selection; or an account that needs reconnection. The service marks -the original error, and setup preserves its closed `selectionFailure` reason. +incompatible selection; an account that needs reconnection; or an active +responsible user who lacks the selected credential's human sharing permission. +The sharing rejection keeps its existing HTTP 403 and does not grant access. +Other permission denials, including absent identity and inactive membership, +remain outside this sharing classification. The service marks the original error, +and setup preserves its closed `selectionFailure` reason. The same failed/preparing/setup/bootstrap checks apply, and resumed native runs with persisted provider input remain reportable. The generic `ai_connection_unavailable` wrapper alone is insufficient: database, credential diff --git a/server/src/__tests__/ai-connections.test.ts b/server/src/__tests__/ai-connections.test.ts index 57217dd2381..5a919e63fc1 100644 --- a/server/src/__tests__/ai-connections.test.ts +++ b/server/src/__tests__/ai-connections.test.ts @@ -1230,7 +1230,10 @@ console.log(JSON.stringify({ type: "result", subtype: "success", result: "hello" it("uses human access for every selection; an agent delegation cannot override Just me", async () => { const personal = await service.select({ ...input, userId: "alice" }); const delegated = { ...binding, mode: "delegated" as const, connectionId: personal.connection.id, grantId: personal.grant.id }; - await expect(service.select({ ...input, userId: "bob", binding: delegated })).rejects.toThrow("not shared"); + const rejection = await service.select({ ...input, userId: "bob", binding: delegated }).catch(error => error); + expect(rejection).toMatchObject({ status: 403, message: "This credential is not shared with the responsible user" }); + expect(rejection.details).toBeUndefined(); + expect(readAiConnectionConfigurationFailure(rejection)).toBe("ai_connection_credential_not_shared"); // Existing delegation records no longer confer an independent AI permission. await db.insert(connectionGrantDelegations).values({ companyId, grantId: personal.grant.id, agentId, createdByUserId: "alice" }); await expect(service.select({ ...input, userId: "bob", binding: delegated })).rejects.toThrow("not shared"); @@ -1243,7 +1246,9 @@ console.log(JSON.stringify({ type: "result", subtype: "success", result: "hello" const sharedBinding = { ...binding, mode: "shared" as const, ...shared }; const tools = toolAccessService(db); await tools.replaceConnectionGrantMembers(shared.connectionId, shared.grantId, ["alice"], { userId: "alice" }); - await expect(service.select({ ...input, userId: "bob", binding: sharedBinding })).rejects.toThrow("not shared"); + const rejection = await service.select({ ...input, userId: "bob", binding: sharedBinding }).catch(error => error); + expect(rejection).toMatchObject({ status: 403, message: "This credential is not shared with the responsible user" }); + expect(readAiConnectionConfigurationFailure(rejection)).toBe("ai_connection_credential_not_shared"); expect((await service.list(companyId, "bob", agentId)).some(account => account.id === shared.connectionId)).toBe(false); await tools.replaceConnectionGrantMembers(shared.connectionId, shared.grantId, ["bob"], { userId: "alice" }); expect((await service.select({ ...input, userId: "bob", binding: sharedBinding })).grant.id).toBe(shared.grantId); @@ -1253,12 +1258,43 @@ console.log(JSON.stringify({ type: "result", subtype: "success", result: "hello" for (const userId of ["alice", "bob"]) { expect((await service.select({ ...input, userId, binding: sharedBinding })).grant.id).toBe(shared.grantId); } - await expect(service.select({ ...input, userId: null, binding: sharedBinding })).rejects.toThrow("not shared"); + const missingIdentity = await service.select({ ...input, userId: null, binding: sharedBinding }).catch(error => error); + expect(missingIdentity).toMatchObject({ status: 403, message: "This credential is not shared with the responsible user" }); + expect(readAiConnectionConfigurationFailure(missingIdentity)).toBeNull(); + const inactiveMember = await service.select({ ...input, userId: "inactive-user", binding: sharedBinding }).catch(error => error); + expect(inactiveMember).toMatchObject({ status: 403, message: "The responsible user is not an active company member" }); + expect(readAiConnectionConfigurationFailure(inactiveMember)).toBeNull(); // Human permission still cannot bypass the separate agent-access setting. await db.delete(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, shared.connectionId)); - await expect(service.select({ ...input, userId: "bob", binding: sharedBinding })).rejects.toThrow("not permitted for this agent"); + const missingAgentPermission = await service.select({ ...input, userId: "bob", binding: sharedBinding }).catch(error => error); + expect(missingAgentPermission).toMatchObject({ status: 403 }); + expect(missingAgentPermission.message).toContain("not permitted for this agent"); + expect(readAiConnectionConfigurationFailure(missingAgentPermission)).toBeNull(); expect(sharedBinding).toEqual({ ...binding, mode: "shared", ...shared }); }); + it.each(["inactive_owner", "wrong_binding_kind", "wrong_company_membership"] as const)( + "does not classify other credential authorization denials as sharing configuration: %s", async (cause) => { + const owner = `sharing-${cause}-owner`; + await db.insert(companyMemberships).values({ companyId, principalId: owner, principalType: "user", status: "active", membershipRole: "member" }); + const account = await create(owner, "Sharing permission fixture"); + const selected = { ...binding, mode: cause === "wrong_binding_kind" ? "shared" as const : "delegated" as const, ...account }; + let userId = "bob"; + if (cause === "inactive_owner") { + await db.update(companyMemberships).set({ status: "inactive" }).where(and(eq(companyMemberships.companyId, companyId), eq(companyMemberships.principalId, owner))); + } + if (cause === "wrong_company_membership") { + userId = "other-company-sharing-user"; + await db.insert(companyMemberships).values({ companyId: otherCompanyId, principalId: userId, principalType: "user", status: "active", membershipRole: "member" }); + } + const error = await service.select({ ...input, userId, binding: selected }).catch(error => error); + expect(error).toMatchObject({ status: 403, message: cause === "inactive_owner" + ? "The account owner is no longer an active company member" + : cause === "wrong_binding_kind" ? "Select a company-shared account" + : "The responsible user is not an active company member" }); + expect(readAiConnectionConfigurationFailure(error)).toBeNull(); + }, + ); + it("isolates concurrent homes and overrides ambient credentials without changing the model", async () => { vi.stubEnv("ANTHROPIC_API_KEY", "ambient-never-use"); const config = { model: "unchanged-model", env: { ANTHROPIC_API_KEY: "project-never-use" } }; diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index a4bde74c6ce..f6f986de520 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -3,7 +3,8 @@ import { readNativePlanWait, hasCommittedNativePlanWait } from "../services/nati import { nativeSha256 } from "../services/native-runtime/canonical.js"; import { buildQuestionResponseDeliveryEnvelope } from "../services/question-response-delivery.js"; import * as aiConnectionRuntime from "../services/ai-connection-runtime.js"; -import { unprocessable } from "../errors.js"; +import { forbidden, unprocessable } from "../errors.js"; +import { aiConnectionCredentialNotSharedFailure } from "../services/ai-connection-configuration-failure.js"; import * as executionContinuation from "../services/execution-continuation.js"; import * as environmentOrchestrator from "../services/environment-run-orchestrator.js"; import { remoteTerminationReceipt } from "../services/remote-execution-termination.js"; @@ -1531,7 +1532,7 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { return { companyId, agentId, runId, wakeupRequestId, issueId }; } - it.each(["missing_default", "database_error", "unmarked_http_error", "provider_error"] as const)( + it.each(["missing_default", "credential_not_shared", "database_error", "unmarked_http_error", "unmarked_forbidden", "provider_error"] as const)( "preserves AI configuration recovery and reports only unexpected causes: %s", async (cause) => { const { companyId, agentId, issueId, runId, wakeupRequestId } = await seedQueuedIssueRunFixture(); await db.insert(companyMemberships).values({ companyId, principalType: "user", principalId: "responsible-user", status: "active", membershipRole: "member" }); @@ -1541,7 +1542,11 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { } }).where(eq(agents.id, agentId)); const prepare = vi.spyOn(aiConnectionRuntime, "prepareManagedAiRuntime"); if (cause !== "missing_default") prepare.mockRejectedValueOnce( - cause === "unmarked_http_error" + cause === "credential_not_shared" + ? aiConnectionCredentialNotSharedFailure() + : cause === "unmarked_forbidden" + ? forbidden("This credential is not shared with the responsible user", { code: "ai_connection_credential_not_shared" }) + : cause === "unmarked_http_error" ? unprocessable("Connect an account and choose your personal default", { code: "ai_connection_default_missing" }) : new Error(cause), ); @@ -1556,9 +1561,12 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { expect(failed).toMatchObject({ status: "failed", errorCode: "configuration_incomplete", resultJson: { configurationIncomplete: { reason: "ai_connection_unavailable" }, executionRecovery: { kind: "bootstrap", providerWorkStarted: false } }, }); - if (cause === "missing_default") { - expect(failed?.error).toBe("Connect an account and choose your personal default"); - expect(failed?.resultJson?.configurationIncomplete).toMatchObject({ selectionFailure: "ai_connection_default_missing" }); + if (cause === "missing_default" || cause === "credential_not_shared") { + expect(failed?.error).toBe(cause === "missing_default" + ? "Connect an account and choose your personal default" + : "This credential is not shared with the responsible user"); + expect(failed?.resultJson?.configurationIncomplete).toMatchObject({ selectionFailure: cause === "missing_default" + ? "ai_connection_default_missing" : "ai_connection_credential_not_shared" }); expect(mockCaptureRunFailure).not.toHaveBeenCalled(); } else { expect(failed?.resultJson?.configurationIncomplete).not.toHaveProperty("selectionFailure"); diff --git a/server/src/services/__tests__/run-failure-report.test.ts b/server/src/services/__tests__/run-failure-report.test.ts index 2b2120e99f9..cf629982350 100644 --- a/server/src/services/__tests__/run-failure-report.test.ts +++ b/server/src/services/__tests__/run-failure-report.test.ts @@ -301,6 +301,7 @@ describeEmbeddedPostgres("reportRunFailure", () => { it.each([ "ai_connection_responsible_user_missing", "ai_connection_default_missing", "ai_connection_missing", "ai_connection_incompatible", "ai_connection_unavailable", + "ai_connection_credential_not_shared", ])("keeps the proven AI selection blocker local: %s", async (selectionFailure) => { await seedCompanyAndAgent(); const run = aiSelectionRun(); @@ -321,6 +322,23 @@ describeEmbeddedPostgres("reportRunFailure", () => { }, ); + it.each(["resume", "provider_started", "outside_setup", "unmarked"] as const)( + "reports credential sharing denials without fresh pre-provider proof: %s", async (cause) => { + await seedCompanyAndAgent(); + const run = aiSelectionRun(); + (run.resultJson!.configurationIncomplete as Record).selectionFailure = "ai_connection_credential_not_shared"; + run.error = "This credential is not shared with the responsible user"; + if (cause === "resume") { + run.runtimeMode = "native"; + run.runnerProfileJson = { nativeExecutionInput: {} }; + } + if (cause === "provider_started") run.resultJson!.executionRecovery = { kind: "bootstrap", providerWorkStarted: true }; + if (cause === "unmarked") delete (run.resultJson!.configurationIncomplete as Record).selectionFailure; + await reportRunFailure(db, run, { phase: cause === "outside_setup" ? "execute" : "setup" }); + expect(mockCaptureRunFailure).toHaveBeenCalledTimes(1); + }, + ); + it.each([ { runtimeMode: "native", runnerProfileJson: { nativeExecutionInput: {} } }, { status: "timed_out" }, { errorCode: "setup_failed" }, { executionStage: "executing" }, diff --git a/server/src/services/ai-connection-configuration-failure.test.ts b/server/src/services/ai-connection-configuration-failure.test.ts index 50e4a6a68f8..11ec55fb26b 100644 --- a/server/src/services/ai-connection-configuration-failure.test.ts +++ b/server/src/services/ai-connection-configuration-failure.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it } from "vitest"; -import { unprocessable } from "../errors.js"; +import { forbidden, unprocessable } from "../errors.js"; import { aiConnectionConfigurationFailure, + aiConnectionCredentialNotSharedFailure, isAiConnectionConfigurationReason, readAiConnectionConfigurationFailure, } from "./ai-connection-configuration-failure.js"; @@ -16,11 +17,22 @@ describe("AI selection failure provenance", () => { expect(Object.keys(error)).toEqual(["status", "details"]); }); + it("preserves the credential sharing 403 without adding public error details", () => { + const error = aiConnectionCredentialNotSharedFailure(); + expect(error).toMatchObject({ status: 403, message: "This credential is not shared with the responsible user" }); + expect(error.details).toBeUndefined(); + expect(Object.keys(error)).toEqual(["status", "details"]); + expect(readAiConnectionConfigurationFailure(error)).toBe("ai_connection_credential_not_shared"); + expect(readAiConnectionConfigurationFailure(Object.freeze(error))).toBe("ai_connection_credential_not_shared"); + }); + it.each([ new Error("Connect an account and choose your personal default"), unprocessable("Choose an account", { code: "ai_connection_default_missing" }), { status: 422, details: { code: "ai_connection_default_missing" } }, Object.assign(new Error("database failed"), { code: "configuration_incomplete" }), + forbidden("This credential is not shared with the responsible user"), + forbidden("This credential is not shared with the responsible user", { code: "ai_connection_credential_not_shared" }), null, ])("does not infer provenance from an error name, message, or HTTP shape: %j", (error) => { expect(readAiConnectionConfigurationFailure(error)).toBeNull(); diff --git a/server/src/services/ai-connection-configuration-failure.ts b/server/src/services/ai-connection-configuration-failure.ts index 01053c24203..095aa81dd7c 100644 --- a/server/src/services/ai-connection-configuration-failure.ts +++ b/server/src/services/ai-connection-configuration-failure.ts @@ -1,4 +1,4 @@ -import { unprocessable } from "../errors.js"; +import { forbidden, unprocessable } from "../errors.js"; const reasons = [ "ai_connection_responsible_user_missing", @@ -6,6 +6,7 @@ const reasons = [ "ai_connection_missing", "ai_connection_incompatible", "ai_connection_unavailable", + "ai_connection_credential_not_shared", ] as const; type AiConnectionConfigurationReason = typeof reasons[number]; const failures = new WeakMap(); @@ -21,6 +22,13 @@ export function aiConnectionConfigurationFailure( return error; } +/** A verified active user lacks this credential's human sharing permission. */ +export function aiConnectionCredentialNotSharedFailure() { + const error = forbidden("This credential is not shared with the responsible user"); + failures.set(error, "ai_connection_credential_not_shared"); + return error; +} + /** Only owned producers supply this evidence; matching names or HTTP codes do not. */ export function readAiConnectionConfigurationFailure(error: unknown): AiConnectionConfigurationReason | null { return error instanceof Error ? failures.get(error) ?? null : null; diff --git a/server/src/services/ai-connections.ts b/server/src/services/ai-connections.ts index e73c0c64fd7..540541138cd 100644 --- a/server/src/services/ai-connections.ts +++ b/server/src/services/ai-connections.ts @@ -39,7 +39,7 @@ import { type AiConnectionLoginIntent, } from "@paperclipai/shared"; import { forbidden, notFound, unprocessable } from "../errors.js"; -import { aiConnectionConfigurationFailure } from "./ai-connection-configuration-failure.js"; +import { aiConnectionConfigurationFailure, aiConnectionCredentialNotSharedFailure } from "./ai-connection-configuration-failure.js"; import { quotaCredentialRecovery, quotaCredentialHash } from "./quota-credential-recovery.js"; import { logger } from "../middleware/logger.js"; import { logActivity } from "./activity-log.js"; @@ -349,8 +349,12 @@ export function aiConnectionService(db: Db) { )); // The existing human-access permission is authoritative for every binding, // including old explicit personal selections. Agent delegation cannot bypass it. - if (!canUseCredential(grant, userId, audience)) + if (!canUseCredential(grant, userId, audience)) { + // Membership was checked above. A known user's sharing choice is an + // operator action; absent identity must remain an unclassified denial. + if (userId) throw aiConnectionCredentialNotSharedFailure(); throw forbidden("This credential is not shared with the responsible user"); + } const installs = await db .select() .from(toolConnectionInstalls)