fix(copilot): enforce secret-admin on env writes and gate KB indexing on usage

TheodoreSpeaks · claude · TheodoreSpeaks · commit 92e1a3ef12bc · 2026-07-31T13:32:15.000-07:00
Two more paths where the agent is a weaker route to the same write than the UI.

upsertWorkspaceEnvVars was a weakened copy of the environment route's write:
no per-key secret-admin check, no advisory lock, no audit row. Its only caller
is the set_environment_variables copilot tool, which gates on workspace 'write'
alone — so any write-level member could have the agent overwrite a workspace
secret they do not administer, with nothing in the audit log and a lost-update
race against the route's locked transaction. The gate now lives in the function
so every caller inherits it, reusing getWorkspaceEnvKeyAdminAccess rather than
restating the route's logic.

knowledge_base add_file computed a billing attribution and then indexed without
calling checkAttributedUsageLimits, which every upload route applies before
accepting indexing work — an over-quota workspace could index without limit
through the agent. The same file's query operation already gated correctly.

Co-Authored-By: Claude Opus 5 &lt;noreply@anthropic.com&gt;
diff --git a/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts b/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts
@@ -82,7 +82,11 @@ vi.mock('@/app/api/knowledge/utils', () => ({
   checkKnowledgeBaseWriteAccess: mockCheckKnowledgeBaseWriteAccess,
 }))
 
+import { checkAttributedUsageLimits } from '@/lib/billing/core/billing-attribution'
 import { knowledgeBaseServerTool } from '@/lib/copilot/tools/server/knowledge/knowledge-base'
+import { createSingleDocument } from '@/lib/knowledge/documents/service'
+import { getKnowledgeBaseById } from '@/lib/knowledge/service'
+import { resolveWorkspaceFileReference } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
 
 const BILLING_ATTRIBUTION = {
   actorUserId: 'external-admin',
@@ -173,3 +177,58 @@ describe('knowledge base connector Copilot operations', () => {
     }
   )
 })
+
+describe('knowledge base add_file usage gate', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+    mockCheckKnowledgeBaseWriteAccess.mockResolvedValue({
+      hasAccess: true,
+      knowledgeBase: { id: 'knowledge-base-1', workspaceId: 'workspace-paid', name: 'Paid KB' },
+    })
+    vi.mocked(getKnowledgeBaseById).mockResolvedValue({
+      id: 'knowledge-base-1',
+      workspaceId: 'workspace-paid',
+    } as Awaited<ReturnType<typeof getKnowledgeBaseById>>)
+  })
+
+  function addFile() {
+    return knowledgeBaseServerTool.execute(
+      {
+        operation: 'add_file',
+        args: { knowledgeBaseId: 'knowledge-base-1', filePaths: ['files/report.pdf'] },
+      },
+      {
+        userId: 'external-admin',
+        workspaceId: 'workspace-paid',
+        billingAttribution: BILLING_ATTRIBUTION,
+      }
+    )
+  }
+
+  it('refuses to index when the payer is over its usage limit', async () => {
+    vi.mocked(checkAttributedUsageLimits).mockResolvedValue({
+      isExceeded: true,
+      message: 'Usage limit exceeded.',
+    } as Awaited<ReturnType<typeof checkAttributedUsageLimits>>)
+
+    const result = await addFile()
+
+    expect(result.success).toBe(false)
+    expect(result.message).toContain('Usage limit exceeded')
+    // The gate must precede any indexing work, matching the upload routes.
+    expect(resolveWorkspaceFileReference).not.toHaveBeenCalled()
+    expect(createSingleDocument).not.toHaveBeenCalled()
+  })
+
+  it('gates on the knowledge base workspace payer, not the caller', async () => {
+    vi.mocked(checkAttributedUsageLimits).mockResolvedValue({
+      isExceeded: false,
+    } as Awaited<ReturnType<typeof checkAttributedUsageLimits>>)
+    vi.mocked(resolveWorkspaceFileReference).mockResolvedValue(null)
+
+    await addFile()
+
+    expect(checkAttributedUsageLimits).toHaveBeenCalledWith(BILLING_ATTRIBUTION)
+  })
+})
diff --git a/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts b/apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts
@@ -349,6 +349,17 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
 
           const kbWorkspaceId: string = targetKb.workspaceId
           const billingAttribution = requireKnowledgeBillingAttribution(context, kbWorkspaceId)
