fix(secrets): re-check the reference gate's volatile input after the scan

Review round 3.

The name-resolution gate reads whether a workspace value exists, then scans. A
workspace secret created between the two makes the map now in hand admin-gated,
so a personal owner could receive it without workspace-secret administration.

The window is small and the data is derivable — a workspace member can already
open every workflow and read its `{{KEY}}` references — but the gate's stated
contract is that references follow the same predicate as revealing the value, and
a point-in-time check that can be overtaken does not honour that. An advisory lock
or a snapshot transaction would serialize a read-only view against secret writes
for it, which is the wrong trade.

Instead the one volatile input is re-read after the scan and the request fails
closed if it flipped. `requireSecretReferencesReadAccess` now reports which branch
authorized: an `admin` grant holds however the name resolves and pays nothing,
while a `personal` grant — the only one resting on absence — is re-checked. A
non-admin loses nothing they were entitled to keep; the request is refused the way
it would have been a moment later.

Adds a `listSecretReferencesUseCase` suite covering both denial paths, the legacy
value, the personal owner, the admin short-circuit, and the race itself.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: icecrasher321

apps/sim/lib/secrets/application/use-cases.test.ts

@@ -21,6 +21,8 @@ const { mocks } = vi.hoisted(() => ({
     deletePersonal: vi.fn(),
     listCredentials: vi.fn(),
     secretUsage: vi.fn(),
+    workspaceEnvValue: vi.fn(),
+    scanReferences: vi.fn(),
     audit: vi.fn(),
   },
 }))
@@ -47,6 +49,10 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
 vi.mock('@/lib/credentials/environment', () => ({
   getWorkspaceEnvKeyAdminAccess: mocks.keyAccess,
   getPersonalEnvCredentialMetadata: mocks.personalMetadata,
+  hasWorkspaceEnvValue: mocks.workspaceEnvValue,
+}))
+vi.mock('@/lib/secrets/references/scan', () => ({
+  scanSecretReferences: mocks.scanReferences,
 }))
 vi.mock('@/lib/credentials/queries', () => ({
   listVisibleWorkspaceCredentials: mocks.listCredentials,
@@ -63,6 +69,8 @@ vi.mock('@/lib/credentials/secret-values', () => ({
 
 import {
   deleteSecretUseCase,
+  type ListSecretReferencesInput,
+  listSecretReferencesUseCase,
   listSecretUsageUseCase,
   setSecretUseCase,
 } from '@/lib/secrets/application/use-cases'
@@ -399,3 +407,105 @@ describe('listSecretUsageUseCase', () => {
     expect(mocks.keyAccess).not.toHaveBeenCalled()
   })
 })
+
+describe('listSecretReferencesUseCase', () => {
+  const execute = listSecretReferencesUseCase.execute as (args: {
+    principal: Principal
+    input: ListSecretReferencesInput
+  }) => Promise<unknown>
+
+  const input: ListSecretReferencesInput = {
+    workspaceId: workspace.workspaceId,
+    name: 'STRIPE_API_KEY',
+  }
+  const scan = { workflows: [], resources: [], truncated: false }
+
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mocks.loadContext.mockResolvedValue(workspace)
+    mocks.resolvePermission.mockResolvedValue('write')
+    mocks.scanReferences.mockResolvedValue(scan)
+  })
+
+  /**
+   * The map names workflows, blocks, tools and servers. A Member who may use the secret but not
+   * read it has no claim on it, so this must fail the same way the usage trail does.
+   */
+  it('denies a member who is not an admin of a workspace key', async () => {
+    mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
+    mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() })
+    mocks.workspaceEnvValue.mockResolvedValue(true)
+
+    await expect(execute({ principal: session, input })).rejects.toThrow(
+      'Credential admin permission required to view this secret usage'
+    )
+    expect(mocks.scanReferences).not.toHaveBeenCalled()
+  })
+
+  /**
+   * The gate reads the authoritative variables map, not `knownKeys`. A legacy value predating
+   * the credential ACL has no `env_workspace` row yet still wins at run time, so treating an
+   * empty `knownKeys` as "no workspace secret" would hand a non-admin exactly the oldest keys.
+   */
+  it('denies a personal owner when a legacy workspace value shares the name', async () => {
+    mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
+    mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() })
+    mocks.workspaceEnvValue.mockResolvedValue(true)
+    mocks.personalMetadata.mockResolvedValue({ id: 'cred-1' })
+
+    await expect(execute({ principal: session, input })).rejects.toThrow(
+      'Credential admin permission required to view this secret usage'
+    )
+    expect(mocks.scanReferences).not.toHaveBeenCalled()
+  })
+
+  it('denies a caller who holds no secret of that name', async () => {
+    mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
+    mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() })
+    mocks.workspaceEnvValue.mockResolvedValue(false)
+    mocks.personalMetadata.mockResolvedValue(null)
+
+    await expect(execute({ principal: session, input })).rejects.toThrow(
+      'Credential admin permission required to view this secret usage'
+    )
+    expect(mocks.scanReferences).not.toHaveBeenCalled()
+  })
+
+  it('allows the owner of a personal secret that no workspace value shadows', async () => {
+    mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
+    mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() })
+    mocks.workspaceEnvValue.mockResolvedValue(false)
+    mocks.personalMetadata.mockResolvedValue({ id: 'cred-1' })
+
+    await expect(execute({ principal: session, input })).resolves.toEqual(scan)
+  })
+
+  it('allows a credential admin of that key', async () => {
+    mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
+    mocks.keyAccess.mockResolvedValue({
+      knownKeys: new Set(['STRIPE_API_KEY']),
+      adminKeys: new Set(['STRIPE_API_KEY']),
+    })
+
+    await expect(execute({ principal: session, input })).resolves.toEqual(scan)
+    // An admin stays authorized however the name resolves, so the volatile input is never read.
+    expect(mocks.workspaceEnvValue).not.toHaveBeenCalled()
+  })
+
+  /**
+   * A `personal` grant rests on no workspace value existing under the name. If one is created
+   * between the check and the scan, the map now in hand is admin-gated — so the condition is
+   * re-read after the scan and the request fails closed rather than returning it.
+   */
+  it('refuses when a workspace value appears between the check and the scan', async () => {
+    mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
+    mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() })
+    mocks.personalMetadata.mockResolvedValue({ id: 'cred-1' })
+    mocks.workspaceEnvValue.mockResolvedValueOnce(false).mockResolvedValueOnce(true)
+
+    await expect(execute({ principal: session, input })).rejects.toThrow(
+      'Credential admin permission required to view this secret usage'
+    )
+    expect(mocks.workspaceEnvValue).toHaveBeenCalledTimes(2)
+  })
+})

