fix(secrets): authorize visibility changes before any write

Two review findings, both real.

Ordering: the PUT committed the value upsert and credential rows, then called
the visibility change, which can 403. A request mixing an allowed new key with
an unauthorized flip therefore persisted half of itself and still failed —
state changed by a rejected call, and the audit record for the successful part
never written. setWorkspaceEnvVisibility splits into authorize and apply so the
route can deny before anything reaches the database. Covered by a test that
asserts no credential creation and no apply happen on the denied path.

Silent no-op: visibilityByKey only reaches credential CREATION, so asking for a
policy on an existing key was dropped while the call still reported success.
That includes a variable -> secret remediation, the case where a false success
is most harmful — a caller told it worked would never retry. Flipping an
existing key needs the stricter disclosure gate this path does not perform, so
it now throws instead of pretending.

Also consumes the consolidated envVars contract: one kind where a secret is
name-only and a non-secret carries its value.

Author: mzxchandra

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

@@ -9,11 +9,29 @@ const {
   mockGetWorkspaceById,
   mockGetUserEntityPermissions,
   mockGetWorkspaceEnvKeyAdminAccess,
+  mockAuthorizeVisibility,
+  mockApplyVisibility,
+  mockCreateWorkspaceEnvCredentials,
+  MockVisibilityAccessError,
 } = vi.hoisted(() => ({
   mockGetPersonalEnvKeyRawAccess: vi.fn(),
   mockGetWorkspaceById: vi.fn(),
   mockGetUserEntityPermissions: vi.fn(),
   mockGetWorkspaceEnvKeyAdminAccess: vi.fn(),
+  mockAuthorizeVisibility: vi.fn(),
+  mockApplyVisibility: 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
+  // runs and the mock module fails to initialize.
+  MockVisibilityAccessError: class extends Error {
+    keys: string[]
+    constructor(keys: string[]) {
+      super('You must be an admin of these secrets to change their visibility')
+      this.name = 'WorkspaceEnvVisibilityAccessError'
+      this.keys = keys
+    }
+  },
 }))
 
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
@@ -26,11 +44,14 @@ const mockGetPersonalAndWorkspaceEnv = environmentUtilsMockFns.mockGetPersonalAn
 vi.mock('@/lib/credentials/environment', () => ({
   getPersonalEnvKeyRawAccess: mockGetPersonalEnvKeyRawAccess,
   getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess,
-  createWorkspaceEnvCredentials: vi.fn(),
+  createWorkspaceEnvCredentials: mockCreateWorkspaceEnvCredentials,
   deleteWorkspaceEnvCredentials: vi.fn(),
+  authorizeWorkspaceEnvVisibilityChange: mockAuthorizeVisibility,
+  applyWorkspaceEnvVisibilityChange: mockApplyVisibility,
+  WorkspaceEnvVisibilityAccessError: MockVisibilityAccessError,
 }))
 
-import { GET } from '@/app/api/workspaces/[id]/environment/route'
+import { GET, PUT } from '@/app/api/workspaces/[id]/environment/route'
 
 const mockGetSession = authMockFns.mockGetSession
 
@@ -202,3 +223,63 @@ describe('GET /api/workspaces/[id]/environment', () => {
     })
   })
 })
