fix(credentials): write an env value and its credential row together

Every writer of a workspace or personal environment map read-modify-writes a
single jsonb column, so they take an advisory lock on the map to serialize.
`deleteCredentialRecord` took none, and did the read, the edit and the
write-back outside a transaction: a secret written concurrently was read
before that write and silently dropped by the write-back.

The credential row was also written after its map transaction committed, in
four places. The delete direction left a row describing a value that was gone;
the create direction was worse than a stale row, because it cannot be repaired
by retrying — the key is in the map by then, so the next attempt reads it as
pre-existing, computes an empty `newKeys`, and never creates the row. Both
helpers already accept `executor`, and `setWorkspaceSecret` has been passing
the transaction since the parameter landed; these four were never migrated.

The personal reconcile stays outside its transaction: it opens its own and
takes the user-identity fence, so nesting it would have two transactions
taking two locks in opposite orders. It reconciles against the stored keys,
so a failure there is repaired by the next one rather than entrenched.

Also folds the four copies of the lock into one helper, since this would have
been the fifth.

Author: waleedlatif1

apps/sim/app/api/workspaces/[id]/environment/route.ts

@@ -4,7 +4,7 @@ import { workspaceEnvironment } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
-import { eq, sql } from 'drizzle-orm'
+import { eq } from 'drizzle-orm'
 import { type NextRequest, NextResponse } from 'next/server'
 import {
   removeWorkspaceEnvironmentContract,
@@ -15,6 +15,7 @@ import { getSession } from '@/lib/auth'
 import { encryptSecret } from '@/lib/core/security/encryption'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
+import { lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
 import {
   createWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials,
@@ -39,8 +40,6 @@ const logger = createLogger('WorkspaceEnvironmentAPI')
  * fast (SQLSTATE 55P03) rather than hanging, even if the deployment lacks a
  * server-side `lock_timeout`. Transaction-scoped via `set_config(..., true)`.
  */
-const WORKSPACE_ENV_LOCK_TIMEOUT_MS = 5_000
-
 /**
  * Restricts decrypted workspace env values to administrators. Members (including
  * read-only) receive the variable names with empty values so editor autocomplete
@@ -237,11 +236,8 @@ export const PUT = withRouteHandler(
         })
       ).then((entries) => Object.fromEntries(entries))
 
-      const { existingEncrypted, merged } = await db.transaction(async (tx) => {
-        await tx.execute(
-          sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)`
-        )
-        await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${workspaceId}, 0))`)
+      const { merged } = await db.transaction(async (tx) => {
+        await lockWorkspaceEnvMap(tx, workspaceId)
 
         const [existingRow] = await tx
           .select()
@@ -269,12 +265,24 @@ export const PUT = withRouteHandler(
             set: { variables: mergedVars, updatedAt: new Date() },
           })
 
-        return { existingEncrypted: existing, merged: mergedVars }
+        /**
+         * Inside the transaction because a value committed without its
+         * credential row cannot be repaired by retrying: the key is in the map
+         * by then, so the next attempt reads it as pre-existing, computes an
+         * empty `newKeys`, and never creates the row.
+         */
+        const newKeys = Object.keys(variables).filter((k) => !(k in existing))
+        await createWorkspaceEnvCredentials({
+          workspaceId,
+          newKeys,
+          actingUserId: userId,
+          executor: tx,
+        })
+
+        return { merged: mergedVars }
       })
 
       invalidateEffectiveDecryptedEnvCache({ workspaceId })
-      const newKeys = Object.keys(variables).filter((k) => !(k in existingEncrypted))
-      await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId: userId })
 
       recordAudit({
         workspaceId,
@@ -370,10 +378,7 @@ export const DELETE = withRouteHandler(
       }
 
       const result = await db.transaction(async (tx) => {
-        await tx.execute(
-          sql`SELECT set_config('lock_timeout', ${`${WORKSPACE_ENV_LOCK_TIMEOUT_MS}ms`}, true)`
-        )
-        await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${workspaceId}, 0))`)
+        await lockWorkspaceEnvMap(tx, workspaceId)
 
         const [existingRow] = await tx
           .select()
@@ -400,6 +405,12 @@ export const DELETE = withRouteHandler(
           .set({ variables: current, updatedAt: new Date() })
           .where(eq(workspaceEnvironment.workspaceId, workspaceId))
 
+        await deleteWorkspaceEnvCredentials({
+          workspaceId,
+          removedKeys: keys,
+          executor: tx,
+        })
+
         return { remainingKeysCount: Object.keys(current).length }
       })
 
@@ -408,7 +419,6 @@ export const DELETE = withRouteHandler(
       }
 
       invalidateEffectiveDecryptedEnvCache({ workspaceId })
-      await deleteWorkspaceEnvCredentials({ workspaceId, removedKeys: keys })
 
       recordAudit({
         workspaceId,

apps/sim/lib/credentials/env-locks.ts

@@ -0,0 +1,32 @@
+import { sql } from 'drizzle-orm'
+import type { DbOrTx } from '@/lib/db/types'
+
+const ENV_MAP_LOCK_TIMEOUT_MS = 5_000
+
+/**
+ * Serializes every writer of one environment variables map.
+ *
+ * Both maps are a single jsonb column that every writer read-modify-writes, so
+ * without this two concurrent writers each persist their own copy of the map
+ * and the later commit silently drops the earlier one's key. The lock is
+ * transaction-scoped, so it releases on commit or rollback with no unlock path
+ * to miss, and it must be taken before the read that the write is derived from.
+ *
+ * The keys are the bare workspace or user id, matching every writer that
+ * already takes this lock — a prefixed key would be a different lock and would
+ * serialize against nothing.
+ */
+async function lockEnvMap(tx: DbOrTx, lockKey: string): Promise<void> {
+  await tx.execute(sql`SELECT set_config('lock_timeout', ${`${ENV_MAP_LOCK_TIMEOUT_MS}ms`}, true)`)
+  await tx.execute(sql`SELECT pg_advisory_xact_lock(hashtextextended(${lockKey}, 0))`)
+}
+
+/** Serializes writers of one workspace's environment variables map. */
+export async function lockWorkspaceEnvMap(tx: DbOrTx, workspaceId: string): Promise<void> {
+  await lockEnvMap(tx, workspaceId)
+}
+
+/** Serializes writers of one user's personal environment variables map. */
+export async function lockPersonalEnvMap(tx: DbOrTx, userId: string): Promise<void> {
+  await lockEnvMap(tx, userId)
+}

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

@@ -20,6 +20,8 @@ const {
   mockGetClientCredentialAccountDescriptor,
   mockDeleteConnectionCredential,
   mockDeleteOrphanedOAuthAccount,
+  mockDeleteWorkspaceEnvCredentials,
+  mockSyncPersonalEnvCredentialsForUser,
 } = vi.hoisted(() => ({
   mockRecordAudit: vi.fn(),
   mockGetCredentialActorContext: vi.fn(),
@@ -31,6 +33,8 @@ const {
   mockGetClientCredentialAccountDescriptor: vi.fn(() => undefined),
   mockDeleteConnectionCredential: vi.fn(),
   mockDeleteOrphanedOAuthAccount: vi.fn(),
+  mockDeleteWorkspaceEnvCredentials: vi.fn(),
+  mockSyncPersonalEnvCredentialsForUser: vi.fn(),
 }))
 
 vi.mock('@sim/audit', () => ({
@@ -57,8 +61,8 @@ vi.mock('@/lib/credentials/deletion', () => ({
   deleteOrphanedOAuthAccount: mockDeleteOrphanedOAuthAccount,
 }))
 vi.mock('@/lib/credentials/environment', () => ({
-  deleteWorkspaceEnvCredentials: vi.fn(),
-  syncPersonalEnvCredentialsForUser: vi.fn(),
+  deleteWorkspaceEnvCredentials: mockDeleteWorkspaceEnvCredentials,
+  syncPersonalEnvCredentialsForUser: mockSyncPersonalEnvCredentialsForUser,
 }))
 vi.mock('@/lib/credentials/atlassian-service-account', () => ({
   AtlassianValidationError: class AtlassianValidationError extends Error {},
@@ -713,6 +717,61 @@ describe('deleteCredentialRecord', () => {
     expect(mockDeleteConnectionCredential).not.toHaveBeenCalled()
   })
 
+  /**
+   * The whole variables map is read, edited and written back here, so a
+   * concurrent secret write is lost unless this holds the same advisory lock
+   * every other writer of that map takes.
+   */
+  it('removes a workspace env value under the map lock, with the row', async () => {
+    await deleteCredentialRecord({
+      credential: {
+        id: 'cred-1',
+        workspaceId: 'ws-1',
+        type: 'env_workspace',
+        envKey: 'STRIPE_API_KEY',
+        providerId: null,
+      } as never,
+      reason: 'user_delete',
+    })
+
+    expect(dbChainMockFns.transaction).toHaveBeenCalled()
+    const locked = dbChainMockFns.execute.mock.calls.some(([statement]) => {
+      const { sql, params } = (
+        statement as { toSQL: () => { sql: string; params: unknown[] } }
+      ).toSQL()
+      return sql.includes('pg_advisory_xact_lock') && params.includes('ws-1')
+    })
+    expect(locked).toBe(true)
+    // Passed the transaction, so the row cannot outlive the value it describes.
+    expect(mockDeleteWorkspaceEnvCredentials).toHaveBeenCalledWith(
+      expect.objectContaining({ workspaceId: 'ws-1', removedKeys: ['STRIPE_API_KEY'] })
+    )
+    expect(mockDeleteWorkspaceEnvCredentials.mock.calls[0][0].executor).toBeDefined()
+  })
+
+  it('removes a personal env value under the map lock', async () => {
+    await deleteCredentialRecord({
+      credential: {
+        id: 'cred-1',
+        workspaceId: 'ws-1',
+        type: 'env_personal',
+        envKey: 'MY_KEY',
+        envOwnerUserId: 'user-1',
+        providerId: null,
+      } as never,
+      reason: 'user_delete',
+    })
+
+    expect(dbChainMockFns.transaction).toHaveBeenCalled()
+    const locked = dbChainMockFns.execute.mock.calls.some(([statement]) => {
+      const { sql, params } = (
+        statement as { toSQL: () => { sql: string; params: unknown[] } }
+      ).toSQL()
+      return sql.includes('pg_advisory_xact_lock') && params.includes('user-1')
+    })
+    expect(locked).toBe(true)
+  })
+
   it('revokes the backing OAuth grant of a deleted oauth credential', async () => {
     mockDeleteConnectionCredential.mockResolvedValueOnce(true)
 

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

@@ -30,6 +30,7 @@ import {
   deleteOrphanedOAuthAccount,
 } from '@/lib/credentials/deletion'
 import { slackCustomBotDisplayName } from '@/lib/credentials/display-name'
+import { lockPersonalEnvMap, lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
 import {
   deleteWorkspaceEnvCredentials,
   syncPersonalEnvCredentialsForUser,
@@ -565,25 +566,39 @@ export async function deleteCredentialRecord(
     if (!credentialRow.envKey || !credentialRow.envOwnerUserId) {
       throw new Error('Personal environment credential is missing its source identity')
     }
-    const [personalRow] = await db
-      .select({ variables: environment.variables })
-      .from(environment)
-      .where(eq(environment.userId, credentialRow.envOwnerUserId))
-      .limit(1)
-    const current = { ...((personalRow?.variables as Record<string, string> | null) ?? {}) }
-    delete current[credentialRow.envKey]
-    await db
-      .insert(environment)
-      .values({
-        id: credentialRow.envOwnerUserId,
-        userId: credentialRow.envOwnerUserId,
-        variables: current,
-        updatedAt: new Date(),
-      })
-      .onConflictDoUpdate({
-        target: [environment.userId],
-        set: { variables: current, updatedAt: new Date() },
-      })
+    const { envKey, envOwnerUserId } = credentialRow
+    /**
+     * Same read-modify-write on the personal map, under the same lock its
+     * other writers take. The credential reconcile stays outside: it opens its
+     * own transaction and takes the user-identity fence, so nesting it here
+     * would have two transactions taking two locks in opposite orders. It is a
+     * reconcile against the stored keys, so a failure is repaired by the next
+     * one rather than entrenched.
+     */
+    const current = await db.transaction(async (tx) => {
+      await lockPersonalEnvMap(tx, envOwnerUserId)
+
+      const [personalRow] = await tx
+        .select({ variables: environment.variables })
+        .from(environment)
+        .where(eq(environment.userId, envOwnerUserId))
+        .limit(1)
+      const variables = { ...((personalRow?.variables as Record<string, string> | null) ?? {}) }
+      delete variables[envKey]
+      await tx
+        .insert(environment)
+        .values({
+          id: envOwnerUserId,
+          userId: envOwnerUserId,
+          variables,
+          updatedAt: new Date(),
+        })
+        .onConflictDoUpdate({
+          target: [environment.userId],
+          set: { variables, updatedAt: new Date() },
+        })
+      return variables
+    })
     await syncPersonalEnvCredentialsForUser({
       userId: credentialRow.envOwnerUserId,
       envKeys: Object.keys(current),
@@ -595,33 +610,46 @@ export async function deleteCredentialRecord(
     if (!credentialRow.envKey) {
       throw new Error('Workspace environment credential is missing its source identity')
     }
-    const [workspaceRow] = await db
-      .select({
-        id: workspaceEnvironment.id,
-        createdAt: workspaceEnvironment.createdAt,
-        variables: workspaceEnvironment.variables,
-      })
-      .from(workspaceEnvironment)
-      .where(eq(workspaceEnvironment.workspaceId, credentialRow.workspaceId))
-      .limit(1)
-    const current = { ...((workspaceRow?.variables as Record<string, string> | null) ?? {}) }
-    delete current[credentialRow.envKey]
-    await db
-      .insert(workspaceEnvironment)
-      .values({
-        id: workspaceRow?.id ?? generateId(),
-        workspaceId: credentialRow.workspaceId,
-        variables: current,
-        createdAt: workspaceRow?.createdAt ?? new Date(),
-        updatedAt: new Date(),
-      })
-      .onConflictDoUpdate({
-        target: [workspaceEnvironment.workspaceId],
-        set: { variables: current, updatedAt: new Date() },
+    const { envKey, workspaceId } = credentialRow
+    /**
+     * The whole variables map is read, edited and written back, so this has to
+     * hold the same lock every other writer of that map takes — without it a
+     * secret written concurrently is read before the write and dropped by this
+     * write-back. The credential row goes in the same transaction so the row
+     * and the value it describes cannot outlive each other.
+     */
+    await db.transaction(async (tx) => {
+      await lockWorkspaceEnvMap(tx, workspaceId)
+
+      const [workspaceRow] = await tx
+        .select({
+          id: workspaceEnvironment.id,
+          createdAt: workspaceEnvironment.createdAt,
+          variables: workspaceEnvironment.variables,
+        })
+        .from(workspaceEnvironment)
+        .where(eq(workspaceEnvironment.workspaceId, workspaceId))
+        .limit(1)
+      const current = { ...((workspaceRow?.variables as Record<string, string> | null) ?? {}) }
+      delete current[envKey]
+      await tx
+        .insert(workspaceEnvironment)
+        .values({
+          id: workspaceRow?.id ?? generateId(),
+          workspaceId,
+          variables: current,
+          createdAt: workspaceRow?.createdAt ?? new Date(),
+          updatedAt: new Date(),
+        })
+        .onConflictDoUpdate({
+          target: [workspaceEnvironment.workspaceId],
+          set: { variables: current, updatedAt: new Date() },
+        })
+      await deleteWorkspaceEnvCredentials({
+        workspaceId,
+        removedKeys: [envKey],
+        executor: tx,
       })
-    await deleteWorkspaceEnvCredentials({
-      workspaceId: credentialRow.workspaceId,
-      removedKeys: [credentialRow.envKey],
     })
     return true
   }