fix(secrets): make env writes and their authorization one transaction

Two rounds of ordering fixes each produced the next finding, because the
value upsert, the credential inserts, and the visibility flip were three
separate commits with the disclosure check somewhere among them. Moving
the check only changed which writes were stranded by a denial.

The PUT now runs authorization and every write it guards in one
transaction, so a denial rolls all of it back and there is no ordering
left to get wrong. createWorkspaceEnvCredentials takes an executor to
join it. upsertWorkspaceEnvVars had the same shape on the copilot tool
path and now validates the requested visibility inside its own
transaction, before the write, where the stored key set is exact.

Authorization also share-locks the permissions and credential_member
rows that grant the caller's access before reading them, so a
concurrent revocation either is observed and denies, or waits for the
transaction rather than racing the UPDATE.

Two UI fixes from the same review: a renamed variable saves as
delete-old plus create-new, so it now carries its visibility across the
rename instead of silently reverting to secret; and variable drafts join
allWorkspaceKeys, so personal-vs-workspace conflict detection no longer
misses names drafted in the Variables section.

Author: mzxchandra

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

@@ -9,18 +9,18 @@ const {
   mockGetWorkspaceById,
   mockGetUserEntityPermissions,
   mockGetWorkspaceEnvKeyAdminAccess,
-  mockAuthorizeVisibility,
   mockSetVisibility,
   mockCreateWorkspaceEnvCredentials,
+  mockRecordAudit,
   MockVisibilityAccessError,
 } = vi.hoisted(() => ({
   mockGetPersonalEnvKeyRawAccess: vi.fn(),
   mockGetWorkspaceById: vi.fn(),
   mockGetUserEntityPermissions: vi.fn(),
   mockGetWorkspaceEnvKeyAdminAccess: vi.fn(),
-  mockAuthorizeVisibility: vi.fn(),
   mockSetVisibility: vi.fn(),
   mockCreateWorkspaceEnvCredentials: vi.fn(),
+  mockRecordAudit: vi.fn(),
   // Declared inside vi.hoisted: `vi.mock` factories hoist above module-scope
   // class declarations, so a plain `class` here is in its TDZ when the factory
   // runs and the mock module fails to initialize.
@@ -34,6 +34,16 @@ const {
   },
 }))
 
+vi.mock('@/lib/core/security/encryption', () => ({
+  encryptSecret: vi.fn(async (value: string) => ({ encrypted: `enc:${value}` })),
+}))
+
+vi.mock('@sim/audit', () => ({
+  recordAudit: mockRecordAudit,
+  AuditAction: { ENVIRONMENT_UPDATED: 'environment.updated' },
+  AuditResourceType: { ENVIRONMENT: 'environment' },
+}))
+
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
   getWorkspaceById: mockGetWorkspaceById,
   getUserEntityPermissions: mockGetUserEntityPermissions,
@@ -46,7 +56,6 @@ vi.mock('@/lib/credentials/environment', () => ({
   getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess,
   createWorkspaceEnvCredentials: mockCreateWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials: vi.fn(),
-  authorizeWorkspaceEnvVisibilityChange: mockAuthorizeVisibility,
   setWorkspaceEnvVisibility: mockSetVisibility,
   WorkspaceEnvVisibilityAccessError: MockVisibilityAccessError,
 }))