+
+describe('PUT /api/workspaces/[id]/environment — visibility ordering', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockGetSession.mockResolvedValue({ user: { id: 'u-1' } })
+    mockGetUserEntityPermissions.mockResolvedValue('write')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set<string>(),
+      knownKeys: new Set<string>(),
+      variableKeys: new Set<string>(),
+    })
+    mockApplyVisibility.mockResolvedValue({ changedKeys: [] })
+  })
+
+  async function callPut(body: unknown) {
+    const request = createMockRequest('PUT', body)
+    const response = await PUT(request, buildParams())
+    return { status: response.status, body: await response.json() }
+  }
+
+  /**
+   * 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.
+   */
+  it('writes nothing when a visibility change is denied', async () => {
+    mockAuthorizeVisibility.mockRejectedValue(new MockVisibilityAccessError(['STRIPE_KEY']))
+
+    const { status } = 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(mockApplyVisibility).not.toHaveBeenCalled()
+  })
+
+  it('authorizes before applying on the success path', async () => {
+    mockAuthorizeVisibility.mockResolvedValue([
+      { credentialId: 'c-1', envKey: 'SUPPORT_EMAIL', next: 'variable' },
+    ])
+    mockApplyVisibility.mockResolvedValue({ changedKeys: ['SUPPORT_EMAIL'] })
+
+    const { status } = await callPut({
+      variables: {},
+      visibility: { SUPPORT_EMAIL: 'variable' },
+    })
+
+    expect(status).toBe(200)
+    expect(mockAuthorizeVisibility).toHaveBeenCalled()
+    expect(mockApplyVisibility).toHaveBeenCalledWith(
+      expect.objectContaining({
+        changes: [{ credentialId: 'c-1', envKey: 'SUPPORT_EMAIL', next: 'variable' }],
+      })
+    )
+  })
+})

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

