fix(execution): suspend personal secrets for banned owners, and gate the ban candidate correctly

Review round 1 (Greptile P1 security, cubic P1 + P2).

Removing the workflow owner from the ban gate let a suspended account's personal
secrets keep flowing into background runs: a ban revokes neither workspace
membership nor the pointer naming that person, so the run continued on their own
keys. `getExecutionEnvironment` now drops the personal namespace when that
identity is suspended, the same answer it already gives a departed one — the run
survives, their credentials do not. Placing it in the shared resolver rather than
in `execution-core` covers the webhook and custom-block paths too, and keeps the
ban module out of the executor's import graph.

The ban candidate itself was also inconsistent: callers overload `userId`, so it
is an authenticated caller on a manual run but a stored pointer everywhere else —
the workflow owner from `checkWebhookPreprocessing`, the chat's creator from the
deployed-chat route, `'unknown'` from a schedule. Reading it unconditionally
meant the same ban suspended a webhook while the schedule beside it kept running.
It is now gated on `useAuthenticatedUserAsActor`, which is exactly the flag that
distinguishes the two — `workflow-column-execution` toggles them together.

Also corrects the custom-block authority TSDoc, which still claimed the owner
supplies both environment slices after this branch split them.

Regenerates the CLI API client for the v2 log contract change, which CI's
`check:cli-api` audit caught.

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

Author: icecrasher321

apps/sim/lib/environment/utils.test.ts

@@ -18,13 +18,15 @@ const {
   mockGetUserEntityPermissions,
   mockGetWorkspaceEnvKeyAdminAccess,
   mockRecordAudit,
+  mockGetActivelyBannedUserIds,
 } = vi.hoisted(() => ({
   mockCreateWorkspaceEnvCredentials: vi.fn(),
   mockCheckWorkspaceAccess: vi.fn(),
   mockGetAccessibleEnvCredentials: vi.fn(),
   mockGetUserEntityPermissions: vi.fn(),
   mockGetWorkspaceEnvKeyAdminAccess: vi.fn(),
   mockRecordAudit: vi.fn(),
+  mockGetActivelyBannedUserIds: vi.fn().mockResolvedValue([]),
 }))
 
 // vitest.setup.ts mocks this module globally; this suite tests the real one.
@@ -42,6 +44,9 @@ vi.mock('@/lib/credentials/environment', () => ({
   getWorkspaceEnvKeyAdminAccess: mockGetWorkspaceEnvKeyAdminAccess,
   syncPersonalEnvCredentialsForUser: vi.fn(),
 }))
