fix(execution): enforce suspension on every personal-secret path, and align webhook cleanup

Review round 2 (Greptile P1 security, cubic P0 — both found the same gap).

The suspension check sat next to the split-identity access lookups, so it was
skipped by the single-identity shortcut above it. That shortcut is taken whenever
the two identities coincide — which is exactly what happens when a custom-block
publisher is also their workspace's billing account. The check now runs before
the shortcut, unconditionally.

Round 1's placement rested on "admission already cleared this identity", and that
is not true everywhere: a custom-block child is admitted by
`admitCustomBlockChildExecution`, which checks usage limits and nothing else, and
a provider URL-validation challenge resolves its secret with no admission at all.
Neither path has ever had a ban gate.

Only the personal namespace is withheld. Workspace variables belong to the
workspace rather than to a person, so they keep resolving and a suspended
member's teammates keep working — the reason admission stopped blocking on this
identity to begin with.

Webhook cleanup now resolves through the same two-identity reader as delivery.
Reading both slices as the owner let cleanup see a narrower selection than the
delivery that created the subscription: a non-admin owner without a credential
grant left `{{VAR}}` unresolved, the provider was handed the literal reference as
its credential, and the non-fatal catch silently orphaned the subscription.

`resolveBackgroundWebhookEnv` imports the billing reader statically. The dynamic
import bought nothing — every boundary audit passes without it — and made each
worker pay a cold module load on the first webhook resolution.

Restores the `@sim/testing` mock-shape test, which pins that the default snapshot
carries every field of the real one.

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

Author: icecrasher321

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

@@ -640,6 +640,35 @@ describe('getExecutionEnvironment', () => {
     expect(snapshot.workspaceDecrypted).toEqual({ WORKSPACE_KEY: 'plain:workspace-cipher' })
   })
 
+  /**
+   * The arrangement that slipped past a split-path-only check: a custom-block
+   * publisher who is also their workspace's billing account makes both
+   * identities equal, taking the single-identity shortcut. That path has no
+   * admission gate at all — `admitCustomBlockChildExecution` checks usage limits
+   * and nothing else — so the suspension has to be enforced here.
+   */
+  it('withholds the personal namespace when both identities are the same suspended user', async () => {
+    grantAdminTo('publisher-1')
+    mockGetActivelyBannedUserIds.mockResolvedValue(['publisher-1'])
+    queueTableRows(environment, [{ variables: { PUBLISHER_KEY: 'publisher-cipher' } }])
+    queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }])
+
+    const snapshot = await getExecutionEnvironment('publisher-1', 'publisher-1', 'workspace-1')
+
+    expect(snapshot.personalDecrypted).toEqual({})
+    expect(snapshot.workspaceDecrypted).toEqual({ WORKSPACE_KEY: 'plain:workspace-cipher' })
+  })
+
+  /** A workspaceless run has no workspace slice either, so a suspended identity lends nothing. */
+  it('resolves nothing personal for a suspended identity with no workspace', async () => {
+    mockGetActivelyBannedUserIds.mockResolvedValue(['suspended-1'])
+    queueTableRows(environment, [{ variables: { PERSONAL_KEY: 'personal-cipher' } }])
+
+    const snapshot = await getExecutionEnvironment('suspended-1', 'suspended-1', undefined)
+
+    expect(snapshot.personalDecrypted).toEqual({})
+  })
+
   /** 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')

apps/sim/lib/environment/utils.ts

@@ -490,14 +490,39 @@ export async function getExecutionEnvironment(
     return toWorkspaceOnlySnapshot(await getPersonalAndWorkspaceEnv(workspaceUserId, workspaceId))
   }
 
+  /**
+   * A suspended account lends nothing, from any path.
+   *
+   * Checked before the single-identity shortcut below rather than alongside the
+   * access lookups, because "the caller already cleared this identity" does not
+   * hold everywhere: a custom-block child is admitted by
+   * `admitCustomBlockChildExecution`, which checks usage limits and nothing
+   * else, and a provider URL-validation challenge resolves with no admission at
+   * all. Behind the shortcut, a publisher who is also their workspace's billing
+   * account made both identities equal and skipped the gate entirely — the one
+   * arrangement where suspension was silently ignored.
+   *
+   * Only the personal namespace is withheld. Workspace variables belong to the
+   * workspace rather than to a person, so they keep resolving and the runs a
+   * suspended member's teammates depend on keep working — which is the whole
+   * reason admission stopped blocking on this identity in the first place.
+   */
+  if ((await getActivelyBannedUserIds([personalUserId])).length > 0) {
+    logger.error('Personal-environment identity is suspended; resolving workspace variables only', {
+      personalUserId,
+      workspaceUserId,
+      workspaceId,
+    })
+    return toWorkspaceOnlySnapshot(await getPersonalAndWorkspaceEnv(workspaceUserId, workspaceId))
+  }
+
   if (!workspaceId || workspaceUserId === personalUserId) {
     return getPersonalAndWorkspaceEnv(personalUserId, workspaceId)
   }
 