@@ -16,12 +16,14 @@ 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 {
@@ -249,6 +251,31 @@ 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[] = []
+      if (visibility) {
+        try {
+          authorizedVisibilityChanges = 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)
@@ -300,34 +327,13 @@ export const PUT = withRouteHandler(
         visibilityByKey: visibility,
       })
 
-      // Flips are applied after the credential rows exist so a create-as-variable
-      // in the same request lands on a row rather than silently no-opping. Only
-      // pre-existing keys can flip; brand-new ones got their policy above.
-      let flippedKeys: string[] = []
-      if (visibility) {
-        const existingKeyUpdates = Object.fromEntries(
-          Object.entries(visibility).filter(([key]) => !newKeys.includes(key))
-        )
-        try {
-          const { changedKeys } = await setWorkspaceEnvVisibility({
-            workspaceId,
-            updates: existingKeyUpdates,
-            actingUserId: userId,
-          })
-          flippedKeys = changedKeys
-        } 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
-        }
-        if (flippedKeys.length > 0) invalidateEffectiveDecryptedEnvCache({ workspaceId })
-      }
+      // 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,
+      })
+      if (flippedKeys.length > 0) invalidateEffectiveDecryptedEnvCache({ workspaceId })
 
       recordAudit({
         workspaceId,

apps/sim/lib/copilot/chat/workspace-context.test.ts

@@ -125,7 +125,10 @@ describe('buildWorkspaceMd - connected integrations / credentials', () => {
     expect(md).toContain('## Environment Variables (2)')
     expect(md).toContain('- OPENAI_API_KEY')
     expect(md).toContain('- STRIPE_SECRET_KEY')
-    expect(buildVfsSnapshot(data).envVars).toEqual(['OPENAI_API_KEY', 'STRIPE_SECRET_KEY'])
+    expect(buildVfsSnapshot(data).envVars).toEqual([
+      { name: 'OPENAI_API_KEY' },
+      { name: 'STRIPE_SECRET_KEY' },
+    ])
   })
 
   it('shows values for non-secret env vars and only names for secrets', () => {
@@ -141,8 +144,10 @@ describe('buildWorkspaceMd - connected integrations / credentials', () => {
     expect(md).toContain('- OPENAI_API_KEY\n')
     expect(md).not.toContain('OPENAI_API_KEY =')
 
-    // Carried as a struct kind so Go can diff a VALUE change, not just a name.
-    expect(buildVfsSnapshot(data).nonSecretEnvVars).toEqual([
+    // One kind carries both: a secret is name-only, a non-secret adds its value
+    // so Go can diff a VALUE change and not just a name.
+    expect(buildVfsSnapshot(data).envVars).toEqual([
+      { name: 'OPENAI_API_KEY' },
       { name: 'SUPPORT_EMAIL', value: 'help@acme.com' },
     ])
   })
@@ -151,7 +156,8 @@ describe('buildWorkspaceMd - connected integrations / credentials', () => {
     const data = baseData({ envVariables: ['OPENAI_API_KEY'] })
 
     expect(buildWorkspaceMd(data)).not.toContain('non-secret')
-    expect(buildVfsSnapshot(data).nonSecretEnvVars).toEqual([])
+    // Every name still ships; none carries a value.
+    expect(buildVfsSnapshot(data).envVars).toEqual([{ name: 'OPENAI_API_KEY' }])
   })
 })
 

apps/sim/lib/copilot/chat/workspace-context.ts

@@ -703,14 +703,17 @@ export function buildVfsSnapshot(data: WorkspaceMdData): VfsSnapshotV1 {
       ...(c.displayName ? { displayName: c.displayName } : {}),
       ...(c.role ? { role: c.role } : {}),
     })),
-    envVars: data.envVariables,
-    // Carries values, unlike `envVars`. That is the point: a non-secret value
-    // edit changes no NAME, so a name-only kind would emit no delta and leave
-    // the model answering from a stale baseline.
-    nonSecretEnvVars: (data.nonSecretEnvVariables ?? []).map((v) => ({
-      name: v.name,
-      value: v.value,
-    })),
+    // One kind for both. A secret carries its name and nothing else; a
+    // non-secret also carries its value, which is what lets the differ report a
+    // value edit — the name alone does not change, so a names-only shape would
+    // emit no delta and leave the model answering from a stale baseline.
+    envVars: (() => {
+      const values = new Map((data.nonSecretEnvVariables ?? []).map((v) => [v.name, v.value]))
+      return data.envVariables.map((name) => {
+        const value = values.get(name)
+        return value === undefined ? { name } : { name, value }
+      })
+    })(),
     customTools: (data.customTools ?? []).map((t) => ({ id: t.id, name: t.name })),
     customBlocks: (data.customBlocks ?? []).map((b) => ({
       type: b.type,

apps/sim/lib/copilot/generated/vfs-snapshot-v1.ts

@@ -7,13 +7,12 @@
 export interface VfsSnapshotV1 {
   customBlocks?: VfsSnapshotV1CustomBlock[]
   customTools?: VfsSnapshotV1NamedResource[]
-  envVars?: string[]
+  envVars?: VfsSnapshotV1EnvVar[]
   files?: VfsSnapshotV1File[]
   integrations?: VfsSnapshotV1Integration[]
   knowledgeBases?: VfsSnapshotV1KnowledgeBase[]
   mcpServers?: VfsSnapshotV1McpServer[]
   members?: VfsSnapshotV1Member[]
-  nonSecretEnvVars?: VfsSnapshotV1NonSecretEnvVar[]
   sandboxes?: VfsSnapshotV1Sandbox[]
   skills?: VfsSnapshotV1Skill[]
   tables?: VfsSnapshotV1Table[]
@@ -37,6 +36,14 @@ export interface VfsSnapshotV1NamedResource {
   id: string
   name: string
 }
+/**
+ * This interface was referenced by `VfsSnapshotV1`'s JSON-Schema
+ * via the `definition` "VfsSnapshotV1EnvVar".
+ */
+export interface VfsSnapshotV1EnvVar {
+  name: string
+  value?: string
+}
 /**
  * This interface was referenced by `VfsSnapshotV1`'s JSON-Schema
  * via the `definition` "VfsSnapshotV1File".
@@ -88,14 +95,6 @@ export interface VfsSnapshotV1Member {
   name?: string
   permissionType?: string
 }
-/**
- * This interface was referenced by `VfsSnapshotV1`'s JSON-Schema
- * via the `definition` "VfsSnapshotV1NonSecretEnvVar".
- */
-export interface VfsSnapshotV1NonSecretEnvVar {
-  name: string
-  value?: string
-}
 /**
  * This interface was referenced by `VfsSnapshotV1`'s JSON-Schema
  * via the `definition` "VfsSnapshotV1Sandbox".