fix(execution): make the suspension gate fail closed on an undeclared userId

Review round 4 (Greptile P1 security).

Round 2 keyed the ban candidate on `useAuthenticatedUserAsActor`, assuming that
flag separates a live caller from a stored reference. It does not. The
interactive resume route reads `access.auth?.userId` and passes that live
resumer as `userId` while leaving the flag false on purpose — attribution is
captured before the pause and must not move — so a suspended user could resume a
paused run whose persisted attribution named a different, unsuspended actor.

The distinction is per-caller and cannot be inferred, so it is now declared.
`userIdIsStoredReference` defaults to false, which means an undeclared call site
keeps blocking; only the three that genuinely pass a stored reference opt out:
the webhook processor (the workflow owner), the deployed-chat route (the chat's
creator), and table-cell dispatch (the owner, but only when nothing triggered
it). Resume, manual, API, and async paths are candidates again.

Withholding a suspended account's personal variables stays where it was, in
`getExecutionEnvironment`, so the two concerns remain separable: a suspended
stored reference does not block the run, and does not lend its credentials
either.

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

Author: icecrasher321

apps/sim/app/api/chat/[identifier]/route.ts

@@ -180,6 +180,8 @@ export const POST = withRouteHandler(
       const preprocessResult = await preprocessExecution({
         workflowId: deployment.workflowId,
         userId: deployment.userId,
+        // Whoever deployed this chat, not whoever is talking to it.
+        userIdIsStoredReference: true,
         triggerType: 'chat',
         executionId,
         requestId,

apps/sim/background/workflow-column-execution.ts

@@ -787,6 +787,8 @@ async function runWorkflowAndWriteTerminal(
               workflowRecord,
               userId: payload.triggeredByUserId ?? workflowRecord.userId,
               useAuthenticatedUserAsActor: Boolean(payload.triggeredByUserId),
+              // Falls back to the workflow owner when nobody triggered this.
+              userIdIsStoredReference: !payload.triggeredByUserId,
               triggerType: 'workflow',
               checkDeployment: false,
               checkRateLimit: false,

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

@@ -500,29 +500,41 @@ describe('preprocessExecution ban gate', () => {
     expect(mockCheckRateLimit).toHaveBeenCalledTimes(1)
   })
 
-  /** 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 })
+  /** The default is the blocking one: an undeclared `userId` stays a candidate. */
+  it('checks the actor and the caller-provided userId by default', async () => {
+    const result = await preprocessExecution(baseOptions)
 
     expect(result.success).toBe(true)
     expect(mockGetActivelyBannedUserIds).toHaveBeenCalledTimes(1)
-    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['owner-1'])
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1', '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.
+   * Resume is the shape that must keep blocking: it passes the live
+   * authenticated resumer as `userId` while attribution stays pinned to the
+   * original actor across the pause, and deliberately leaves
+   * `useAuthenticatedUserAsActor` false. Keying the gate on that flag excluded
+   * exactly the person who just acted.
    */
-  it('checks both when a captured attribution names a different actor', async () => {
+  it('checks a live resumer whose captured attribution names a different actor', async () => {
+    mockGetActivelyBannedUserIds.mockImplementation(async (ids: string[]) =>
+      ids.filter((id) => id === 'suspended-resumer')
+    )
+
     const result = await preprocessExecution({
       ...baseOptions,
-      useAuthenticatedUserAsActor: true,
-      billingAttribution: { ...ORGANIZATION_ATTRIBUTION, actorUserId: 'delegated-actor-1' } as any,
+      userId: 'suspended-resumer',
+      billingAttribution: { ...ORGANIZATION_ATTRIBUTION, actorUserId: 'original-actor-1' } as any,
     })
 
-    expect(result.success).toBe(true)
-    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['delegated-actor-1', 'owner-1'])
+    expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith([
+      'original-actor-1',
+      'suspended-resumer',
+    ])
+    expect(result).toMatchObject({
+      success: false,
+      error: { statusCode: 403, message: 'Account suspended' },
+    })
   })
 
   it('excludes the "unknown" sentinel userId', async () => {
@@ -533,34 +545,25 @@ describe('preprocessExecution ban gate', () => {
   })
 
   /**
-   * 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.
+   * The webhook and deployed-chat shape: `userId` names the workflow owner or
+   * the chat's creator, so a ban on them must not take down automation their
+   * teammates depend on. Those call sites declare it explicitly rather than the
+   * gate inferring it.
    */
-  it('does not block a system-triggered run because the workflow owner is banned', async () => {
+  it('skips a userId the caller declares a stored reference', async () => {
     mockGetActivelyBannedUserIds.mockImplementation(async (ids: string[]) =>
       ids.filter((id) => id === 'creator-1')
     )
 
-    const result = await preprocessExecution({ ...baseOptions, userId: 'creator-1' })
+    const result = await preprocessExecution({
+      ...baseOptions,
+      userId: 'creator-1',
+      userIdIsStoredReference: true,
+    })
 
     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

@@ -97,6 +97,18 @@ export interface PreprocessExecutionOptions {
   triggerData?: SessionStartParams['triggerData']
   /** Use the authenticated user as actor for client executions and personal API keys. */
   useAuthenticatedUserAsActor?: boolean
+  /**
+   * Declares that `userId` names a stored reference — a workflow owner, a chat's
+   * creator — rather than someone who just acted, so the suspension gate skips
+   * it. Suspending one member must not take down the schedules, webhooks, and
+   * deployed chats their teammates depend on merely because that person's name
+   * sits on the row.
+   *
+   * Defaults to false so an unset call site keeps blocking. Withholding the
+   * suspended account's personal variables is handled separately, in
+   * {@link getExecutionEnvironment}.
+   */
+  userIdIsStoredReference?: boolean
   /** Pre-fetched workflow row for caller context; preprocessing still re-checks active state. */
   workflowRecord?: WorkflowRecord
   /**
@@ -189,6 +201,7 @@ export async function preprocessExecution(
     loggingSession: providedLoggingSession,
     triggerData,
     useAuthenticatedUserAsActor = false,
+    userIdIsStoredReference = false,
     workflowRecord: prefetchedWorkflowRecord,
     billingAttribution: providedBillingAttribution,
     executionType = 'sync',
@@ -452,23 +465,25 @@ export async function preprocessExecution(
      * Blocks when an identity this run actually acts as has an active ban or
      * blocked email domain.
      *
-     * `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.
+     * `userId` is a candidate unless the caller declares it a stored reference.
+     * The default is deliberately the blocking one: callers overload the
+     * parameter, and only the caller knows which kind it passed, so a call site
+     * that forgets to say must fail closed rather than silently admit a
+     * suspended account.
+     *
+     * `useAuthenticatedUserAsActor` cannot stand in for that declaration, which
+     * an earlier revision of this gate assumed. Resume passes the live
+     * authenticated resumer as `userId` and leaves that flag false on purpose —
+     * attribution is captured before the pause and must not move — so keying on
+     * it excluded exactly the person who just acted.
      *
-     * A stored pointer being banned must not take down work their teammates
+     * A stored reference 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.
+     * which is why {@link getExecutionEnvironment} drops a suspended identity's
+     * personal namespace rather than this gate blocking the whole run.
      */
     const banCandidateIds = [actorUserId]
-    if (useAuthenticatedUserAsActor && userId && userId !== 'unknown' && userId !== actorUserId) {
+    if (!userIdIsStoredReference && userId && userId !== 'unknown' && userId !== actorUserId) {
       banCandidateIds.push(userId)
     }
     try {

apps/sim/lib/webhooks/processor.ts

@@ -611,6 +611,8 @@ export async function checkWebhookPreprocessing(
     const preprocessResult = await preprocessExecution({
       workflowId: foundWorkflow.id,
       userId: foundWorkflow.userId,
+      // The workflow owner, not whoever sent this delivery — nobody sent it.
+      userIdIsStoredReference: true,
       triggerType: 'webhook',
       executionId,
       requestId,