fix(secrets): re-decide visibility authorization at write time

The pre-flight authorization ran before the value upsert and the
credential inserts, then its resolved credential IDs were applied
afterwards. Access revoked in that window was never observed, so a
former credential admin could still flip a secret to a workspace-visible
variable.

The flip now goes through setWorkspaceEnvVisibility inside its own
transaction, which re-authorizes adjacent to the UPDATE. The pre-flight
stays, with its result deliberately discarded, so a denied request still
attempts no writes at all. applyWorkspaceEnvVisibilityChange is no
longer exported — a decision carried across unrelated awaits is exactly
what went stale, so there is no supported way to do that again.

Author: mzxchandra

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

@@ -10,7 +10,7 @@ const {
   mockGetUserEntityPermissions,
   mockGetWorkspaceEnvKeyAdminAccess,
   mockAuthorizeVisibility,
-  mockApplyVisibility,
+  mockSetVisibility,
   mockCreateWorkspaceEnvCredentials,
   MockVisibilityAccessError,
 } = vi.hoisted(() => ({
@@ -19,7 +19,7 @@ const {
   mockGetUserEntityPermissions: vi.fn(),
   mockGetWorkspaceEnvKeyAdminAccess: vi.fn(),
   mockAuthorizeVisibility: vi.fn(),
-  mockApplyVisibility: vi.fn(),
+  mockSetVisibility: vi.fn(),
   mockCreateWorkspaceEnvCredentials: 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
@@ -47,7 +47,7 @@ vi.mock('@/lib/credentials/environment', () => ({
   createWorkspaceEnvCredentials: mockCreateWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials: vi.fn(),
   authorizeWorkspaceEnvVisibilityChange: mockAuthorizeVisibility,
-  applyWorkspaceEnvVisibilityChange: mockApplyVisibility,
+  setWorkspaceEnvVisibility: mockSetVisibility,
   WorkspaceEnvVisibilityAccessError: MockVisibilityAccessError,
 }))
 
@@ -234,7 +234,7 @@ describe('PUT /api/workspaces/[id]/environment — visibility ordering', () => {
       knownKeys: new Set<string>(),
       variableKeys: new Set<string>(),
     })
-    mockApplyVisibility.mockResolvedValue({ changedKeys: [] })
+    mockSetVisibility.mockResolvedValue({ changedKeys: [] })
   })
 
   async function callPut(body: unknown) {
@@ -260,14 +260,14 @@ describe('PUT /api/workspaces/[id]/environment — visibility ordering', () => {
     expect(status).toBe(403)
     // The whole point: authorization ran before any write reached the database.
     expect(mockCreateWorkspaceEnvCredentials).not.toHaveBeenCalled()
-    expect(mockApplyVisibility).not.toHaveBeenCalled()
+    expect(mockSetVisibility).not.toHaveBeenCalled()
   })
 
-  it('authorizes before applying on the success path', async () => {
+  it('re-decides authorization at write time rather than reusing the pre-flight', async () => {
     mockAuthorizeVisibility.mockResolvedValue([
       { credentialId: 'c-1', envKey: 'SUPPORT_EMAIL', next: 'variable' },
     ])
-    mockApplyVisibility.mockResolvedValue({ changedKeys: ['SUPPORT_EMAIL'] })
+    mockSetVisibility.mockResolvedValue({ changedKeys: ['SUPPORT_EMAIL'] })
 
     const { status } = await callPut({
       variables: {},
@@ -276,10 +276,36 @@ describe('PUT /api/workspaces/[id]/environment — visibility ordering', () => {
 
     expect(status).toBe(200)
     expect(mockAuthorizeVisibility).toHaveBeenCalled()
-    expect(mockApplyVisibility).toHaveBeenCalledWith(
+    // 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(mockSetVisibility).toHaveBeenCalledWith(
       expect.objectContaining({
-        changes: [{ credentialId: 'c-1', envKey: 'SUPPORT_EMAIL', next: 'variable' }],
+        workspaceId: WORKSPACE_ID,
+        actingUserId: 'u-1',
+        updates: { SUPPORT_EMAIL: 'variable' },
       })
     )
   })
+
+  /**
+   * 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')
+  })
 })

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

@@ -16,14 +16,13 @@ import { encryptSecret } from '@/lib/core/security/encryption'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import {
-  type AuthorizedVisibilityChange,
-  applyWorkspaceEnvVisibilityChange,
   authorizeWorkspaceEnvVisibilityChange,
   createWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials,
   type EnvVisibility,
   getPersonalEnvKeyRawAccess,
   getWorkspaceEnvKeyAdminAccess,
+  setWorkspaceEnvVisibility,
   WorkspaceEnvVisibilityAccessError,
 } from '@/lib/credentials/environment'
 import {
@@ -251,14 +250,15 @@ export const PUT = withRouteHandler(
         )
       }
 
-      // Authorized BEFORE any write. Running this 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.
-      let authorizedVisibilityChanges: AuthorizedVisibilityChange[] = []
+      // 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 {
-          authorizedVisibilityChanges = await authorizeWorkspaceEnvVisibilityChange({
+          await authorizeWorkspaceEnvVisibilityChange({
             workspaceId,
             updates: visibility,
             actingUserId: userId,
@@ -327,12 +327,40 @@ export const PUT = withRouteHandler(
         visibilityByKey: visibility,
       })
 
-      // Already authorized above; this only writes. Brand-new keys took their
-      // policy from `visibilityByKey` on create, so the authorized set only
-      // ever contains pre-existing keys.
-      const { changedKeys: flippedKeys } = await applyWorkspaceEnvVisibilityChange({
-        changes: authorizedVisibilityChanges,
-      })
+      // 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`, {
+              workspaceId,
+              userId,
+              keys: error.keys,
+              reason: 'access-revoked-mid-request',
+            })
+            return NextResponse.json({ error: error.message }, { status: 403 })
+          }
+          throw error
+        }
+      }
       if (flippedKeys.length > 0) invalidateEffectiveDecryptedEnvCache({ workspaceId })
 
       recordAudit({

apps/sim/lib/credentials/environment.ts

@@ -572,9 +572,16 @@ export async function authorizeWorkspaceEnvVisibilityChange(params: {
 /**
  * Applies changes already authorized by
  * {@link authorizeWorkspaceEnvVisibilityChange}. Performs no permission check of
- * its own — never call it with unauthorized input.
+ * its own.
+ *
+ * Deliberately NOT exported. An authorization decision carried across unrelated
+ * awaits goes stale — the caller's credential-admin or workspace-admin access
+ * can be revoked in between, and applying by credential ID would then disclose a
+ * secret to someone who has just lost the right to disclose it. Keeping this
+ * private forces every caller through {@link setWorkspaceEnvVisibility}, which
+ * decides and writes adjacently.
  */
-export async function applyWorkspaceEnvVisibilityChange(params: {
+async function applyWorkspaceEnvVisibilityChange(params: {
   changes: AuthorizedVisibilityChange[]
   executor?: DbOrTx
 }): Promise<{ changedKeys: string[] }> {
@@ -601,8 +608,14 @@ export async function applyWorkspaceEnvVisibilityChange(params: {
  * admin on that specific key) rather than the workspace `write` that suffices
  * for editing a value.
  *
- * Authorizes then applies. Callers that must authorize BEFORE unrelated writes
- * should use the two halves directly.
+ * Authorizes and applies adjacently, so the decision cannot go stale between the
+ * two. Callers that also need to fail fast before unrelated writes can call
+ * {@link authorizeWorkspaceEnvVisibilityChange} as a pre-flight and discard its
+ * result — this function still re-decides authoritatively at write time.
+ *
+ * Pass `executor` to run the UPDATE inside a surrounding transaction. The
+ * permission reads intentionally go through `db`, not that transaction, so they
+ * observe the latest committed membership instead of the transaction's snapshot.
  */
 export async function setWorkspaceEnvVisibility(params: {
   workspaceId: string