From 1d000bc9035d2b1fd61f079216e8101ddc066fa1 Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 11 Aug 2026 21:58:10 +0000 Subject: [PATCH] fix(security): make removing an app revoke its credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removing an app only flipped the connection to `archived`. Everything that made it work stayed behind: the operator's pasted key or OAuth tokens as live secrets, the `company_secret_bindings` that project them into runtimes, the install, the app-managed `app:` profile, gateway tokens minted against that profile, and any authorization still in flight. Reconnecting reused the retained credential, so "remove" never actually revoked anything. Removal is now a teardown, and the order is the security property. Every database-side access path closes first — grants revoked, bindings and installs deleted, the app profile removed (archived instead when a gateway's RESTRICT foreign key forbids deleting it, with that gateway's tokens and sessions revoked), live local runtimes stopped, catalog entries marked `removed`, outstanding OAuth state discarded, the connection and its application archived — so the app is undispatchable before the first call out to a secret provider. Only then are the credentials revoked, and each secret's ref survives until that secret is gone: a provider that errors leaves the operation failed closed and resumable, because the secret row is marked deleted before the provider call and the surviving refs are what a retry reads to finish the job. A secret is destroyed only when two independent tests agree: it was minted into the `tool_app.` namespace by connect/reconnect/OAuth, and nothing outside this connection references it. Anything ambiguous keeps its row and is reported as retained — deleting a credential another target still resolves is an outage, retaining an unreachable row is not. What stays is deliberate. Connection and application identity, names and activity survive so a reconnect reuses them, but with no credential, install or profile, reconnecting has to ask for fresh authentication and rebuild access. The activity receipt carries cleanup counts only; six of those count keys name what they counted ("secrets", "token"), so `sanitizeRecord` lets them through while the value is a finite number and still blanks the same key carrying anything else. Refs PAP-17119, PAP-17118, PAP-17089 (QA finding F6). Co-Authored-By: Claude Opus 5 (1M context) --- packages/shared/src/index.ts | 2 + packages/shared/src/types/index.ts | 2 + packages/shared/src/types/tool-access.ts | 45 ++ server/src/__tests__/redaction.test.ts | 39 + .../__tests__/tool-connection-removal.test.ts | 672 ++++++++++++++++++ server/src/redaction.ts | 26 +- server/src/routes/tool-access.ts | 13 +- server/src/services/tool-access.ts | 490 ++++++++++++- ui/src/api/tools.ts | 7 +- ui/src/pages/apps/AppDetail.tsx | 2 +- ui/src/pages/apps/Connections.test.tsx | 9 +- ui/src/pages/apps/Connections.tsx | 6 +- .../app-detail/AdvancedPanel.render.test.tsx | 59 ++ .../pages/apps/app-detail/AdvancedPanel.tsx | 3 +- 14 files changed, 1329 insertions(+), 46 deletions(-) create mode 100644 server/src/__tests__/tool-connection-removal.test.ts create mode 100644 ui/src/pages/apps/app-detail/AdvancedPanel.render.test.tsx diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index cbf7acea361..dd36f784464 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -1338,6 +1338,8 @@ export type { ToolConnectionInstall, ToolConnectionInstallSnapshot, ToolConnectionInstallTargetType, + ToolConnectionRemovalResult, + ToolConnectionRemovalSummary, ConnectionGrant, ConnectionGrantKind, ConnectionGrantStatus, diff --git a/packages/shared/src/types/index.ts b/packages/shared/src/types/index.ts index 38f9fb4c328..44d58fc6652 100644 --- a/packages/shared/src/types/index.ts +++ b/packages/shared/src/types/index.ts @@ -470,6 +470,8 @@ export type { ToolConnectionInstall, ToolConnectionInstallSnapshot, ToolConnectionInstallTargetType, + ToolConnectionRemovalResult, + ToolConnectionRemovalSummary, ConnectionTokenAttribution, ConnectionRecoverableErrorCode, ConnectionRecoverableErrorPayload, diff --git a/packages/shared/src/types/tool-access.ts b/packages/shared/src/types/tool-access.ts index 1bcd2743d83..b3d6e9001ed 100644 --- a/packages/shared/src/types/tool-access.ts +++ b/packages/shared/src/types/tool-access.ts @@ -190,6 +190,51 @@ export interface ToolConnectionInstallSnapshot { installs: ToolConnectionInstall[]; } +/** + * What a connection removal actually tore down (PAP-17119). + * + * Removing an app is a revocation boundary, not a cosmetic archive, so the + * receipt is counts and outcomes only — never a secret name, key, or value, + * because this summary is echoed into the activity log the whole company reads. + */ +export interface ToolConnectionRemovalSummary { + /** Connection-owned secrets revoked at the provider and deleted locally. */ + secretsRevoked: number; + /** Secrets left in place because another consumer still binds them. */ + secretsRetainedShared: number; + /** Credential refs cleared off the connection row. */ + credentialRefsCleared: number; + /** `company_secret_bindings` rows removed for this connection. */ + secretBindingsRemoved: number; + grantsRevoked: number; + installsRemoved: number; + /** + * `deleted` when the app-managed `app:` profile could go away, + * `archived` when a gateway still references it (the row survives with no + * entries and a non-active status), `absent` when there was never one. + */ + appProfile: "deleted" | "archived" | "absent"; + appProfileEntriesRemoved: number; + appProfileBindingsRemoved: number; + catalogEntriesMarkedRemoved: number; + oauthStatesDiscarded: number; + /** Token hashes wiped from the retained connection-token issuance ledger. */ + tokenIssuanceHashesCleared: number; + /** + * Live local runtimes shut down. A running child process holds the injected + * credential in memory, so it is an access path of its own. + */ + runtimeSlotsStopped: number; + gatewayTokensRevoked: number; + gatewaySessionsRevoked: number; + applicationArchived: boolean; +} + +export interface ToolConnectionRemovalResult { + connection: ToolConnection; + removal: ToolConnectionRemovalSummary; +} + export type ConnectionTokenScope = string | string[]; export type ConnectionTokenSubject = { type: "app" } | { type: "user"; userId: string }; diff --git a/server/src/__tests__/redaction.test.ts b/server/src/__tests__/redaction.test.ts index 4a295f103e3..3189070b196 100644 --- a/server/src/__tests__/redaction.test.ts +++ b/server/src/__tests__/redaction.test.ts @@ -76,6 +76,45 @@ describe("redaction", () => { }); }); + /** + * A removal receipt (PAP-17119) has to show what it revoked, so a fixed set of + * count keys is exempt from the secret-key guard — but only while the value is + * a number. The second half of this test is the point: the same key carrying + * anything else is still blanked, so the exemption cannot be used to smuggle + * material out under a familiar name. + */ + it("keeps numeric removal-receipt counts but still redacts non-numeric values on the same keys", () => { + expect(sanitizeRecord({ + secretsRevoked: 2, + secretsRetainedShared: 0, + credentialRefsCleared: 3, + secretBindingsRemoved: 3, + tokenIssuanceHashesCleared: 1, + gatewayTokensRevoked: 0, + appProfile: "deleted", + })).toEqual({ + secretsRevoked: 2, + secretsRetainedShared: 0, + credentialRefsCleared: 3, + secretBindingsRemoved: 3, + tokenIssuanceHashesCleared: 1, + gatewayTokensRevoked: 0, + appProfile: "deleted", + }); + + expect(sanitizeRecord({ + secretsRevoked: "pasted-api-key-value", + secretBindingsRemoved: { name: "tool_app.abc.headers_authorization" }, + tokenIssuanceHashesCleared: Number.NaN, + gatewayTokensRevoked: ["pcgw_live_token"], + })).toEqual({ + secretsRevoked: REDACTED_EVENT_VALUE, + secretBindingsRemoved: REDACTED_EVENT_VALUE, + tokenIssuanceHashesCleared: REDACTED_EVENT_VALUE, + gatewayTokensRevoked: REDACTED_EVENT_VALUE, + }); + }); + it("redacts common secret shapes from unstructured text", () => { const jwt = "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c"; const githubToken = "ghp_1234567890abcdefghijklmnopqrstuvwxyz"; diff --git a/server/src/__tests__/tool-connection-removal.test.ts b/server/src/__tests__/tool-connection-removal.test.ts new file mode 100644 index 00000000000..e85b9ac68f8 --- /dev/null +++ b/server/src/__tests__/tool-connection-removal.test.ts @@ -0,0 +1,672 @@ +import { randomUUID } from "node:crypto"; +import express from "express"; +import request from "supertest"; +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { + activityLog, + agents, + authUsers, + companies, + companyMemberships, + companySecretBindings, + companySecrets, + companySecretVersions, + connectionGrants, + connectionTokenIssuances, + createDb, + heartbeatRuns, + issues, + secretAccessEvents, + toolAccessAuditEvents, + toolApplications, + toolCatalogEntries, + toolConnectionInstalls, + toolConnections, + toolGatewaySessions, + toolMcpGateways, + toolMcpGatewayTokens, + toolOauthStates, + toolProfileBindings, + toolProfileEntries, + toolProfiles, + toolRuntimeSlots, +} from "@paperclipai/db"; +import { and, eq } from "drizzle-orm"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; +import { toolAccessService } from "../services/tool-access.js"; +import { toolAccessPolicyService } from "../services/tool-access-policy.js"; +import { toolAccessRoutes } from "../routes/tool-access.js"; +import { secretService } from "../services/secrets.js"; +import { localEncryptedProvider } from "../secrets/local-encrypted-provider.js"; +import { errorHandler } from "../middleware/index.js"; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +/** + * PAP-17119 — "Remove app" is a credential revocation boundary, not a status + * flip. Everything here asks the same question from a different angle: after a + * removal, is there any surviving row that still resolves a credential or still + * grants an agent a way to call the app? + * + * The connection is built through the real connect path where that is possible, + * so the tests fail if `connectGalleryApp` ever starts writing credential state + * somewhere the teardown does not look. + */ + +// A public IP literal, matching the generic MCP suite: remote endpoints are +// DNS-pinned even in local/private mode (PAP-17098), so a made-up hostname would +// fail resolution before it ever reached this fixture. +const MCP_ORIGIN = "https://203.0.113.10"; +const MCP_URL = `${MCP_ORIGIN}/mcp`; + +const FIXTURE_TOOLS = [ + { name: "list_things", description: "List things", annotations: { readOnlyHint: true } }, + { name: "delete_thing", description: "Delete a thing", annotations: { readOnlyHint: false, destructiveHint: true } }, +]; + +function jsonResponse(payload: unknown, status = 200): Response { + const body = JSON.stringify(payload); + return { + ok: status >= 200 && status < 300, + status, + headers: { + get: (name: string) => (name.toLowerCase() === "content-type" ? "application/json" : null), + }, + text: async () => body, + json: async () => payload, + } as unknown as Response; +} + +function headerRecord(init: RequestInit | undefined): Record { + const raw = init?.headers; + if (!raw) return {}; + if (raw instanceof Headers) return Object.fromEntries(raw.entries()); + if (Array.isArray(raw)) return Object.fromEntries(raw as Array<[string, string]>); + return Object.fromEntries( + Object.entries(raw as Record).map(([key, value]) => [key.toLowerCase(), value]), + ); +} + +/** + * The narrowest endpoint that satisfies a header-authenticated connect: it lists + * tools when the expected header arrives and 401s otherwise, which is all the + * catalog refresh and health check need. + */ +function installMcpFixture(requiredHeader: { name: string; value: string }) { + return vi.spyOn(globalThis, "fetch").mockImplementation(async (url, init) => { + const href = String(url); + const method = (init?.method ?? "GET").toUpperCase(); + const headers = headerRecord(init); + if (href === MCP_URL && method === "POST") { + if (headers[requiredHeader.name.toLowerCase()] !== requiredHeader.value) { + return jsonResponse({ error: "unauthorized" }, 401); + } + return jsonResponse({ jsonrpc: "2.0", id: "paperclip-catalog-refresh", result: { tools: FIXTURE_TOOLS } }); + } + return jsonResponse({ error: "not_found" }, 404); + }); +} + +async function createCompany(db: ReturnType) { + return db + .insert(companies) + .values({ + name: `Removal ${randomUUID()}`, + issuePrefix: `RM${randomUUID().slice(0, 6).toUpperCase()}`, + }) + .returning() + .then((rows) => rows[0]!); +} + +async function createAgent(db: ReturnType, companyId: string) { + return db + .insert(agents) + .values({ + companyId, + name: `Removal agent ${randomUUID()}`, + role: "engineer", + status: "active", + adapterType: "process", + adapterConfig: {}, + runtimeConfig: {}, + }) + .returning() + .then((rows) => rows[0]!); +} + +function createRouteApp(db: ReturnType) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = { + type: "board", + userId: "board-user", + userName: "Board User", + userEmail: null, + isInstanceAdmin: true, + source: "local_implicit", + }; + next(); + }); + app.use("/api", toolAccessRoutes(db)); + app.use(errorHandler); + return app; +} + +describeEmbeddedPostgres("tool connection removal", () => { + let db!: ReturnType; + let tempDb: Awaited> | null = null; + + const HEADER = { name: "X-Api-Key", value: "fixture-secret-value" }; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase("paperclip-removal-"); + db = createDb(tempDb.connectionString); + }, 20_000); + + afterEach(async () => { + vi.restoreAllMocks(); + vi.unstubAllEnvs(); + await db.delete(toolGatewaySessions); + await db.delete(toolMcpGatewayTokens); + await db.delete(toolMcpGateways); + await db.delete(connectionTokenIssuances); + await db.delete(connectionGrants); + await db.delete(toolOauthStates); + await db.delete(secretAccessEvents); + await db.delete(companySecretBindings); + await db.delete(companySecretVersions); + await db.delete(companySecrets); + await db.delete(activityLog); + await db.delete(toolAccessAuditEvents); + await db.delete(toolRuntimeSlots); + await db.delete(toolConnectionInstalls); + await db.delete(toolProfileBindings); + await db.delete(toolProfileEntries); + await db.delete(toolProfiles); + await db.delete(toolCatalogEntries); + await db.delete(toolConnections); + await db.delete(toolApplications); + await db.delete(heartbeatRuns); + await db.delete(issues); + await db.delete(companyMemberships); + await db.delete(agents); + await db.delete(companies); + await db.delete(authUsers); + }); + + afterAll(async () => { + await tempDb?.cleanup(); + }); + + /** Connect a header-authenticated app and finish the wizard for one agent. */ + async function connectHeaderApp(companyId: string, agentId: string) { + const service = toolAccessService(db); + const connected = await service.connectGalleryApp(companyId, { + link: MCP_URL, + name: `Removal fixture ${randomUUID().slice(0, 8)}`, + credentialValues: { [`headers.${HEADER.name}`]: HEADER.value }, + }); + const readEntry = connected.catalog.find((entry) => entry.toolName === "list_things")!; + const finished = await service.finishGalleryAppConnection(companyId, connected.connectionId, { + enabledCatalogEntryIds: [readEntry.id], + askFirstCatalogEntryIds: [], + access: { agentIds: [agentId] }, + }, { actorType: "user", actorId: "board-user" }); + await service.putConnectionInstalls(connected.connectionId, { + installs: [{ targetType: "agent", targetId: agentId, enabled: true }], + }, { actorType: "user", actorId: "board-user" }); + return { service, connectionId: connected.connectionId, readEntry, profile: finished.profile }; + } + + it("revokes the custom-header credential and every access path it granted", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const policy = toolAccessPolicyService(db); + const { service, connectionId, readEntry, profile } = await connectHeaderApp(company.id, agent.id); + + const [before] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + // One pasted header value is one secret, referenced twice: once as a header + // credential ref and once as a secret ref. + const secretIds = [...new Set([ + ...before!.credentialRefs.map((ref) => ref.secretId), + ...before!.credentialSecretRefs.map((ref) => ref.secretId), + ])]; + expect(secretIds).toHaveLength(1); + expect(before!.credentialRefs).toHaveLength(1); + expect(before!.credentialSecretRefs).toHaveLength(1); + // The credential really does resolve before removal, so the assertions after + // it are measuring a change rather than a permanent failure. + await expect(secretService(db).resolveSecretValue(company.id, secretIds[0]!, "latest", { + consumerType: "tool_connection", + consumerId: connectionId, + configPath: `credentials.headers.${HEADER.name}`, + actorType: "system", + })).resolves.toBe(HEADER.value); + await expect(policy.decide({ + companyId: company.id, + actor: { actorType: "agent", actorId: agent.id, agentId: agent.id }, + request: { connectionId, catalogEntryId: readEntry.id, toolName: "list_things" }, + })).resolves.toMatchObject({ allowed: true }); + + const removed = await service.archiveConnection(connectionId, company.id, { + actorType: "user", + actorId: "board-user", + }); + + expect(removed.connection).toMatchObject({ status: "archived", enabled: false }); + expect(removed.removal).toMatchObject({ + secretsRevoked: secretIds.length, + secretsRetainedShared: 0, + appProfile: "deleted", + installsRemoved: 1, + applicationArchived: true, + }); + expect(removed.removal.credentialRefsCleared).toBeGreaterThan(0); + expect(removed.removal.secretBindingsRemoved).toBeGreaterThan(0); + expect(removed.removal.catalogEntriesMarkedRemoved).toBe(FIXTURE_TOOLS.length); + + // No secret row, no version row, nothing resolvable. + for (const secretId of secretIds) { + expect(await db.select().from(companySecrets).where(eq(companySecrets.id, secretId))).toEqual([]); + expect(await db.select().from(companySecretVersions).where(eq(companySecretVersions.secretId, secretId))).toEqual([]); + await expect(secretService(db).resolveSecretValue(company.id, secretId, "latest", { + consumerType: "tool_connection", + consumerId: connectionId, + configPath: `credentials.headers.${HEADER.name}`, + actorType: "system", + })).rejects.toMatchObject({ status: 404 }); + } + expect(await db.select().from(companySecretBindings).where(and( + eq(companySecretBindings.targetType, "tool_connection"), + eq(companySecretBindings.targetId, connectionId), + ))).toEqual([]); + + // No ref left on the connection to point at a credential. + const [after] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + expect(after!.credentialRefs).toEqual([]); + expect(after!.credentialSecretRefs).toEqual([]); + + // No install, no app-managed profile, no profile entries or bindings. + expect(await db.select().from(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connectionId))).toEqual([]); + expect(await db.select().from(toolProfiles).where(eq(toolProfiles.profileKey, `app:${connectionId}`))).toEqual([]); + expect(await db.select().from(toolProfileEntries).where(eq(toolProfileEntries.profileId, profile.id))).toEqual([]); + expect(await db.select().from(toolProfileBindings).where(eq(toolProfileBindings.profileId, profile.id))).toEqual([]); + + // Catalog history survives, marked non-executable. + const catalog = await db.select().from(toolCatalogEntries).where(eq(toolCatalogEntries.connectionId, connectionId)); + expect(catalog.length).toBe(FIXTURE_TOOLS.length); + expect(catalog.every((entry) => entry.status === "removed")).toBe(true); + + // And the agent is denied. + await expect(policy.decide({ + companyId: company.id, + actor: { actorType: "agent", actorId: agent.id, agentId: agent.id }, + request: { connectionId, catalogEntryId: readEntry.id, toolName: "list_things" }, + })).resolves.toMatchObject({ allowed: false }); + expect(await service.listConnectionInstalls(connectionId, company.id)).toEqual([]); + }); + + it("revokes OAuth secrets, grant credentials and outstanding authorization state", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { service, connectionId } = await connectHeaderApp(company.id, agent.id); + const secrets = secretService(db); + + // Stand in for a completed sign-in: access and refresh tokens as dedicated + // `tool_app.*` secrets, referenced by the connection and by a user grant. + const accessSecret = await secrets.create(company.id, { + name: `Removal access ${randomUUID().slice(0, 8)}`, + key: `tool_app.${randomUUID()}.oauth_access_token`, + provider: "local_encrypted", + value: "oauth-access-token", + }); + const refreshSecret = await secrets.create(company.id, { + name: `Removal refresh ${randomUUID().slice(0, 8)}`, + key: `tool_app.${randomUUID()}.oauth_refresh_token`, + provider: "local_encrypted", + value: "oauth-refresh-token", + }); + const grantSecret = await secrets.create(company.id, { + name: `Removal grant ${randomUUID().slice(0, 8)}`, + key: `tool_app.${randomUUID()}.oauth_access_token`, + provider: "local_encrypted", + value: "grant-access-token", + }); + const [existing] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + const headerSecretIds = existing!.credentialSecretRefs.map((ref) => ref.secretId); + await service.updateConnection(connectionId, { + credentialSecretRefs: [ + ...existing!.credentialSecretRefs, + { secretId: accessSecret.id, versionSelector: "latest", configPath: "oauth.access_token", required: true, label: "OAuth access token" }, + { secretId: refreshSecret.id, versionSelector: "latest", configPath: "oauth.refresh_token", required: false, label: "OAuth refresh token" }, + ], + }); + const [grant] = await db.insert(connectionGrants).values({ + companyId: company.id, + connectionId, + kind: "user", + subjectUserId: "user-1", + credentialSecretRefs: [ + { secretId: grantSecret.id, versionSelector: "latest", configPath: "oauth.access_token", required: true, label: "OAuth access token" }, + ], + status: "active", + }).returning(); + await db.insert(toolOauthStates).values({ + state: `state-${randomUUID()}`, + companyId: company.id, + connectionId, + codeVerifier: "verifier", + expiresAt: new Date(Date.now() + 600_000), + }); + await db.insert(connectionTokenIssuances).values({ + companyId: company.id, + connectionId, + agentId: agent.id, + path: "oauth_access", + outcome: "success", + tokenHash: "a".repeat(64), + expiresAt: new Date(Date.now() + 600_000), + }); + + const removed = await service.archiveConnection(connectionId, company.id); + + expect(removed.removal).toMatchObject({ + secretsRevoked: headerSecretIds.length + 3, + secretsRetainedShared: 0, + grantsRevoked: 1, + oauthStatesDiscarded: 1, + tokenIssuanceHashesCleared: 1, + }); + for (const secretId of [accessSecret.id, refreshSecret.id, grantSecret.id, ...headerSecretIds]) { + expect(await db.select().from(companySecrets).where(eq(companySecrets.id, secretId))).toEqual([]); + } + const [grantAfter] = await db.select().from(connectionGrants).where(eq(connectionGrants.id, grant!.id)); + expect(grantAfter).toMatchObject({ status: "revoked", isDefault: false }); + expect(grantAfter!.revokedAt).not.toBeNull(); + expect(grantAfter!.credentialSecretRefs).toEqual([]); + expect(await db.select().from(toolOauthStates).where(eq(toolOauthStates.connectionId, connectionId))).toEqual([]); + const [issuance] = await db.select().from(connectionTokenIssuances).where(eq(connectionTokenIssuances.connectionId, connectionId)); + expect(issuance!.tokenHash).toBeNull(); + // The ledger row itself is history and stays. + expect(issuance!.path).toBe("oauth_access"); + }); + + it("leaves another consumer's credential in place", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { service, connectionId } = await connectHeaderApp(company.id, agent.id); + const secrets = secretService(db); + + // Two secrets removal must not destroy: one the operator manages by hand + // (outside the `tool_app.` namespace), and one dedicated-looking secret a + // second connection also binds. + const operatorSecret = await secrets.create(company.id, { + name: `Operator managed ${randomUUID().slice(0, 8)}`, + key: `shared_api_key_${randomUUID().slice(0, 8)}`, + provider: "local_encrypted", + value: "operator-managed-value", + }); + const alsoUsedSecret = await secrets.create(company.id, { + name: `Shared app secret ${randomUUID().slice(0, 8)}`, + key: `tool_app.${randomUUID()}.headers_x_shared`, + provider: "local_encrypted", + value: "shared-app-value", + }); + const [existing] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + await service.updateConnection(connectionId, { + credentialSecretRefs: [ + ...existing!.credentialSecretRefs, + { secretId: operatorSecret.id, versionSelector: "latest", configPath: "credentials.operator", required: false, label: "Operator key" }, + { secretId: alsoUsedSecret.id, versionSelector: "latest", configPath: "credentials.shared", required: false, label: "Shared key" }, + ], + }); + + // A second connection referencing the same secret is the other consumer. + const [otherApplication] = await db.insert(toolApplications).values({ + companyId: company.id, + applicationKey: `other:${randomUUID()}`, + name: `Other app ${randomUUID().slice(0, 8)}`, + type: "mcp_http", + status: "active", + }).returning(); + const otherConnection = await service.createConnection(company.id, { + applicationId: otherApplication!.id, + name: `Other connection ${randomUUID().slice(0, 8)}`, + transport: "mcp_remote", + config: { url: MCP_URL }, + credentialSecretRefs: [ + { secretId: alsoUsedSecret.id, versionSelector: "latest", configPath: "credentials.shared", required: false, label: "Shared key" }, + ], + }); + + const removed = await service.archiveConnection(connectionId, company.id); + + expect(removed.removal.secretsRetainedShared).toBe(2); + for (const secretId of [operatorSecret.id, alsoUsedSecret.id]) { + const [row] = await db.select().from(companySecrets).where(eq(companySecrets.id, secretId)); + expect(row).toBeTruthy(); + expect(row!.status).not.toBe("deleted"); + } + // The other consumer can still resolve its credential. + await expect(secretService(db).resolveSecretValue(company.id, alsoUsedSecret.id, "latest", { + consumerType: "tool_connection", + consumerId: otherConnection.id, + configPath: "credentials.shared", + actorType: "system", + })).resolves.toBe("shared-app-value"); + // Its binding survives; the removed connection's does not. + expect(await db.select().from(companySecretBindings).where(and( + eq(companySecretBindings.targetType, "tool_connection"), + eq(companySecretBindings.targetId, connectionId), + ))).toEqual([]); + expect((await db.select().from(companySecretBindings).where(and( + eq(companySecretBindings.targetType, "tool_connection"), + eq(companySecretBindings.targetId, otherConnection.id), + ))).length).toBeGreaterThan(0); + // And the removed connection no longer points at either of them. + const [after] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + expect(after!.credentialSecretRefs).toEqual([]); + }); + + it("fails closed and stays resumable when the secret provider errors", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { service, connectionId, readEntry } = await connectHeaderApp(company.id, agent.id); + const policy = toolAccessPolicyService(db); + + const [before] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + const secretIds = before!.credentialSecretRefs.map((ref) => ref.secretId); + expect(secretIds.length).toBeGreaterThan(0); + + // Break provider deletion for the first attempt only. The registry hands out + // this very object, so patching the method is enough. + let failures = 0; + const spy = vi.spyOn(localEncryptedProvider, "deleteOrArchive").mockImplementation(async () => { + failures += 1; + throw new Error("provider unavailable"); + }); + + await expect(service.archiveConnection(connectionId, company.id)).rejects.toThrow("provider unavailable"); + expect(failures).toBe(1); + + // Failed closed: the app is already undispatchable and the credential is + // already unresolvable, even though the provider copy survives. + const [midway] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + expect(midway).toMatchObject({ status: "archived", enabled: false }); + expect(await db.select().from(toolConnectionInstalls).where(eq(toolConnectionInstalls.connectionId, connectionId))).toEqual([]); + expect(await db.select().from(toolProfiles).where(eq(toolProfiles.profileKey, `app:${connectionId}`))).toEqual([]); + await expect(policy.decide({ + companyId: company.id, + actor: { actorType: "agent", actorId: agent.id, agentId: agent.id }, + request: { connectionId, catalogEntryId: readEntry.id, toolName: "list_things" }, + })).resolves.toMatchObject({ allowed: false }); + await expect(secretService(db).resolveSecretValue(company.id, secretIds[0]!, "latest", { + consumerType: "tool_connection", + consumerId: connectionId, + configPath: `credentials.headers.${HEADER.name}`, + actorType: "system", + })).rejects.toMatchObject({ status: 404 }); + // The ref survives the failure on purpose: it is the only pointer a retry + // has to the secret it still has to revoke. + expect(midway!.credentialSecretRefs.length).toBeGreaterThan(0); + + // Retrying the same removal finishes the job. + spy.mockRestore(); + const removed = await service.archiveConnection(connectionId, company.id); + expect(removed.removal.secretsRevoked).toBe(secretIds.length); + expect(removed.removal.secretsRetainedShared).toBe(0); + for (const secretId of secretIds) { + expect(await db.select().from(companySecrets).where(eq(companySecrets.id, secretId))).toEqual([]); + } + const [after] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + expect(after!.credentialSecretRefs).toEqual([]); + + // A third removal is a no-op that still succeeds. + const again = await service.archiveConnection(connectionId, company.id); + expect(again.removal).toMatchObject({ + secretsRevoked: 0, + secretsRetainedShared: 0, + installsRemoved: 0, + appProfile: "absent", + grantsRevoked: 0, + }); + }); + + it("archives the app profile and revokes gateway tokens when a gateway still points at it", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { service, connectionId, profile } = await connectHeaderApp(company.id, agent.id); + + const [gateway] = await db.insert(toolMcpGateways).values({ + companyId: company.id, + name: `Gateway ${randomUUID().slice(0, 8)}`, + slug: `gw-${randomUUID().slice(0, 8)}`, + profileId: profile.id, + status: "active", + }).returning(); + const [token] = await db.insert(toolMcpGatewayTokens).values({ + companyId: company.id, + gatewayId: gateway!.id, + name: "client", + tokenHash: randomUUID().replace(/-/g, "").padEnd(64, "0"), + }).returning(); + + const removed = await service.archiveConnection(connectionId, company.id); + + expect(removed.removal).toMatchObject({ + appProfile: "archived", + gatewayTokensRevoked: 1, + }); + const [profileAfter] = await db.select().from(toolProfiles).where(eq(toolProfiles.id, profile.id)); + // The row survives only because the gateway foreign key forbids deleting it; + // a non-active profile with no entries is what the policy engine ignores. + expect(profileAfter).toMatchObject({ status: "archived", defaultAction: "deny" }); + expect(await db.select().from(toolProfileEntries).where(eq(toolProfileEntries.profileId, profile.id))).toEqual([]); + const [tokenAfter] = await db.select().from(toolMcpGatewayTokens).where(eq(toolMcpGatewayTokens.id, token!.id)); + expect(tokenAfter!.revokedAt).not.toBeNull(); + }); + + it("stops a live local runtime so no process keeps serving the revoked credential", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { service, connectionId } = await connectHeaderApp(company.id, agent.id); + const [connection] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + + const [slot] = await db.insert(toolRuntimeSlots).values({ + companyId: company.id, + applicationId: connection!.applicationId, + connectionId, + slotKey: `mcp:${company.id}:${connectionId}`, + ownerScopeType: "connection", + ownerScopeId: connectionId, + runtimeKind: "local_stdio", + status: "running", + provider: "paperclip", + healthStatus: "ok", + }).returning(); + + const removed = await service.archiveConnection(connectionId, company.id); + + expect(removed.removal.runtimeSlotsStopped).toBe(1); + const [slotAfter] = await db.select().from(toolRuntimeSlots).where(eq(toolRuntimeSlots.id, slot!.id)); + expect(slotAfter!.status).toBe("stopped"); + expect(slotAfter!.metadata).toMatchObject({ stoppedReason: "connection_removed" }); + + // A second removal has nothing left to stop. + const again = await service.archiveConnection(connectionId, company.id); + expect(again.removal.runtimeSlotsStopped).toBe(0); + }); + + it("removes over the route with a secret-free receipt, then requires fresh credentials to reconnect", async () => { + installMcpFixture(HEADER); + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + const { service, connectionId } = await connectHeaderApp(company.id, agent.id); + const app = createRouteApp(db); + const [beforeRemoval] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + const originalSecretIds = beforeRemoval!.credentialSecretRefs.map((ref) => ref.secretId); + expect(originalSecretIds.length).toBeGreaterThan(0); + + const response = await request(app).delete(`/api/tool-connections/${connectionId}`).expect(200); + + expect(response.body).toMatchObject({ id: connectionId, status: "archived", enabled: false }); + expect(response.body.removal).toMatchObject({ appProfile: "deleted", secretsRetainedShared: 0 }); + expect(response.body.removal.secretsRevoked).toBeGreaterThan(0); + // Nothing in the response body echoes the pasted value or a secret name. + expect(JSON.stringify(response.body)).not.toContain(HEADER.value); + + const [receipt] = await db + .select() + .from(activityLog) + .where(and(eq(activityLog.companyId, company.id), eq(activityLog.action, "tool_connection.archived"))); + expect(receipt!.details).toMatchObject({ + transport: "mcp_remote", + appProfile: "deleted", + secretsRetainedShared: 0, + applicationArchived: true, + }); + expect(Number((receipt!.details as Record).secretsRevoked)).toBeGreaterThan(0); + const receiptText = JSON.stringify(receipt!.details); + expect(receiptText).not.toContain(HEADER.value); + expect(receiptText).not.toContain("tool_app."); + + // Reconnect cannot lean on a retained credential: the archived connection + // refuses both reconnect and sign-in, and a fresh connect has to be given + // new credential values. + await expect(service.reconnectGalleryApp(connectionId, company.id, { credentialValues: {} })) + .rejects.toMatchObject({ status: 409 }); + await expect(service.startOAuth(company.id, connectionId, { + redirectUri: "https://paperclip.fixture.test/api/tools/oauth/callback", + actor: { actorType: "user", actorId: "board-user" }, + })).rejects.toMatchObject({ status: 409 }); + + const [archived] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + const reconnected = await service.connectGalleryApp(company.id, { + applicationId: archived!.applicationId, + link: MCP_URL, + name: archived!.name, + credentialValues: { [`headers.${HEADER.name}`]: HEADER.value }, + }); + // Same connection identity, brand new credential. + expect(reconnected.connectionId).toBe(connectionId); + const [after] = await db.select().from(toolConnections).where(eq(toolConnections.id, connectionId)); + expect(after!.credentialSecretRefs.length).toBeGreaterThan(0); + const reconnectedSecretIds = after!.credentialSecretRefs.map((ref) => ref.secretId); + // Not one of the revoked secrets came back. + for (const secretId of originalSecretIds) expect(reconnectedSecretIds).not.toContain(secretId); + // And the operator has to choose access again — the app profile is not back. + expect(await db.select().from(toolProfiles).where(eq(toolProfiles.profileKey, `app:${connectionId}`))).toEqual([]); + expect(await service.listConnectionInstalls(connectionId, company.id)).toEqual([]); + }); +}); diff --git a/server/src/redaction.ts b/server/src/redaction.ts index eea53edc67c..75cdb086b4e 100644 --- a/server/src/redaction.ts +++ b/server/src/redaction.ts @@ -9,6 +9,26 @@ const SECRET_PAYLOAD_KEY_RE = new RegExp(SECRET_FIELD_NAME_PATTERN, "i"); // "authorization". JWT-shaped values are still caught by the value guard below. const AUDIT_REASON_PAYLOAD_KEY_RE = /^authorizationReason$/; const AUDIT_SURFACE_PAYLOAD_KEY_RE = /^surface$/; +/** + * Cleanup counts on a connection-removal receipt (PAP-17119). Their names name + * the thing they counted — secrets, bindings, tokens — so the key guard above + * would blank the whole receipt and leave the operator unable to see what a + * revocation actually tore down. They pass only while the value really is a + * finite number, so nothing that could carry material rides through on the + * strength of a familiar key name. + */ +const AUDIT_COUNT_PAYLOAD_KEYS = new Set([ + "secretsRevoked", + "secretsRetainedShared", + "credentialRefsCleared", + "secretBindingsRemoved", + "tokenIssuanceHashesCleared", + "gatewayTokensRevoked", +]); + +function isAuditCountField(key: string, value: unknown): boolean { + return AUDIT_COUNT_PAYLOAD_KEYS.has(key) && typeof value === "number" && Number.isFinite(value); +} const COMMAND_PAYLOAD_KEY_RE = /(^command$|^cmd$|command[-_]?line|resolved[-_]?command|PAPERCLIP_RESOLVED_COMMAND)/i; const COMMAND_ARGS_PAYLOAD_KEY_RE = /^(commandArgs|command_?args|argv)$/i; @@ -107,7 +127,11 @@ export function sanitizeRecord(record: Record): Record { diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index d8abf8374a0..c10b1257dae 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -1,6 +1,6 @@ import { createHash, randomBytes, randomUUID } from "node:crypto"; import { readFileSync } from "node:fs"; -import { and, asc, desc, eq, gte, inArray, lt, max, ne, sql } from "drizzle-orm"; +import { and, asc, desc, eq, gte, inArray, isNull, lt, max, ne, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { activityLog, @@ -27,7 +27,9 @@ import { toolCallEvents, toolInvocations, toolPolicies, + toolGatewaySessions, toolMcpGateways, + toolMcpGatewayTokens, toolProfileBindings, toolProfileEntries, toolProfiles, @@ -64,6 +66,8 @@ import type { ToolConnection, ToolConnectionInstall, ToolConnectionInstallSnapshot, + ToolConnectionRemovalResult, + ToolConnectionRemovalSummary, ToolConnectionHealthCheckResult, ToolConnectionHealthStatus, ToolConnectionAuthKind, @@ -123,6 +127,7 @@ import { type OAuthEndpointUrlRejection, } from "@paperclipai/shared"; import { badRequest, conflict, forbidden, HttpError, notFound, unprocessable } from "../errors.js"; +import { logger } from "../middleware/logger.js"; import { logActivity } from "./activity-log.js"; import { mcpHttpRequestHeaders, parseMcpHttpResponseBody } from "./mcp-http.js"; import { assertPublicRemoteHttpEndpoint, parseRemoteHttpEndpoint } from "./remote-http-endpoint-guard.js"; @@ -883,6 +888,48 @@ function connectionUid(namespace: string, name: string, connectionId: string) { return `${normalizeKey(namespace)}/${normalizeKey(name)}-${connectionId.slice(0, 8)}`; } +/** + * The key namespace `connectGalleryApp`, `reconnectGalleryApp` and + * `createOrRotateOAuthSecret` mint for credentials a connection owns outright. + * Nothing else writes this prefix, which is what lets removal tell a dedicated + * app credential apart from a secret the operator manages by hand — see + * `classifyConnectionSecrets`. Matched as a prefix on purpose: `secrets.remove` + * suffixes `__deleted__` onto the key before it deletes the row, so a + * removal that is retried after a provider failure must still recognise it. + */ +const CONNECTION_OWNED_SECRET_KEY_PREFIX = "tool_app."; + +/** Token fields a legacy row may have inlined into `config.oauth`. */ +const INLINE_OAUTH_TOKEN_FIELDS = [ + "access_token", + "refresh_token", + "accessToken", + "refreshToken", + "client_secret", + "clientSecret", +]; + +/** + * Drop inline OAuth token material from a connection config, keeping the + * non-secret identity (client id, issuer, registration source) a later + * reconnect reuses. Current code stores tokens as secret refs, never in the + * config; this exists so a row written by an older build cannot keep a usable + * token after the operator removed the app. + */ +function withoutInlineOAuthTokens(config: Record): Record { + const oauth = config.oauth; + if (!oauth || typeof oauth !== "object" || Array.isArray(oauth)) return config; + const next = { ...(oauth as Record) }; + let changed = false; + for (const field of INLINE_OAUTH_TOKEN_FIELDS) { + if (field in next) { + delete next[field]; + changed = true; + } + } + return changed ? { ...config, oauth: next } : config; +} + function actorBinding(actor: ActorInfo | undefined) { return { actorType: actor?.actorType ?? null, @@ -3227,6 +3274,414 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} }))); } + /** + * Split the secrets a connection points at into the ones it owns outright and + * the ones another consumer still depends on. + * + * Removing an app is a credential revocation boundary (PAP-17119), but it must + * never destroy a secret the operator manages by hand or shares with another + * target. Two independent tests have to agree before a secret is destroyed: + * + * 1. Provenance — the key sits in the `tool_app.` namespace only the + * connect/reconnect/OAuth paths mint, and the row is a company-scoped + * Paperclip secret rather than a per-user credential. + * 2. Exclusivity — nothing outside this connection references it: no + * `company_secret_bindings` row from another target, and no other + * connection or connection grant naming the same secret id. + * + * A secret failing either test is reported as retained; removal still drops + * this connection's binding and ref, so the connection loses the credential + * either way. Retaining a secret nobody can reach is a leak of an unused row; + * deleting one another target still resolves is an outage, so the ambiguous + * case fails towards retention. + */ + async function classifyConnectionSecrets( + connection: typeof toolConnections.$inferSelect, + secretIds: string[], + ): Promise<{ owned: string[]; retained: string[] }> { + const unique = [...new Set(secretIds.filter((id) => typeof id === "string" && id.length > 0))]; + if (unique.length === 0) return { owned: [], retained: [] }; + + const secretRows = await db + .select({ + id: companySecrets.id, + key: companySecrets.key, + scope: companySecrets.scope, + userSecretDefinitionId: companySecrets.userSecretDefinitionId, + }) + .from(companySecrets) + .where(and(eq(companySecrets.companyId, connection.companyId), inArray(companySecrets.id, unique))); + const byId = new Map(secretRows.map((row) => [row.id, row])); + + const referencedElsewhere = new Set(); + const foreignBindings = await db + .select({ secretId: companySecretBindings.secretId }) + .from(companySecretBindings) + .where(and( + eq(companySecretBindings.companyId, connection.companyId), + inArray(companySecretBindings.secretId, unique), + sql`not (${companySecretBindings.targetType} = 'tool_connection' and ${companySecretBindings.targetId} = ${connection.id})`, + )); + for (const row of foreignBindings) referencedElsewhere.add(row.secretId); + + // Bindings are the authority, but read the sibling refs too: a row written + // before `syncCredentialBindings` existed — or by hand — can reference a + // secret with no binding to prove it. + const siblingConnections = await db + .select({ + credentialRefs: toolConnections.credentialRefs, + credentialSecretRefs: toolConnections.credentialSecretRefs, + }) + .from(toolConnections) + .where(and(eq(toolConnections.companyId, connection.companyId), ne(toolConnections.id, connection.id))); + for (const row of siblingConnections) { + for (const ref of row.credentialRefs ?? []) referencedElsewhere.add(ref.secretId); + for (const ref of row.credentialSecretRefs ?? []) referencedElsewhere.add(ref.secretId); + } + const siblingGrants = await db + .select({ credentialSecretRefs: connectionGrants.credentialSecretRefs }) + .from(connectionGrants) + .where(and( + eq(connectionGrants.companyId, connection.companyId), + ne(connectionGrants.connectionId, connection.id), + )); + for (const row of siblingGrants) { + for (const ref of row.credentialSecretRefs ?? []) referencedElsewhere.add(ref.secretId); + } + + const owned: string[] = []; + const retained: string[] = []; + for (const secretId of unique) { + const row = byId.get(secretId); + // No row means an earlier pass of this same removal already deleted it. + // Hand it back as owned so a retry re-runs the (idempotent) revocation + // instead of reporting a credential this connection never shared. + if (!row) { + owned.push(secretId); + continue; + } + const dedicated = row.scope === "company" + && row.userSecretDefinitionId === null + && row.key.startsWith(CONNECTION_OWNED_SECRET_KEY_PREFIX); + if (dedicated && !referencedElsewhere.has(secretId)) owned.push(secretId); + else retained.push(secretId); + } + return { owned, retained }; + } + + /** + * Remove an app: a credential-revoking teardown, not a status flip (PAP-17119). + * + * Order is the security property. Every database-side access path closes + * first — grants, installs, the app-managed profile, gateway tokens minted + * against it, outstanding OAuth state, the catalog, and the connection itself + * — so the app is already undispatchable before the first call out to a secret + * provider. Secret revocation runs last, and each secret's ref survives until + * that secret is gone, so a provider that errors leaves the operation failed + * closed and resumable: the credential is already unresolvable (its row is + * marked deleted first), and retrying the same removal finishes the job. + * + * What stays behind is deliberate: the connection and application rows, their + * ids, names and activity keep working so a later reconnect reuses the same + * identity — but with no credential, no install and no profile, so + * reconnecting has to ask for fresh authentication and rebuild access. + */ + async function removeConnection( + connectionId: string, + companyId?: string, + actor?: ActorInfo, + ): Promise { + const connection = await getConnectionRow(connectionId, companyId); + const now = new Date(); + const binding = actorBinding(actor); + + // Grants are read before they are revoked: a retried removal must still see + // the credential refs of a grant an earlier pass already marked revoked. + const grantRows = await db + .select({ + id: connectionGrants.id, + status: connectionGrants.status, + credentialSecretRefs: connectionGrants.credentialSecretRefs, + }) + .from(connectionGrants) + .where(and( + eq(connectionGrants.companyId, connection.companyId), + eq(connectionGrants.connectionId, connection.id), + )); + const grantsToRevoke = grantRows.filter((row) => row.status !== "revoked"); + if (grantsToRevoke.length > 0) { + await db + .update(connectionGrants) + .set({ + status: "revoked", + isDefault: false, + revokedAt: now, + revokedByAgentId: binding.actorType === "agent" ? binding.actorId : null, + revokedByUserId: binding.actorType === "user" ? binding.actorId : null, + updatedAt: now, + }) + .where(inArray(connectionGrants.id, grantsToRevoke.map((row) => row.id))); + } + + // Bindings are how a credential reaches a runtime, so they go before the + // provider round-trip rather than after it. They are also not needed to + // finish the job: the refs on the connection row are what a resumed removal + // reads to find the secrets it still owes a revocation. + const removedSecretBindings = await db + .delete(companySecretBindings) + .where(and( + eq(companySecretBindings.companyId, connection.companyId), + eq(companySecretBindings.targetType, "tool_connection"), + eq(companySecretBindings.targetId, connection.id), + )) + .returning({ id: companySecretBindings.id }); + + const removedInstalls = await db + .delete(toolConnectionInstalls) + .where(and( + eq(toolConnectionInstalls.companyId, connection.companyId), + eq(toolConnectionInstalls.connectionId, connection.id), + )) + .returning({ id: toolConnectionInstalls.id }); + + // The app-managed profile exists only to carry this connection's action + // selection, so it goes with the connection. Operator-authored profiles that + // happen to mention the connection are left alone — the archived connection + // is denied by the policy engine regardless. + const [appProfile] = await db + .select({ id: toolProfiles.id }) + .from(toolProfiles) + .where(and( + eq(toolProfiles.companyId, connection.companyId), + eq(toolProfiles.profileKey, `app:${connection.id}`), + )) + .limit(1); + let appProfileOutcome: ToolConnectionRemovalSummary["appProfile"] = "absent"; + let appProfileEntriesRemoved = 0; + let appProfileBindingsRemoved = 0; + let gatewayTokensRevoked = 0; + let gatewaySessionsRevoked = 0; + if (appProfile) { + appProfileEntriesRemoved = (await db + .delete(toolProfileEntries) + .where(and( + eq(toolProfileEntries.companyId, connection.companyId), + eq(toolProfileEntries.profileId, appProfile.id), + )) + .returning({ id: toolProfileEntries.id })).length; + appProfileBindingsRemoved = (await db + .delete(toolProfileBindings) + .where(and( + eq(toolProfileBindings.companyId, connection.companyId), + eq(toolProfileBindings.profileId, appProfile.id), + )) + .returning({ id: toolProfileBindings.id })).length; + + const gatewayRows = await db + .select({ id: toolMcpGateways.id }) + .from(toolMcpGateways) + .where(and( + eq(toolMcpGateways.companyId, connection.companyId), + eq(toolMcpGateways.profileId, appProfile.id), + )); + if (gatewayRows.length === 0) { + await db.delete(toolProfiles).where(eq(toolProfiles.id, appProfile.id)); + appProfileOutcome = "deleted"; + } else { + // `tool_mcp_gateways.profile_id` is ON DELETE RESTRICT, so a gateway + // pointing here keeps the row alive. Archive it instead — the policy + // engine only consults `active` profiles, and it has no entries left — + // and revoke the tokens those gateways already handed out, which are the + // one credential a caller could still present. + await db + .update(toolProfiles) + .set({ status: "archived", defaultAction: "deny", updatedAt: now }) + .where(eq(toolProfiles.id, appProfile.id)); + appProfileOutcome = "archived"; + const revokedTokens = await db + .update(toolMcpGatewayTokens) + .set({ revokedAt: now, updatedAt: now }) + .where(and( + eq(toolMcpGatewayTokens.companyId, connection.companyId), + inArray(toolMcpGatewayTokens.gatewayId, gatewayRows.map((row) => row.id)), + isNull(toolMcpGatewayTokens.revokedAt), + )) + .returning({ id: toolMcpGatewayTokens.id }); + gatewayTokensRevoked = revokedTokens.length; + if (revokedTokens.length > 0) { + gatewaySessionsRevoked = (await db + .update(toolGatewaySessions) + .set({ revokedAt: now, updatedAt: now }) + .where(and( + eq(toolGatewaySessions.companyId, connection.companyId), + inArray(toolGatewaySessions.gatewayTokenId, revokedTokens.map((row) => row.id)), + isNull(toolGatewaySessions.revokedAt), + )) + .returning({ id: toolGatewaySessions.id })).length; + } + } + } + + // A local runtime already holds the injected credential inside a live child + // process, so archiving rows is not enough — the process itself is an access + // path. Stopping is best effort on purpose: if the supervisor cannot be + // reached, revoking the credential anyway (so nothing can start again) beats + // abandoning the teardown, and the warning says which slot was left running. + let runtimeSlotsStopped = 0; + const runtimeSlotRows = await db + .select({ id: toolRuntimeSlots.id, status: toolRuntimeSlots.status }) + .from(toolRuntimeSlots) + .where(and( + eq(toolRuntimeSlots.companyId, connection.companyId), + eq(toolRuntimeSlots.connectionId, connection.id), + )); + for (const slot of runtimeSlotRows) { + if (slot.status === "stopped") continue; + try { + await runtimeSupervisor.stopSlot({ + companyId: connection.companyId, + slotId: slot.id, + reason: "connection_removed", + }); + runtimeSlotsStopped += 1; + } catch (error) { + logger.warn( + { err: error, companyId: connection.companyId, connectionId: connection.id, slotId: slot.id }, + "tool connection removal could not stop a runtime slot", + ); + } + } + + // The catalog stays as history, but `removed` is the status that stops a + // standing trust rule from auto-allowing one of these actions again. + const removedCatalogEntries = await db + .update(toolCatalogEntries) + .set({ status: "removed", updatedAt: now }) + .where(and( + eq(toolCatalogEntries.companyId, connection.companyId), + eq(toolCatalogEntries.connectionId, connection.id), + ne(toolCatalogEntries.status, "removed"), + )) + .returning({ id: toolCatalogEntries.id }); + + // An authorization already in flight would otherwise come back and mint a + // fresh token for an app the operator just removed. + const discardedOAuthStates = await db + .delete(toolOauthStates) + .where(and( + eq(toolOauthStates.companyId, connection.companyId), + eq(toolOauthStates.connectionId, connection.id), + )) + .returning({ state: toolOauthStates.state }); + + // Token-derived material in the issuance ledger is not an access path — + // nothing validates against it — but there is no reason to keep a hash of a + // credential the operator asked us to revoke. Path, outcome, actor and time + // stay, so the usage history survives. + const clearedIssuanceHashes = await db + .update(connectionTokenIssuances) + .set({ tokenHash: null }) + .where(and( + eq(connectionTokenIssuances.companyId, connection.companyId), + eq(connectionTokenIssuances.connectionId, connection.id), + sql`${connectionTokenIssuances.tokenHash} is not null`, + )) + .returning({ id: connectionTokenIssuances.id }); + + const archived = await db.transaction(async (tx) => { + const [updatedConnection] = await tx + .update(toolConnections) + .set({ status: "archived", enabled: false, updatedAt: now }) + .where(eq(toolConnections.id, connection.id)) + .returning(); + if (!updatedConnection) throw notFound("Tool connection not found"); + + const remainingConnections = await tx + .select({ id: toolConnections.id }) + .from(toolConnections) + .where(and( + eq(toolConnections.applicationId, updatedConnection.applicationId), + ne(toolConnections.status, "archived"), + )) + .limit(1); + + let applicationArchived = false; + if (remainingConnections.length === 0) { + const [application] = await tx + .update(toolApplications) + .set({ status: "archived", archivedAt: now, updatedAt: now }) + .where(and( + eq(toolApplications.id, updatedConnection.applicationId), + ne(toolApplications.status, "archived"), + )) + .returning({ id: toolApplications.id }); + applicationArchived = Boolean(application); + } + + return { connection: updatedConnection, applicationArchived }; + }); + + // Only now, with every access path closed, revoke the credentials. Each + // `secrets.remove` marks the row deleted before it calls the provider, so a + // provider error leaves an unresolvable secret and a resumable removal + // rather than a half-open app. + const candidateSecretIds = [ + ...connection.credentialRefs.map((ref) => ref.secretId), + ...connection.credentialSecretRefs.map((ref) => ref.secretId), + ...grantRows.flatMap((grant) => (grant.credentialSecretRefs ?? []).map((ref) => ref.secretId)), + ]; + const { owned, retained } = await classifyConnectionSecrets(connection, candidateSecretIds); + let secretsRevoked = 0; + for (const secretId of owned) { + const removed = await secrets.remove(secretId); + if (removed) secretsRevoked += 1; + } + + const credentialRefsCleared = connection.credentialRefs.length + connection.credentialSecretRefs.length; + const [cleared] = await db + .update(toolConnections) + .set({ + credentialRefs: [], + credentialSecretRefs: [], + config: withoutInlineOAuthTokens(connection.config), + transportConfig: withoutInlineOAuthTokens(connection.transportConfig), + updatedAt: now, + }) + .where(eq(toolConnections.id, connection.id)) + .returning(); + if (grantRows.some((grant) => (grant.credentialSecretRefs ?? []).length > 0)) { + await db + .update(connectionGrants) + .set({ credentialSecretRefs: [], updatedAt: now }) + .where(and( + eq(connectionGrants.companyId, connection.companyId), + eq(connectionGrants.connectionId, connection.id), + )); + } + + return { + connection: toConnection(cleared ?? archived.connection), + removal: { + secretsRevoked, + secretsRetainedShared: retained.length, + credentialRefsCleared, + secretBindingsRemoved: removedSecretBindings.length, + grantsRevoked: grantsToRevoke.length, + installsRemoved: removedInstalls.length, + appProfile: appProfileOutcome, + appProfileEntriesRemoved, + appProfileBindingsRemoved, + catalogEntriesMarkedRemoved: removedCatalogEntries.length, + oauthStatesDiscarded: discardedOAuthStates.length, + tokenIssuanceHashesCleared: clearedIssuanceHashes.length, + runtimeSlotsStopped, + gatewayTokensRevoked, + gatewaySessionsRevoked, + applicationArchived: archived.applicationArchived, + }, + }; + } + async function ensureRuntimeSlot(connection: typeof toolConnections.$inferSelect): Promise { if (connection.transport !== "local_stdio") return null; const slotKey = `mcp:${connection.companyId}:${connection.id}`; @@ -7661,38 +8116,7 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} return toConnection(row); }, - archiveConnection: async (connectionId: string): Promise => { - const row = await db.transaction(async (tx) => { - const [updatedConnection] = await tx - .update(toolConnections) - .set({ status: "archived", enabled: false, updatedAt: new Date() }) - .where(eq(toolConnections.id, connectionId)) - .returning(); - if (!updatedConnection) throw notFound("Tool connection not found"); - - const remainingConnections = await tx - .select({ id: toolConnections.id }) - .from(toolConnections) - .where( - and( - eq(toolConnections.applicationId, updatedConnection.applicationId), - ne(toolConnections.status, "archived"), - ), - ) - .limit(1); - - if (remainingConnections.length === 0) { - const now = new Date(); - await tx - .update(toolApplications) - .set({ status: "archived", archivedAt: now, updatedAt: now }) - .where(eq(toolApplications.id, updatedConnection.applicationId)); - } - - return updatedConnection; - }); - return toConnection(row); - }, + archiveConnection: removeConnection, checkHealth: checkConnectionHealth, diff --git a/ui/src/api/tools.ts b/ui/src/api/tools.ts index 47692aec3dd..acc06d4e432 100644 --- a/ui/src/api/tools.ts +++ b/ui/src/api/tools.ts @@ -3,6 +3,7 @@ import type { ToolConnection, ToolConnectionInstall, ToolConnectionInstallSnapshot, + ToolConnectionRemovalSummary, ConnectToolAppResult, FinishToolAppResult, ToolCatalogEntry, @@ -290,8 +291,12 @@ export const toolsApi = { api.post(`/companies/${companyId}/tools/connections`, input), updateConnection: (connectionId: string, input: UpdateToolConnectionInput) => api.patch(`/tool-connections/${connectionId}`, input), + // Removal is a credential-revoking teardown (PAP-17119), so the response + // carries the cleanup receipt alongside the archived connection. archiveConnection: (connectionId: string) => - api.delete(`/tool-connections/${connectionId}`), + api.delete( + `/tool-connections/${connectionId}`, + ), checkConnectionHealth: (connectionId: string) => api.post(`/tool-connections/${connectionId}/health-check`, {}), reconnectConnection: (connectionId: string, credentialValues: Record) => diff --git a/ui/src/pages/apps/AppDetail.tsx b/ui/src/pages/apps/AppDetail.tsx index 0621a5680dd..2f2740eca93 100644 --- a/ui/src/pages/apps/AppDetail.tsx +++ b/ui/src/pages/apps/AppDetail.tsx @@ -273,7 +273,7 @@ export function AppDetail() { queryClient.invalidateQueries({ queryKey: queryKeys.apps.attention(selectedCompanyId!) }); pushToast({ title: "App removed", - body: `${appName} no longer has access. You can connect it again any time.`, + body: `${appName} no longer has access and its credentials are deleted. Connecting it again needs a new sign-in or key.`, tone: "success", }); navigate("/apps/connections"); diff --git a/ui/src/pages/apps/Connections.test.tsx b/ui/src/pages/apps/Connections.test.tsx index bf3fe0b32ce..80e333c3bad 100644 --- a/ui/src/pages/apps/Connections.test.tsx +++ b/ui/src/pages/apps/Connections.test.tsx @@ -368,7 +368,7 @@ describe("Connections table (M1b / PAP-13254 door 2)", () => { expect(pushToastMock).toHaveBeenCalledWith( expect.objectContaining({ title: "Connection deleted", - body: "GitHub is no longer available to agents. You can connect it again later.", + body: "GitHub is no longer available to agents and its credentials are deleted. Connecting it again needs a new sign-in or key.", tone: "success", }), ); @@ -442,7 +442,10 @@ describe("Connections table (M1b / PAP-13254 door 2)", () => { deleteButton?.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); - expect(document.body.textContent).toContain("Agents will lose access immediately."); + expect(document.body.textContent).toContain( + "The saved credentials are deleted and agents lose access immediately.", + ); + expect(document.body.textContent).toContain("needs a new sign-in or key"); expect(document.body.textContent).not.toContain("Agents can still use GitHub"); const confirmButton = Array.from(document.body.querySelectorAll("button")).find( @@ -455,7 +458,7 @@ describe("Connections table (M1b / PAP-13254 door 2)", () => { expect(pushToastMock).toHaveBeenCalledWith( expect.objectContaining({ - body: "GitHub is no longer available to agents. You can connect it again later.", + body: "GitHub is no longer available to agents and its credentials are deleted. Connecting it again needs a new sign-in or key.", }), ); }); diff --git a/ui/src/pages/apps/Connections.tsx b/ui/src/pages/apps/Connections.tsx index 7d2a9ac0543..2f69fadce3b 100644 --- a/ui/src/pages/apps/Connections.tsx +++ b/ui/src/pages/apps/Connections.tsx @@ -149,7 +149,7 @@ export function Connections() { title: "Connection deleted", body: target.remainingConnectionCount > 0 ? `${target.appName} still has ${target.remainingConnectionCount} active ${target.remainingConnectionCount === 1 ? "connection" : "connections"} available to agents.` - : `${target.appName} is no longer available to agents. You can connect it again later.`, + : `${target.appName} is no longer available to agents and its credentials are deleted. Connecting it again needs a new sign-in or key.`, tone: "success", }); setConnectionToDelete(null); @@ -456,8 +456,8 @@ export function Connections() { {connectionToDelete && connectionToDelete.remainingConnectionCount > 0 - ? `This connection will be removed. Agents can still use ${connectionToDelete.appName} through ${connectionToDelete.remainingConnectionCount} other active ${connectionToDelete.remainingConnectionCount === 1 ? "connection" : "connections"}.` - : "Agents will lose access immediately. You can connect it again later."} + ? `This connection's saved credentials are deleted and agents lose access through it immediately. Agents can still use ${connectionToDelete.appName} through ${connectionToDelete.remainingConnectionCount} other active ${connectionToDelete.remainingConnectionCount === 1 ? "connection" : "connections"}.` + : "The saved credentials are deleted and agents lose access immediately. Connecting it again later needs a new sign-in or key."} diff --git a/ui/src/pages/apps/app-detail/AdvancedPanel.render.test.tsx b/ui/src/pages/apps/app-detail/AdvancedPanel.render.test.tsx new file mode 100644 index 00000000000..ad99a6c0a84 --- /dev/null +++ b/ui/src/pages/apps/app-detail/AdvancedPanel.render.test.tsx @@ -0,0 +1,59 @@ +// @vitest-environment jsdom + +import { flushSync } from "react-dom"; +import { createRoot } from "react-dom/client"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { DangerZone } from "./AdvancedPanel"; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +(globalThis as any).IS_REACT_ACT_ENVIRONMENT = true; + +function act(callback: () => void) { + flushSync(callback); +} + +let container: HTMLDivElement | null = null; + +afterEach(() => { + container?.remove(); + container = null; +}); + +function renderDangerZone() { + container = document.createElement("div"); + document.body.appendChild(container); + const root = createRoot(container); + act(() => root.render()); + return container; +} + +/** + * Remove app deletes the operator's credentials and revokes agent access + * (PAP-17119). The confirmation has to say so before the operator commits — a + * "you can connect it again later" reassurance implies the pasted key survives, + * so these assertions exist to stop that copy coming back. + */ +describe("DangerZone", () => { + it("promises credential deletion and re-authentication before the operator confirms", () => { + const text = renderDangerZone().textContent ?? ""; + + expect(text).toContain("Deletes the saved credentials for PostHog"); + expect(text).toContain("takes agent access away right away"); + expect(text).toContain("needs a new sign-in or key"); + expect(text).not.toContain("You can connect it again later"); + }); + + it("keeps the warning visible in the confirming state", () => { + const node = renderDangerZone(); + const trigger = Array.from(node.querySelectorAll("button")) + .find((button) => button.textContent?.trim() === "Remove app"); + expect(trigger).toBeTruthy(); + + act(() => trigger!.dispatchEvent(new MouseEvent("click", { bubbles: true }))); + + const text = node.textContent ?? ""; + expect(text).toContain("Yes, remove it"); + expect(text).toContain("Deletes the saved credentials for PostHog"); + expect(text).toContain("needs a new sign-in or key"); + }); +}); diff --git a/ui/src/pages/apps/app-detail/AdvancedPanel.tsx b/ui/src/pages/apps/app-detail/AdvancedPanel.tsx index d7a5043ea0a..d9935710142 100644 --- a/ui/src/pages/apps/app-detail/AdvancedPanel.tsx +++ b/ui/src/pages/apps/app-detail/AdvancedPanel.tsx @@ -286,7 +286,8 @@ export function DangerZone({

Remove this app

- Agents lose access to {appName} right away. You can connect it again later. + Deletes the saved credentials for {appName} and takes agent access away right away. + Connecting it again later needs a new sign-in or key.

{confirming ? (