fix: close connection delegation security gaps

Co-Authored-By: Paperclip <noreply@paperclip.ing>

packages/db/src/connection-grants-phase4-migration.test.ts

@@ -1,31 +1,154 @@
+import { createHash, randomUUID } from "node:crypto";
 import fs from "node:fs";
 import path from "node:path";
-import { describe, expect, it } from "vitest";
+import postgres from "postgres";
+import { afterEach, describe, expect, it } from "vitest";
+import { applyPendingMigrations, inspectMigrations } from "./client.js";
+import {
+  getEmbeddedPostgresTestSupport,
+  startEmbeddedPostgresTestDatabase,
+} from "./test-embedded-postgres.js";
 
-const sql = fs.readFileSync(
-  path.join(import.meta.dirname, "migrations/0229_sticky_nightcrawler.sql"),
+const MIGRATION_FILE = "0229_sticky_nightcrawler.sql";
+const migrationSql = fs.readFileSync(
+  path.join(import.meta.dirname, "migrations", MIGRATION_FILE),
   "utf8",
 );
+const cleanups: Array<() => Promise<void>> = [];
+const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
+const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
+
+function migrationHash() {
+  return createHash("sha256").update(migrationSql).digest("hex");
+}
+
+afterEach(async () => {
+  while (cleanups.length > 0) await cleanups.pop()?.();
+});
 
 describe("connection grants phase 4 migration", () => {
-  it("adds named-agent standing delegations", () => {
-    expect(sql).toContain(`CREATE TABLE IF NOT EXISTS "connection_grant_delegations"`);
-    expect(sql).toContain(`"connection_grant_delegations_grant_agent_uq"`);
-    expect(sql).toContain(`FOREIGN KEY ("company_id","grant_id")`);
+  it("adds replay-safe named-agent standing delegations", () => {
+    expect(migrationSql).toContain(`CREATE TABLE IF NOT EXISTS "connection_grant_delegations"`);
+    expect(migrationSql).toContain(`"connection_grant_delegations_grant_agent_uq"`);
+    expect(migrationSql).toContain(`FOREIGN KEY ("company_id","grant_id")`);
+    expect(migrationSql).toContain(`DROP CONSTRAINT IF EXISTS "connection_grant_delegations_company_grant_fk"`);
+    expect(migrationSql).toContain(`CREATE INDEX IF NOT EXISTS "connection_grant_delegations_company_agent_idx"`);
+    expect(migrationSql).toContain(`ON CONFLICT DO NOTHING`);
   });
 
-  it("can replay after schema state moves ahead of the migration journal", () => {
-    expect(sql).toContain(`DROP CONSTRAINT IF EXISTS "connection_grant_delegations_company_grant_fk"`);
-    expect(sql).toContain(`CREATE INDEX IF NOT EXISTS "connection_grant_delegations_company_agent_idx"`);
-    expect(sql).toContain(`CREATE UNIQUE INDEX IF NOT EXISTS "connection_grant_delegations_grant_agent_uq"`);
-    expect(sql).toContain(`ON CONFLICT DO NOTHING`);
-  });
-
-  it("moves personal grant secrets to their owner's user scope", () => {
-    expect(sql).toContain(`g."kind" = 'user'`);
-    expect(sql).toContain(`"scope" = 'user'`);
-    expect(sql).toContain(`"owner_user_id" = owner_map."owner_user_id"`);
-    expect(sql).toContain(`"user_secret_definition_id" = d."id"`);
-    expect(sql).toContain(`HAVING count(DISTINCT g."subject_user_id") = 1`);
+  it("fails ambiguous personal-secret ownership closed with reauthorization guidance", () => {
+    expect(migrationSql).toContain(`Cannot migrate personal connection credential`);
+    expect(migrationSql).toContain(`Reauthorize the affected personal connection grants`);
+    expect(migrationSql).toContain(`count(DISTINCT g."subject_user_id") <> 1`);
+    expect(migrationSql).toContain(`other_g."kind" <> 'user'`);
+    expect(migrationSql).toContain(`jsonb_array_elements(c."credential_secret_refs")`);
   });
 });
+
+describeEmbeddedPostgres("connection grants phase 4 executable migration", () => {
+  it("converts one owner and rejects two-owner or mixed legacy references", async () => {
+    const database = await startEmbeddedPostgresTestDatabase("paperclip-connection-phase4-migration-");
+    cleanups.push(database.cleanup);
+    const sql = postgres(database.connectionString, { max: 1, onnotice: () => {} });
+
+    async function rewindMigration() {
+      await sql`DELETE FROM "drizzle"."__drizzle_migrations" WHERE "hash" = ${migrationHash()}`;
+      expect(await inspectMigrations(database.connectionString)).toMatchObject({
+        status: "needsMigrations",
+        pendingMigrations: [MIGRATION_FILE],
+      });
+    }
+
+    async function seedLegacyCredential(input: {
+      ownerUserIds: string[];
+      includeOrganizationGrant?: boolean;
+      includeConnectionReference?: boolean;
+    }) {
+      const companyId = randomUUID();
+      const applicationId = randomUUID();
+      const connectionId = randomUUID();
+      const secretId = randomUUID();
+      await sql`
+        INSERT INTO "companies" ("id", "name", "issue_prefix")
+        VALUES (${companyId}, ${`Phase 4 ${companyId}`}, ${`P${companyId.slice(0, 5)}`})
+      `;
+      await sql`
+        INSERT INTO "tool_applications" ("id", "company_id", "name", "type")
+        VALUES (${applicationId}, ${companyId}, ${`App ${applicationId}`}, 'mcp_http')
+      `;
+      await sql`
+        INSERT INTO "tool_connections" (
+          "id", "company_id", "application_id", "name", "uid", "transport", "credential_secret_refs"
+        ) VALUES (
+          ${connectionId}, ${companyId}, ${applicationId}, ${`Connection ${connectionId}`},
+          ${`connection-${connectionId}`}, 'mcp_remote',
+          ${sql.json(input.includeConnectionReference ? [{ secretId, version: "latest" }] : [])}
+        )
+      `;
+      await sql`
+        INSERT INTO "company_secrets" ("id", "company_id", "key", "name", "scope")
+        VALUES (${secretId}, ${companyId}, ${`secret-${secretId}`}, ${`Secret ${secretId}`}, 'company')
+      `;
+      for (const ownerUserId of input.ownerUserIds) {
+        await sql`
+          INSERT INTO "connection_grants" (
+            "company_id", "connection_id", "kind", "subject_user_id", "credential_secret_refs"
+          ) VALUES (
+            ${companyId}, ${connectionId}, 'user', ${ownerUserId},
+            ${sql.json([{ secretId, version: "latest" }])}
+          )
+        `;
+      }
+      if (input.includeOrganizationGrant) {
+        await sql`
+          INSERT INTO "connection_grants" (
+            "company_id", "connection_id", "kind", "credential_secret_refs"
+          ) VALUES (
+            ${companyId}, ${connectionId}, 'organization',
+            ${sql.json([{ secretId, version: "latest" }])}
+          )
+        `;
+      }
+      return { companyId, secretId, ownerUserId: input.ownerUserIds[0]! };
+    }
+
+    try {
+      const valid = await seedLegacyCredential({ ownerUserIds: ["alice"] });
+      await rewindMigration();
+      await applyPendingMigrations(database.connectionString);
+      const converted = await sql<{
+        scope: string;
+        owner_user_id: string | null;
+        user_secret_definition_id: string | null;
+      }[]>`
+        SELECT "scope", "owner_user_id", "user_secret_definition_id"
+        FROM "company_secrets"
+        WHERE "id" = ${valid.secretId}
+      `;
+      expect(converted).toEqual([{
+        scope: "user",
+        owner_user_id: valid.ownerUserId,
+        user_secret_definition_id: expect.any(String),
+      }]);
+
+      const ambiguous = await seedLegacyCredential({ ownerUserIds: ["alice", "bob"] });
+      await rewindMigration();
+      await expect(applyPendingMigrations(database.connectionString)).rejects.toThrow(
+        /legacy ownership is ambiguous/,
+      );
+
+      await sql`DELETE FROM "company_secrets" WHERE "company_id" = ${ambiguous.companyId}`;
+      await sql`DELETE FROM "companies" WHERE "id" = ${ambiguous.companyId}`;
+      await seedLegacyCredential({
+        ownerUserIds: ["carol"],
+        includeOrganizationGrant: true,
+        includeConnectionReference: true,
+      });
+      await expect(applyPendingMigrations(database.connectionString)).rejects.toThrow(
+        /legacy ownership is ambiguous/,
+      );
+    } finally {
+      await sql.end();
+    }
+  }, 45_000);
+});

packages/db/src/migrations/0229_sticky_nightcrawler.sql

@@ -19,6 +19,57 @@ ALTER TABLE "connection_grant_delegations" ADD CONSTRAINT "connection_grant_dele
 CREATE INDEX IF NOT EXISTS "connection_grant_delegations_company_agent_idx" ON "connection_grant_delegations" USING btree ("company_id","agent_id");--> statement-breakpoint
 CREATE UNIQUE INDEX IF NOT EXISTS "connection_grant_delegations_grant_agent_uq" ON "connection_grant_delegations" USING btree ("grant_id","agent_id");
 --> statement-breakpoint
+DO $$
+DECLARE
+	ambiguous_secret record;
+BEGIN
+	SELECT
+		s."id" AS "secret_id",
+		count(DISTINCT g."subject_user_id") AS "personal_owner_count",
+		count(*) FILTER (WHERE other_g."id" IS NOT NULL) AS "organization_reference_count",
+		count(*) FILTER (WHERE c."id" IS NOT NULL) AS "connection_reference_count"
+	INTO ambiguous_secret
+	FROM "company_secrets" s
+	JOIN "connection_grants" g
+		ON g."company_id" = s."company_id"
+		AND g."kind" = 'user'
+	CROSS JOIN LATERAL jsonb_array_elements(g."credential_secret_refs") personal_ref
+	LEFT JOIN "connection_grants" other_g
+		ON other_g."company_id" = s."company_id"
+		AND other_g."kind" <> 'user'
+		AND EXISTS (
+			SELECT 1
+			FROM jsonb_array_elements(other_g."credential_secret_refs") other_ref
+			WHERE other_ref ->> 'secretId' = s."id"::text
+		)
+	LEFT JOIN "tool_connections" c
+		ON c."company_id" = s."company_id"
+		AND EXISTS (
+			SELECT 1
+			FROM jsonb_array_elements(c."credential_secret_refs") connection_ref
+			WHERE connection_ref ->> 'secretId' = s."id"::text
+		)
+	WHERE s."id"::text = personal_ref ->> 'secretId'
+		AND s."scope" = 'company'
+	GROUP BY s."id"
+	HAVING count(DISTINCT g."subject_user_id") <> 1
+		OR count(*) FILTER (WHERE other_g."id" IS NOT NULL) > 0
+		OR count(*) FILTER (WHERE c."id" IS NOT NULL) > 0
+	LIMIT 1;
+
+	IF FOUND THEN
+		RAISE EXCEPTION 'Cannot migrate personal connection credential %: legacy ownership is ambiguous', ambiguous_secret."secret_id"
+			USING ERRCODE = '23514',
+				DETAIL = format(
+					'personal owners=%s, organization grant references=%s, connection references=%s',
+					ambiguous_secret."personal_owner_count",
+					ambiguous_secret."organization_reference_count",
+					ambiguous_secret."connection_reference_count"
+				),
+				HINT = 'Reauthorize the affected personal connection grants with one credential per user before retrying this migration.';
+	END IF;
+END $$;
+--> statement-breakpoint
 INSERT INTO "user_secret_definitions" (
 	"company_id", "key", "name", "description", "provider", "managed_mode",
 	"provider_config_id", "provider_metadata", "created_by_agent_id", "created_by_user_id"

server/src/__tests__/tool-access-service.test.ts

@@ -38,7 +38,7 @@ import {
   toolRuntimeSlots,
   toolStdioCommandTemplates,
 } from "@paperclipai/db";
-import { and, eq, inArray } from "drizzle-orm";
+import { and, eq, inArray, sql } from "drizzle-orm";
 import { getConnectableAppDefinition } from "@paperclipai/shared";
 import {
   getEmbeddedPostgresTestSupport,
@@ -879,6 +879,8 @@ describeEmbeddedPostgres("tool access service", () => {
   it("allows only the personal grant owner to create named-agent delegations and audits revocation", async () => {
     const company = await createCompany(db);
     const agent = await createAgent(db, company.id);
+    await grantBoardUser(db, company.id, "alice", [], "member");
+    await grantBoardUser(db, company.id, "mallory", [], "member");
     const { connection } = await createBrokerConnection(db, company.id);
     const grant = await db.insert(connectionGrants).values({
       companyId: company.id,
@@ -910,6 +912,88 @@ describeEmbeddedPostgres("tool access service", () => {
       ]));
   });
 
+  it("serializes delegation creation behind membership removal so reauthorization cannot revive stale consent", async () => {
+    const company = await createCompany(db);
+    const agent = await createAgent(db, company.id);
+    await grantBoardUser(db, company.id, "alice", [], "member");
+    const { connection } = await createBrokerConnection(db, company.id);
+    const grant = await db.insert(connectionGrants).values({
+      companyId: company.id,
+      connectionId: connection.id,
+      kind: "user",
+      subjectUserId: "alice",
+      status: "active",
+      isDefault: false,
+    }).returning().then((rows) => rows[0]!);
+    const serviceDb = createDb(tempDb!.connectionString, { maxConnections: 1 });
+    const service = createTestToolAccessService(serviceDb);
+    const removalDb = createDb(tempDb!.connectionString, { maxConnections: 1 });
+    let releaseRemoval!: () => void;
+    const removalMayCommit = new Promise<void>((resolve) => {
+      releaseRemoval = resolve;
+    });
+    let membershipLocked!: () => void;
+    const membershipIsLocked = new Promise<void>((resolve) => {
+      membershipLocked = resolve;
+    });
+
+    const removal = removalDb.transaction(async (tx) => {
+      await tx.execute(sql`
+        SELECT "id"
+        FROM "company_memberships"
+        WHERE "company_id" = ${company.id}
+          AND "principal_type" = 'user'
+          AND "principal_id" = 'alice'
+        FOR UPDATE
+      `);
+      await tx.execute(sql`
+        UPDATE "connection_grants"
+        SET "status" = 'revoked', "revoked_at" = now(), "updated_at" = now()
+        WHERE "id" = ${grant.id}
+      `);
+      await tx.execute(sql`
+        DELETE FROM "connection_grant_delegations"
+        WHERE "company_id" = ${company.id} AND "grant_id" = ${grant.id}
+      `);
+      membershipLocked();
+      await removalMayCommit;
+      await tx.execute(sql`
+        UPDATE "company_memberships"
+        SET "status" = 'suspended', "updated_at" = now()
+        WHERE "company_id" = ${company.id}
+          AND "principal_type" = 'user'
+          AND "principal_id" = 'alice'
+      `);
+    });
+
+    await membershipIsLocked;
+    let creationSettled = false;
+    const creation = service.createConnectionGrantDelegation(connection.id, grant.id, agent.id, "alice")
+      .then(
+        (value) => ({ value, error: null }),
+        (error: unknown) => ({ value: null, error }),
+      )
+      .finally(() => {
+        creationSettled = true;
+      });
+    await new Promise((resolve) => setTimeout(resolve, 50));
+    expect(creationSettled).toBe(false);
+    releaseRemoval();
+    await removal;
+    const creationResult = await creation;
+    expect(creationResult.error).toEqual(expect.objectContaining({
+      message: "Only an active company member can delegate their personal grant",
+    }));
+
+    await db.update(companyMemberships).set({ status: "active" }).where(and(
+      eq(companyMemberships.companyId, company.id),
+      eq(companyMemberships.principalId, "alice"),
+    ));
+    await db.update(connectionGrants).set({ status: "active", revokedAt: null }).where(eq(connectionGrants.id, grant.id));
+    expect(await db.select().from(connectionGrantDelegations).where(eq(connectionGrantDelegations.grantId, grant.id)))
+      .toHaveLength(0);
+  });
+
   it("fails autonomous token minting closed until the named agent has a standing delegation", async () => {
     const company = await createCompany(db);
     const agent = await createAgent(db, company.id);

server/src/services/tool-access.ts

@@ -8219,42 +8219,62 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {}
       ownerUserId: string,
     ) => {
       const connection = await getConnectionRow(idOrUid);
-      const [grant] = await db.select().from(connectionGrants).where(and(
-        eq(connectionGrants.id, grantId),
-        eq(connectionGrants.companyId, connection.companyId),
-        eq(connectionGrants.connectionId, connection.id),
-        eq(connectionGrants.kind, "user"),
-        eq(connectionGrants.subjectUserId, ownerUserId),
-        eq(connectionGrants.status, "active"),
-      )).limit(1);
-      if (!grant) throw forbidden("Only the active personal grant owner can create a delegation");
-      const [targetAgent] = await db.select({ id: agents.id }).from(agents).where(and(
-        eq(agents.id, agentId),
-        eq(agents.companyId, connection.companyId),
-      )).limit(1);
-      if (!targetAgent) throw notFound("Agent not found");
-      const [existing] = await db.select().from(connectionGrantDelegations).where(and(
-        eq(connectionGrantDelegations.grantId, grant.id),
-        eq(connectionGrantDelegations.agentId, agentId),
-      )).limit(1);
-      if (existing) return existing;
-      const [delegation] = await db.insert(connectionGrantDelegations).values({
-        companyId: connection.companyId,
-        grantId: grant.id,
-        agentId,
-        createdByUserId: ownerUserId,
-      }).returning();
-      await db.insert(toolAccessAuditEvents).values({
-        companyId: connection.companyId,
-        connectionId: connection.id,
-        actorType: "user",
-        actorId: ownerUserId,
-        action: "connection_grant.delegated",
-        outcome: "success",
-        reasonCode: "delegation_created",
-        details: { grantId: grant.id, delegationId: delegation!.id, agentId },
+      return db.transaction(async (tx) => {
+        // Membership removal/suspension takes this same row lock before sweeping
+        // personal grants. Whichever operation wins is therefore authoritative:
+        // removal deletes a delegation committed first, while creation that runs
+        // second observes the inactive membership and fails closed.
+        const [membership] = await tx.select({ id: companyMemberships.id })
+          .from(companyMemberships)
+          .where(and(
+            eq(companyMemberships.companyId, connection.companyId),
+            eq(companyMemberships.principalType, "user"),
+            eq(companyMemberships.principalId, ownerUserId),
+            eq(companyMemberships.status, "active"),
+          ))
+          .for("update")
+          .limit(1);
+        if (!membership) {
+          throw forbidden("Only an active company member can delegate their personal grant");
+        }
+
+        const [grant] = await tx.select().from(connectionGrants).where(and(
+          eq(connectionGrants.id, grantId),
+          eq(connectionGrants.companyId, connection.companyId),
+          eq(connectionGrants.connectionId, connection.id),
+          eq(connectionGrants.kind, "user"),
+          eq(connectionGrants.subjectUserId, ownerUserId),
+          eq(connectionGrants.status, "active"),
+        )).limit(1);
+        if (!grant) throw forbidden("Only the active personal grant owner can create a delegation");
+        const [targetAgent] = await tx.select({ id: agents.id }).from(agents).where(and(
+          eq(agents.id, agentId),
+          eq(agents.companyId, connection.companyId),
+        )).limit(1);
+        if (!targetAgent) throw notFound("Agent not found");
+        const [existing] = await tx.select().from(connectionGrantDelegations).where(and(
+          eq(connectionGrantDelegations.grantId, grant.id),
+          eq(connectionGrantDelegations.agentId, agentId),
+        )).limit(1);
+        if (existing) return existing;
+        const [delegation] = await tx.insert(connectionGrantDelegations).values({
+          companyId: connection.companyId,
+          grantId: grant.id,
+          agentId,
+          createdByUserId: ownerUserId,
+        }).returning();
+        await tx.insert(toolAccessAuditEvents).values({
+          companyId: connection.companyId,
+          connectionId: connection.id,
+          actorType: "user",
+          actorId: ownerUserId,
+          action: "connection_grant.delegated",
+          outcome: "success",
+          reasonCode: "delegation_created",
+          details: { grantId: grant.id, delegationId: delegation!.id, agentId },
+        });
+        return delegation!;
       });
-      return delegation!;
     },
 
     revokeConnectionGrantDelegation: async (idOrUid: string, grantId: string, delegationId: string, actor?: ActorInfo) => {