From c2be0407ba1c04a2bb6ceb3adae23a43276b035d Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Mon, 31 Aug 2026 18:02:33 -0700 Subject: [PATCH 1/5] fix(execution): stop treating the workflow owner as a live execution identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A background run acts as the workspace billing account; `workflow.userId` is only the personal-variable fallback. Several surfaces treated that stored pointer as a live permission, so each broke when its owner left the workspace. Deployed chat read `chat.userId` — the person who clicked "Deploy as chat" — where every other trigger reads `workflow.userId`. Org member removal reassigns `workflow.userId` to keep it an active workspace identity and has no equivalent for the chat row, so the same transaction repaired the pointer every other trigger reads and broke the only one chat read. Chat now passes the owner. `getExecutionEnvironment` already tolerated a stale actor but not a stale personal identity. Both are stored pointers, so a personal identity that cannot reach the workspace now contributes no personal namespace — the judgment already applied to an anonymous public-API run, and it stops lending a removed member's secrets to their former organization. Only "neither identity reachable" raises. The public API gated `validatePublicApiAllowed` and the workflow read on the owner, though an anonymous call acts as the billing account and resolves no personal variables at all. Both now use `getWorkspaceBilledAccountUserId`. The enable-time gate, which checks the acting user, is unchanged. Custom-block children and webhook provider-config resolved both environment slices as the owner. They now split the two identities like any deployed run, which also closes a silent inconsistency: a custom block saw a narrower workspace-secret selection than a schedule on the very same workflow. The ban gate no longer blocks on the workflow owner — banning one member should not take down the schedules, webhooks, and chats their teammates depend on. Logs gain a run-level `executedByEmail`, joined from the immutable per-run attribution rather than from a workflow row that ownership transfer rewrites. `workflow.ownerEmail` stays as a deprecated field, fed by its own aliased join, because it is required in the published v2 schema. Co-Authored-By: Claude Opus 5 (1M context) --- .../content/docs/platform/credentials.mdx | 7 +- apps/docs/openapi-v2-logs.json | 18 ++- apps/sim/app/api/chat/[identifier]/route.ts | 11 +- apps/sim/app/api/v1/logs/[id]/route.ts | 1 - .../sim/app/api/v2/logs/[runId]/route.test.ts | 9 ++ apps/sim/app/api/v2/logs/[runId]/route.ts | 2 + .../[workflowId]/execute/route.test.ts | 42 ++++--- .../workflows/[workflowId]/execute/route.ts | 20 +++- .../[id]/execute/route.async.test.ts | 4 +- .../app/api/workflows/[id]/execute/route.ts | 19 ++- apps/sim/background/webhook-execution.test.ts | 16 ++- apps/sim/background/webhook-execution.ts | 31 ++++- .../workflow/workflow-handler.test.ts | 68 +++++++++++ .../handlers/workflow/workflow-handler.ts | 60 +++++++--- apps/sim/lib/api/contracts/v2/logs.ts | 26 +++- apps/sim/lib/api/contracts/v2/openapi/logs.ts | 7 ++ .../lib/billing/core/billing-attribution.ts | 24 ++++ apps/sim/lib/environment/utils.test.ts | 94 ++++++++++++++- apps/sim/lib/environment/utils.ts | 113 ++++++++++++++---- apps/sim/lib/execution/preprocessing.test.ts | 30 +++-- apps/sim/lib/execution/preprocessing.ts | 16 ++- .../lib/logs/application/get-public-log.ts | 11 +- apps/sim/lib/logs/fetch-log-detail.ts | 2 - apps/sim/lib/logs/list-logs.ts | 2 - apps/sim/lib/logs/public-queries.ts | 33 ++++- apps/sim/lib/webhooks/env-resolver.test.ts | 70 ++++++++++- apps/sim/lib/webhooks/env-resolver.ts | 39 +++++- apps/sim/lib/webhooks/providers/zoom.ts | 12 +- .../lib/workflows/custom-blocks/operations.ts | 22 +++- .../src/mocks/environment-utils.mock.ts | 14 +++ 30 files changed, 705 insertions(+), 118 deletions(-) diff --git a/apps/docs/content/docs/platform/credentials.mdx b/apps/docs/content/docs/platform/credentials.mdx index f91b41c6630..922080c7a98 100644 --- a/apps/docs/content/docs/platform/credentials.mdx +++ b/apps/docs/content/docs/platform/credentials.mdx @@ -177,11 +177,13 @@ When a workflow runs, secrets resolve in this order: | Run started by | Personal secrets come from | | --- | --- | | Clicking Run, or a personal API key | The person running it | -| A workspace API key, schedule, or webhook | The workflow owner | +| A workspace API key, schedule, webhook, or deployed chat | The workflow owner | | A public API URL with no authentication | Nobody — personal secrets do not resolve | The workflow owner is the fallback only where nobody can be identified but somebody in the workspace set the trigger up, since those workflows are usually built against the owner's own keys. A public URL can be called by anyone, so it never borrows a person's keys at all — put every secret such a workflow needs in **Workspace**. +If the workflow owner later leaves the workspace, the run keeps working: it resolves workspace secrets as normal and simply resolves no personal ones, so any block that needed a personal secret fails on its own with the missing key named. Move that secret to **Workspace** to fix it for good. + ## Best Practices - **Use workspace secrets for production** so workflows work regardless of who triggers them @@ -193,7 +195,8 @@ The workflow owner is the fallback only where nobody can be identified but someb { question: "Are my secrets encrypted at rest?", answer: "Yes. Values saved under Secrets are encrypted before being stored in the database." }, { question: "Can a saved secret still appear in a workflow result?", answer: "Yes. Functional workflow data is not rewritten, so the raw value can still reach downstream blocks and tools and can appear in workflow execution responses, streams, or callbacks if your workflow deliberately returns or prints it. Log-facing views and read APIs receive a protected copy after a successful {{KEY}} resolution. Before content is sent to a model, exact values from the run's authorized secret catalog are replaced with placeholders, but encoded or otherwise transformed values remain outside that protection." }, { question: "What happens if both a workspace secret and a personal secret have the same key name?", answer: "Among secrets available to the execution actor, the workspace secret takes precedence and the personal secret is the fallback. An inaccessible workspace secret does not shadow an authorized personal value." }, - { question: "Who determines which personal secret is used for automated runs?", answer: "Whoever is running it, when that can be identified. Clicking Run or calling with a personal API key uses that person's personal secrets. A workspace API key, schedule, or webhook has no identifiable caller, so it falls back to the workflow owner's — those triggers are set up inside the workspace and the workflow is usually built against the owner's own keys. A public API URL with no authentication can be called by anyone, so no personal secrets resolve at all — those workflows run on workspace secrets only." }, + { question: "Who determines which personal secret is used for automated runs?", answer: "Whoever is running it, when that can be identified. Clicking Run or calling with a personal API key uses that person's personal secrets. A workspace API key, schedule, webhook, or deployed chat has no identifiable caller, so it falls back to the workflow owner's — those triggers are set up inside the workspace and the workflow is usually built against the owner's own keys. A public API URL with no authentication can be called by anyone, so no personal secrets resolve at all — those workflows run on workspace secrets only." }, + { question: "What happens to automated runs if the workflow owner leaves the workspace?", answer: "They keep running. Workspace secrets resolve as normal, because they are checked against the workspace's billing account rather than the owner. Personal secrets stop resolving, so any block that referenced one fails with that key named — move it to Workspace to fix it permanently." }, { question: "Can I import secrets from a .env file?", answer: "Yes. Paste .env-style content (KEY=VALUE format) into any key or value field and the secrets will be auto-populated. The parser supports export KEY=VALUE, quoted values, and inline comments." }, { question: "What happens if I delete a secret that is used in a workflow?", answer: "The workflow will fail at any block that references the deleted secret during execution because the value cannot be resolved. Update any references before deleting a secret." }, ]} /> diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 3f69f6dd896..6127a605f7b 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -1344,6 +1344,19 @@ ], "description": "Files the run produced, or null when none are recorded. Only the run's own output files appear; input attachments a caller supplied are addressed through the files API instead." }, + "executedByEmail": { + "anyOf": [ + { + "type": "string", + "format": "email", + "pattern": "^(?!\\.)(?!.*\\.\\.)([A-Za-z0-9_'+\\-\\.]*)[A-Za-z0-9_+-]@([A-Za-z0-9][A-Za-z0-9\\-]*\\.)+[A-Za-z]{2,}$" + }, + { + "type": "null" + } + ], + "description": "Email of the identity the run executed as: the caller for an interactive or personal-API-key run, and the workspace billing account for a schedule, webhook, deployed chat, or public API call. Null when the run failed before an identity was resolved." + }, "workflow": { "type": "object", "properties": { @@ -1398,7 +1411,8 @@ "type": "null" } ], - "description": "Workflow owner email, or null when unavailable." + "description": "Deprecated — use the run-level `executedByEmail` instead. Email of the workflow's current owner, or null when unavailable. This is a property of the workflow as it stands today, not of the run: it changes when workflow ownership is reassigned, and the owner is not the identity a background run executes as.", + "deprecated": true }, "workspaceId": { "anyOf": [ @@ -1577,6 +1591,7 @@ "endedAt", "totalDurationMs", "files", + "executedByEmail", "workflow", "workflowState", "traceSpans", @@ -1614,6 +1629,7 @@ "endedAt": "2026-01-15T10:30:01.250Z", "totalDurationMs": 1250, "files": null, + "executedByEmail": "billing@example.com", "workflow": { "id": "3b1f7c92-8d4e-4a6b-9c0d-5e2f8a714b36", "name": "Customer Support Agent", diff --git a/apps/sim/app/api/chat/[identifier]/route.ts b/apps/sim/app/api/chat/[identifier]/route.ts index dedc480fc2f..41e282ebd17 100644 --- a/apps/sim/app/api/chat/[identifier]/route.ts +++ b/apps/sim/app/api/chat/[identifier]/route.ts @@ -273,7 +273,16 @@ export const POST = withRouteHandler( const workflowForExecution = { id: deployment.workflowId, - userId: deployment.userId, + /** + * The workflow owner, not the chat's creator: `executeWorkflow` reads this + * one field to set `workflowUserId`, the personal-environment fallback for + * runs with no identifiable caller. `chat.userId` records who deployed the + * chat and is never maintained as an execution identity — member removal + * reassigns `workflow.userId` to keep it an active workspace identity and + * has no equivalent for the chat row — so reading it here made deployed + * chat resolve a pointer that every other trigger had already repaired. + */ + userId: workflowRecord.userId, workspaceId, isDeployed: workflowRecord?.isDeployed ?? false, variables: (workflowRecord?.variables as Record) ?? undefined, diff --git a/apps/sim/app/api/v1/logs/[id]/route.ts b/apps/sim/app/api/v1/logs/[id]/route.ts index 12066f2f9cc..5d5c3639093 100644 --- a/apps/sim/app/api/v1/logs/[id]/route.ts +++ b/apps/sim/app/api/v1/logs/[id]/route.ts @@ -51,7 +51,6 @@ export const GET = withRouteHandler( name: log.workflowName || 'Deleted Workflow', description: log.workflowDescription, folderId: log.workflowFolderId, - userId: log.workflowUserId, workspaceId: log.workflowWorkspaceId, createdAt: log.workflowCreatedAt, updatedAt: log.workflowUpdatedAt, diff --git a/apps/sim/app/api/v2/logs/[runId]/route.test.ts b/apps/sim/app/api/v2/logs/[runId]/route.test.ts index a0f466cfb8e..8da71ead2e7 100644 --- a/apps/sim/app/api/v2/logs/[runId]/route.test.ts +++ b/apps/sim/app/api/v2/logs/[runId]/route.test.ts @@ -49,6 +49,7 @@ const log = { files: null, workflowName: 'Support Agent', workflowDescription: null, + executedByEmail: 'actor@example.com', workflowOwnerEmail: 'owner@example.com', workflowWorkspaceId: 'workspace-1', workflowCreatedAt: new Date('2026-01-01T00:00:00Z'), @@ -86,8 +87,16 @@ describe('GET /api/v2/logs/[runId]', () => { const body = await response.json() expect(response.status).toBe(200) + /** + * The two identities are deliberately different people here. `ownerEmail` is + * deprecated but still a required field of the published schema, so it must + * keep resolving from the workflow owner rather than quietly aliasing to the + * executing identity — dropping its own join would make both read the same + * and break every client still on it. + */ expect(body.data).toMatchObject({ runId: 'run-1', + executedByEmail: 'actor@example.com', workflow: { folderPath: '/agents', ownerEmail: 'owner@example.com' }, finalOutput: { ok: true }, }) diff --git a/apps/sim/app/api/v2/logs/[runId]/route.ts b/apps/sim/app/api/v2/logs/[runId]/route.ts index 5433825e6ae..a40163224c2 100644 --- a/apps/sim/app/api/v2/logs/[runId]/route.ts +++ b/apps/sim/app/api/v2/logs/[runId]/route.ts @@ -49,11 +49,13 @@ export const GET = defineV2JsonRoute({ endedAt: log.endedAt ? log.endedAt.toISOString() : null, totalDurationMs: log.totalDurationMs, files: projectLogFiles(log), + executedByEmail: log.executedByEmail, workflow: { id: log.workflowId, name: log.workflowName || 'Deleted Workflow', description: log.workflowDescription, folderPath: workflowFolderPath, + /** Deprecated in favour of the run-level `executedByEmail`. */ ownerEmail: log.workflowOwnerEmail, workspaceId: log.workflowWorkspaceId, createdAt: log.workflowCreatedAt ? log.workflowCreatedAt.toISOString() : null, diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.test.ts b/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.test.ts index abc19127e1d..d15821b6e27 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.test.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.test.ts @@ -218,6 +218,22 @@ function callPublicExecute(body: Record, headers: Record { it('runs the anonymous public path sync but refuses async', async () => { dbChainMockFns.limit.mockReset() - dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: true, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, - ]) + queuePublicWorkflowReads() const okRes = await callPublicExecute({ input: {} }) expect(okRes.status).toBe(200) @@ -774,18 +788,14 @@ describe('POST /api/v2/workflows/[workflowId]/execute', () => { expect.objectContaining({ rateLimitCounter: 'sync' }) ) - dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: true, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, - ]) + queuePublicWorkflowReads() const asyncRes = await callPublicExecute({ input: {}, async: true }) expect(asyncRes.status).toBe(400) }) it('never permits manual execution on the anonymous public path', async () => { dbChainMockFns.limit.mockReset() - dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: true, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, - ]) + queuePublicWorkflowReads() const response = await callPublicExecute({ run: { source: 'manual' } }) @@ -798,9 +808,7 @@ describe('POST /api/v2/workflows/[workflowId]/execute', () => { it('returns not found when a public workflow disappears before authorization', async () => { dbChainMockFns.limit.mockReset() - dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: true, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, - ]) + queuePublicWorkflowReads() mockAuthorize.mockResolvedValueOnce({ allowed: false, status: 404, @@ -837,7 +845,7 @@ describe('POST /api/v2/workflows/[workflowId]/execute', () => { it('401s non-public workflows without a key', async () => { dbChainMockFns.limit.mockReset() dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: false, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, + { isPublicApi: false, isDeployed: true, workspaceId: 'workspace-1' }, ]) const res = await callPublicExecute({ input: {} }) @@ -870,9 +878,7 @@ describe('POST /api/v2/workflows/[workflowId]/execute', () => { expect(keyedBody.error.message).toContain('Maximum workflow call chain depth (25) exceeded') dbChainMockFns.limit.mockReset() - dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: true, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, - ]) + queuePublicWorkflowReads() const anonymous = await callPublicExecute({ input: {} }, { 'X-Sim-Via': maxChain }) expect(anonymous.status).toBe(409) expect((await anonymous.json()).error.code).toBe('CONFLICT') @@ -891,9 +897,7 @@ describe('POST /api/v2/workflows/[workflowId]/execute', () => { ]) dbChainMockFns.limit.mockReset() - dbChainMockFns.limit.mockResolvedValueOnce([ - { isPublicApi: true, isDeployed: true, userId: 'owner-1', workspaceId: 'workspace-1' }, - ]) + queuePublicWorkflowReads() const anonymous = await callPublicExecute({ input: {} }, { 'X-Sim-Via': 'wf-a, wf-b' }) expect(anonymous.status).toBe(200) expect(mockExecuteWorkflowCore.mock.calls[1][0].snapshot.metadata.callChain).toEqual([ diff --git a/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.ts b/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.ts index 407c5f355f8..b3271d894a1 100644 --- a/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.ts +++ b/apps/sim/app/api/v2/workflows/[workflowId]/execute/route.ts @@ -21,6 +21,7 @@ import { v2RateLimits, } from '@/lib/api/server/routes' import type { V2ApiKeyPrincipal } from '@/lib/api/server/routes/v2-api-key-auth' +import { getWorkspaceBilledAccountUserId } from '@/lib/billing/core/billing-attribution' import { tryAdmit } from '@/lib/core/admission/gate' import { ADMISSION_ERROR_DESCRIPTOR } from '@/lib/core/admission/transient-failure' import type { ForbiddenDetailCode } from '@/lib/core/application' @@ -173,7 +174,6 @@ export const POST = withRouteHandler( .select({ isPublicApi: workflowTable.isPublicApi, isDeployed: workflowTable.isDeployed, - userId: workflowTable.userId, workspaceId: workflowTable.workspaceId, }) .from(workflowTable) @@ -183,15 +183,27 @@ export const POST = withRouteHandler( if (!wf?.isPublicApi || !wf.isDeployed || !wf.workspaceId) { return v2Error('UNAUTHORIZED', 'Unauthorized') } + /** + * An anonymous public-API call has no caller, so it acts as the workspace + * billing account — the identity preprocessing elects for exactly this + * case. The workflow owner is only the personal-variable fallback, and a + * public run resolves no personal variables at all, so gating on the + * owner's governance config and workspace read would fail a public + * endpoint the moment that stored pointer's access lapsed. + */ + const billedAccountUserId = await getWorkspaceBilledAccountUserId(wf.workspaceId) + if (!billedAccountUserId) { + return v2Error('UNAUTHORIZED', 'Unauthorized') + } try { - await validatePublicApiAllowed(wf.userId, wf.workspaceId) + await validatePublicApiAllowed(billedAccountUserId, wf.workspaceId) } catch (err) { if (err instanceof PublicApiNotAllowedError) { return v2Error('UNAUTHORIZED', 'Unauthorized') } throw err } - publicApiUserId = wf.userId + publicApiUserId = billedAccountUserId isPublicApiAccess = true } @@ -343,7 +355,7 @@ export const POST = withRouteHandler( } } else { if (!publicApiUserId) { - throw new Error('Public workflow execution is missing its owner') + throw new Error('Public workflow execution is missing its workspace billing account') } const workflowAuthorization = await authorizeWorkflowByWorkspacePermission({ workflowId, diff --git a/apps/sim/app/api/workflows/[id]/execute/route.async.test.ts b/apps/sim/app/api/workflows/[id]/execute/route.async.test.ts index 78112e19322..727879a89ad 100644 --- a/apps/sim/app/api/workflows/[id]/execute/route.async.test.ts +++ b/apps/sim/app/api/workflows/[id]/execute/route.async.test.ts @@ -38,6 +38,7 @@ import { const { mockAssertBillingAttributionSnapshot, + mockGetWorkspaceBilledAccountUserId, mockClaimExecutionId, mockClaimWorkflowToolExecution, mockCheckNeedsRedeployment, @@ -89,12 +90,14 @@ const { mockReleaseExecutionSlot: vi.fn(), mockReleaseWorkflowToolExecutionClaim: vi.fn(), mockRequireBillingAttributionHeader: vi.fn(), + mockGetWorkspaceBilledAccountUserId: vi.fn().mockResolvedValue('billing-1'), mockShouldExecuteInline: vi.fn().mockReturnValue(false), mockValidatePublicApiAllowed: vi.fn(), })) vi.mock('@/lib/billing/core/billing-attribution', () => ({ assertBillingAttributionSnapshot: mockAssertBillingAttributionSnapshot, + getWorkspaceBilledAccountUserId: mockGetWorkspaceBilledAccountUserId, requireBillingAttributionHeader: mockRequireBillingAttributionHeader, })) @@ -345,7 +348,6 @@ function configureExecutionCaller(caller: ExecutionCallerCase, requestCount = 1) { isPublicApi: true, isDeployed: true, - userId: 'owner-1', workspaceId: 'workspace-1', }, ]) diff --git a/apps/sim/app/api/workflows/[id]/execute/route.ts b/apps/sim/app/api/workflows/[id]/execute/route.ts index 61d028287e9..fab9a869074 100644 --- a/apps/sim/app/api/workflows/[id]/execute/route.ts +++ b/apps/sim/app/api/workflows/[id]/execute/route.ts @@ -20,6 +20,7 @@ import { releaseExecutionSlot } from '@/lib/billing/calculations/usage-reservati import { assertBillingAttributionSnapshot, type BillingAttributionSnapshot, + getWorkspaceBilledAccountUserId, requireBillingAttributionHeader, } from '@/lib/billing/core/billing-attribution' import { @@ -596,7 +597,6 @@ async function handleExecutePost( .select({ isPublicApi: workflowTable.isPublicApi, isDeployed: workflowTable.isDeployed, - userId: workflowTable.userId, workspaceId: workflowTable.workspaceId, }) .from(workflowTable) @@ -607,8 +607,21 @@ async function handleExecutePost( return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 }) } + /** + * An anonymous public-API call has no caller, so it acts as the workspace + * billing account — the identity preprocessing elects for exactly this + * case. The workflow owner is only the personal-variable fallback, and a + * public run resolves no personal variables at all, so gating on the + * owner's governance config would fail a public endpoint the moment that + * stored pointer's access lapsed. + */ + const billedAccountUserId = await getWorkspaceBilledAccountUserId(wf.workspaceId) + if (!billedAccountUserId) { + return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 }) + } + try { - await validatePublicApiAllowed(wf.userId, wf.workspaceId) + await validatePublicApiAllowed(billedAccountUserId, wf.workspaceId) } catch (err) { if (err instanceof PublicApiNotAllowedError) { return NextResponse.json({ error: auth.error || 'Unauthorized' }, { status: 401 }) @@ -616,7 +629,7 @@ async function handleExecutePost( throw err } - userId = wf.userId + userId = billedAccountUserId isPublicApiAccess = true } else { userId = auth.userId diff --git a/apps/sim/background/webhook-execution.test.ts b/apps/sim/background/webhook-execution.test.ts index b81cc566dba..a8643254eb1 100644 --- a/apps/sim/background/webhook-execution.test.ts +++ b/apps/sim/background/webhook-execution.test.ts @@ -54,8 +54,12 @@ const { } }) -const mockGetEffectiveEnvironmentSnapshot = - environmentUtilsMockFns.mockGetEffectiveEnvironmentSnapshot +/** + * The execution path resolves two identities (workflow owner for personal + * variables, run actor for workspace ones), so it goes through + * `getExecutionEnvironment` rather than the single-identity snapshot reader. + */ +const mockGetExecutionEnvironment = environmentUtilsMockFns.mockGetExecutionEnvironment afterAll(resetEnvironmentUtilsMock) @@ -299,7 +303,7 @@ describe('executeWebhookJob fault vs error handling', () => { executionTimeout: { async: 120_000 }, }) mockResolveWebhookRecordProviderConfig.mockImplementation(async (record) => record) - mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({ + mockGetExecutionEnvironment.mockResolvedValue({ personalEncrypted: {}, workspaceEncrypted: {}, personalDecrypted: {}, @@ -496,7 +500,7 @@ describe('executeWebhookJob fault vs error handling', () => { }) it('does not pass provider-config provenance absent from the trigger input', async () => { - mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({ + mockGetExecutionEnvironment.mockResolvedValue({ personalEncrypted: { WEBHOOK_SECRET: 'personal-ciphertext' }, workspaceEncrypted: { WEBHOOK_SECRET: 'workspace-ciphertext' }, personalDecrypted: { WEBHOOK_SECRET: 'personal-value' }, @@ -550,7 +554,7 @@ describe('executeWebhookJob fault vs error handling', () => { }) it('passes provider-config provenance when its value crosses in the trigger input', async () => { - mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({ + mockGetExecutionEnvironment.mockResolvedValue({ personalEncrypted: {}, workspaceEncrypted: { WEBHOOK_SECRET: 'workspace-ciphertext' }, personalDecrypted: {}, @@ -601,7 +605,7 @@ describe('executeWebhookJob fault vs error handling', () => { it('installs provenance before a post-resolution webhook setup failure', async () => { const rawMessage = 'Webhook handler exposed activated-secret-value' const rawError = new Error(rawMessage) - mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({ + mockGetExecutionEnvironment.mockResolvedValue({ personalEncrypted: {}, workspaceEncrypted: { WEBHOOK_SECRET: 'workspace-ciphertext' }, personalDecrypted: {}, diff --git a/apps/sim/background/webhook-execution.ts b/apps/sim/background/webhook-execution.ts index fa4d02d3788..fa82e7b244d 100644 --- a/apps/sim/background/webhook-execution.ts +++ b/apps/sim/background/webhook-execution.ts @@ -48,6 +48,7 @@ import { import { type EnvironmentResolutionSnapshot, getEffectiveEnvironmentSnapshot, + getExecutionEnvironment, } from '@/lib/environment/utils' import { preprocessExecution } from '@/lib/execution/preprocessing' import { LoggingSession } from '@/lib/logs/execution/logging-session' @@ -632,6 +633,21 @@ export async function executeWebhookJob( } } +/** + * Resolves `{{VAR}}` references inside a webhook's provider config. + * + * `userId` is the workflow owner, which is the personal-variable identity this + * config was authored against. `actorUserId` is who the run acts as, and the + * two are resolved separately for the same reason the executor resolves them + * separately: workspace variables authorize against the running identity, while + * personal ones stay with whoever owns them. Reading both slices as the owner — + * as this did — meant a webhook stopped resolving its own signing secret the + * moment that person left the workspace, even though the run itself was acting + * as the workspace billing account the whole time. + * + * Omitting `actorUserId` keeps the single-identity behavior, for callers with no + * run to speak of. + */ export async function resolveWebhookExecutionProviderConfig< T extends { id: string; providerConfig?: unknown }, >( @@ -641,6 +657,7 @@ export async function resolveWebhookExecutionProviderConfig< workspaceId?: string, options?: WebhookEnvResolutionOptions & { onEnvironmentSnapshot?: (snapshot: EnvironmentResolutionSnapshot) => void | Promise + actorUserId?: string } ): Promise }> { try { @@ -648,9 +665,12 @@ export async function resolveWebhookExecutionProviderConfig< return await resolveWebhookRecordProviderConfig(webhookRecord, userId, workspaceId) } - const { onEnvironmentSnapshot, ...resolutionOptions } = options + const { onEnvironmentSnapshot, actorUserId, ...resolutionOptions } = options if (onEnvironmentSnapshot && resolutionOptions.envVars === undefined) { - const snapshot = await getEffectiveEnvironmentSnapshot(userId, workspaceId) + const snapshot = + actorUserId && workspaceId + ? await getExecutionEnvironment(userId, actorUserId, workspaceId) + : await getEffectiveEnvironmentSnapshot(userId, workspaceId) await onEnvironmentSnapshot(snapshot) resolutionOptions.envVars = { ...snapshot.personalDecrypted, @@ -861,6 +881,13 @@ async function executeWebhookJobInternal( workflowRecord.userId, workspaceId, { + /** + * The identity preprocessing already elected for this run, so the + * provider config resolves against exactly the workspace variables the + * run's own blocks will see rather than against a second, narrower + * selection derived from the workflow owner. + */ + actorUserId, onEnvironmentSnapshot: async (secretEnvironment) => { try { resolvedSecretTraceRegistry = await createResolvedSecretTraceRegistry({ diff --git a/apps/sim/executor/handlers/workflow/workflow-handler.test.ts b/apps/sim/executor/handlers/workflow/workflow-handler.test.ts index b1dc669b4bf..e77cb455cae 100644 --- a/apps/sim/executor/handlers/workflow/workflow-handler.test.ts +++ b/apps/sim/executor/handlers/workflow/workflow-handler.test.ts @@ -725,6 +725,74 @@ describe('WorkflowBlockHandler', () => { }) }) + /** + * A custom block's child is a deployed run of the source workflow, so it + * must resolve secrets the way a schedule on that workflow does: personal + * variables from the publisher, workspace variables authorized against the + * source workspace's billing account. Reading both slices as the publisher + * gave the child a narrower workspace selection than the same workflow got + * on any other trigger, and failed outright once the publisher left. + */ + it('resolves a custom block child under the publisher plus the source billing account', async () => { + const customBlock = { + ...mockBlock, + metadata: { id: 'custom_block_abc', name: 'Published Block' }, + } + const ctx = { + ...mockContext, + workspaceId: 'workspace-consumer', + } as unknown as ExecutionContext + + mockGetCustomBlockAuthority.mockResolvedValue({ + workflowId: 'source-workflow-id', + organizationId: 'org-1', + ownerUserId: 'owner-9', + exposedOutputs: [{ blockId: 'b1', path: 'content', name: 'answer' }], + requiredInputIds: [], + }) + mockResolveBillingAttribution.mockResolvedValue({ + actorUserId: 'owner-9', + workspaceId: 'workspace-source', + billedAccountUserId: 'billing-account-9', + }) + mockFetch.mockImplementation(async (url: unknown) => { + if (String(url).includes('/deployed')) { + return { + ok: true, + json: () => + Promise.resolve({ + data: { + deployedState: { + blocks: {}, + edges: [], + loops: {}, + parallels: {}, + deploymentVersionId: 'deployment-version-1', + }, + }, + }), + } + } + return { + ok: true, + json: () => + Promise.resolve({ + data: { name: 'Source Workflow', workspaceId: 'workspace-source', variables: {} }, + }), + } + }) + mockCreateSnapshot.mockResolvedValue({ snapshot: { id: 'snapshot-1' } }) + mockExecutorExecute.mockResolvedValue({ success: true, output: { data: 'ok' } }) + + await handler.execute(ctx, customBlock, {}) + + expect(environmentUtilsMockFns.mockGetExecutionEnvironment).toHaveBeenCalledWith( + 'owner-9', + 'billing-account-9', + 'workspace-source' + ) + }) + it('builds trusted caller metadata for custom block children with the toggle on', async () => { const customBlock = { ...mockBlock, diff --git a/apps/sim/executor/handlers/workflow/workflow-handler.ts b/apps/sim/executor/handlers/workflow/workflow-handler.ts index c8a29c2fe8d..c339a8667d1 100644 --- a/apps/sim/executor/handlers/workflow/workflow-handler.ts +++ b/apps/sim/executor/handlers/workflow/workflow-handler.ts @@ -6,7 +6,7 @@ import type { Variable, WorkflowState } from '@sim/workflow-types/workflow' import { resolveBillingAttribution } from '@/lib/billing/core/billing-attribution' import { getExecutionDeadlineAt } from '@/lib/core/execution-limits' import { asOrchestrationError } from '@/lib/core/orchestration/types' -import { getPersonalAndWorkspaceEnv } from '@/lib/environment/utils' +import { getExecutionEnvironment } from '@/lib/environment/utils' import { buildNextCallChain, validateCallChain } from '@/lib/execution/call-chain' import { readWorkflowDefinitionAsExecutor } from '@/lib/internal/workflows/read-definition' import { LoggingSession } from '@/lib/logs/execution/logging-session' @@ -230,10 +230,18 @@ export class WorkflowBlockHandler implements BlockHandler { // Custom (deploy-as-block) blocks are an invocation boundary: resolve the bound // workflow + authority from the DB (never trust the serialized value) and run the - // source workflow's LATEST deployment under its OWNER's authority — the same - // identity a normal deployed API/schedule/webhook run uses — so a cross-workspace - // consumer needs no permission on the source workflow. Owner deletion cascade- - // deletes the workflow → the custom_block row, so the block never orphans. + // source workflow's LATEST deployment under its OWNER's authority, so a cross- + // workspace consumer needs no permission on the source workflow. Owner deletion + // cascade-deletes the workflow → the custom_block row, so the block never orphans. + // + // This is a STRONGER use of the owner than any other trigger makes, and the + // difference is deliberate. A deployed API/schedule/webhook run acts as the + // workspace billing account and reads the owner only as its personal-variable + // fallback. A custom block instead runs wholly as the owner — both environment + // slices, the billing actor, and the subject of its delegated tool calls — + // because the contract it publishes is "this block behaves exactly as its + // publisher built it", and the publisher's own integrations and personal keys + // are part of that behavior for a consumer who can see none of them. // Unique ID per invocation — used to correlate child block events with this specific // workflow block execution, preventing cross-iteration child mixing in loop contexts. // Generated up front so the pre-`try` boundary failures below can carry it too. @@ -521,7 +529,38 @@ export class WorkflowBlockHandler implements BlockHandler { const sourceWorkspaceId = childWorkflow.workspaceId childUserId = loadUserId childWorkspaceId = sourceWorkspaceId - const ownerEnv = await getPersonalAndWorkspaceEnv(loadUserId, sourceWorkspaceId) + // Custom-block children authenticate internal tool calls as the source + // owner in the source workspace, so the consumer's snapshot would fail + // the internal routes' actor/workspace scope match. Resolve the + // source-scoped payer instead — the same decision those routes made + // themselves before attribution headers became required. + // + // Resolved before the environment because its `billedAccountUserId` is + // the identity that environment resolution authorizes the workspace + // slice against, and reading it from here costs no extra query. + childBillingAttribution = await resolveBillingAttribution({ + actorUserId: loadUserId, + workspaceId: sourceWorkspaceId, + }) + /** + * Two identities, exactly as a deployed run of this same workflow + * resolves them: personal variables stay with the source owner, because + * "behaves as published" includes the publisher's own keys, while + * workspace variables authorize against the source workspace's billing + * account — the identity a schedule or webhook on this workflow already + * uses. + * + * Reading both slices as the owner made a custom block resolve a + * narrower workspace selection than the very same workflow got on a + * schedule, and fail outright once the owner left the source workspace. + * Neither difference was visible to the consumer, who cannot see the + * source workflow at all. + */ + const ownerEnv = await getExecutionEnvironment( + loadUserId, + childBillingAttribution.billedAccountUserId, + sourceWorkspaceId + ) childEnvVarValues = { ...ownerEnv.personalDecrypted, ...ownerEnv.workspaceDecrypted } childEnvVariablesForLogging = { ...ownerEnv.personalEncrypted, @@ -548,15 +587,6 @@ export class WorkflowBlockHandler implements BlockHandler { origin: 'workflowHandler.childCrossing', }) } - // Custom-block children authenticate internal tool calls as the source - // owner in the source workspace, so the consumer's snapshot would fail - // the internal routes' actor/workspace scope match. Resolve the - // source-scoped payer instead — the same decision those routes made - // themselves before attribution headers became required. - childBillingAttribution = await resolveBillingAttribution({ - actorUserId: loadUserId, - workspaceId: childWorkflow.workspaceId, - }) // Admit against the source payer before any spend. No reservation — see // `admitCustomBlockChildExecution`. await admitCustomBlockChildExecution(childBillingAttribution) diff --git a/apps/sim/lib/api/contracts/v2/logs.ts b/apps/sim/lib/api/contracts/v2/logs.ts index f2e65ea41ab..04c69efc40f 100644 --- a/apps/sim/lib/api/contracts/v2/logs.ts +++ b/apps/sim/lib/api/contracts/v2/logs.ts @@ -245,6 +245,20 @@ export const v2LogDetailSchema = z .nullable() .describe('Total execution duration in milliseconds, or null while unavailable.'), files: v2LogFilesSchema, + /** + * The identity the run acted as, captured by the run itself rather than read + * from the workflow row. Supersedes the deprecated `workflow.ownerEmail`, + * which named whoever the workflow currently belongs to — a mutable pointer + * that member removal reassigns, so it could describe someone who had nothing + * to do with a run that happened months earlier and contributed nothing to it + * beyond a personal-variable fallback. + */ + executedByEmail: z + .email() + .nullable() + .describe( + 'Email of the identity the run executed as: the caller for an interactive or personal-API-key run, and the workspace billing account for a schedule, webhook, deployed chat, or public API call. Null when the run failed before an identity was resolved.' + ), workflow: z .object({ id: z.string().nullable().describe('Workflow identifier, or null when unavailable.'), @@ -255,10 +269,20 @@ export const v2LogDetailSchema = z .describe( 'Canonical folder path of the workflow, in the same form `folderPaths` accepts as a filter: `/` for a workflow at the workspace root. Null only when the path cannot be resolved — the folder has been deleted, or the workflow itself no longer exists.' ), + /** + * Retained only because it was a required field of this schema before + * `executedByEmail` replaced it, and removing it would break typed + * clients. It answers a different question than most readers assume: who + * the workflow belongs to now, which member removal reassigns and which + * says nothing about who ran any particular execution. + */ ownerEmail: z .email() .nullable() - .describe('Workflow owner email, or null when unavailable.'), + .describe( + "Deprecated — use the run-level `executedByEmail` instead. Email of the workflow's current owner, or null when unavailable. This is a property of the workflow as it stands today, not of the run: it changes when workflow ownership is reassigned, and the owner is not the identity a background run executes as." + ) + .meta({ deprecated: true }), workspaceId: z .string() .nullable() diff --git a/apps/sim/lib/api/contracts/v2/openapi/logs.ts b/apps/sim/lib/api/contracts/v2/openapi/logs.ts index 6b2e6e3c2f4..721472f6c9e 100644 --- a/apps/sim/lib/api/contracts/v2/openapi/logs.ts +++ b/apps/sim/lib/api/contracts/v2/openapi/logs.ts @@ -63,6 +63,13 @@ const LOG_DETAIL_EXAMPLE = { endedAt: '2026-01-15T10:30:01.250Z', totalDurationMs: 1250, files: null, + /** + * Deliberately a different address from `workflow.ownerEmail` below. This is + * an `api` run, so it executed as the workspace billing account while the + * workflow still belongs to the person who built it — the distinction the + * deprecated field cannot express. + */ + executedByEmail: 'billing@example.com', workflow: { id: WORKFLOW_ID, name: 'Customer Support Agent', diff --git a/apps/sim/lib/billing/core/billing-attribution.ts b/apps/sim/lib/billing/core/billing-attribution.ts index 7ebfc453b93..be232f7a181 100644 --- a/apps/sim/lib/billing/core/billing-attribution.ts +++ b/apps/sim/lib/billing/core/billing-attribution.ts @@ -592,6 +592,30 @@ export function toUsageLimitSubscription(attribution: BillingAttributionSnapshot } } +/** + * Reads only the identity an unauthenticated run acts as: the workspace's + * billing account. + * + * The same identity {@link resolveSystemBillingAttribution} elects as + * `actorUserId`, exposed on its own for gates that must name that identity + * before execution and have no use for the payer's subscription. Surfaces with + * no identifiable caller — a public API URL, a schedule, a webhook — must + * authorize against this rather than against the workflow owner, which is a + * stored pointer whose access can lapse without the workspace changing. + * + * Returns `null` when the workspace has no billing account or does not exist, + * so a gate can fail closed without distinguishing the two. + */ +export async function getWorkspaceBilledAccountUserId(workspaceId: string): Promise { + const [row] = await db + .select({ billedAccountUserId: workspace.billedAccountUserId }) + .from(workspace) + .where(eq(workspace.id, workspaceId)) + .limit(1) + + return row?.billedAccountUserId ?? null +} + /** * Resolves the workspace-selected payer and its exact subscription without * consulting an actor's organization memberships. diff --git a/apps/sim/lib/environment/utils.test.ts b/apps/sim/lib/environment/utils.test.ts index a0e7cfb582e..52bd2cc0455 100644 --- a/apps/sim/lib/environment/utils.test.ts +++ b/apps/sim/lib/environment/utils.test.ts @@ -462,15 +462,17 @@ describe('getExecutionEnvironment', () => { it('resolves each slice against its own identity', async () => { grantAdminTo('actor-1') /** - * Queued rows are FIFO per table, and the actor resolves first: its access was - * already decided, so it skips the `checkWorkspaceAccess` await the personal - * resolution still performs. Only the actor is a workspace admin, so the owner's - * own workspace slice resolves empty and could not be the one that lands. + * Queued rows are FIFO per table, and the personal slice resolves first because + * it is the first element of the implementation's `Promise.all` — both accesses + * are now decided up front and handed in, so neither resolution awaits before + * issuing its queries and the order is plain argument evaluation rather than a + * race between interleaved awaits. Only the actor is a workspace admin, so the + * owner's own workspace slice resolves empty and could not be the one that lands. */ - queueTableRows(environment, [{ variables: { ACTOR_ONLY: 'actor-cipher' } }]) - queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }]) queueTableRows(environment, [{ variables: { PERSONAL_KEY: 'personal-cipher' } }]) queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }]) + queueTableRows(environment, [{ variables: { ACTOR_ONLY: 'actor-cipher' } }]) + queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }]) const snapshot = await getExecutionEnvironment('owner-1', 'actor-1', 'workspace-1') @@ -560,6 +562,86 @@ describe('getExecutionEnvironment', () => { expect(snapshot.personalDecrypted).toEqual({ PERSONAL_KEY: 'plain:personal-cipher' }) expect(snapshot.workspaceDecrypted).toEqual({ WORKSPACE_KEY: 'plain:workspace-cipher' }) }) + + /** + * A deployed chat, schedule, or webhook keeps running after the identity its + * personal-variable fallback points at leaves the workspace. That pointer is + * stored state, not a permission the run holds, so it must not fail the run + * before any block has started. + */ + it('resolves workspace variables only when the personal identity cannot reach the workspace', async () => { + mockCheckWorkspaceAccess.mockImplementation(async (_workspaceId: string, userId: string) => ({ + exists: true, + hasAccess: userId === 'actor-1', + canWrite: true, + canAdmin: true, + })) + queueTableRows(environment, [{ variables: { PERSONAL_KEY: 'personal-cipher' } }]) + queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }]) + + const snapshot = await getExecutionEnvironment('departed-owner', 'actor-1', 'workspace-1') + + expect(snapshot.workspaceDecrypted).toEqual({ WORKSPACE_KEY: 'plain:workspace-cipher' }) + expect(snapshot.personalDecrypted).toEqual({}) + expect(snapshot.personalEncrypted).toEqual({}) + expect(snapshot.personalOwners).toEqual({}) + expect(snapshot.conflicts).toEqual([]) + }) + + /** The departed identity's own variables must not reach the run that dropped it. */ + /** + * Degrading must not widen the credential-group filter. The workspace slice is + * still selected by the actor's own grants and the actor's own admin flag — + * dropping the personal slice removes secrets, it never adds any. + */ + it('does not read the departed personal identity when resolving workspace variables only', async () => { + mockCheckWorkspaceAccess.mockImplementation(async (_workspaceId: string, userId: string) => ({ + exists: true, + hasAccess: userId === 'actor-1', + canWrite: true, + canAdmin: false, + })) + queueTableRows(environment, [{ variables: { ACTOR_ONLY: 'actor-cipher' } }]) + queueTableRows(workspaceEnvironment, [{ variables: { WORKSPACE_KEY: 'workspace-cipher' } }]) + + const snapshot = await getExecutionEnvironment('departed-owner', 'actor-1', 'workspace-1') + + expect(mockGetAccessibleEnvCredentials).toHaveBeenCalledOnce() + expect(mockGetAccessibleEnvCredentials).toHaveBeenCalledWith('workspace-1', 'actor-1', { + isWorkspaceAdmin: false, + }) + // No credential grant, and the actor is not an admin, so the workspace + // secret stays filtered out rather than falling through unfiltered. + expect(snapshot.workspaceDecrypted).toEqual({}) + }) + + /** 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({ + exists: true, + hasAccess: false, + canWrite: false, + canAdmin: false, + }) + + await expect( + getExecutionEnvironment('departed-owner', 'departed-payer', 'workspace-1') + ).rejects.toThrow('Access denied to workspace workspace-1') + }) + + /** A workspace that is gone is a different fact from one an identity may not read. */ + it('raises when the workspace no longer exists', async () => { + mockCheckWorkspaceAccess.mockResolvedValue({ + exists: false, + hasAccess: false, + canWrite: false, + canAdmin: false, + }) + + await expect(getExecutionEnvironment('owner-1', 'actor-1', 'workspace-1')).rejects.toThrow( + 'Workspace workspace-1 does not exist' + ) + }) }) describe('upsertWorkspaceEnvVars', () => { diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index 5913841566a..f053caa88b4 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/apps/sim/lib/environment/utils.ts @@ -407,6 +407,30 @@ export async function getPersonalAndWorkspaceEnv( } } +/** + * Keeps only the workspace slice of a snapshot resolved for a single identity. + * + * Used wherever a run has no personal namespace to lend, so the identity that + * authorized the workspace variables cannot leak its own personal ones in + * alongside them. `conflicts` is empty by construction once the personal slice + * is, and a decryption failure is only carried over when it belongs to the slice + * being kept. + */ +function toWorkspaceOnlySnapshot( + snapshot: EnvironmentResolutionSnapshot +): EnvironmentResolutionSnapshot { + return { + ...snapshot, + personalEncrypted: {}, + personalDecrypted: {}, + personalOwners: {}, + conflicts: [], + decryptionFailures: snapshot.decryptionFailures.filter( + (key) => key in snapshot.workspaceEncrypted + ), + } +} + /** * Resolves one execution's environment from two independent identities. * @@ -424,15 +448,34 @@ export async function getPersonalAndWorkspaceEnv( * A run whose two identities coincide, which is every interactive run, resolves * exactly as before through a single query. * - * When the actor has no access to the workspace at all, the personal identity is - * reused for both slices and the fault is reported rather than raised. - * `workspace.billedAccountUserId` is a stored column rather than a derivation, - * so an organization ownership transfer can leave it pointing at a user with no - * remaining access; failing here would take down every background execution in - * that workspace for a misconfiguration the run itself did not cause. The error - * line is what makes that state visible while it is repaired. + * Neither identity is a permission the run holds — both are stored pointers that + * outlive the access that made them valid, so a stale one is reported rather than + * raised. `workspace.billedAccountUserId` is a stored column rather than a + * derivation, so an ownership transfer can leave the actor pointing at a user with + * no remaining access; `workflow.userId` is likewise a stored pointer that + * member-removal repairs on the paths it knows about. Failing on either would take + * down every background execution in the workspace for a misconfiguration the run + * itself did not cause. The error lines are what make that state visible while it + * is repaired. + * + * The two stale cases degrade differently because the identities mean different + * things. An actor that cannot reach the workspace leaves the owner as the only + * identity to authorize the workspace slice against, so the run falls back to + * resolving both slices as the owner. A personal identity that cannot reach the + * workspace is no longer someone whose private namespace it is reasonable to lend + * — the same judgment already applied to an anonymous public-API call — so the run + * keeps the actor's workspace slice and resolves no personal variables at all. + * Continuing to lend a removed member's personal secrets to their former + * organization's background runs is the outcome to avoid, not the one to preserve. + * A reference to a variable that is no longer resolvable survives as its literal + * `{{NAME}}` and fails at the block that needs it, which names the missing + * variable instead of failing the run before any block has started. + * + * With no reachable identity on either side there is nobody to authorize the + * workspace slice against, and a filtered selection cannot be computed, so that + * case still raises. * - * That fallback is gated on the access decision alone, never on a failed query. + * These fallbacks are gated on the access decision alone, never on a failed query. * Widening to a `catch` would let a transient database fault silently promote the * run to the owner's broader secret selection, which is the opposite of what an * infrastructure error should do — those propagate and fail the run. @@ -443,35 +486,61 @@ export async function getExecutionEnvironment( workspaceId?: string ): Promise { if (personalUserId === undefined) { - const workspaceOnly = await getPersonalAndWorkspaceEnv(workspaceUserId, workspaceId) - return { - ...workspaceOnly, - personalEncrypted: {}, - personalDecrypted: {}, - personalOwners: {}, - conflicts: [], - decryptionFailures: workspaceOnly.decryptionFailures.filter( - (key) => key in workspaceOnly.workspaceEncrypted - ), - } + return toWorkspaceOnlySnapshot(await getPersonalAndWorkspaceEnv(workspaceUserId, workspaceId)) } if (!workspaceId || workspaceUserId === personalUserId) { return getPersonalAndWorkspaceEnv(personalUserId, workspaceId) } - const actorAccess = await checkWorkspaceAccess(workspaceId, workspaceUserId) + const [actorAccess, personalAccess] = await Promise.all([ + checkWorkspaceAccess(workspaceId, workspaceUserId), + checkWorkspaceAccess(workspaceId, personalUserId), + ]) + + /** + * A workspace that no longer exists and one an identity may not read are + * different facts, exactly as in {@link getPersonalAndWorkspaceEnv}. Only the + * second is a stale pointer worth degrading for. + */ + if (!personalAccess.exists) { + throw new Error(`Workspace ${workspaceId} does not exist`) + } + + if (!personalAccess.hasAccess) { + if (!actorAccess.hasAccess) { + logger.error('Neither execution identity can reach the workspace', { + personalUserId, + workspaceUserId, + workspaceId, + }) + throw new Error(`Access denied to workspace ${workspaceId}`) + } + + logger.error( + 'Personal-environment identity cannot reach the workspace; resolving workspace variables only', + { personalUserId, workspaceUserId, workspaceId } + ) + return toWorkspaceOnlySnapshot( + await getPersonalAndWorkspaceEnv(workspaceUserId, workspaceId, { + workspaceAccess: actorAccess, + }) + ) + } + if (!actorAccess.hasAccess) { logger.error('Execution actor cannot reach the workspace; falling back to the owner', { personalUserId, workspaceUserId, workspaceId, }) - return getPersonalAndWorkspaceEnv(personalUserId, workspaceId) + return getPersonalAndWorkspaceEnv(personalUserId, workspaceId, { + workspaceAccess: personalAccess, + }) } const [personal, actor] = await Promise.all([ - getPersonalAndWorkspaceEnv(personalUserId, workspaceId), + getPersonalAndWorkspaceEnv(personalUserId, workspaceId, { workspaceAccess: personalAccess }), getPersonalAndWorkspaceEnv(workspaceUserId, workspaceId, { workspaceAccess: actorAccess }), ]) diff --git a/apps/sim/lib/execution/preprocessing.test.ts b/apps/sim/lib/execution/preprocessing.test.ts index 03f8a83c1f8..fc93fb4eda5 100644 --- a/apps/sim/lib/execution/preprocessing.test.ts +++ b/apps/sim/lib/execution/preprocessing.test.ts @@ -500,23 +500,37 @@ describe('preprocessExecution ban gate', () => { expect(mockCheckRateLimit).toHaveBeenCalledTimes(1) }) - it('checks the actor, caller-provided userId, and workflow owner in one call', async () => { + it('checks the actor and the caller-provided userId in one call', async () => { const result = await preprocessExecution(baseOptions) expect(result.success).toBe(true) expect(mockGetActivelyBannedUserIds).toHaveBeenCalledTimes(1) - expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith([ - 'billed-account-1', - 'owner-1', - 'creator-1', - ]) + expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1', 'owner-1']) }) - it('excludes the "unknown" sentinel userId but still checks the workflow owner', async () => { + it('excludes the "unknown" sentinel userId', async () => { const result = await preprocessExecution({ ...baseOptions, userId: 'unknown' }) expect(result.success).toBe(true) - expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1', 'creator-1']) + expect(mockGetActivelyBannedUserIds).toHaveBeenCalledWith(['billed-account-1']) + }) + + /** + * 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. + */ + 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' }) + + 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 () => { diff --git a/apps/sim/lib/execution/preprocessing.ts b/apps/sim/lib/execution/preprocessing.ts index c68dda80679..bd77fed2345 100644 --- a/apps/sim/lib/execution/preprocessing.ts +++ b/apps/sim/lib/execution/preprocessing.ts @@ -449,17 +449,21 @@ export async function preprocessExecution( const banCheck = (async (): Promise => { /** - * Blocks when the resolved actor, workflow owner, or caller-provided user - * has an active ban or blocked email domain. Including the workflow owner - * covers system-triggered executions. + * 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. + * + * 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. */ const banCandidateIds = [actorUserId] if (userId && userId !== 'unknown' && userId !== actorUserId) { banCandidateIds.push(userId) } - if (workflowRecord.userId && !banCandidateIds.includes(workflowRecord.userId)) { - banCandidateIds.push(workflowRecord.userId) - } try { const bannedUserIds = await getActivelyBannedUserIds(banCandidateIds) if (bannedUserIds.length > 0) { diff --git a/apps/sim/lib/logs/application/get-public-log.ts b/apps/sim/lib/logs/application/get-public-log.ts index d99e2703d19..dd86463580d 100644 --- a/apps/sim/lib/logs/application/get-public-log.ts +++ b/apps/sim/lib/logs/application/get-public-log.ts @@ -113,9 +113,14 @@ export const getPublicLog = defineAuthorizedWorkspaceUseCase({ userId: principal.kind === 'personal_api_key' ? principal.userId : undefined, } ) - if (log.workflowUserId && !log.workflowOwnerEmail) { - throw new Error(`Unable to resolve workflow owner email for ${log.workflowUserId}`) - } + /** + * No assertion on `executedByEmail`. The owner-email version of this field + * could reasonably insist a non-null user id resolve to an email, because + * the workflow row's owner was expected to exist. The executing identity is + * read from attribution the run captured for itself, and a run that failed + * before resolving one legitimately has none — so null is an answer here, + * not a missing join. + */ const costLedger = await buildCostLedger(log.executionId) return { log: { ...log, workflowState: sanitizeExecutionSnapshotState(log.workflowState) }, diff --git a/apps/sim/lib/logs/fetch-log-detail.ts b/apps/sim/lib/logs/fetch-log-detail.ts index 41a89227a12..32a13e208b5 100644 --- a/apps/sim/lib/logs/fetch-log-detail.ts +++ b/apps/sim/lib/logs/fetch-log-detail.ts @@ -82,7 +82,6 @@ export async function readLogDetail({ workflowName: workflow.name, workflowDescription: workflow.description, workflowFolderId: workflow.folderId, - workflowUserId: workflow.userId, workflowWorkspaceId: workflow.workspaceId, workflowCreatedAt: workflow.createdAt, workflowUpdatedAt: workflow.updatedAt, @@ -113,7 +112,6 @@ export async function readLogDetail({ name: log.workflowName, description: log.workflowDescription, folderId: log.workflowFolderId, - userId: log.workflowUserId, workspaceId: log.workflowWorkspaceId, createdAt: log.workflowCreatedAt?.toISOString() ?? null, updatedAt: log.workflowUpdatedAt?.toISOString() ?? null, diff --git a/apps/sim/lib/logs/list-logs.ts b/apps/sim/lib/logs/list-logs.ts index 074cdd4753b..81a5d5d2dcc 100644 --- a/apps/sim/lib/logs/list-logs.ts +++ b/apps/sim/lib/logs/list-logs.ts @@ -185,7 +185,6 @@ export async function readLogs(params: ListLogsParams): Promise'billingAttribution'->>'actorUserId'`, user.id) + ) + /** + * Kept only to serve the deprecated `workflow.ownerEmail`, which was a + * required field of the published v2 log schema before `executedByEmail` + * replaced it. Removing it outright would break typed clients, so it stays + * until that field does. Aliased because the actor join above already holds + * `user` — the two identities coincide on an interactive run and diverge on + * every background one, which is the whole reason the field was replaced. + */ + .leftJoin(workflowOwner, eq(workflow.userId, workflowOwner.id)) .where( and( lookupCondition, diff --git a/apps/sim/lib/webhooks/env-resolver.test.ts b/apps/sim/lib/webhooks/env-resolver.test.ts index 0f802ed5947..1d42d0249dc 100644 --- a/apps/sim/lib/webhooks/env-resolver.test.ts +++ b/apps/sim/lib/webhooks/env-resolver.test.ts @@ -5,11 +5,20 @@ import { environmentUtilsMockFns, resetEnvironmentUtilsMock } from '@sim/testing' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' -const { mockGetEffectiveDecryptedEnv } = environmentUtilsMockFns +const { mockGetEffectiveDecryptedEnv, mockGetExecutionEnvironment } = environmentUtilsMockFns + +const { mockGetWorkspaceBilledAccountUserId } = vi.hoisted(() => ({ + mockGetWorkspaceBilledAccountUserId: vi.fn(), +})) + +vi.mock('@/lib/billing/core/billing-attribution', () => ({ + getWorkspaceBilledAccountUserId: mockGetWorkspaceBilledAccountUserId, +})) afterAll(resetEnvironmentUtilsMock) import { + resolveBackgroundWebhookEnv, resolveWebhookProviderConfig, resolveWebhookRecordProviderConfig, } from '@/lib/webhooks/env-resolver' @@ -103,3 +112,62 @@ describe('webhook env resolver', () => { expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled() }) }) + +/** + * An inbound delivery or a provider URL-validation challenge has no caller, so + * it must resolve the two identities the executor resolves — otherwise the + * workflow owner leaving the workspace silently stops the webhook's own signing + * secret from resolving, and every caller here reads that as a rejected request + * rather than an error. + */ +describe('resolveBackgroundWebhookEnv', () => { + beforeEach(() => { + vi.clearAllMocks() + mockGetEffectiveDecryptedEnv.mockResolvedValue({ FROM_SINGLE_IDENTITY: 'single' }) + mockGetExecutionEnvironment.mockResolvedValue({ + personalDecrypted: { OWNER_KEY: 'owner-value' }, + workspaceDecrypted: { WORKSPACE_KEY: 'workspace-value' }, + }) + }) + + it('splits the workflow owner from the workspace billing account', async () => { + mockGetWorkspaceBilledAccountUserId.mockResolvedValue('billing-1') + + const env = await resolveBackgroundWebhookEnv('owner-1', 'workspace-1') + + expect(mockGetExecutionEnvironment).toHaveBeenCalledWith('owner-1', 'billing-1', 'workspace-1') + expect(env).toEqual({ OWNER_KEY: 'owner-value', WORKSPACE_KEY: 'workspace-value' }) + expect(mockGetEffectiveDecryptedEnv).not.toHaveBeenCalled() + }) + + /** Workspace variables win a name collision, matching every other execution path. */ + it('lets the workspace slice shadow the owner personal slice', async () => { + mockGetWorkspaceBilledAccountUserId.mockResolvedValue('billing-1') + mockGetExecutionEnvironment.mockResolvedValue({ + personalDecrypted: { SHARED: 'personal' }, + workspaceDecrypted: { SHARED: 'workspace' }, + }) + + const env = await resolveBackgroundWebhookEnv('owner-1', 'workspace-1') + + expect(env).toEqual({ SHARED: 'workspace' }) + }) + + it('falls back to the single identity 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' }) + }) + + it('resolves a workspaceless webhook against the owner alone', async () => { + const env = await resolveBackgroundWebhookEnv('owner-1') + + expect(mockGetWorkspaceBilledAccountUserId).not.toHaveBeenCalled() + expect(mockGetEffectiveDecryptedEnv).toHaveBeenCalledWith('owner-1') + expect(env).toEqual({ FROM_SINGLE_IDENTITY: 'single' }) + }) +}) diff --git a/apps/sim/lib/webhooks/env-resolver.ts b/apps/sim/lib/webhooks/env-resolver.ts index 13975c2c956..75e0771ed08 100644 --- a/apps/sim/lib/webhooks/env-resolver.ts +++ b/apps/sim/lib/webhooks/env-resolver.ts @@ -1,5 +1,5 @@ import { isRecordLike } from '@sim/utils/object' -import { getEffectiveDecryptedEnv } from '@/lib/environment/utils' +import { getEffectiveDecryptedEnv, getExecutionEnvironment } from '@/lib/environment/utils' import { resolveEnvVarReferences } from '@/executor/utils/reference-validation' export interface WebhookEnvResolutionOptions { @@ -7,6 +7,43 @@ export interface WebhookEnvResolutionOptions { onResolved?: (name: string, value: string) => void } +/** + * Resolves the env a webhook config is read against when there is no caller to + * speak of — an inbound delivery or a provider's URL-validation challenge. + * + * Splits the two identities the same way the executor does: personal variables + * stay with the workflow owner who authored the config, and workspace variables + * authorize against the workspace's billing account, which is the identity such + * a run acts as. Reading both slices as the owner made a webhook stop resolving + * its own signing secret the moment that person left the workspace — silently, + * 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. + */ +export async function resolveBackgroundWebhookEnv( + workflowOwnerUserId: string, + workspaceId?: string +): Promise> { + if (!workspaceId) { + return getEffectiveDecryptedEnv(workflowOwnerUserId) + } + + const { getWorkspaceBilledAccountUserId } = await import('@/lib/billing/core/billing-attribution') + const billedAccountUserId = await getWorkspaceBilledAccountUserId(workspaceId) + if (!billedAccountUserId) { + return getEffectiveDecryptedEnv(workflowOwnerUserId, workspaceId) + } + + const snapshot = await getExecutionEnvironment( + workflowOwnerUserId, + billedAccountUserId, + workspaceId + ) + return { ...snapshot.personalDecrypted, ...snapshot.workspaceDecrypted } +} + /** * Recursively resolves all environment variable references in a configuration object. * Supports both exact matches (`{{VAR_NAME}}`) and embedded patterns (`https://{{HOST}}/path`). diff --git a/apps/sim/lib/webhooks/providers/zoom.ts b/apps/sim/lib/webhooks/providers/zoom.ts index 677b39aa9cb..0052dbd39e5 100644 --- a/apps/sim/lib/webhooks/providers/zoom.ts +++ b/apps/sim/lib/webhooks/providers/zoom.ts @@ -7,7 +7,7 @@ import { isRecordLike } from '@sim/utils/object' import { and, eq } from 'drizzle-orm' import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' -import { resolveEnvVarsInObject } from '@/lib/webhooks/env-resolver' +import { resolveBackgroundWebhookEnv, resolveEnvVarsInObject } from '@/lib/webhooks/env-resolver' import type { AuthContext, EventMatchContext, @@ -73,10 +73,18 @@ async function resolveZoomChallengeSecrets( : {} try { + /** + * Two identities, because a failed challenge is not a failed delivery: + * Zoom deactivates the endpoint outright when URL validation does not + * answer, so an owner who left the workspace would take the webhook down + * at the provider rather than drop one request. + */ + const envVars = await resolveBackgroundWebhookEnv(row.userId, row.workspaceId ?? undefined) const config = await resolveEnvVarsInObject( rawConfig, row.userId, - row.workspaceId ?? undefined + row.workspaceId ?? undefined, + { envVars } ) const secretToken = typeof config.secretToken === 'string' ? config.secretToken : '' return { secretToken } diff --git a/apps/sim/lib/workflows/custom-blocks/operations.ts b/apps/sim/lib/workflows/custom-blocks/operations.ts index a27909089f9..a624303c54b 100644 --- a/apps/sim/lib/workflows/custom-blocks/operations.ts +++ b/apps/sim/lib/workflows/custom-blocks/operations.ts @@ -307,11 +307,23 @@ export async function getCustomBlockManageContext(id: string): Promise<{ * executor to run the bound workflow under the invocation-boundary model: the * consumer needs no permission on the source workflow. Returns the authoritative * `workflowId` from the DB (never trust a serialized value) plus the source - * workflow's **owner** (`workflow.userId`) — the same identity a normal deployed - * API/schedule/webhook run executes as. Using the owner (not the publisher) means - * the owner always has read on their own workflow, and owner deletion cascade- - * deletes the workflow → the custom_block row, so there is never an orphaned block. - * `null` when no enabled block matches the type. + * workflow's **owner** (`workflow.userId`). Using the owner (not the publisher) + * means the owner always has read on their own workflow, and owner deletion + * 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. + * + * 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. */ export async function getCustomBlockAuthority( type: string, diff --git a/packages/testing/src/mocks/environment-utils.mock.ts b/packages/testing/src/mocks/environment-utils.mock.ts index 7e90d37329e..d2021dd0e8c 100644 --- a/packages/testing/src/mocks/environment-utils.mock.ts +++ b/packages/testing/src/mocks/environment-utils.mock.ts @@ -1,20 +1,30 @@ import { vi } from 'vitest' +/** + * Every field of the real `EnvironmentResolutionSnapshot`, including the two a + * caller reads as arrays/records rather than testing for presence. Omitting them + * let 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. + */ function emptyPersonalAndWorkspaceEnv(): { personalEncrypted: Record workspaceEncrypted: Record personalDecrypted: Record workspaceDecrypted: Record + personalOwners: Record conflicts: string[] decryptionFailures: string[] + workspaceUnredactedKeys: string[] } { return { personalEncrypted: {}, workspaceEncrypted: {}, personalDecrypted: {}, workspaceDecrypted: {}, + personalOwners: {}, conflicts: [], decryptionFailures: [], + workspaceUnredactedKeys: [], } } @@ -77,6 +87,10 @@ async function delegateExecutionEnvironment( ...(actor.decryptionFailures ?? []).filter((k: string) => k in workspaceEncrypted), ]), ], + // Belongs to the workspace slice, so it comes from the actor. Spreading + // `personal` alone carried the wrong identity's keys — and `undefined` when + // the stub omitted them. + workspaceUnredactedKeys: actor.workspaceUnredactedKeys, } } From f5c37bebbbdb0f04d872aef71887b4e9db2f020d Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Mon, 31 Aug 2026 18:23:07 -0700 Subject: [PATCH 2/5] fix(execution): suspend personal secrets for banned owners, and gate the ban candidate correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/sim/lib/environment/utils.test.ts | 39 +++++++++++++++ apps/sim/lib/environment/utils.ts | 26 ++++++++-- apps/sim/lib/execution/preprocessing.test.ts | 48 +++++++++++++++---- apps/sim/lib/execution/preprocessing.ts | 27 +++++++---- .../lib/workflows/custom-blocks/operations.ts | 22 ++++----- packages/sim-cli/src/generated/v2-api.ts | 1 + 6 files changed, 130 insertions(+), 33 deletions(-) diff --git a/apps/sim/lib/environment/utils.test.ts b/apps/sim/lib/environment/utils.test.ts index 52bd2cc0455..74c898eb96b 100644 --- a/apps/sim/lib/environment/utils.test.ts +++ b/apps/sim/lib/environment/utils.test.ts @@ -18,6 +18,7 @@ const { mockGetUserEntityPermissions, mockGetWorkspaceEnvKeyAdminAccess, mockRecordAudit, + mockGetActivelyBannedUserIds, } = vi.hoisted(() => ({ mockCreateWorkspaceEnvCredentials: vi.fn(), mockCheckWorkspaceAccess: vi.fn(), @@ -25,6 +26,7 @@ const { 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({ diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index f053caa88b4..244140dd987 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/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( diff --git a/apps/sim/lib/execution/preprocessing.test.ts b/apps/sim/lib/execution/preprocessing.test.ts index fc93fb4eda5..845c69b49c1 100644 --- a/apps/sim/lib/execution/preprocessing.test.ts +++ b/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 () => { diff --git a/apps/sim/lib/execution/preprocessing.ts b/apps/sim/lib/execution/preprocessing.ts index bd77fed2345..6a56bc72e00 100644 --- a/apps/sim/lib/execution/preprocessing.ts +++ b/apps/sim/lib/execution/preprocessing.ts @@ -449,19 +449,26 @@ export async function preprocessExecution( const banCheck = (async (): Promise => { /** - * 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 { diff --git a/apps/sim/lib/workflows/custom-blocks/operations.ts b/apps/sim/lib/workflows/custom-blocks/operations.ts index a624303c54b..a35f980b7e1 100644 --- a/apps/sim/lib/workflows/custom-blocks/operations.ts +++ b/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, diff --git a/packages/sim-cli/src/generated/v2-api.ts b/packages/sim-cli/src/generated/v2-api.ts index cb2ebdba3df..05df1c1f1f8 100644 --- a/packages/sim-cli/src/generated/v2-api.ts +++ b/packages/sim-cli/src/generated/v2-api.ts @@ -4038,6 +4038,7 @@ type GetLogResponseRef2 = { endedAt: string | null totalDurationMs: number | null files: Array | null + executedByEmail: string | null workflow: { id: string | null name: string From f43035180c3ddaab8b33d7e329d3352bbd303fbc Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Mon, 31 Aug 2026 19:14:54 -0700 Subject: [PATCH 3/5] fix(execution): enforce suspension on every personal-secret path, and align webhook cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/sim/lib/environment/utils.test.ts | 29 +++++++++++ apps/sim/lib/environment/utils.ts | 51 +++++++++++-------- apps/sim/lib/webhooks/env-resolver.ts | 2 +- .../lib/webhooks/provider-subscriptions.ts | 16 ++++-- .../src/mocks/environment-utils.mock.test.ts | 11 ++++ 5 files changed, 83 insertions(+), 26 deletions(-) diff --git a/apps/sim/lib/environment/utils.test.ts b/apps/sim/lib/environment/utils.test.ts index 74c898eb96b..c3c7d211c19 100644 --- a/apps/sim/lib/environment/utils.test.ts +++ b/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') diff --git a/apps/sim/lib/environment/utils.ts b/apps/sim/lib/environment/utils.ts index 244140dd987..0c72b260fb2 100644 --- a/apps/sim/lib/environment/utils.ts +++ b/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( diff --git a/apps/sim/lib/webhooks/env-resolver.ts b/apps/sim/lib/webhooks/env-resolver.ts index 75e0771ed08..52f25eb2521 100644 --- a/apps/sim/lib/webhooks/env-resolver.ts +++ b/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) diff --git a/apps/sim/lib/webhooks/provider-subscriptions.ts b/apps/sim/lib/webhooks/provider-subscriptions.ts index 615a5ea7722..8cd221622b9 100644 --- a/apps/sim/lib/webhooks/provider-subscriptions.ts +++ b/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({ diff --git a/packages/testing/src/mocks/environment-utils.mock.test.ts b/packages/testing/src/mocks/environment-utils.mock.test.ts index 6b4a32fd974..6aa215561f0 100644 --- a/packages/testing/src/mocks/environment-utils.mock.test.ts +++ b/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') From 82f5f698d01ed5dd59292611fb6ad6cfd660bd88 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Mon, 31 Aug 2026 19:42:10 -0700 Subject: [PATCH 4/5] fix(webhooks): route every background env resolution through the identity resolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/sim/lib/webhooks/env-resolver.test.ts | 37 +++++++++++++++---- apps/sim/lib/webhooks/env-resolver.ts | 23 ++++++------ .../webhooks/provider-subscriptions.test.ts | 23 ++++++++++-- 3 files changed, 62 insertions(+), 21 deletions(-) diff --git a/apps/sim/lib/webhooks/env-resolver.test.ts b/apps/sim/lib/webhooks/env-resolver.test.ts index 1d42d0249dc..507f8a7592f 100644 --- a/apps/sim/lib/webhooks/env-resolver.test.ts +++ b/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({}) }) }) diff --git a/apps/sim/lib/webhooks/env-resolver.ts b/apps/sim/lib/webhooks/env-resolver.ts index 52f25eb2521..3220d71d1ec 100644 --- a/apps/sim/lib/webhooks/env-resolver.ts +++ b/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> { - 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 } diff --git a/apps/sim/lib/webhooks/provider-subscriptions.test.ts b/apps/sim/lib/webhooks/provider-subscriptions.test.ts index cdccc03b4a7..26ec78e9d86 100644 --- a/apps/sim/lib/webhooks/provider-subscriptions.test.ts +++ b/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({ From 3f6090338689886945d9bad11cd42125faafdd9b Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Mon, 31 Aug 2026 19:53:11 -0700 Subject: [PATCH 5/5] fix(execution): make the suspension gate fail closed on an undeclared userId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- apps/sim/app/api/chat/[identifier]/route.ts | 2 + .../background/workflow-column-execution.ts | 2 + apps/sim/lib/execution/preprocessing.test.ts | 67 ++++++++++--------- apps/sim/lib/execution/preprocessing.ts | 41 ++++++++---- apps/sim/lib/webhooks/processor.ts | 2 + 5 files changed, 69 insertions(+), 45 deletions(-) diff --git a/apps/sim/app/api/chat/[identifier]/route.ts b/apps/sim/app/api/chat/[identifier]/route.ts index 41e282ebd17..629fe2d9a75 100644 --- a/apps/sim/app/api/chat/[identifier]/route.ts +++ b/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, diff --git a/apps/sim/background/workflow-column-execution.ts b/apps/sim/background/workflow-column-execution.ts index 33c21174d70..8bf1eb1544d 100644 --- a/apps/sim/background/workflow-column-execution.ts +++ b/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, diff --git a/apps/sim/lib/execution/preprocessing.test.ts b/apps/sim/lib/execution/preprocessing.test.ts index 845c69b49c1..e2168f21d58 100644 --- a/apps/sim/lib/execution/preprocessing.test.ts +++ b/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 () => { diff --git a/apps/sim/lib/execution/preprocessing.ts b/apps/sim/lib/execution/preprocessing.ts index 6a56bc72e00..07f648a378e 100644 --- a/apps/sim/lib/execution/preprocessing.ts +++ b/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 { diff --git a/apps/sim/lib/webhooks/processor.ts b/apps/sim/lib/webhooks/processor.ts index da5c2d2c6c7..e618d917621 100644 --- a/apps/sim/lib/webhooks/processor.ts +++ b/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,