-  const [actorAccess, personalAccess, suspendedPersonalIds] = await Promise.all([
+  const [actorAccess, personalAccess] = await Promise.all([
     checkWorkspaceAccess(workspaceId, workspaceUserId),
     checkWorkspaceAccess(workspaceId, personalUserId),
-    getActivelyBannedUserIds([personalUserId]),
   ])
 
   /**
@@ -509,23 +534,7 @@ export async function getExecutionEnvironment(
     throw new Error(`Workspace ${workspaceId} does not exist`)
   }
 
-  /**
-   * 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 (!personalAccess.hasAccess) {
     if (!actorAccess.hasAccess) {
       logger.error('Neither execution identity can reach the workspace', {
         personalUserId,
@@ -536,9 +545,7 @@ export async function getExecutionEnvironment(
     }
 
     logger.error(
-      personalIdentitySuspended
-        ? 'Personal-environment identity is suspended; resolving workspace variables only'
-        : 'Personal-environment identity cannot reach the workspace; resolving workspace variables only',
+      'Personal-environment identity cannot reach the workspace; resolving workspace variables only',
       { personalUserId, workspaceUserId, workspaceId }
     )
     return toWorkspaceOnlySnapshot(

apps/sim/lib/webhooks/env-resolver.ts

@@ -1,4 +1,5 @@
 import { isRecordLike } from '@sim/utils/object'
+import { getWorkspaceBilledAccountUserId } from '@/lib/billing/core/billing-attribution'
 import { getEffectiveDecryptedEnv, getExecutionEnvironment } from '@/lib/environment/utils'
 import { resolveEnvVarReferences } from '@/executor/utils/reference-validation'
 
@@ -30,7 +31,6 @@ export async function resolveBackgroundWebhookEnv(
     return getEffectiveDecryptedEnv(workflowOwnerUserId)
   }
 
-  const { getWorkspaceBilledAccountUserId } = await import('@/lib/billing/core/billing-attribution')
   const billedAccountUserId = await getWorkspaceBilledAccountUserId(workspaceId)
   if (!billedAccountUserId) {
     return getEffectiveDecryptedEnv(workflowOwnerUserId, workspaceId)

apps/sim/lib/webhooks/provider-subscriptions.ts

@@ -3,6 +3,7 @@ import { toError } from '@sim/utils/errors'
 import { omit } from '@sim/utils/object'
 import type { NextRequest } from 'next/server'
 import {
+  resolveBackgroundWebhookEnv,
   resolveWebhookProviderConfig,
   resolveWebhookRecordProviderConfig,
 } from '@/lib/webhooks/env-resolver'
@@ -172,8 +173,15 @@ export async function createExternalWebhookSubscription(
 
 /**
  * Clean up external webhook subscriptions for a webhook.
- * Resolves persisted `{{ENV_VAR}}` references with the workflow owner's
- * effective environment before invoking the provider.
+ *
+ * Resolves persisted `{{ENV_VAR}}` references the same way the delivery that
+ * created the subscription resolved them — owner for personal variables, the
+ * workspace billing account for workspace ones. Reading both slices as the owner
+ * meant cleanup could see a narrower selection than execution did: a non-admin
+ * owner without a credential grant for the referenced key left `{{VAR}}`
+ * unresolved (`onMissing` defaults to `keep`), and the provider was then handed
+ * the literal reference as its credential. Since the failure below is non-fatal
+ * by default, that silently orphaned the subscription at the provider.
  *
  * By default, cleanup failure is logged but non-fatal for legacy best-effort callers.
  * Deployment outbox cleanup passes `throwOnError` so provider failures stay retryable.
@@ -197,10 +205,12 @@ export async function cleanupExternalWebhook(
     }
 
     const workspaceId = typeof workflow.workspaceId === 'string' ? workflow.workspaceId : undefined
+    const envVars = await resolveBackgroundWebhookEnv(workflow.userId, workspaceId)
     const resolvedWebhook = await resolveWebhookRecordProviderConfig(
       webhook,
       workflow.userId,
-      workspaceId
+      workspaceId,
+      { envVars }
     )
 
     await handler.deleteSubscription({

packages/testing/src/mocks/environment-utils.mock.test.ts

@@ -10,6 +10,13 @@ describe('environment-utils mock', () => {
     resetEnvironmentUtilsMock()
   })
 
+  /**
+   * The default must carry EVERY field of the real `EnvironmentResolutionSnapshot`,
+   * including `personalOwners` and `workspaceUnredactedKeys`. A mirror that omits
+   * one lets a mocked snapshot reach production code as `undefined` and fail
+   * there instead of in the assertion, which is the opposite of what a default
+   * should do.
+   */
   it('defaults model a user with no environment variables', async () => {
     await expect(environmentUtilsMock.getEnvironmentVariableKeys('user-1')).resolves.toEqual({
       variableNames: [],
@@ -21,16 +28,20 @@ describe('environment-utils mock', () => {
       workspaceEncrypted: {},
       personalDecrypted: {},
       workspaceDecrypted: {},
+      personalOwners: {},
       conflicts: [],
       decryptionFailures: [],
+      workspaceUnredactedKeys: [],
     })
     await expect(environmentUtilsMock.getEffectiveEnvironmentSnapshot('user-1')).resolves.toEqual({
       personalEncrypted: {},
       workspaceEncrypted: {},
       personalDecrypted: {},
       workspaceDecrypted: {},
+      personalOwners: {},
       conflicts: [],
       decryptionFailures: [],
+      workspaceUnredactedKeys: [],
     })
     await expect(
       environmentUtilsMock.getEffectiveEnvironmentVariableNames('user-1')