+
+          // Gate the payer before accepting indexing work, same as the upload routes.
+          const usage = await checkAttributedUsageLimits(billingAttribution)
+          if (usage.isExceeded) {
+            return {
+              success: false,
+              message:
+                usage.message || 'Usage limit exceeded. Please upgrade your plan to continue.',
+            }
+          }
+
           const added: Array<{ documentId: string; filename: string }> = []
           const failedFiles: string[] = []
 
diff --git a/apps/sim/lib/environment/utils.test.ts b/apps/sim/lib/environment/utils.test.ts
@@ -0,0 +1,118 @@
+/**
+ * @vitest-environment node
+ */
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const {
+  mockCreateWorkspaceEnvCredentials,
+  mockEncryptSecret,
+  mockGetUserEntityPermissions,
+  mockGetWorkspaceEnvKeyAdminAccess,
+  mockRecordAudit,
+} = vi.hoisted(() => ({
+  mockCreateWorkspaceEnvCredentials: vi.fn(),
+  mockEncryptSecret: vi.fn(),
+  mockGetUserEntityPermissions: vi.fn(),
+  mockGetWorkspaceEnvKeyAdminAccess: vi.fn(),
+  mockRecordAudit: vi.fn(),
+}))
+
+// vitest.setup.ts mocks this module globally; this suite tests the real one.
+vi.unmock('@/lib/environment/utils')
+
+vi.mock('@sim/audit', () => ({
+  AuditAction: { ENVIRONMENT_UPDATED: 'environment.updated' },
+  AuditResourceType: { ENVIRONMENT: 'environment' },
+  recordAudit: mockRecordAudit,
+}))
+vi.mock('@/lib/core/security/encryption', () => ({
+  decryptSecret: vi.fn(),
+  encryptSecret: mockEncryptSecret,
+}))
+vi.mock('@/lib/credentials/environment', () => ({
+  createWorkspaceEnvCredentials: mockCreateWorkspaceEnvCredentials,
+  getAccessibleEnvCredentials: vi.fn(),
+  getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess,
+  syncPersonalEnvCredentialsForUser: vi.fn(),
+}))
+vi.mock('@/lib/workspaces/permissions/utils', () => ({
+  checkWorkspaceAccess: vi.fn(),
+  getUserEntityPermissions: mockGetUserEntityPermissions,
+}))
+
+import { upsertWorkspaceEnvVars, WorkspaceEnvAccessError } from '@/lib/environment/utils'
+
+describe('upsertWorkspaceEnvVars', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockEncryptSecret.mockResolvedValue({ encrypted: 'cipher' })
+  })
+
+  it('refuses to overwrite an existing secret the caller does not administer', async () => {
+    // Workspace `write` is what the copilot tool checks; the route additionally
+    // requires secret-admin on the specific key. Without this the agent was the
+    // weaker path to the same write.
+    mockGetUserEntityPermissions.mockResolvedValue('write')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set<string>(),
+      knownKeys: new Set(['STRIPE_KEY']),
+    })
+
+    await expect(
+      upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1')
+    ).rejects.toBeInstanceOf(WorkspaceEnvAccessError)
+
+    expect(mockEncryptSecret).not.toHaveBeenCalled()
+    expect(mockRecordAudit).not.toHaveBeenCalled()
+  })
+
+  it('refuses to add a new secret without workspace write', async () => {
+    mockGetUserEntityPermissions.mockResolvedValue('read')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set<string>(),
+      knownKeys: new Set<string>(),
+    })
+
+    await expect(
+      upsertWorkspaceEnvVars('ws-1', { NEW_KEY: 'value' }, 'user-1')
+    ).rejects.toBeInstanceOf(WorkspaceEnvAccessError)
+
+    expect(mockEncryptSecret).not.toHaveBeenCalled()
+  })
+
+  it('allows a key admin to rotate the key they administer', async () => {
+    mockGetUserEntityPermissions.mockResolvedValue('write')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set(['STRIPE_KEY']),
+      knownKeys: new Set(['STRIPE_KEY']),
+    })
+
+    await expect(
+      upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1')
+    ).resolves.toEqual(['STRIPE_KEY'])
+
+    expect(mockEncryptSecret).toHaveBeenCalledWith('rotated')
+    expect(mockRecordAudit).toHaveBeenCalledWith(
+      expect.objectContaining({ workspaceId: 'ws-1', actorId: 'user-1' })
+    )
+  })
+
+  it('treats a workspace admin as an admin of every key', async () => {
+    mockGetUserEntityPermissions.mockResolvedValue('admin')
+    mockGetWorkspaceEnvKeyAdminAccess.mockResolvedValue({
+      adminKeys: new Set<string>(),
+      knownKeys: new Set(['STRIPE_KEY']),
+    })
+
+    await expect(
+      upsertWorkspaceEnvVars('ws-1', { STRIPE_KEY: 'rotated' }, 'user-1')
+    ).resolves.toEqual(['STRIPE_KEY'])
+  })
+
+  it('records no audit and takes no lock for an empty update', async () => {
+    await expect(upsertWorkspaceEnvVars('ws-1', {}, 'user-1')).resolves.toEqual([])
+
+    expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
+    expect(mockRecordAudit).not.toHaveBeenCalled()
+  })
+})
diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts
@@ -1,19 +1,34 @@
+import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
 import { db } from '@sim/db'
 import { environment, workspaceEnvironment } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { getErrorMessage } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
