fix(webhooks): route every background env resolution through the identity resolver

Review round 3 (cubic P1).

`resolveBackgroundWebhookEnv` short-circuited to `getEffectiveDecryptedEnv` for
the two cases with no second identity — a legacy workspaceless webhook, and a
workspace with no billing account — which read the owner's variables without
passing the resolver's suspension check. cubic flagged the first; the second was
the same bypass one line down. Both now name the owner as both identities and go
through the resolver, which produces the identical resolution while putting them
behind the same gate.

Also corrects `provider-subscriptions.test.ts`, which still asserted cleanup
resolves via `getEffectiveDecryptedEnv`. That assertion was passing
intermittently rather than failing outright: `mockGetEffectiveDecryptedEnv` is a
shared singleton on `environmentUtilsMockFns`, so whether it had been called
depended on which other files shared the worker. It now asserts the two-identity
call, with the billing reader mocked so the split is actually exercised.

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

Author: icecrasher321

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

@@ -153,21 +153,44 @@ describe('resolveBackgroundWebhookEnv', () => {
     expect(env).toEqual({ SHARED: 'workspace' })
   })
 
-  it('falls back to the single identity when the workspace has no billing account', async () => {
+  /**
+   * Both degenerate cases still go through the resolver, naming the owner as
+   * both identities. Short-circuiting them to `getEffectiveDecryptedEnv` read the
+   * owner's variables without passing the resolver's suspension check.
+   */
+  it('names the owner as both identities when the workspace has no billing account', async () => {
     mockGetWorkspaceBilledAccountUserId.mockResolvedValue(null)
 
     const env = await resolveBackgroundWebhookEnv('owner-1', 'workspace-1')
 
-    expect(mockGetExecutionEnvironment).not.toHaveBeenCalled()
-    expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('owner-1', 'workspace-1')
-    expect(env).toEqual({ FROM_SINGLE_IDENTITY: 'single' })
+    expect(mockGetExecutionEnvironment).toHaveBeenCalledWith('owner-1', 'owner-1', 'workspace-1')
+    expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled()
+    expect(env).toEqual({ OWNER_KEY: 'owner-value', WORKSPACE_KEY: 'workspace-value' })
   })
 
-  it('resolves a workspaceless webhook against the owner alone', async () => {
+  it('routes a workspaceless webhook through the resolver too', async () => {
     const env = await resolveBackgroundWebhookEnv('owner-1')
 
     expect(mockGetWorkspaceBilledAccountUserId).not.toHaveBeenCalled()
-    expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('owner-1')
-    expect(env).toEqual({ FROM_SINGLE_IDENTITY: 'single' })
+    expect(mockGetExecutionEnvironment).toHaveBeenCalledWith('owner-1', 'owner-1', undefined)
+    expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled()
+    expect(env).toEqual({ OWNER_KEY: 'owner-value', WORKSPACE_KEY: 'workspace-value' })
+  })
+
+  /** A suspended owner contributes nothing, including on the workspaceless path. */
+  it('yields no personal variables for a suspended owner with no workspace', async () => {
+    mockGetExecutionEnvironment.mockResolvedValue({
+      personalDecrypted: {},
+      workspaceDecrypted: {},
+    })
+
+    const env = await resolveBackgroundWebhookEnv('suspended-owner')
+
+    expect(mockGetExecutionEnvironment).toHaveBeenCalledWith(
+      'suspended-owner',
+      'suspended-owner',
+      undefined
+    )
+    expect(env).toEqual({})
   })
 })

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

@@ -20,25 +20,26 @@ export interface WebhookEnvResolutionOptions {
  * because every caller here treats an unresolvable secret as a rejected request
  * rather than an error.
  *
- * Falls back to owner-as-both when the workspace has no billing account or no
- * workspace is involved at all, which is exactly the previous behavior.
+ * Every case goes through {@link getExecutionEnvironment}, including the two
+ * that have no second identity to split against — a legacy workspaceless
+ * webhook, and a workspace with no billing account. Returning
+ * `getEffectiveDecryptedEnv` directly for those read the owner's variables
+ * without passing the resolver's suspension check, so the one arrangement that
+ * still lent a suspended account's secrets was the one with the least going on.
+ * Naming the owner as both identities keeps the resolution identical to what
+ * those cases produced before while putting them behind the same gate.
  */
 export async function resolveBackgroundWebhookEnv(
   workflowOwnerUserId: string,
   workspaceId?: string
 ): Promise<Record<string, string>> {
-  if (!workspaceId) {
-    return getEffectiveDecryptedEnv(workflowOwnerUserId)
-  }
-
-  const billedAccountUserId = await getWorkspaceBilledAccountUserId(workspaceId)
-  if (!billedAccountUserId) {
-    return getEffectiveDecryptedEnv(workflowOwnerUserId, workspaceId)
-  }
+  const billedAccountUserId = workspaceId
+    ? await getWorkspaceBilledAccountUserId(workspaceId)
+    : null
 
   const snapshot = await getExecutionEnvironment(
     workflowOwnerUserId,
-    billedAccountUserId,
+    billedAccountUserId ?? workflowOwnerUserId,
     workspaceId
   )
   return { ...snapshot.personalDecrypted, ...snapshot.workspaceDecrypted }

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

@@ -6,18 +6,23 @@ import { environmentUtilsMockFns, resetEnvironmentUtilsMock } from '@sim/testing
 import type { NextRequest } from 'next/server'
 import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
 
-const { mockGetEffectiveDecryptedEnv } = environmentUtilsMockFns
+const { mockGetEffectiveDecryptedEnv, mockGetExecutionEnvironment } = environmentUtilsMockFns
 
 afterAll(resetEnvironmentUtilsMock)
 
-const { mockGetProviderHandler } = vi.hoisted(() => ({
+const { mockGetProviderHandler, mockGetWorkspaceBilledAccountUserId } = vi.hoisted(() => ({
   mockGetProviderHandler: vi.fn(),
+  mockGetWorkspaceBilledAccountUserId: vi.fn(),
 }))
 
 vi.mock('@/lib/webhooks/providers', () => ({
   getProviderHandler: mockGetProviderHandler,
 }))
 
+vi.mock('@/lib/billing/core/billing-attribution', () => ({
+  getWorkspaceBilledAccountUserId: mockGetWorkspaceBilledAccountUserId,
+}))
+
 import {
   cleanupExternalWebhook,
   createExternalWebhookSubscription,
@@ -128,8 +133,20 @@ describe('cleanupExternalWebhook', () => {
   beforeEach(() => {
     vi.clearAllMocks()
     mockGetEffectiveDecryptedEnv.mockResolvedValue({ CALENDLY_API_KEY: 'real-secret-key' })
+    mockGetWorkspaceBilledAccountUserId.mockResolvedValue('billing-1')
+    mockGetExecutionEnvironment.mockResolvedValue({
+      personalDecrypted: {},
+      workspaceDecrypted: { CALENDLY_API_KEY: 'real-secret-key' },
+    })
   })
 
+  /**
+   * Cleanup resolves through the same two-identity reader as the delivery that
+   * created the subscription — owner for personal variables, the workspace
+   * billing account for workspace ones. Reading both slices as the owner let a
+   * non-admin owner without a credential grant leave `{{VAR}}` unresolved, and
+   * the provider was handed the literal reference as its credential.
+   */
   it('resolves {{ENV_VAR}} references before deleting the provider subscription', async () => {
     const deleteSubscription = vi.fn().mockResolvedValue(undefined)
     mockGetProviderHandler.mockReturnValue({ deleteSubscription })
@@ -150,7 +167,7 @@ describe('cleanupExternalWebhook', () => {
 
     await cleanupExternalWebhook(webhook, workflow, 'request-1')
 
-    expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('user-1', 'workspace-1')
+    expect(mockGetExecutionEnvironment).toHaveBeenCalledWith('user-1', 'billing-1', 'workspace-1')
     expect(deleteSubscription).toHaveBeenCalledWith(
       expect.objectContaining({
         webhook: expect.objectContaining({