apps/sim/lib/secrets/application/use-cases.ts

@@ -462,11 +462,18 @@ export interface ListSecretReferencesInput {
  *  - no workspace secret exists → the caller must actually hold a personal secret of that name,
  *    which stops a member enumerating arbitrary names for a map they have no claim to.
  */
+/**
+ * Which branch authorized the read. `personal` depends on no workspace value existing under the
+ * name — a condition another request can change — so only that branch needs re-checking after
+ * the scan. An `admin` caller stays authorized however the name resolves, and pays nothing.
+ */
+type SecretReferencesGrant = 'admin' | 'personal'
+
 async function requireSecretReferencesReadAccess(params: {
   workspaceId: string
   name: string
   userId: string
-}): Promise<void> {
+}): Promise<SecretReferencesGrant> {
   const [workspaceAccess, keyAccess] = await Promise.all([
     checkWorkspaceAccess(params.workspaceId, params.userId),
     getWorkspaceEnvKeyAdminAccess({
@@ -475,7 +482,7 @@ async function requireSecretReferencesReadAccess(params: {
       userId: params.userId,
     }),
   ])
-  if (workspaceAccess.canAdmin || keyAccess.adminKeys.has(params.name)) return
+  if (workspaceAccess.canAdmin || keyAccess.adminKeys.has(params.name)) return 'admin'
 
   const forbidden = new ForbiddenOperationError(
     'SECRET_ADMIN_ACCESS_REQUIRED',
@@ -501,6 +508,7 @@ async function requireSecretReferencesReadAccess(params: {
     envKey: params.name,
   })
   if (!owned) throw forbidden
+  return 'personal'
 }
 
 export const listSecretReferencesUseCase = defineAuthorizedWorkspaceUseCase({
@@ -509,17 +517,40 @@ export const listSecretReferencesUseCase = defineAuthorizedWorkspaceUseCase({
     resolveWorkspaceContext(input.workspaceId),
   authorizationOptions,
   async execute({ principal, input, context }) {
-    await requireSecretReferencesReadAccess({
+    const userId = principalUserId(principal)
+    const grant = await requireSecretReferencesReadAccess({
       workspaceId: context.workspaceId,
       name: input.name,
-      userId: principalUserId(principal),
+      userId,
     })
 
     /**
      * Name-based, not scope-narrowed: the same reference sites answer for a workspace secret and
      * for the personal one it shadows. Narrowing by scope would report a personal secret as
      * unreferenced the moment a workspace variable of the same name existed.
      */
-    return scanSecretReferences({ workspaceId: context.workspaceId, name: input.name })
+    const scan = await scanSecretReferences({
+      workspaceId: context.workspaceId,
+      name: input.name,
+    })
+
+    /**
+     * Re-check the one input that can change under us. A `personal` grant rests on no workspace
+     * value existing under this name; a workspace secret created between the check and the scan
+     * would make the very map now in hand admin-gated. The read is cheap and skipped entirely
+     * for an `admin` grant, and failing closed here costs a non-admin nothing they were entitled
+     * to keep — the request is simply refused the way it would have been a moment later.
+     */
+    if (
+      grant === 'personal' &&
+      (await hasWorkspaceEnvValue({ workspaceId: context.workspaceId, envKey: input.name }))
+    ) {
+      throw new ForbiddenOperationError(
+        'SECRET_ADMIN_ACCESS_REQUIRED',
+        'Credential admin permission required to view this secret usage'
+      )
+    }
+
+    return scan
   },
 })