-import { eq, inArray } from 'drizzle-orm'
+import { eq, inArray, sql } from 'drizzle-orm'
 import { LRUCache } from 'lru-cache'
 import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption'
 import {
   createWorkspaceEnvCredentials,
   getAccessibleEnvCredentials,
+  getWorkspaceEnvKeyAdminAccess,
   syncPersonalEnvCredentialsForUser,
 } from '@/lib/credentials/environment'
-import { checkWorkspaceAccess, type WorkspaceAccess } from '@/lib/workspaces/permissions/utils'
+import {
+  checkWorkspaceAccess,
+  getUserEntityPermissions,
+  type WorkspaceAccess,
+} from '@/lib/workspaces/permissions/utils'
 
 const logger = createLogger('EnvironmentUtils')
+const WORKSPACE_ENV_LOCK_TIMEOUT_MS = 5_000
+
+/** Thrown when the acting user may not write one of the requested env keys. */
+export class WorkspaceEnvAccessError extends Error {
+  constructor(readonly keys: string[]) {
+    super('You must be an admin of these secrets to edit them')
+    this.name = 'WorkspaceEnvAccessError'
+  }
+}
 const EFFECTIVE_DECRYPTED_ENV_CACHE_TTL_MS = 2_000
 const EFFECTIVE_DECRYPTED_ENV_CACHE_MAX_ENTRIES = 1_000
 