+vi.mock('@/lib/auth/ban', () => ({
+  getActivelyBannedUserIds: mockGetActivelyBannedUserIds,
+}))
 vi.mock('@/lib/workspaces/permissions/utils', () => ({
   checkWorkspaceAccess: mockCheckWorkspaceAccess,
   getUserEntityPermissions: mockGetUserEntityPermissions,
@@ -444,6 +449,7 @@ describe('getExecutionEnvironment', () => {
     vi.clearAllMocks()
     resetDbChainMock()
     mockGetAccessibleEnvCredentials.mockResolvedValue([])
+    mockGetActivelyBannedUserIds.mockResolvedValue([])
     encryptionMockFns.mockDecryptSecret.mockImplementation(async (encryptedValue: string) => ({
       decrypted: `plain:${encryptedValue}`,
     }))
@@ -615,6 +621,39 @@ describe('getExecutionEnvironment', () => {
     expect(snapshot.workspaceDecrypted).toEqual({})
   })
 
+  /**
+   * Admission deliberately stops blocking runs on the personal-variable
+   * identity, so that a suspended member does not take down their teammates'
+   * schedules and webhooks. That must not become a way for a suspended account's
+   * own credentials to keep running — the run continues, their namespace does not.
+   */
+  it('resolves workspace variables only when the personal identity is suspended', async () => {
+    grantAdminTo('actor-1')
+    mockGetActivelyBannedUserIds.mockResolvedValue(['suspended-owner'])
+    queueTableRows(environment, [{ variables: { OWNER_KEY: 'owner-cipher' } }])
+    queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }])
+
+    const snapshot = await getExecutionEnvironment('suspended-owner', 'actor-1', 'workspace-1')
+
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['suspended-owner'])
+    expect(snapshot.personalDecrypted).toEqual({})
+    expect(snapshot.workspaceDecrypted).toEqual({ WORKSPACE_KEY: 'plain:workspace-cipher' })
+  })
+
+  /** The actor is cleared by admission, so only the personal identity is looked up. */
+  it('does not re-check the execution actor for a ban', async () => {
+    grantAdminTo('actor-1')
+    queueTableRows(environment, [{ variables: {} }])
+    queueTableRows(workspaceEnvironment, [{ variables: {} }])
+    queueTableRows(environment, [{ variables: {} }])
+    queueTableRows(workspaceEnvironment, [{ variables: {} }])
+
+    await getExecutionEnvironment('owner-1', 'actor-1', 'workspace-1')
+
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledOnce()
+    expect(mockGetActivelyBannedUserIds.mock.calls[0][0]).not.toContain('actor-1')
+  })
+
   /** With no reachable identity there is nobody to authorize the workspace slice against. */
   it('raises when neither identity can reach the workspace', async () => {
     mockCheckWorkspaceAccess.mockResolvedValue({

apps/sim/lib/environment/utils.ts

@@ -5,6 +5,7 @@ import { createLogger } from '@sim/logger'
 import { generateId } from '@sim/utils/id'
 import { eq, inArray } from 'drizzle-orm'
 import { LRUCache } from 'lru-cache'
+import { getActivelyBannedUserIds } from '@/lib/auth/ban'
 import { decryptSecret, encryptSecret } from '@/lib/core/security/encryption'
 import { lockPersonalEnvMap, lockWorkspaceEnvMap } from '@/lib/credentials/env-locks'
 import {
@@ -493,9 +494,10 @@ export async function getExecutionEnvironment(
     return getPersonalAndWorkspaceEnv(personalUserId, workspaceId)
   }
 
-  const [actorAccess, personalAccess] = await Promise.all([
+  const [actorAccess, personalAccess, suspendedPersonalIds] = await Promise.all([
     checkWorkspaceAccess(workspaceId, workspaceUserId),
     checkWorkspaceAccess(workspaceId, personalUserId),
+    getActivelyBannedUserIds([personalUserId]),
   ])
 
   /**
@@ -507,7 +509,23 @@ export async function getExecutionEnvironment(
     throw new Error(`Workspace ${workspaceId} does not exist`)
   }
 
-  if (!personalAccess.hasAccess) {
+  /**
+   * A suspended account lends nothing, even when the run itself may continue.
+   *
+   * Admission blocks on the identities a run acts as and deliberately not on the
+   * personal-variable fallback, so that suspending one member does not take down
+   * the schedules, webhooks, and deployed chats their teammates depend on. But
+   * "this run may continue" and "that person's private credentials may still be
+   * used" are different questions, and a ban revokes neither workspace
+   * membership nor the pointer naming them — so without this the run proceeds on
+   * a suspended account's own keys.
+   *
+   * Only the personal identity is checked here; admission already cleared the
+   * actor before execution reached this point.
+   */
+  const personalIdentitySuspended = suspendedPersonalIds.length > 0
+
+  if (!personalAccess.hasAccess || personalIdentitySuspended) {
     if (!actorAccess.hasAccess) {
       logger.error('Neither execution identity can reach the workspace', {
         personalUserId,
@@ -518,7 +536,9 @@ export async function getExecutionEnvironment(
     }
 
     logger.error(
-      'Personal-environment identity cannot reach the workspace; resolving workspace variables only',
+      personalIdentitySuspended
+        ? 'Personal-environment identity is suspended; resolving workspace variables only'
+        : 'Personal-environment identity cannot reach the workspace; resolving workspace variables only',
       { personalUserId, workspaceUserId, workspaceId }
     )
     return toWorkspaceOnlySnapshot(

apps/sim/lib/execution/preprocessing.test.ts

@@ -500,12 +500,29 @@ describe('preprocessExecution ban gate', () => {
     expect(mockCheckRateLimit).toHaveBeenCalledTimes(1)
   })
 
-  it('checks the actor and the caller-provided userId in one call', async () => {
-    const result = await preprocessExecution(baseOptions)
+  /** An authenticated caller becomes the actor, so one candidate covers them. */
+  it('checks the authenticated caller as the actor', async () => {
+    const result = await preprocessExecution({ ...baseOptions, useAuthenticatedUserAsActor: true })
 
     expect(result.success).toBe(true)
     expect(mockGetActivelyBannedUserIds).toHaveBeenCalledTimes(1)
-    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1', 'owner-1'])
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['owner-1'])
+  })
+
+  /**
+   * The one shape where the two genuinely differ: an upstream boundary captured
+   * the attribution, so the actor comes from there while `userId` still names
+   * the authenticated caller. Both are identities the run acts as.
+   */
+  it('checks both when a captured attribution names a different actor', async () => {
+    const result = await preprocessExecution({
+      ...baseOptions,
+      useAuthenticatedUserAsActor: true,
+      billingAttribution: { ...ORGANIZATION_ATTRIBUTION, actorUserId: 'delegated-actor-1' } as any,
+    })
+
+    expect(result.success).toBe(true)
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['delegated-actor-1', 'owner-1'])
   })
 
   it('excludes the "unknown" sentinel userId', async () => {
@@ -516,21 +533,34 @@ describe('preprocessExecution ban gate', () => {
   })
 
   /**
-   * Banning one member must not take down the schedules, webhooks, and deployed
-   * chats their teammates depend on. Those runs act as the workspace billing
-   * account; the owner's name on the workflow row is a personal-variable
-   * fallback, and member removal reassigns it anyway.
+   * Callers overload `userId`: an authenticated caller on a manual run, but a
+   * stored pointer on a system-triggered one — the workflow owner from
+   * `checkWebhookPreprocessing`, the chat's creator from the deployed-chat
+   * route. Without `useAuthenticatedUserAsActor` gating it, the same ban
+   * suspended a webhook while the schedule beside it kept running.
+   */
+  it('ignores a stored-pointer userId when it is not the authenticated caller', async () => {
+    const result = await preprocessExecution({ ...baseOptions, userId: 'creator-1' })
+
+    expect(result.success).toBe(true)
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1'])
+    expect(mockGetActivelyBannedUserIds.mock.calls[0][0]).not.toContain('creator-1')
+  })
+
+  /**
+   * The webhook shape specifically: `checkWebhookPreprocessing` passes the
+   * workflow owner as `userId` with no `useAuthenticatedUserAsActor`, so a
+   * banned owner must not take the webhook down.
    */
   it('does not block a system-triggered run because the workflow owner is banned', async () => {
     mockGetActivelyBannedUserIds.mockImplementation(async (ids: string[]) =>
       ids.filter((id) => id === 'creator-1')
     )
 
-    const result = await preprocessExecution({ ...baseOptions, userId: 'unknown' })
+    const result = await preprocessExecution({ ...baseOptions, userId: 'creator-1' })
 
     expect(result.success).toBe(true)
     expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1'])
-    expect(mockGetActivelyBannedUserIds.mock.calls[0][0]).not.toContain('creator-1')
   })
 
   it('fails closed with 500 when the ban check errors', async () => {

apps/sim/lib/execution/preprocessing.ts

@@ -449,19 +449,26 @@ export async function preprocessExecution(
 
   const banCheck = (async (): Promise<GateFailure | null> => {
     /**
-     * Blocks when the resolved actor or the caller-provided user has an active
-     * ban or blocked email domain — the identities this run actually acts as.
+     * Blocks when an identity this run actually acts as has an active ban or
+     * blocked email domain.
      *
-     * The workflow owner is deliberately NOT a candidate. A system-triggered run
-     * acts as the workspace billing account, and that account is already the
-     * actor here; the owner is only the personal-variable fallback and is a
-     * stored pointer that member removal reassigns. Banning one member of a
-     * workspace should suspend the work they do, not silently take down every
-     * schedule, webhook, and deployed chat their teammates still depend on
-     * because their name happens to sit on the workflow row.
+     * `userId` is only such an identity when `useAuthenticatedUserAsActor` says
+     * so. Callers overload that parameter: it is an authenticated caller on a
+     * manual or personal-key run, but a stored pointer everywhere else — the
+     * workflow owner from `checkWebhookPreprocessing`, the chat's creator from
+     * the deployed-chat route, the literal `'unknown'` from a schedule. Reading
+     * it unconditionally made the same ban suspend a webhook while leaving the
+     * schedule beside it running, for no reason a workspace could observe.
+     * `workflow-column-execution` toggles the two together and is the clearest
+     * statement of the rule.
+     *
+     * A stored pointer being banned must not take down work their teammates
+     * still depend on — but it must not lend that person's credentials either,
+     * which is why the executor drops a banned identity's personal namespace
+     * rather than this gate blocking the whole run.
      */
     const banCandidateIds = [actorUserId]
-    if (userId && userId !== 'unknown' && userId !== actorUserId) {
+    if (useAuthenticatedUserAsActor && userId && userId !== 'unknown' && userId !== actorUserId) {
       banCandidateIds.push(userId)
     }
     try {

apps/sim/lib/workflows/custom-blocks/operations.ts

@@ -312,18 +312,18 @@ export async function getCustomBlockManageContext(id: string): Promise<{
  * cascade-deletes the workflow → the custom_block row, so there is never an
  * orphaned block. `null` when no enabled block matches the type.
  *
- * `ownerUserId` is the child run's whole identity — both environment slices, the
- * billing actor, and the subject of its delegated tool calls. That is NOT what a
- * deployed API/schedule/webhook run does: those act as the workspace billing
- * account and fall back to the owner only for personal variables. A custom block
- * needs the stronger form because it publishes a fixed behavior to consumers who
- * can see none of its internals, and the publisher's own integrations and personal
- * keys are part of that behavior.
+ * `ownerUserId` carries further than the owner does on any other trigger. It is
+ * the child run's actor, the personal-variable identity, and the subject of its
+ * delegated tool calls, because a custom block publishes a fixed behavior to
+ * consumers who can see none of its internals and the publisher's own
+ * integrations and personal keys are part of that behavior.
  *
- * The cost is that the owner is load-bearing rather than a fallback: an owner who
- * leaves the source workspace takes the block's environment resolution down with
- * them, where a schedule on the same workflow keeps running. Any repair belongs
- * here or in the member-removal reassignment, not at the call site.
+ * It is NOT the identity for the two things a workspace owns. Workspace
+ * variables authorize against the source workspace's billing account, and that
+ * account is the payer, exactly as they would for a schedule on the same
+ * workflow — see the environment resolution in `workflow-handler`. Reading those
+ * as the owner too gave a published block a narrower workspace-secret selection
+ * than the workflow got on every other trigger, which no consumer could see.
  */
 export async function getCustomBlockAuthority(
   type: string,

packages/sim-cli/src/generated/v2-api.ts

@@ -4038,6 +4038,7 @@ type GetLogResponseRef2 = {
   endedAt: string | null
   totalDurationMs: number | null
   files: Array<GetLogResponseRef0> | null
+  executedByEmail: string | null
   workflow: {
     id: string | null
     name: string