@@ -244,68 +253,54 @@ describe('PUT /api/workspaces/[id]/environment — visibility ordering', () => {
   }
 
   /**
-   * The ordering defect Greptile caught. A request mixing an allowed NEW key
-   * with an unauthorized visibility flip used to commit the key and its
-   * credential rows and only THEN return 403 — a rejected call that changed
-   * workspace state and skipped its audit record.
+   * A rejected request must change nothing. The value upsert, the credential
+   * rows, and the visibility flip all run in ONE transaction now, so a denial
+   * anywhere throws and rolls the rest back — no ordering left to get wrong.
    */
-  it('writes nothing when a visibility change is denied', async () => {
-    mockAuthorizeVisibility.mockRejectedValue(new MockVisibilityAccessError(['STRIPE_KEY']))
+  it('returns 403 and records no audit when a visibility change is denied', async () => {
+    mockSetVisibility.mockRejectedValue(new MockVisibilityAccessError(['STRIPE_KEY']))
 
-    const { status } = await callPut({
+    const { status, body } = await callPut({
       variables: { BRAND_NEW: 'v' },
       visibility: { STRIPE_KEY: 'variable' },
     })
 
     expect(status).toBe(403)
-    // The whole point: authorization ran before any write reached the database.
-    expect(mockCreateWorkspaceEnvCredentials).not.toHaveBeenCalled()
-    expect(mockSetVisibility).not.toHaveBeenCalled()
+    expect(body.error).toContain('admin')
+    // The audit record lives after the transaction, so a rejected request
+    // cannot reach it — which is the observable proof nothing was committed.
+    expect(mockRecordAudit).not.toHaveBeenCalled()
   })
 
-  it('re-decides authorization at write time rather than reusing the pre-flight', async () => {
-    mockAuthorizeVisibility.mockResolvedValue([
-      { credentialId: 'c-1', envKey: 'SUPPORT_EMAIL', next: 'variable' },
-    ])
+  /**
+   * Both writes must be transaction-scoped. If either call moves back outside
+   * the transaction it loses its `executor` and a denial in the other would
+   * strand it committed — the exact failure this consolidation removed.
+   */
+  it('runs the credential inserts and the visibility flip inside the transaction', async () => {
     mockSetVisibility.mockResolvedValue({ changedKeys: ['SUPPORT_EMAIL'] })
 
     const { status } = await callPut({
-      variables: {},
+      variables: { NEW_KEY: 'v' },
       visibility: { SUPPORT_EMAIL: 'variable' },
     })
 
     expect(status).toBe(200)
-    expect(mockAuthorizeVisibility).toHaveBeenCalled()
-    // The pre-flight's resolved credential IDs are deliberately NOT carried into
-    // the write: the request the setter receives is the raw visibility map, so
-    // access is re-checked against current membership next to the UPDATE.
+    expect(mockCreateWorkspaceEnvCredentials).toHaveBeenCalledWith(
+      expect.objectContaining({ executor: expect.anything() })
+    )
     expect(mockSetVisibility).toHaveBeenCalledWith(
       expect.objectContaining({
         workspaceId: WORKSPACE_ID,
         actingUserId: 'u-1',
         updates: { SUPPORT_EMAIL: 'variable' },
+        executor: expect.anything(),
       })
     )
-  })
-
-  /**
-   * The stale-authorization window. The pre-flight above passes, then the
-   * caller's credential-admin access is revoked while the value writes run. The
-   * write-time decision must still refuse — reusing the pre-flight result here
-   * would let a former admin disclose a secret after losing the right to.
-   */
-  it('returns 403 when access is revoked between the pre-flight and the write', async () => {
-    mockAuthorizeVisibility.mockResolvedValue([
-      { credentialId: 'c-1', envKey: 'STRIPE_KEY', next: 'variable' },
-    ])
-    mockSetVisibility.mockRejectedValue(new MockVisibilityAccessError(['STRIPE_KEY']))
-
-    const { status, body } = await callPut({
-      variables: {},
-      visibility: { STRIPE_KEY: 'variable' },
-    })
-
-    expect(status).toBe(403)
-    expect(body.error).toContain('admin')
+    // Ordering matters: the disclosure change is applied last, after the writes
+    // it must not be separated from.
+    expect(mockCreateWorkspaceEnvCredentials.mock.invocationCallOrder[0]).toBeLessThan(
+      mockSetVisibility.mock.invocationCallOrder[0]
+    )
   })
 })

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

@@ -16,7 +16,6 @@ import { encryptSecret } from '@/lib/core/security/encryption'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import {
-  authorizeWorkspaceEnvVisibilityChange,
   createWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials,
   type EnvVisibility,
@@ -250,118 +249,104 @@ export const PUT = withRouteHandler(
         )
       }
 
-      // Pre-flight, BEFORE any write. Running the check after the value upsert
-      // let a request that mixes an allowed new key with an unauthorized flip
-      // commit the key and its credential rows and THEN return 403 — a rejected
-      // call that changed workspace state and skipped its audit record. The
-      // decision is deliberately discarded: it is re-made adjacent to the UPDATE
-      // below, so this pass only guarantees a denied request attempts no writes.
-      if (visibility) {
-        try {
-          await authorizeWorkspaceEnvVisibilityChange({
-            workspaceId,
-            updates: visibility,
-            actingUserId: userId,
-          })
-        } catch (error) {
-          if (error instanceof WorkspaceEnvVisibilityAccessError) {
-            logger.warn(`[${requestId}] Workspace env visibility change denied`, {
-              workspaceId,
-              userId,
-              keys: error.keys,
-            })
-            return NextResponse.json({ error: error.message }, { status: 403 })
-          }
-          throw error
-        }
-      }
-
       const encryptedIncoming = await Promise.all(
         Object.entries(variables).map(async ([key, value]) => {
           const { encrypted } = await encryptSecret(value)
           return [key, encrypted] as const
         })
       ).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 [existingRow] = await tx
-          .select()
-          .from(workspaceEnvironment)
-          .where(eq(workspaceEnvironment.workspaceId, workspaceId))
-          .limit(1)
-
-        const existing = ((existingRow?.variables as Record<string, string>) ?? {}) as Record<
-          string,
-          string
-        >
-        const mergedVars = { ...existing, ...encryptedIncoming }
+      /**
+       * One transaction for authorization AND every write it guards.
+       *
+       * The value upsert, the credential rows, and the visibility flip used to
+       * be three separate commits with the disclosure check somewhere among
+       * them, so a request that was ultimately rejected could still leave the
+       * first writes committed and skip its audit record. Ordering the check
+       * earlier only moved which writes were stranded. With all of it in one
+       * transaction there is no ordering to get wrong: a denial throws and
+       * every write in the request rolls back together.
+       *
+       * `setWorkspaceEnvVisibility` runs LAST, after the writes it must not be
+       * separated from, and share-locks the rows granting the caller's access
+       * before reading them — so a revocation committing mid-request either is
+       * observed and denies, or waits for this transaction.
+       */
+      let flippedKeys: string[] = []
+      let existingEncrypted: Record<string, string>
+      let merged: Record<string, string>
+      try {
+        ;({ 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 [existingRow] = await tx
+            .select()
+            .from(workspaceEnvironment)
+            .where(eq(workspaceEnvironment.workspaceId, workspaceId))
+            .limit(1)
+
+          const existing = ((existingRow?.variables as Record<string, string>) ?? {}) as Record<
+            string,
+            string
+          >
+          const mergedVars = { ...existing, ...encryptedIncoming }
+
+          if (Object.keys(encryptedIncoming).length > 0) {
+            await tx
+              .insert(workspaceEnvironment)
+              .values({
+                id: generateId(),
+                workspaceId,
+                variables: mergedVars,
+                createdAt: new Date(),
+                updatedAt: new Date(),
+              })
+              .onConflictDoUpdate({
+                target: [workspaceEnvironment.workspaceId],
+                set: { variables: mergedVars, updatedAt: new Date() },
+              })
+          }
 
-        await tx
-          .insert(workspaceEnvironment)
-          .values({
-            id: generateId(),
+          // Derived from the stored map, not the credential rows: a legacy key
+          // present in jsonb without a credential row is NOT new, and minting an
+          // ACL for it would make the caller its secret-admin.
+          await createWorkspaceEnvCredentials({
             workspaceId,
-            variables: mergedVars,
-            createdAt: new Date(),
-            updatedAt: new Date(),
-          })
-          .onConflictDoUpdate({
-            target: [workspaceEnvironment.workspaceId],
-            set: { variables: mergedVars, updatedAt: new Date() },
+            newKeys: Object.keys(variables).filter((k) => !(k in existing)),
+            actingUserId: userId,
+            visibilityByKey: visibility,
+            executor: tx,
           })
 
-        return { existingEncrypted: existing, merged: mergedVars }
-      })
-
-      invalidateEffectiveDecryptedEnvCache({ workspaceId })
-      const newKeys = Object.keys(variables).filter((k) => !(k in existingEncrypted))
-      await createWorkspaceEnvCredentials({
-        workspaceId,
-        newKeys,
-        actingUserId: userId,
-        visibilityByKey: visibility,
-      })
-
-      // Re-authorized here rather than reusing the pre-flight decision above.
-      // That decision was made before the value writes and the credential
-      // inserts, so acting on it would let an admin whose access was revoked in
-      // the meantime still disclose a secret. The check runs adjacent to the
-      // UPDATE inside one transaction, and reads permissions through `db` (not
-      // `tx`) so it observes the latest committed membership rather than this
-      // transaction's snapshot. Brand-new keys took their policy from
-      // `visibilityByKey` on create, so only pre-existing keys can appear here.
-      let flippedKeys: string[] = []
-      if (visibility) {
-        try {
-          flippedKeys = (
-            await db.transaction((tx) =>
-              setWorkspaceEnvVisibility({
-                workspaceId,
-                updates: visibility,
-                actingUserId: userId,
-                executor: tx,
-              })
-            )
-          ).changedKeys
-        } catch (error) {
-          if (error instanceof WorkspaceEnvVisibilityAccessError) {
-            logger.warn(`[${requestId}] Workspace env visibility change denied at apply`, {
+          if (visibility) {
+            const applied = await setWorkspaceEnvVisibility({
               workspaceId,
-              userId,
-              keys: error.keys,
-              reason: 'access-revoked-mid-request',
+              updates: visibility,
+              actingUserId: userId,
+              executor: tx,
             })
-            return NextResponse.json({ error: error.message }, { status: 403 })
+            flippedKeys = applied.changedKeys
           }
-          throw error
+
+          return { existingEncrypted: existing, merged: mergedVars }
+        }))
+      } catch (error) {
+        if (error instanceof WorkspaceEnvVisibilityAccessError) {
+          logger.warn(`[${requestId}] Workspace env visibility change denied`, {
+            workspaceId,
+            userId,
+            keys: error.keys,
+          })
+          // Nothing to undo: the denial rolled the whole transaction back.
+          return NextResponse.json({ error: error.message }, { status: 403 })
         }
+        throw error
       }
-      if (flippedKeys.length > 0) invalidateEffectiveDecryptedEnvCache({ workspaceId })
+
+      invalidateEffectiveDecryptedEnvCache({ workspaceId })
 
       recordAudit({
         workspaceId,