@@ -317,43 +332,94 @@ export async function upsertWorkspaceEnvVars(
   newVars: Record<string, string>,
   actingUserId: string
 ): Promise<string[]> {
-  const updatedKeys: string[] = []
-  if (Object.keys(newVars).length === 0) return updatedKeys
+  const updatedKeys = Object.keys(newVars)
+  if (updatedKeys.length === 0) return []
 
-  const wsRows = await db
-    .select()
-    .from(workspaceEnvironment)
-    .where(eq(workspaceEnvironment.workspaceId, workspaceId))
-    .limit(1)
-  const existingWsEncrypted = (wsRows[0]?.variables as Record<string, string>) || {}
+  const permission = await getUserEntityPermissions(actingUserId, 'workspace', workspaceId)
+  const { adminKeys, knownKeys } = await getWorkspaceEnvKeyAdminAccess({
+    workspaceId,
+    envKeys: updatedKeys,
+    userId: actingUserId,
+  })
+
+  // Overwriting an existing secret needs secret-admin on that specific key;
+  // workspace `write` alone only covers adding new ones.
+  const forbidden = updatedKeys.filter(
+    (key) => knownKeys.has(key) && permission !== 'admin' && !adminKeys.has(key)
+  )
+  if (forbidden.length > 0) {
+    logger.warn('Workspace env update denied', {
+      workspaceId,
+      userId: actingUserId,
+      reason: 'not-secret-admin',
+      keys: forbidden,
+    })
+    throw new WorkspaceEnvAccessError(forbidden)
+  }
+  const addingNew = updatedKeys.some((key) => !knownKeys.has(key))
+  if (addingNew && permission !== 'admin' && permission !== 'write') {
+    logger.warn('Workspace env update denied', {
+      workspaceId,
+      userId: actingUserId,
+      reason: 'write-access-required',
+      keys: updatedKeys.filter((key) => !knownKeys.has(key)),
+    })
+    throw new WorkspaceEnvAccessError(updatedKeys.filter((key) => !knownKeys.has(key)))
+  }
 
   const newlyEncrypted: Record<string, string> = {}
   for (const [key, val] of Object.entries(newVars)) {
     const { encrypted } = await encryptSecret(val)
     newlyEncrypted[key] = encrypted
-    updatedKeys.push(key)
   }
 
-  const merged = { ...existingWsEncrypted, ...newlyEncrypted }
+  // Read-modify-write on a single jsonb column, so serialize against the
+  // route's identically-locked transaction or concurrent writers lose keys.
+  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 db
-    .insert(workspaceEnvironment)
-    .values({
-      id: generateId(),
-      workspaceId,
-      variables: merged,
-      createdAt: new Date(),
-      updatedAt: new Date(),
-    })
-    .onConflictDoUpdate({
-      target: [workspaceEnvironment.workspaceId],
-      set: { variables: merged, updatedAt: new Date() },
-    })
+    const [existingRow] = await tx
+      .select()
+      .from(workspaceEnvironment)
+      .where(eq(workspaceEnvironment.workspaceId, workspaceId))
+      .limit(1)
+    const merged = {
+      ...((existingRow?.variables as Record<string, string>) || {}),
+      ...newlyEncrypted,
+    }
+
+    await tx
+      .insert(workspaceEnvironment)
+      .values({
+        id: generateId(),
+        workspaceId,
+        variables: merged,
+        createdAt: new Date(),
+        updatedAt: new Date(),
+      })
+      .onConflictDoUpdate({
+        target: [workspaceEnvironment.workspaceId],
+        set: { variables: merged, updatedAt: new Date() },
+      })
+  })
 
   invalidateEffectiveDecryptedEnvCache({ workspaceId })
-  const newKeys = Object.keys(newVars).filter((k) => !(k in existingWsEncrypted))
+  const newKeys = updatedKeys.filter((key) => !knownKeys.has(key))
   await createWorkspaceEnvCredentials({ workspaceId, newKeys, actingUserId })
 
+  recordAudit({
+    workspaceId,
+    actorId: actingUserId,
+    action: AuditAction.ENVIRONMENT_UPDATED,
+    resourceType: AuditResourceType.ENVIRONMENT,
+    resourceId: workspaceId,
+    description: `Updated ${updatedKeys.length} workspace environment variable(s)`,
+    metadata: { variableCount: updatedKeys.length, updatedKeys },
+  })
+
   return updatedKeys
 }
 
