fix(credentials): clean up personal secrets in archived workspaces

Author: icecrasher321

apps/sim/app/api/environment/route.ts

@@ -60,12 +60,8 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
      * persists a map derived from the pre-replace state, discarding this one
      * entirely.
      *
-     * The reconcile below stays outside because it opens its own transaction.
-     * That leaves a known gap: it prunes mirrors against this request's key
-     * list, so a secret added after the commit loses its mirror while its
-     * value survives. Closing it means having the reconcile read the map
-     * itself rather than trust a caller's list, across all four of its
-     * callers.
+     * The reconcile below opens its own transaction and re-reads the map
+     * under this same lock so a later save cannot be undone by stale keys.
      */
     await db.transaction(async (tx) => {
       await lockPersonalEnvMap(tx, session.user.id)
@@ -89,7 +85,6 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
 
     await syncPersonalEnvCredentialsForUser({
       userId: session.user.id,
-      envKeys: Object.keys(variables),
     })
 
     recordAudit({

apps/sim/lib/credentials/environment.test.ts

@@ -1,20 +1,31 @@
 /**
  * @vitest-environment node
  */
-import { credential, permissions, workspace } from '@sim/db/schema'
-import { dbChainMock, dbChainMockFns, queueTableRows, resetDbChainMock } from '@sim/testing'
+import { credential, environment, permissions, workspace } from '@sim/db/schema'
+import {
+  dbChainMock,
+  dbChainMockFns,
+  flattenMockConditions,
+  queueTableRows,
+  resetDbChainMock,
+} from '@sim/testing'
 import { eq } from 'drizzle-orm'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 import type { DbOrTx } from '@/lib/db/types'
 
-const { mockAcquireUserBillingIdentityLock } = vi.hoisted(() => ({
+const { mockAcquireUserBillingIdentityLock, mockLockPersonalEnvMap } = vi.hoisted(() => ({
   mockAcquireUserBillingIdentityLock: vi.fn(),
+  mockLockPersonalEnvMap: vi.fn(),
 }))
 
 vi.mock('@/lib/billing/organizations/billing-identity-lock', () => ({
   acquireUserBillingIdentityLock: mockAcquireUserBillingIdentityLock,
 }))
 
+vi.mock('@/lib/credentials/env-locks', () => ({
+  lockPersonalEnvMap: mockLockPersonalEnvMap,
+}))
+
 import {
   createWorkspaceEnvCredentials,
   getEnrolledManagedOAuthCredentials,
@@ -196,30 +207,35 @@ describe('syncPersonalEnvCredentialsForUser', () => {
     vi.clearAllMocks()
     resetDbChainMock()
     mockAcquireUserBillingIdentityLock.mockResolvedValue(undefined)
+    mockLockPersonalEnvMap.mockResolvedValue(undefined)
   })
 
-  it('uses one transaction and acquires the transfer fence before discovering workspaces', async () => {
+  it('locks the map before the transfer fence and reads current keys before reconciling', async () => {
     const base = dbChainMock.db
     const tx = {
       select: vi.fn(base.select),
       insert: vi.fn(base.insert),
       delete: vi.fn(base.delete),
     } as unknown as DbOrTx
     dbChainMockFns.transaction.mockImplementationOnce(async (callback) => callback(tx))
+    queueTableRows(environment, [{ variables: { API_KEY: 'encrypted' } }])
     queueTableRows(permissions, [{ workspaceId: 'ws-1' }])
     queueTableRows(workspace, [])
     queueTableRows(credential, [{ id: 'credential-1' }])
 
     await syncPersonalEnvCredentialsForUser({
       userId: 'user-1',
-      envKeys: ['API_KEY'],
     })
 
+    expect(mockLockPersonalEnvMap).toHaveBeenCalledWith(tx, 'user-1')
+    expect(mockLockPersonalEnvMap.mock.invocationCallOrder[0]).toBeLessThan(
+      mockAcquireUserBillingIdentityLock.mock.invocationCallOrder[0]
+    )
     expect(mockAcquireUserBillingIdentityLock).toHaveBeenCalledWith(tx, 'user-1')
     expect(mockAcquireUserBillingIdentityLock.mock.invocationCallOrder[0]).toBeLessThan(
       (tx.select as ReturnType<typeof vi.fn>).mock.invocationCallOrder[0]
     )
-    expect(tx.select).toHaveBeenCalledTimes(3)
+    expect(tx.select).toHaveBeenCalledTimes(4)
     expect(tx.insert).toHaveBeenCalledTimes(2)
     expect(tx.delete).toHaveBeenCalledTimes(1)
   })
@@ -232,19 +248,19 @@ describe('syncPersonalEnvCredentialsForUser', () => {
       delete: vi.fn(base.delete),
     } as unknown as DbOrTx
     dbChainMockFns.transaction.mockImplementationOnce(async (callback) => callback(tx))
+    queueTableRows(environment, [{ variables: { API_KEY: 'encrypted' } }])
     queueTableRows(permissions, [])
     queueTableRows(workspace, [])
 
     await syncPersonalEnvCredentialsForUser({
       userId: 'user-1',
-      envKeys: ['API_KEY'],
     })
 
     expect(mockAcquireUserBillingIdentityLock.mock.invocationCallOrder[0]).toBeLessThan(
       (tx.select as ReturnType<typeof vi.fn>).mock.invocationCallOrder[0]
     )
     expect(tx.insert).not.toHaveBeenCalled()
-    expect(tx.delete).not.toHaveBeenCalled()
+    expect(tx.delete).toHaveBeenCalledTimes(1)
   })
 
   it('syncs every workspace with one credential insert, lookup, membership insert, and cleanup', async () => {
@@ -255,23 +271,74 @@ describe('syncPersonalEnvCredentialsForUser', () => {
       delete: vi.fn(base.delete),
     } as unknown as DbOrTx
     dbChainMockFns.transaction.mockImplementationOnce(async (callback) => callback(tx))
+    queueTableRows(environment, [{ variables: { API_KEY: 'encrypted' } }])
     queueTableRows(permissions, [{ workspaceId: 'ws-2' }, { workspaceId: 'ws-1' }])
     queueTableRows(workspace, [])
     queueTableRows(credential, [{ id: 'credential-1' }, { id: 'credential-2' }])
 
     await syncPersonalEnvCredentialsForUser({
       userId: 'user-1',
-      envKeys: ['API_KEY'],
     })
 
-    expect(tx.select).toHaveBeenCalledTimes(3)
+    expect(tx.select).toHaveBeenCalledTimes(4)
     expect(tx.insert).toHaveBeenCalledTimes(2)
     expect(tx.delete).toHaveBeenCalledTimes(1)
     expect(dbChainMockFns.values).toHaveBeenNthCalledWith(1, [
       expect.objectContaining({ workspaceId: 'ws-1', envKey: 'API_KEY' }),
       expect.objectContaining({ workspaceId: 'ws-2', envKey: 'API_KEY' }),
     ])
   })
+
+  it.each([
+    { label: 'missing', rows: [] },
+    { label: 'empty', rows: [{ variables: {} }] },
+  ])(
+    'cleans archived-workspace mirrors with a $label map and no active workspaces',
+    async ({ rows }) => {
+      queueTableRows(environment, rows)
+      const deleteWhere = vi.fn().mockResolvedValue([])
+      dbChainMock.db.delete.mockReturnValue({ where: deleteWhere })
+
+      await syncPersonalEnvCredentialsForUser({ userId: 'user-1' })
+
+      expect(dbChainMock.db.delete).toHaveBeenCalledWith(credential)
+      expect(flattenMockConditions(deleteWhere.mock.calls[0][0])).toEqual([
+        { type: 'eq', left: credential.type, right: 'env_personal' },
+        { type: 'eq', left: credential.envOwnerUserId, right: 'user-1' },
+      ])
+      expect(dbChainMockFns.insert).not.toHaveBeenCalled()
+    }
+  )
+
+  it.each([
+    { label: 'no active workspaces', activeWorkspaces: [] },
+    { label: 'an active workspace', activeWorkspaces: [{ workspaceId: 'active-workspace' }] },
+  ])(
+    'prunes deleted keys across archived workspaces with $label, preserving current keys and owners',
+    async ({ activeWorkspaces }) => {
+      queueTableRows(environment, [{ variables: { KEEP: 'encrypted', NEW: 'encrypted-new' } }])
+      queueTableRows(permissions, activeWorkspaces)
+      queueTableRows(workspace, [])
+      const deleteWhere = vi.fn().mockResolvedValue([])
+      dbChainMock.db.delete.mockReturnValue({ where: deleteWhere })
+
+      await syncPersonalEnvCredentialsForUser({ userId: 'user-1' })
+
+      expect(flattenMockConditions(deleteWhere.mock.calls[0][0])).toEqual([
+        { type: 'eq', left: credential.type, right: 'env_personal' },
+        { type: 'eq', left: credential.envOwnerUserId, right: 'user-1' },
+        { type: 'notInArray', column: credential.envKey, values: ['KEEP', 'NEW'] },
+      ])
+      if (activeWorkspaces.length === 0) {
+        expect(dbChainMockFns.insert).not.toHaveBeenCalled()
+      } else {
+        expect(dbChainMockFns.values).toHaveBeenCalledWith([
+          expect.objectContaining({ workspaceId: 'active-workspace', envKey: 'KEEP' }),
+          expect.objectContaining({ workspaceId: 'active-workspace', envKey: 'NEW' }),
+        ])
+      }
+    }
+  )
 })
 
 describe('createWorkspaceEnvCredentials', () => {

apps/sim/lib/credentials/environment.ts

@@ -4,6 +4,7 @@ import {
   credentialGroup,
   credentialGroupEnrollment,
   credentialMember,
+  environment,
   permissions,
   user,
   workspace,
@@ -15,6 +16,7 @@ import { generateId } from '@sim/utils/id'
 import { and, asc, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm'
 import { acquireUserBillingIdentityLock } from '@/lib/billing/organizations/billing-identity-lock'
 import { isManagedCredentialGroupBindingLive } from '@/lib/credential-groups/credentials'
+import { lockPersonalEnvMap } from '@/lib/credentials/env-locks'
 import type { DbOrTx } from '@/lib/db/types'
 import {
   getEffectiveWorkspacePermission,
@@ -677,101 +679,95 @@ export async function deletePersonalEnvCredentialForUser(params: {
   await db.transaction(remove)
 }
 
-export async function syncPersonalEnvCredentialsForUser(params: {
-  userId: string
-  envKeys: string[]
-}): Promise<void> {
-  const { userId, envKeys } = params
-  const normalizedKeys = Array.from(new Set(envKeys.filter(Boolean)))
+/** Reconciles user-global secret deletions and active-workspace mirrors against the locked map. */
+export async function syncPersonalEnvCredentialsForUser(params: { userId: string }): Promise<void> {
+  const { userId } = params
   const now = new Date()
 
   await db.transaction(async (tx) => {
+    await lockPersonalEnvMap(tx, userId)
     /**
      * Cross-organization transfer takes this same user-identity fence before
      * checking source-owned credentials. If this sync wins, transfer observes
      * the new env_personal rows and blocks; if transfer wins, this post-lock
      * workspace re-read cannot recreate credentials in the departed org.
      */
     await acquireUserBillingIdentityLock(tx, userId)
-    const workspaceIds = (await getUserWorkspaceIds(userId, tx)).sort()
-
-    if (workspaceIds.length === 0) return
-
-    if (normalizedKeys.length > 0) {
-      const credentialValues = workspaceIds.flatMap((workspaceId) =>
-        normalizedKeys.map((envKey) => ({
-          id: generateId(),
-          workspaceId,
-          type: 'env_personal' as const,
-          displayName: envKey,
-          envKey,
-          envOwnerUserId: userId,
-          createdBy: userId,
-          createdAt: now,
-          updatedAt: now,
-        }))
+    const [personalEnvironment] = await tx
+      .select({ variables: environment.variables })
+      .from(environment)
+      .where(eq(environment.userId, userId))
+      .limit(1)
+    const envKeys = Object.keys(personalEnvironment?.variables ?? {}).filter(Boolean)
+
+    /** Deleted keys must lose mirrors even in archived or no-longer-accessible workspaces. */
+    await tx
+      .delete(credential)
+      .where(
+        and(
+          eq(credential.type, 'env_personal'),
+          eq(credential.envOwnerUserId, userId),
+          envKeys.length > 0 ? notInArray(credential.envKey, envKeys) : undefined
+        )
       )
-      for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
-        await tx.insert(credential).values(values).onConflictDoNothing()
-      }
 
-      const currentCredentials = await tx
-        .select({ id: credential.id })
-        .from(credential)
-        .where(
-          and(
-            inArray(credential.workspaceId, workspaceIds),
-            eq(credential.type, 'env_personal'),
-            eq(credential.envOwnerUserId, userId),
-            inArray(credential.envKey, normalizedKeys)
-          )
-        )
+    if (envKeys.length === 0) return
 
-      if (currentCredentials.length > 0) {
-        const membershipValues = currentCredentials.map(({ id: credentialId }) => ({
-          id: generateId(),
-          credentialId,
-          userId,
-          role: 'admin' as const,
-          status: 'active' as const,
-          joinedAt: now,
-          invitedBy: userId,
-          createdAt: now,
-          updatedAt: now,
-        }))
-        for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
-          await tx
-            .insert(credentialMember)
-            .values(values)
-            .onConflictDoUpdate({
-              target: [credentialMember.credentialId, credentialMember.userId],
-              set: { role: 'admin', status: 'active', updatedAt: now },
-            })
-        }
-      }
+    const workspaceIds = (await getUserWorkspaceIds(userId, tx)).sort()
 
-      await tx
-        .delete(credential)
-        .where(
-          and(
-            inArray(credential.workspaceId, workspaceIds),
-            eq(credential.type, 'env_personal'),
-            eq(credential.envOwnerUserId, userId),
-            notInArray(credential.envKey, normalizedKeys)
-          )
-        )
-      return
+    if (workspaceIds.length === 0) return
+
+    const credentialValues = workspaceIds.flatMap((workspaceId) =>
+      envKeys.map((envKey) => ({
+        id: generateId(),
+        workspaceId,
+        type: 'env_personal' as const,
+        displayName: envKey,
+        envKey,
+        envOwnerUserId: userId,
+        createdBy: userId,
+        createdAt: now,
+        updatedAt: now,
+      }))
+    )
+    for (const values of chunkArray(credentialValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+      await tx.insert(credential).values(values).onConflictDoNothing()
     }
 
-    await tx
-      .delete(credential)
+    const currentCredentials = await tx
+      .select({ id: credential.id })
+      .from(credential)
       .where(
         and(
           inArray(credential.workspaceId, workspaceIds),
           eq(credential.type, 'env_personal'),
-          eq(credential.envOwnerUserId, userId)
+          eq(credential.envOwnerUserId, userId),
+          inArray(credential.envKey, envKeys)
         )
       )
+
+    if (currentCredentials.length > 0) {
+      const membershipValues = currentCredentials.map(({ id: credentialId }) => ({
+        id: generateId(),
+        credentialId,
+        userId,
+        role: 'admin' as const,
+        status: 'active' as const,
+        joinedAt: now,
+        invitedBy: userId,
+        createdAt: now,
+        updatedAt: now,
+      }))
+      for (const values of chunkArray(membershipValues, ENV_CREDENTIAL_WRITE_CHUNK_SIZE)) {
+        await tx
+          .insert(credentialMember)
+          .values(values)
+          .onConflictDoUpdate({
+            target: [credentialMember.credentialId, credentialMember.userId],
+            set: { role: 'admin', status: 'active', updatedAt: now },
+          })
+      }
+    }
   })
 }
 

apps/sim/lib/credentials/orchestration/index.ts

@@ -602,11 +602,8 @@ export async function deleteCredentialRecord(
      * Same read-modify-write on the personal map, under the same lock its
      * other writers take, with the mirrors removed in the same transaction.
      *
-     * Targeted rather than a reconcile: the reconcile prunes every mirror
-     * absent from a caller-supplied key list, so a secret added between the
-     * read and the prune lost its mirror while its value survived. Deleting
-     * this one key's mirrors cannot strand another secret, and the lock order
-     * — map, then user identity — is the one `setPersonalSecret` already takes.
+     * Delete only this key's mirrors across every workspace. The lock order
+     * — map, then user identity — matches `setPersonalSecret` and bulk sync.
      */
     await db.transaction(async (tx) => {
       await lockPersonalEnvMap(tx, envOwnerUserId)