From 18d4690f67273c9010fdaa49245cd70eb2107c33 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 28 Aug 2026 11:19:46 -0700 Subject: [PATCH 1/2] fix(logs): stop requiring a human subject on actorless runs Scheduled, public-API, and subject-less webhook runs carry no user on their principal. Several use cases resolved one with requirePrincipalSubjectUserId where the user was only attribution, so those runs failed with an opaque 500. Authorization for an actorless caller comes from the workflow running a deployment, never from a userId; each site now treats the user as what it actually is. --- apps/sim/lib/auth/principal.test.ts | 65 +++++ .../application/create-invite-link.test.ts | 42 +++ .../application/create-invite-link.ts | 6 +- .../application/send-invite.ts | 3 +- apps/sim/lib/credential-groups/enrollments.ts | 14 +- .../authorized-credential-use-case.ts | 1 + .../application/connection-target.ts | 1 + .../lib/custom-tools/application/use-cases.ts | 4 + .../lib/internal/deployments/execute-tool.ts | 17 +- apps/sim/lib/internal/file/execute-tool.ts | 24 +- .../lib/internal/knowledge/execute-tool.ts | 17 +- .../lib/internal/logs/execute-tool.test.ts | 25 +- apps/sim/lib/internal/logs/execute-tool.ts | 17 +- apps/sim/lib/internal/mcp/discover-tools.ts | 3 +- apps/sim/lib/internal/mcp/execute-tool.ts | 20 +- apps/sim/lib/internal/memory/execute-tool.ts | 16 +- apps/sim/lib/internal/table/execute-tool.ts | 17 +- .../tool-operations/identity-faults.ts | 50 ++++ .../lib/internal/windchill/execute-tool.ts | 17 +- apps/sim/lib/internal/windchill/operations.ts | 3 + apps/sim/lib/knowledge/api/internal-route.ts | 1 + .../authorized-knowledge-use-case.ts | 1 + apps/sim/lib/knowledge/application/billing.ts | 1 + .../application/read-execution-snapshot.ts | 6 +- .../logs/application/read-log-detail.test.ts | 126 ++++++++ .../lib/logs/application/read-log-detail.ts | 6 +- .../execution/hydrate-child-traces.test.ts | 29 ++ .../logs/execution/hydrate-child-traces.ts | 10 +- apps/sim/lib/logs/fetch-log-detail.test.ts | 74 +++++ apps/sim/lib/logs/fetch-log-detail.ts | 8 +- apps/sim/lib/mcp/application/authorization.ts | 30 ++ .../lib/mcp/application/execute-tool.test.ts | 56 +++- apps/sim/lib/mcp/application/execute-tool.ts | 22 +- apps/sim/lib/mcp/application/use-cases.ts | 25 +- apps/sim/lib/table/application/rows.ts | 7 +- .../lib/workflows/application/deployments.ts | 1 + .../application/share-workspace-file.ts | 1 + package.json | 4 +- packages/auth/src/principal.ts | 19 +- ...heck-actorless-executor-operations.test.ts | 138 +++++++++ .../check-actorless-executor-operations.ts | 275 ++++++++++++++++++ 41 files changed, 1112 insertions(+), 90 deletions(-) create mode 100644 apps/sim/lib/internal/tool-operations/identity-faults.ts create mode 100644 apps/sim/lib/logs/application/read-log-detail.test.ts create mode 100644 scripts/check-actorless-executor-operations.test.ts create mode 100644 scripts/check-actorless-executor-operations.ts diff --git a/apps/sim/lib/auth/principal.test.ts b/apps/sim/lib/auth/principal.test.ts index 1121a6d9fc5..e938f062a58 100644 --- a/apps/sim/lib/auth/principal.test.ts +++ b/apps/sim/lib/auth/principal.test.ts @@ -8,6 +8,7 @@ import { resolvePrincipalAttribution, resolvePrincipalAuditAttribution, resolvePrincipalSubject, + resolvePrincipalSubjectUserId, serializePrincipal, toPrincipalActor, } from '@sim/auth/principal' @@ -43,6 +44,70 @@ describe('principal subject users', () => { ).toBe('delegated-user') }) + it('resolves the same subject without demanding one', () => { + expect( + resolvePrincipalSubjectUserId({ + kind: 'session', + userId: 'session-user', + sessionId: 'session-1', + }) + ).toBe('session-user') + expect( + resolvePrincipalSubjectUserId({ + kind: 'delegated', + serviceId: 'executor', + subjectUserId: 'delegated-user', + workspaceId: 'workspace-1', + delegationId: 'delegation-1', + audience: 'sim:test', + issuedAt: new Date('2026-01-01T00:00:00Z'), + expiresAt: new Date('2026-01-01T00:05:00Z'), + }) + ).toBe('delegated-user') + }) + + it('answers undefined for an actorless caller rather than throwing', () => { + // The distinction the two helpers exist to make visible: a schedule, a webhook + // with no external subject, and a workspace key are all authorized callers that + // simply have no person. Attribution-only reads take this branch. + expect( + resolvePrincipalSubjectUserId({ + kind: 'system', + serviceId: 'schedule', + workspaceId: 'workspace-1', + workflowId: 'workflow-1', + }) + ).toBeUndefined() + expect( + resolvePrincipalSubjectUserId({ + kind: 'delegated', + serviceId: 'executor', + workspaceId: 'workspace-1', + delegationId: 'delegation-1', + audience: 'sim:test', + issuedAt: new Date('2026-01-01T00:00:00Z'), + expiresAt: new Date('2026-01-01T00:05:00Z'), + delegationContext: { + kind: 'workflow_execution', + workflowId: 'workflow-1', + principal: { + kind: 'system', + serviceId: 'schedule', + workspaceId: 'workspace-1', + workflowId: 'workflow-1', + }, + }, + }) + ).toBeUndefined() + expect( + resolvePrincipalSubjectUserId({ + kind: 'workspace_api_key', + keyId: 'key-1', + workspaceId: 'workspace-1', + }) + ).toBeUndefined() + }) + it('fails fast instead of fabricating a workspace-key subject', () => { expect(() => requirePrincipalSubjectUserId({ diff --git a/apps/sim/lib/credential-groups/application/create-invite-link.test.ts b/apps/sim/lib/credential-groups/application/create-invite-link.test.ts index 80625359fed..388ee4e8f5a 100644 --- a/apps/sim/lib/credential-groups/application/create-invite-link.test.ts +++ b/apps/sim/lib/credential-groups/application/create-invite-link.test.ts @@ -104,6 +104,48 @@ describe('createCredentialGroupInviteLink', () => { expect(mocks.resolveGroup).not.toHaveBeenCalled() }) + it('issues an unattributed link for an actorless run', async () => { + // A schedule (or a webhook with no external subject) reaches this with a real + // admin-scoped delegation and no person on it. The delegation is the authority; + // the issuer is only recorded, and `created_by` is nullable — so this issues the + // link with no issuer rather than refusing, which is what it did when the + // subject was demanded here. + const { subjectUserId: _subject, ...base } = executorPrincipal() + // What actually authorizes an actorless caller: the delegation is running a + // deployment. No user is consulted anywhere in that decision. + const actorless = { + ...base, + delegationContext: { + kind: 'workflow_execution' as const, + workflowId: 'workflow-1', + principal: { + kind: 'system' as const, + serviceId: 'schedule' as const, + workspaceId: 'workspace-1', + workflowId: 'workflow-1', + }, + currentWorkflow: { + workflowId: 'workflow-1', + mode: 'deployment' as const, + deploymentVersionId: 'version-1', + }, + }, + } + + const result = await createCredentialGroupInviteLink.execute({ + principal: actorless, + input: { credentialGroupId: 'group-1', email: 'person@example.com' }, + }) + + expect(result.invitationLink).toBe('https://sim.ai/credential-groups/enroll/token-1') + expect(mocks.createInvitationLink).toHaveBeenCalledWith( + 'workspace-1', + 'group-1', + undefined, + 'person@example.com' + ) + }) + it('rejects delegation scoped to another Credential Group', async () => { await expect( createCredentialGroupInviteLink.execute({ diff --git a/apps/sim/lib/credential-groups/application/create-invite-link.ts b/apps/sim/lib/credential-groups/application/create-invite-link.ts index 90046594d62..9290ba46e59 100644 --- a/apps/sim/lib/credential-groups/application/create-invite-link.ts +++ b/apps/sim/lib/credential-groups/application/create-invite-link.ts @@ -1,5 +1,5 @@ import { AuditAction, AuditResourceType } from '@sim/audit' -import { requirePrincipalSubjectUserId } from '@sim/auth/principal' +import { resolvePrincipalSubjectUserId } from '@sim/auth/principal' import { isValidEmailSyntax, normalizeEmail } from '@sim/utils/string' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' @@ -39,7 +39,9 @@ export const createCredentialGroupInviteLink = defineAuthorizedWorkspaceUseCase( return await createCredentialGroupInvitationLink( context.workspaceId, context.credentialGroupId, - requirePrincipalSubjectUserId(principal), + // Attribution, not authority: the delegation's admin-scoped Credential Group + // grant is what permits this. An actorless run records no issuer. + resolvePrincipalSubjectUserId(principal), email ) } catch (error) { diff --git a/apps/sim/lib/credential-groups/application/send-invite.ts b/apps/sim/lib/credential-groups/application/send-invite.ts index 071d0136204..31ed72e9330 100644 --- a/apps/sim/lib/credential-groups/application/send-invite.ts +++ b/apps/sim/lib/credential-groups/application/send-invite.ts @@ -1,5 +1,4 @@ import { AuditAction, AuditResourceType } from '@sim/audit' -import { requirePrincipalSubjectUserId } from '@sim/auth/principal' import { isValidEmailSyntax, normalizeEmail } from '@sim/utils/string' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' @@ -41,7 +40,7 @@ export const sendCredentialGroupInvite = defineAuthorizedWorkspaceUseCase({ } await requireCredentialGroupsAvailable(context.workspaceId) - const userId = requirePrincipalSubjectUserId(principal) + const userId = requireCredentialGroupWorkflowSubject(principal) const inviter = await loadCredentialGroupInviterIdentity(userId) const inviterName = inviter?.name?.trim() || inviter?.email if (!inviterName) { diff --git a/apps/sim/lib/credential-groups/enrollments.ts b/apps/sim/lib/credential-groups/enrollments.ts index 093ddaad70a..91e661c7238 100644 --- a/apps/sim/lib/credential-groups/enrollments.ts +++ b/apps/sim/lib/credential-groups/enrollments.ts @@ -330,7 +330,14 @@ async function getInvitationContext( async function issueInvitation( context: InvitationContext, - userId: string, + /** + * Who to record as the issuer, when there is someone. Attribution only — the + * authority to invite comes from the delegation, so an actorless run (a schedule, + * or a webhook with no external subject) issues an unattributed invitation rather + * than none. `created_by` is nullable and `on delete set null`, so a row with no + * issuer is a shape the schema already carries. + */ + userId: string | undefined, email: string, options: SendInvitationOptions ): Promise { @@ -387,7 +394,7 @@ async function issueInvitation( completedAt: preservesProgress ? current.completedAt : null, revokedAt: null, lastDeliveryError: null, - createdBy: userId, + createdBy: userId ?? null, updatedAt: now, } const [next] = current @@ -664,7 +671,8 @@ export async function inviteCredentialGroupEnrollment( export async function createCredentialGroupInvitationLink( workspaceId: string, groupId: string, - userId: string, + /** See {@link issueInvitation}: the issuer is attribution, never the authority. */ + userId: string | undefined, email: string ): Promise { const context = await getInvitationContext(workspaceId, groupId) diff --git a/apps/sim/lib/credentials/application/authorized-credential-use-case.ts b/apps/sim/lib/credentials/application/authorized-credential-use-case.ts index 7902af3d970..6fe719b99a8 100644 --- a/apps/sim/lib/credentials/application/authorized-credential-use-case.ts +++ b/apps/sim/lib/credentials/application/authorized-credential-use-case.ts @@ -88,6 +88,7 @@ export function defineAuthorizedCredentialUseCase< async authorizeResource({ principal, context }) { const actor = await getCredentialActorContext( context.credential.id, + // actorless-unsupported: credential access is decided per person; an actorless run has no credential grants requirePrincipalSubjectUserId(principal) ) if ( diff --git a/apps/sim/lib/credentials/application/connection-target.ts b/apps/sim/lib/credentials/application/connection-target.ts index a7cb618ceab..38dfcd482b7 100644 --- a/apps/sim/lib/credentials/application/connection-target.ts +++ b/apps/sim/lib/credentials/application/connection-target.ts @@ -46,6 +46,7 @@ export async function resolveCredentialConnectionTarget(params: { } if (!credentialId) throw new Error('Credential reconnect target is missing its credential ID') + // actorless-unsupported: reconnecting rebinds a person's own OAuth grant const userId = requirePrincipalSubjectUserId(principal) const targetCredentialId = credentialId const credential = await getWorkspaceCredential({ diff --git a/apps/sim/lib/custom-tools/application/use-cases.ts b/apps/sim/lib/custom-tools/application/use-cases.ts index 177b6e379b1..a0a20abaebe 100644 --- a/apps/sim/lib/custom-tools/application/use-cases.ts +++ b/apps/sim/lib/custom-tools/application/use-cases.ts @@ -70,6 +70,7 @@ async function resolveAvailableToolContext(args: { const workspace = await resolveWorkspaceContext(args.workspaceId) const tool = await getCustomToolById({ toolId: args.toolId, + // actorless-unsupported: a custom tool is owned by one user; an actorless run has no library to look in userId: requirePrincipalSubjectUserId(args.principal), workspaceId: workspace.workspaceId, }) @@ -129,6 +130,7 @@ export const listAvailableCustomToolsUseCase = defineAuthorizedWorkspaceUseCase( authorizationOptions, async execute({ principal, context }) { const tools = await listCustomTools({ + // actorless-unsupported: the listing is the acting user's own tool library, which an actorless run does not have userId: requirePrincipalSubjectUserId(principal), workspaceId: context.workspaceId, }) @@ -353,6 +355,7 @@ export const updateAvailableCustomToolUseCase = defineAuthorizedWorkspaceUseCase const tool = await updateCustomTool({ workspaceId: context.workspaceId, toolId: context.tool.id, + // actorless-unsupported: editing a tool is scoped to its owner; an actorless run owns none userId: requirePrincipalSubjectUserId(principal), title, schema: input.schema ?? context.tool.schema, @@ -422,6 +425,7 @@ export const deleteAvailableCustomToolUseCase = defineAuthorizedWorkspaceUseCase const deleted = await deleteCustomTool({ workspaceId: context.workspaceId, toolId: context.tool.id, + // actorless-unsupported: deleting a tool is scoped to its owner; an actorless run owns none userId: requirePrincipalSubjectUserId(principal), }) if (!deleted) throw new OrchestrationError('not_found', 'Custom tool not found') diff --git a/apps/sim/lib/internal/deployments/execute-tool.ts b/apps/sim/lib/internal/deployments/execute-tool.ts index 13f70b35f68..7bfe4d4bf42 100644 --- a/apps/sim/lib/internal/deployments/execute-tool.ts +++ b/apps/sim/lib/internal/deployments/execute-tool.ts @@ -3,7 +3,6 @@ import { isPlainRecord } from '@sim/utils/object' import type { ZodError, ZodType } from 'zod' import { getValidationErrorMessage } from '@/lib/api/server' import { concealCrossTenantResourceError } from '@/lib/api/server/routes' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { deploymentsDeployBodySchema, @@ -20,6 +19,11 @@ import { executeDeploymentsUndeploy, } from '@/lib/internal/deployments/operations' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import type { InternalToolOperationCall, InternalToolOperationHandler, @@ -52,11 +56,12 @@ function parseInput(schema: ZodType, request: InternalToolOperationCall) { } function errorResponse(request: InternalToolOperationCall, error: unknown): Response { - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return Response.json({ success: false, error: 'Authentication required' }, { status: 401 }) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return Response.json( + { success: false, error: internalToolIdentityFaultMessage(identityFault) }, + { status: internalToolIdentityFaultStatus(identityFault) } + ) } const classified = asOrchestrationError( diff --git a/apps/sim/lib/internal/file/execute-tool.ts b/apps/sim/lib/internal/file/execute-tool.ts index b709572e237..1d70d1f5a0f 100644 --- a/apps/sim/lib/internal/file/execute-tool.ts +++ b/apps/sim/lib/internal/file/execute-tool.ts @@ -1,16 +1,16 @@ -import { - PrincipalSubjectUserRequiredError, - resolvePrincipalAttribution, - resolvePrincipalSubject, -} from '@sim/auth/principal' +import { resolvePrincipalAttribution, resolvePrincipalSubject } from '@sim/auth/principal' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { fileParseContract } from '@/lib/api/contracts/storage-transfer' import { fileManageContract } from '@/lib/api/contracts/tools/file' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { executeFileManageOperation } from '@/lib/internal/file/operations' import { executeFileParserOperation } from '@/lib/internal/file/parser' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import { parseInternalToolInput } from '@/lib/internal/tool-operations/parse-input' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { WORKSPACE_FILES_DELEGATION_AUDIENCE } from '@/lib/workspace-files/application/authorization' @@ -104,12 +104,12 @@ export const executeFileTool: InternalToolOperationHandler = async (request) => return response } catch (error) { request.signal?.throwIfAborted() - if ( - error instanceof InvalidInternalDelegationBindingError || - error instanceof PrincipalSubjectUserRequiredError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return Response.json({ success: false, error: 'Authentication required' }, { status: 401 }) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return Response.json( + { success: false, error: internalToolIdentityFaultMessage(identityFault) }, + { status: internalToolIdentityFaultStatus(identityFault) } + ) } const message = getErrorMessage(error, 'Unknown error') logger.error('File operation dispatch failed', { diff --git a/apps/sim/lib/internal/knowledge/execute-tool.ts b/apps/sim/lib/internal/knowledge/execute-tool.ts index a9fc25818ae..16f5535f5fc 100644 --- a/apps/sim/lib/internal/knowledge/execute-tool.ts +++ b/apps/sim/lib/internal/knowledge/execute-tool.ts @@ -18,7 +18,6 @@ import { upsertKnowledgeDocumentContract, } from '@/lib/api/contracts/knowledge' import type { JsonErrorResponseDescriptor } from '@/lib/api/server/routes/types' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { createChunkOperation, createDocumentsOperation, @@ -37,6 +36,11 @@ import { upsertDocumentOperation, } from '@/lib/internal/knowledge/operations' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import { parseInternalContractInput, parseInternalOperationInput, @@ -125,11 +129,12 @@ export const executeKnowledgeTool: InternalToolOperationHandler = async (request audience: KNOWLEDGE_DELEGATION_AUDIENCE, }) } catch (error) { - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return Response.json({ error: 'Authentication required' }, { status: 401 }) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return Response.json( + { error: internalToolIdentityFaultMessage(identityFault) }, + { status: internalToolIdentityFaultStatus(identityFault) } + ) } throw error } diff --git a/apps/sim/lib/internal/logs/execute-tool.test.ts b/apps/sim/lib/internal/logs/execute-tool.test.ts index e4daa1dc157..e7cdae768ca 100644 --- a/apps/sim/lib/internal/logs/execute-tool.test.ts +++ b/apps/sim/lib/internal/logs/execute-tool.test.ts @@ -2,7 +2,10 @@ * @vitest-environment node */ -import type { WorkflowExecutionDelegatedPrincipal } from '@sim/auth/principal' +import { + PrincipalSubjectUserRequiredError, + type WorkflowExecutionDelegatedPrincipal, +} from '@sim/auth/principal' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { ExecutionContext } from '@/executor/types' @@ -169,4 +172,24 @@ describe('executeLogsTool', () => { expect(invalidResponse.status).toBe(500) expect(await invalidResponse.json()).toEqual({ error: 'Failed to fetch log' }) }) + + it('names the missing identity instead of answering an opaque 500', async () => { + // The regression this guards: an operation that still demands a person answered + // every scheduled run with `Failed to fetch log`, which says nothing about why. + mocks.getRun.mockRejectedValueOnce(new PrincipalSubjectUserRequiredError('delegated')) + + const response = await executeLogsTool({ + toolId: 'logs_get_run_details', + input: { executionId: 'execution-1' }, + headers: new Headers(), + context: CONTEXT, + requestId: 'request-1', + }) + + expect(response.status).toBe(403) + expect(await response.json()).toEqual({ + error: + 'This tool requires a user identity, and this run has none — scheduled and webhook triggers run without a user', + }) + }) }) diff --git a/apps/sim/lib/internal/logs/execute-tool.ts b/apps/sim/lib/internal/logs/execute-tool.ts index 59870040852..58ba3b5ef73 100644 --- a/apps/sim/lib/internal/logs/execute-tool.ts +++ b/apps/sim/lib/internal/logs/execute-tool.ts @@ -12,7 +12,6 @@ import { logIdParamsSchema, } from '@/lib/api/contracts/logs' import { serializeZodIssues } from '@/lib/api/server/validation' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { executeLogsGet, @@ -22,6 +21,11 @@ import { type LogsToolOperationContext, } from '@/lib/internal/logs/operations' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { LOGS_DELEGATION_AUDIENCE } from '@/lib/logs/application/authorization' @@ -137,11 +141,12 @@ export const executeLogsTool: InternalToolOperationHandler = async (request) => return Response.json(dispatched.contract.response.schema.parse(dispatched.body)) } catch (error) { request.signal?.throwIfAborted() - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return Response.json({ error: 'Authentication required' }, { status: 401 }) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return Response.json( + { error: internalToolIdentityFaultMessage(identityFault) }, + { status: internalToolIdentityFaultStatus(identityFault) } + ) } return errorResponse(request.toolId, error) } diff --git a/apps/sim/lib/internal/mcp/discover-tools.ts b/apps/sim/lib/internal/mcp/discover-tools.ts index 78050f11e9c..aa2e35452e4 100644 --- a/apps/sim/lib/internal/mcp/discover-tools.ts +++ b/apps/sim/lib/internal/mcp/discover-tools.ts @@ -25,7 +25,8 @@ export async function discoverMcpServerToolsAsExecutor({ signal?.throwIfAborted() const result = await discoverMcpServerToolsUseCase.execute({ principal, - input: { workspaceId, serverId }, + // See `credentialUserId`: an actorless run's credentials are the workflow's own user's. + input: { workspaceId, serverId, credentialUserId: context.userId }, }) signal?.throwIfAborted() return result.tools diff --git a/apps/sim/lib/internal/mcp/execute-tool.ts b/apps/sim/lib/internal/mcp/execute-tool.ts index 33ce7d4413f..dd36e859e65 100644 --- a/apps/sim/lib/internal/mcp/execute-tool.ts +++ b/apps/sim/lib/internal/mcp/execute-tool.ts @@ -3,7 +3,6 @@ import { resolvePrincipalSubject } from '@sim/auth/principal' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { isPlainRecord } from '@sim/utils/object' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { capExecutionTimeoutMs, getAsyncExecutionTimeoutForBillingAttribution, @@ -11,6 +10,11 @@ import { } from '@/lib/core/execution-limits' import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { MCP_SERVER_DELEGATION_AUDIENCE } from '@/lib/mcp/application/authorization' import { executeMcpToolUseCase, McpToolsNotAllowedError } from '@/lib/mcp/application/execute-tool' @@ -145,6 +149,9 @@ export const executeMcpTool: InternalToolOperationHandler = async (request) => { input: { workspaceId: request.context.workspaceId, serverId, + // An actorless run (a schedule, a webhook with no external subject) has no + // subject on its principal; the workflow's own user owns the credentials. + credentialUserId: request.context.userId, toolName, arguments: args, callChain: request.context.callChain, @@ -168,11 +175,12 @@ export const executeMcpTool: InternalToolOperationHandler = async (request) => { ) } catch (error) { request.signal?.throwIfAborted() - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return Response.json({ success: false, error: 'Authentication required' }, { status: 401 }) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return Response.json( + { success: false, error: internalToolIdentityFaultMessage(identityFault) }, + { status: internalToolIdentityFaultStatus(identityFault) } + ) } if (error instanceof McpToolsNotAllowedError) { return createResponse( diff --git a/apps/sim/lib/internal/memory/execute-tool.ts b/apps/sim/lib/internal/memory/execute-tool.ts index 19374b80108..b3c0fe5dd91 100644 --- a/apps/sim/lib/internal/memory/execute-tool.ts +++ b/apps/sim/lib/internal/memory/execute-tool.ts @@ -13,7 +13,6 @@ import { memoryPostBodySchema, } from '@/lib/api/contracts/memory' import { serializeZodIssues } from '@/lib/api/server/validation' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { asOrchestrationError, statusForOrchestrationError } from '@/lib/core/orchestration/types' import { executeMemoryAdd, @@ -25,6 +24,11 @@ import { } from '@/lib/internal/memory/operations' import { createMemoryToolResponse, MemoryProvenanceError } from '@/lib/internal/memory/provenance' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { MEMORY_DELEGATION_AUDIENCE } from '@/lib/memory/application/authorization' @@ -142,13 +146,11 @@ export const executeMemoryTool: InternalToolOperationHandler = async (request) = ) } catch (error) { request.signal?.throwIfAborted() - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { return Response.json( - { success: false, error: { message: 'Authentication required' } }, - { status: 401 } + { success: false, error: { message: internalToolIdentityFaultMessage(identityFault) } }, + { status: internalToolIdentityFaultStatus(identityFault) } ) } return failureResponse(request.toolId, error) diff --git a/apps/sim/lib/internal/table/execute-tool.ts b/apps/sim/lib/internal/table/execute-tool.ts index 299d94f4a75..d0776a23fde 100644 --- a/apps/sim/lib/internal/table/execute-tool.ts +++ b/apps/sim/lib/internal/table/execute-tool.ts @@ -16,7 +16,6 @@ import { upsertTableRowContract, } from '@/lib/api/contracts/tables' import { type InternalErrorPolicy, internalOrchestrationErrorPolicy } from '@/lib/api/server/routes' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' import { executeTableCreate, @@ -35,6 +34,11 @@ import { type TableToolOperationResult, } from '@/lib/internal/table/operations' import { createTableToolResponse } from '@/lib/internal/table/provenance' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import { parseInternalContractInput } from '@/lib/internal/tool-operations/parse-contract-input' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { internalTableErrorPolicies } from '@/lib/table/api/route-policies' @@ -292,11 +296,12 @@ export const executeTableTool: InternalToolOperationHandler = async (request) => return createTableToolResponse(validatedBody, result.result.provenance) } catch (error) { request.signal?.throwIfAborted() - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return Response.json({ error: 'Authentication required' }, { status: 401 }) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return Response.json( + { error: internalToolIdentityFaultMessage(identityFault) }, + { status: internalToolIdentityFaultStatus(identityFault) } + ) } return errorResponse( request.toolId, diff --git a/apps/sim/lib/internal/tool-operations/identity-faults.ts b/apps/sim/lib/internal/tool-operations/identity-faults.ts new file mode 100644 index 00000000000..a0132aaf386 --- /dev/null +++ b/apps/sim/lib/internal/tool-operations/identity-faults.ts @@ -0,0 +1,50 @@ +import { PrincipalSubjectUserRequiredError } from '@sim/auth/principal' +import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' + +/** + * Identity failures that every in-process tool handler answers the same way. + * + * `unauthenticated` is a caller that never established a runtime identity — a + * missing or unbindable executor delegation. + * + * `subject_user_required` is a caller that IS authenticated but has no human + * subject to act as, which is the normal shape of an actorless run: a schedule, + * or a webhook carrying no external subject. It is distinct from + * `unauthenticated` because retrying, re-authenticating, or fixing credentials + * cannot resolve it — the operation is simply not available to that trigger, and + * an operator reading the log needs to be told so rather than shown a generic + * failure. The Logs detail tools returned an opaque 500 for exactly this for one + * evening, which is why the classification lives here rather than per handler. + */ +export type InternalToolIdentityFault = 'unauthenticated' | 'subject_user_required' + +const FAULTS: Record = { + unauthenticated: { status: 401, message: 'Authentication required' }, + subject_user_required: { + status: 403, + message: + 'This tool requires a user identity, and this run has none — scheduled and webhook triggers run without a user', + }, +} + +/** Classifies an identity fault, or returns `undefined` for any other failure. */ +export function classifyInternalToolIdentityFault( + error: unknown +): InternalToolIdentityFault | undefined { + if (error instanceof PrincipalSubjectUserRequiredError) return 'subject_user_required' + if ( + error instanceof InvalidInternalDelegationBindingError || + (error instanceof Error && error.message === 'Authentication required') + ) { + return 'unauthenticated' + } + return undefined +} + +export function internalToolIdentityFaultStatus(fault: InternalToolIdentityFault): number { + return FAULTS[fault].status +} + +export function internalToolIdentityFaultMessage(fault: InternalToolIdentityFault): string { + return FAULTS[fault].message +} diff --git a/apps/sim/lib/internal/windchill/execute-tool.ts b/apps/sim/lib/internal/windchill/execute-tool.ts index 689e140e6ad..35a0141ed5f 100644 --- a/apps/sim/lib/internal/windchill/execute-tool.ts +++ b/apps/sim/lib/internal/windchill/execute-tool.ts @@ -7,9 +7,13 @@ import type { import { windchillOperationBodySchema } from '@/lib/api/contracts/tools/windchill' import { getValidationErrorMessage } from '@/lib/api/server' import { DEFAULT_MAX_JSON_BODY_BYTES } from '@/lib/api/server/validation' -import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-delegation' import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits' import { createExecutorPrincipalFromExecutionContext } from '@/lib/internal/principals/executor' +import { + classifyInternalToolIdentityFault, + internalToolIdentityFaultMessage, + internalToolIdentityFaultStatus, +} from '@/lib/internal/tool-operations/identity-faults' import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types' import { WindchillProviderError } from '@/lib/internal/windchill/client' import { WindchillOperationError } from '@/lib/internal/windchill/errors' @@ -90,11 +94,12 @@ export const executeWindchillTool: InternalToolOperationHandler = async (request return Response.json({ success: true, output } satisfies WindchillOperationResponse) } catch (error) { signal?.throwIfAborted() - if ( - error instanceof InvalidInternalDelegationBindingError || - (error instanceof Error && error.message === 'Authentication required') - ) { - return failureResponse('Authentication required', 401) + const identityFault = classifyInternalToolIdentityFault(error) + if (identityFault) { + return failureResponse( + internalToolIdentityFaultMessage(identityFault), + internalToolIdentityFaultStatus(identityFault) + ) } logger.error(`[${requestId}] Windchill operation failed`, { operation: toolId, diff --git a/apps/sim/lib/internal/windchill/operations.ts b/apps/sim/lib/internal/windchill/operations.ts index 5bd72c664d1..f8bc8c30e2f 100644 --- a/apps/sim/lib/internal/windchill/operations.ts +++ b/apps/sim/lib/internal/windchill/operations.ts @@ -482,6 +482,7 @@ async function storeDownloadedFile({ buffer, fileName, contentType, + // actorless-unsupported: the uploaded file needs an owning user row; attributing it to the workflow's user is a follow-up requirePrincipalSubjectUserId(principal) ) signal?.throwIfAborted() @@ -491,6 +492,7 @@ async function storeDownloadedFile({ buffer, fileName, contentType, + // actorless-unsupported: the uploaded file needs an owning user row; attributing it to the workflow's user is a follow-up userId: requirePrincipalSubjectUserId(principal), }) signal?.throwIfAborted() @@ -575,6 +577,7 @@ export async function executeWindchillOperation( : body.attachmentFiles const files = await loadUploadFiles( inputs, + // actorless-unsupported: reads the acting user's own files; attributing it to the workflow's user is a follow-up requirePrincipalSubjectUserId(principal), requestId, signal diff --git a/apps/sim/lib/knowledge/api/internal-route.ts b/apps/sim/lib/knowledge/api/internal-route.ts index 68fa01cd54f..5608a476923 100644 --- a/apps/sim/lib/knowledge/api/internal-route.ts +++ b/apps/sim/lib/knowledge/api/internal-route.ts @@ -32,6 +32,7 @@ import { captureServerEvent } from '@/lib/posthog/server' import type { UploadSessionRecord } from '@/lib/uploads/upload-session/service' export function internalKnowledgeActorUserId(principal: Principal): string { + // actorless-unsupported: knowledge writes are attributed to a person; the actorless path is a follow-up return requirePrincipalSubjectUserId(principal) } diff --git a/apps/sim/lib/knowledge/application/authorized-knowledge-use-case.ts b/apps/sim/lib/knowledge/application/authorized-knowledge-use-case.ts index bf1e3659c38..6deb81fe26c 100644 --- a/apps/sim/lib/knowledge/application/authorized-knowledge-use-case.ts +++ b/apps/sim/lib/knowledge/application/authorized-knowledge-use-case.ts @@ -126,6 +126,7 @@ export function defineAuthorizedKnowledgeUseCase< if (isLegacyPersonalKnowledgeContext(context)) { if ( principal.kind === 'workspace_api_key' || + // actorless-unsupported: a legacy personal knowledge base has exactly one owner, so an actorless caller is never it requirePrincipalSubjectUserId(principal) !== context.legacyPersonalOwnerUserId ) { throw new OrchestrationError('not_found', 'Knowledge base not found') diff --git a/apps/sim/lib/knowledge/application/billing.ts b/apps/sim/lib/knowledge/application/billing.ts index e7ca3425751..ccf4af4a8f0 100644 --- a/apps/sim/lib/knowledge/application/billing.ts +++ b/apps/sim/lib/knowledge/application/billing.ts @@ -20,6 +20,7 @@ export function resolveKnowledgeAttributedUserId( principal: Principal, context: KnowledgeResourceContext ): string { + // actorless-unsupported: a workspace-less knowledge base bills its owner directly, with no workspace to attribute to if (context.workspaceId === undefined) return requirePrincipalSubjectUserId(principal) return resolvePrincipalAttribution(principal, { workspaceBillingOwnerUserId: context.billedAccountUserId, diff --git a/apps/sim/lib/logs/application/read-execution-snapshot.ts b/apps/sim/lib/logs/application/read-execution-snapshot.ts index e5b645710c0..c44f803852a 100644 --- a/apps/sim/lib/logs/application/read-execution-snapshot.ts +++ b/apps/sim/lib/logs/application/read-execution-snapshot.ts @@ -1,4 +1,4 @@ -import { requirePrincipalSubjectUserId } from '@sim/auth/principal' +import { resolvePrincipalSubjectUserId } from '@sim/auth/principal' import { db } from '@sim/db' import { jobExecutionLogs, workflowExecutionLogs, workflowExecutionSnapshots } from '@sim/db/schema' import { eq, inArray } from 'drizzle-orm' @@ -168,8 +168,10 @@ const authorizedReadExecutionSnapshotUseCase = defineAuthorizedWorkspaceUseCase( )) as WorkflowExecutionLog['executionData'] const traceSpans = (executionData?.traceSpans as TraceSpan[]) || [] if (traceSpans.length > 0) { + // Attribution, not authorization: the publisher's policy is the only gate, + // and an actorless run has no user to name. await hydrateChildTraces(traceSpans, { - viewerUserId: requirePrincipalSubjectUserId(principal), + viewerUserId: resolvePrincipalSubjectUserId(principal), }) } diff --git a/apps/sim/lib/logs/application/read-log-detail.test.ts b/apps/sim/lib/logs/application/read-log-detail.test.ts new file mode 100644 index 00000000000..ee291213706 --- /dev/null +++ b/apps/sim/lib/logs/application/read-log-detail.test.ts @@ -0,0 +1,126 @@ +/** + * @vitest-environment node + */ + +import type { Principal } from '@sim/auth/principal' +import { workflowExecutionLogs } from '@sim/db/schema' +import { queueTableRows, resetDbChainMock } from '@sim/testing' +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + readLogDetail: vi.fn(), + resolveWorkspace: vi.fn(), + resolvePermission: vi.fn(), +})) + +vi.mock('@/lib/logs/fetch-log-detail', () => ({ + readLogDetail: mocks.readLogDetail, +})) + +vi.mock('@/lib/workspaces/application/workspace-context', () => ({ + resolveActiveWorkspaceApplicationContext: mocks.resolveWorkspace, +})) + +vi.mock('@sim/platform-authz/workspace', () => ({ + permissionSatisfies: (held: string | null, required: string) => + held === 'admin' || held === required || (held === 'write' && required === 'read'), + resolveEffectiveWorkspacePermission: mocks.resolvePermission, +})) + +import { readLogDetailUseCase } from '@/lib/logs/application/read-log-detail' + +const WORKSPACE_ID = 'workspace-1' +const EXECUTION_ID = 'execution-1' + +/** + * What a scheduled run actually holds: a delegation whose workflow principal is the + * actorless `system:schedule`, so `subjectUserId` is absent. Its workspace reach comes + * from running a deployment, which is the branch `workspace-authorization.ts` admits + * without a subject — so this exercises the real authorization path, not a stub. + */ +const SCHEDULED_PRINCIPAL: Principal = { + kind: 'delegated', + serviceId: 'executor', + workspaceId: WORKSPACE_ID, + delegationId: 'delegation-1', + audience: 'sim:logs', + issuedAt: new Date(Date.now() - 1_000), + expiresAt: new Date(Date.now() + 5 * 60 * 1000), + delegationContext: { + kind: 'workflow_execution', + workflowId: 'workflow-1', + principal: { + kind: 'system', + serviceId: 'schedule', + workspaceId: WORKSPACE_ID, + workflowId: 'workflow-1', + }, + currentWorkflow: { + workflowId: 'workflow-1', + mode: 'deployment', + deploymentVersionId: 'version-1', + }, + }, +} + +const HUMAN_PRINCIPAL: Principal = { + ...SCHEDULED_PRINCIPAL, + subjectUserId: 'user-1', + delegationContext: { + kind: 'workflow_execution', + workflowId: 'workflow-1', + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + currentWorkflow: { + workflowId: 'workflow-1', + mode: 'deployment', + deploymentVersionId: 'version-1', + }, + }, +} + +function queueLogRow(): void { + queueTableRows(workflowExecutionLogs, [{ workspaceId: WORKSPACE_ID, executionId: EXECUTION_ID }]) +} + +describe('readLogDetailUseCase', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + mocks.resolveWorkspace.mockResolvedValue({ + workspaceId: WORKSPACE_ID, + workspaceOrganizationId: null, + allowPersonalApiKeys: true, + }) + mocks.readLogDetail.mockResolvedValue({ id: 'log-1', executionId: EXECUTION_ID }) + mocks.resolvePermission.mockResolvedValue('admin') + }) + + afterAll(resetDbChainMock) + + it('reads a run for an actorless schedule, passing no viewer', async () => { + queueLogRow() + + const result = await readLogDetailUseCase.execute({ + principal: SCHEDULED_PRINCIPAL, + input: { workspaceId: WORKSPACE_ID, lookupColumn: 'executionId', lookupValue: EXECUTION_ID }, + }) + + expect(result.detail).toMatchObject({ id: 'log-1' }) + expect(mocks.readLogDetail).toHaveBeenCalledWith( + expect.objectContaining({ workspaceId: WORKSPACE_ID, viewerUserId: undefined }) + ) + }) + + it('still names the human behind a run that has one', async () => { + queueLogRow() + + await readLogDetailUseCase.execute({ + principal: HUMAN_PRINCIPAL, + input: { workspaceId: WORKSPACE_ID, lookupColumn: 'executionId', lookupValue: EXECUTION_ID }, + }) + + expect(mocks.readLogDetail).toHaveBeenCalledWith( + expect.objectContaining({ viewerUserId: 'user-1' }) + ) + }) +}) diff --git a/apps/sim/lib/logs/application/read-log-detail.ts b/apps/sim/lib/logs/application/read-log-detail.ts index 3af63a0dc53..326a3047ee1 100644 --- a/apps/sim/lib/logs/application/read-log-detail.ts +++ b/apps/sim/lib/logs/application/read-log-detail.ts @@ -1,4 +1,4 @@ -import { requirePrincipalSubjectUserId } from '@sim/auth/principal' +import { resolvePrincipalSubjectUserId } from '@sim/auth/principal' import { db } from '@sim/db' import { jobExecutionLogs, workflowExecutionLogs } from '@sim/db/schema' import { eq } from 'drizzle-orm' @@ -77,8 +77,10 @@ const authorizedReadLogDetailUseCase = defineAuthorizedWorkspaceUseCase({ authorizationOptions: logDelegationAuthorization(), async execute({ principal, input, context }) { input.signal?.throwIfAborted() + // Attribution, not authorization: an actorless run (a schedule, or a webhook + // with no external subject) reads its own workspace's logs with no user to name. const detail = await readLogDetail({ - viewerUserId: requirePrincipalSubjectUserId(principal), + viewerUserId: resolvePrincipalSubjectUserId(principal), workspaceId: context.workspaceId, lookupColumn: input.lookupColumn, lookupValue: input.lookupValue, diff --git a/apps/sim/lib/logs/execution/hydrate-child-traces.test.ts b/apps/sim/lib/logs/execution/hydrate-child-traces.test.ts index 627e36de1b9..0af90346a48 100644 --- a/apps/sim/lib/logs/execution/hydrate-child-traces.test.ts +++ b/apps/sim/lib/logs/execution/hydrate-child-traces.test.ts @@ -134,6 +134,35 @@ describe('hydrateChildTraces', () => { expect(mockCheckWorkspaceAccess).not.toHaveBeenCalled() }) + it('joins the same run for an actorless read, which carries no reader at all', async () => { + // A scheduled run inspecting its own child has no user on its principal. The + // viewer is attribution, never a gate, so its absence must change nothing about + // what is joined — and must not throw, which is how this broke in production. + mockSelect.mockResolvedValue([ + { + executionId: 'child-exec-1', + workspaceId: 'ws-source', + workflowId: 'wf-source', + stateSnapshotId: 'snap-1', + executionData: {}, + }, + ]) + const spans = [boundarySpan('child-exec-1')] + + const result = await hydrateChildTraces(spans, {}) + + expect(result.hydrated).toBe(1) + expect(spans[0].childTraceAccess).toBe('granted') + expect(spans[0].children?.[0].name).toBe('Agent 1') + expect(spans[0].childWorkflowSnapshotId).toBe('snap-1') + // Pinned rather than left implicit: materialization is told there is no owner, + // instead of being handed a stand-in the run never authorized. + expect(mockMaterialize).toHaveBeenCalledWith( + {}, + expect.objectContaining({ workspaceId: 'ws-source', userId: undefined }) + ) + }) + it('refuses a handle whose block is not opted in, however it got there', async () => { // The decisive case: handles persisted before this policy existed meant "a child // ran, authorize the reader", not "the publisher consented". Reading presence as diff --git a/apps/sim/lib/logs/execution/hydrate-child-traces.ts b/apps/sim/lib/logs/execution/hydrate-child-traces.ts index ae815f88f70..cb4281eb92e 100644 --- a/apps/sim/lib/logs/execution/hydrate-child-traces.ts +++ b/apps/sim/lib/logs/execution/hydrate-child-traces.ts @@ -37,11 +37,13 @@ export interface ChildTraceDropCounts { export interface HydrateChildTracesOptions { /** - * The user reading the log. NOT an authorization input — the only policy is the - * publisher's, and it is the same answer for every reader. Carried so large-value - * materialization and secret projection have an owner to attribute their reads to. + * The user reading the log, when there is one. NOT an authorization input — the + * only policy is the publisher's, and it is the same answer for every reader. + * Carried so large-value materialization and secret projection have an owner to + * attribute their reads to; an actorless run has none, and needs none, because + * the display path only ever reads. */ - viewerUserId: string + viewerUserId?: string maxDepth?: number maxRows?: number } diff --git a/apps/sim/lib/logs/fetch-log-detail.test.ts b/apps/sim/lib/logs/fetch-log-detail.test.ts index 259eb9b8fa2..326032257bc 100644 --- a/apps/sim/lib/logs/fetch-log-detail.test.ts +++ b/apps/sim/lib/logs/fetch-log-detail.test.ts @@ -8,12 +8,17 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ materializeExecutionData: vi.fn(), + hydrateChildTraces: vi.fn(), })) vi.mock('@/lib/logs/execution/trace-store', () => ({ materializeExecutionDataForDisplay: mocks.materializeExecutionData, })) +vi.mock('@/lib/logs/execution/hydrate-child-traces', () => ({ + hydrateChildTraces: mocks.hydrateChildTraces, +})) + vi.mock('@/lib/logs/execution-origin', () => ({ workflowExecutionOriginSql: () => ({ as: () => ({}) }), })) @@ -25,6 +30,7 @@ describe('readLogDetail', () => { vi.clearAllMocks() resetDbChainMock() mocks.materializeExecutionData.mockResolvedValue({}) + mocks.hydrateChildTraces.mockResolvedValue({ hydrated: 0, dropped: {} }) }) afterAll(resetDbChainMock) @@ -80,4 +86,72 @@ describe('readLogDetail', () => { expect(joinedTables).not.toContain(workflowExecutionSnapshots) expect(joinedTables).not.toContain(user) }) + + it('reads a log for an actorless run, which has no viewer to attribute to', async () => { + // A scheduled run inspecting its own execution has no user on its principal. + // Attribution is the only thing the viewer feeds on this path, so its absence + // must return the same detail rather than throwing, which is how the Logs tools + // started answering every scheduled run with an opaque 500. + queueTableRows(workflowExecutionLogs, [ + { + id: 'log-1', + workflowId: 'workflow-1', + executionId: 'execution-1', + deploymentVersionId: null, + deploymentVersion: null, + deploymentVersionName: null, + level: 'info', + status: 'completed', + trigger: 'manual', + startedAt: new Date('2026-01-01T00:00:00.000Z'), + endedAt: new Date('2026-01-01T00:00:01.000Z'), + totalDurationMs: 1000, + executionData: {}, + costTotal: null, + files: null, + createdAt: new Date('2026-01-01T00:00:00.000Z'), + workflowName: 'Workflow', + workflowDescription: null, + workflowFolderId: null, + workflowUserId: 'user-1', + workflowWorkspaceId: 'workspace-1', + workflowCreatedAt: new Date('2026-01-01T00:00:00.000Z'), + workflowUpdatedAt: new Date('2026-01-01T00:00:00.000Z'), + pausedStatus: null, + pausedTotalPauseCount: 0, + pausedResumedCount: 0, + executionOrigin: null, + }, + ]) + queueTableRows(usageLog, []) + mocks.materializeExecutionData.mockResolvedValue({ + traceSpans: [ + { + id: 'span-1', + name: 'Agent 1', + type: 'agent', + duration: 5, + startTime: '2026-01-01T00:00:00.000Z', + endTime: '2026-01-01T00:00:00.005Z', + }, + ], + }) + + const result = await readLogDetail({ + workspaceId: 'workspace-1', + lookupColumn: 'id', + lookupValue: 'log-1', + }) + + expect(result).toMatchObject({ id: 'log-1', executionId: 'execution-1' }) + // Pinned explicitly: both consumers are told there is no owner, rather than + // being handed a stand-in the run never authorized. + expect(mocks.materializeExecutionData).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ workspaceId: 'workspace-1', userId: undefined }) + ) + expect(mocks.hydrateChildTraces).toHaveBeenCalledWith(expect.any(Array), { + viewerUserId: undefined, + }) + }) }) diff --git a/apps/sim/lib/logs/fetch-log-detail.ts b/apps/sim/lib/logs/fetch-log-detail.ts index ef5423b94a9..41a89227a12 100644 --- a/apps/sim/lib/logs/fetch-log-detail.ts +++ b/apps/sim/lib/logs/fetch-log-detail.ts @@ -29,7 +29,13 @@ export function jobCostTotal(raw: unknown): { total: number } | null { } interface FetchLogDetailArgs { - viewerUserId: string + /** + * The user reading the log, when there is one. Attribution only — workspace + * authorization already happened upstream, and the display path never writes. + * An actorless run (a schedule, or a webhook with no external subject) has no + * user to name and passes none. + */ + viewerUserId?: string workspaceId: string lookupColumn: LookupColumn lookupValue: string diff --git a/apps/sim/lib/mcp/application/authorization.ts b/apps/sim/lib/mcp/application/authorization.ts index 3e854da883b..69060a4f1c6 100644 --- a/apps/sim/lib/mcp/application/authorization.ts +++ b/apps/sim/lib/mcp/application/authorization.ts @@ -1,4 +1,6 @@ +import { type Principal, resolvePrincipalSubjectUserId } from '@sim/auth/principal' import type { WorkspaceDelegationPolicy } from '@/lib/core/application' +import { OrchestrationError } from '@/lib/core/orchestration/types' export const MCP_SERVER_DELEGATION_AUDIENCE = 'sim:mcp-servers' @@ -10,3 +12,31 @@ export const mcpServerDelegationPolicy = { workspaceOrganizationId: string | null allowPersonalApiKeys: boolean }> + +/** + * The user whose MCP server credentials an operation presents. + * + * Every MCP path — execution and both discoveries — connects to a third-party + * server with one person's OAuth credentials and is gated by that person's + * permission group, so unlike an attribution-only read it genuinely cannot run + * with nobody: there is no credential set and the permission gate would be + * skipped rather than satisfied. + * + * The principal's own subject always wins. `fallbackUserId` is for an actorless + * run — a schedule, or a webhook with no external subject — whose surface passes + * the workflow's own user; because it is consulted only when the principal names + * nobody, a caller cannot use it to nominate someone else's credentials. + * + * Fails as `forbidden` rather than throwing `PrincipalSubjectUserRequiredError`, + * so a run with no user at all gets a 403 that says why instead of an opaque 500. + */ +export function requireMcpCredentialUserId(principal: Principal, fallbackUserId?: string): string { + const userId = resolvePrincipalSubjectUserId(principal) ?? fallbackUserId + if (!userId) { + throw new OrchestrationError( + 'forbidden', + 'MCP servers are reached with a user\u2019s own credentials, and this run has no user' + ) + } + return userId +} diff --git a/apps/sim/lib/mcp/application/execute-tool.test.ts b/apps/sim/lib/mcp/application/execute-tool.test.ts index c775680d98d..99e4d1ca99a 100644 --- a/apps/sim/lib/mcp/application/execute-tool.test.ts +++ b/apps/sim/lib/mcp/application/execute-tool.test.ts @@ -1,10 +1,7 @@ /** * @vitest-environment node */ -import { - PrincipalSubjectUserRequiredError, - type WorkflowExecutionDelegatedPrincipal, -} from '@sim/auth/principal' +import type { WorkflowExecutionDelegatedPrincipal } from '@sim/auth/principal' import { beforeEach, describe, expect, it, vi } from 'vitest' const mocks = vi.hoisted(() => ({ @@ -172,6 +169,9 @@ describe('executeMcpToolUseCase', () => { }) it('does not invent a user for actorless system execution', async () => { + // An MCP call presents one person's credentials, so with nobody named at all it + // must refuse rather than reach for a stand-in. It refuses as `forbidden` so the + // caller is told why, instead of the unclassified 500 this used to produce. await expect( executeMcpToolUseCase.execute({ principal: ACTORLESS_PRINCIPAL, @@ -181,13 +181,59 @@ describe('executeMcpToolUseCase', () => { toolName: 'lookup', }, }) - ).rejects.toEqual(new PrincipalSubjectUserRequiredError('delegated')) + ).rejects.toMatchObject({ + code: 'forbidden', + message: + 'MCP servers are reached with a user\u2019s own credentials, and this run has no user', + }) expect(mocks.assertPermissionsAllowed).not.toHaveBeenCalled() expect(mocks.discoverServerTools).not.toHaveBeenCalled() expect(mocks.executeTool).not.toHaveBeenCalled() }) + it('runs an actorless workflow against the credentials its surface names', async () => { + // A scheduled run has no subject on its principal, but the workflow it runs has + // an owner, and the in-process surface passes that user. Without this a schedule + // could not call an MCP block at all. + await executeMcpToolUseCase.execute({ + principal: ACTORLESS_PRINCIPAL, + input: { + workspaceId: WORKSPACE.workspaceId, + serverId: SERVER.id, + toolName: 'lookup', + credentialUserId: 'workflow-owner', + arguments: { count: '2', enabled: 'true', tags: 'a,b' }, + }, + }) + + expect(mocks.assertPermissionsAllowed).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'workflow-owner' }) + ) + // Discovery resolves the same person's OAuth credentials, so it must be told + // the same user the permission gate was. + expect(mocks.discoverServerTools.mock.calls[0][0]).toBe('workflow-owner') + }) + + it('ignores a named user when the principal already has one', async () => { + // The property that keeps the fallback from becoming an impersonation handle: + // it is consulted only when the principal names nobody. + await executeMcpToolUseCase.execute({ + principal: PRINCIPAL, + input: { + workspaceId: WORKSPACE.workspaceId, + serverId: SERVER.id, + toolName: 'lookup', + credentialUserId: 'someone-else', + arguments: { count: '2', enabled: 'true', tags: 'a,b' }, + }, + }) + + expect(mocks.assertPermissionsAllowed).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'user-1' }) + ) + }) + it('does not execute when schema validation fails', async () => { await expect( executeMcpToolUseCase.execute({ diff --git a/apps/sim/lib/mcp/application/execute-tool.ts b/apps/sim/lib/mcp/application/execute-tool.ts index be542e71f0d..b3ab5aab610 100644 --- a/apps/sim/lib/mcp/application/execute-tool.ts +++ b/apps/sim/lib/mcp/application/execute-tool.ts @@ -1,10 +1,12 @@ -import { requirePrincipalSubjectUserId } from '@sim/auth/principal' import { createLogger } from '@sim/logger' import { getErrorMessage } from '@sim/utils/errors' import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { SIM_VIA_HEADER, serializeCallChain } from '@/lib/execution/call-chain' -import { mcpServerDelegationPolicy } from '@/lib/mcp/application/authorization' +import { + mcpServerDelegationPolicy, + requireMcpCredentialUserId, +} from '@/lib/mcp/application/authorization' import { resolveMcpServerContext } from '@/lib/mcp/application/context' import { mcpServerOperations } from '@/lib/mcp/application/operations' import { mcpService } from '@/lib/mcp/service' @@ -24,6 +26,20 @@ interface SchemaProperty { export interface ExecuteMcpToolInput { workspaceId: string serverId: string + /** + * The user whose MCP credentials and permission group apply. + * + * NOT an authorization input — workspace reach is decided by the principal, and + * this is only consulted after that. It exists because an MCP call connects to a + * third-party server with a specific person's OAuth credentials, so unlike a log + * read it cannot proceed with nobody: there is no credential set to present. + * + * An actorless run — a schedule, or a webhook with no external subject — has no + * subject on its principal, so its surface supplies the workflow's own user here. + * Ignored when the principal already names a subject, which is what stops a + * caller from nominating someone else's credentials. + */ + credentialUserId?: string toolName: string arguments?: Record callChain?: string[] @@ -132,7 +148,7 @@ export const executeMcpToolUseCase = defineAuthorizedWorkspaceUseCase({ authorizationOptions: { delegation: mcpServerDelegationPolicy }, async execute({ principal, input, context }): Promise { input.signal?.throwIfAborted() - const userId = requirePrincipalSubjectUserId(principal) + const userId = requireMcpCredentialUserId(principal, input.credentialUserId) await assertPermissionsAllowed({ userId, workspaceId: context.workspaceId, diff --git a/apps/sim/lib/mcp/application/use-cases.ts b/apps/sim/lib/mcp/application/use-cases.ts index e35a74c6606..5fed534bf4f 100644 --- a/apps/sim/lib/mcp/application/use-cases.ts +++ b/apps/sim/lib/mcp/application/use-cases.ts @@ -1,11 +1,14 @@ import { AuditAction, AuditResourceType } from '@sim/audit' -import { requirePrincipalSubjectUserId, resolvePrincipalAttribution } from '@sim/auth/principal' +import { resolvePrincipalAttribution } from '@sim/auth/principal' import { getPostgresErrorCode } from '@sim/utils/errors' import type { CursorKey, ListSortOrder } from '@/lib/api/list-query' import { defineAuthorizedWorkspaceUseCase, ForbiddenOperationError } from '@/lib/core/application' import { OrchestrationError } from '@/lib/core/orchestration/types' import { sanitizeUrlForLog } from '@/lib/core/utils/logging' -import { mcpServerDelegationPolicy } from '@/lib/mcp/application/authorization' +import { + mcpServerDelegationPolicy, + requireMcpCredentialUserId, +} from '@/lib/mcp/application/authorization' import { type McpServerContext, type McpWorkspaceContext, @@ -88,6 +91,13 @@ export const listMcpServersUseCase = defineAuthorizedWorkspaceUseCase({ export interface DiscoverMcpToolsInput { workspaceId: string + /** + * The user whose MCP credentials apply. See `ExecuteMcpToolInput.credentialUserId`: + * discovery resolves the same third-party credentials, so an actorless run's + * surface supplies the workflow's own user, and the principal's subject wins + * whenever it has one. + */ + credentialUserId?: string refresh?: boolean } @@ -98,7 +108,7 @@ export const discoverMcpToolsUseCase = defineAuthorizedWorkspaceUseCase({ authorizationOptions, async execute({ principal, input, context }) { const tools = await mcpService.discoverTools( - requirePrincipalSubjectUserId(principal), + requireMcpCredentialUserId(principal, input.credentialUserId), context.workspaceId, /** * A public `refresh` skips the positive cache but keeps the failure @@ -114,6 +124,13 @@ export const discoverMcpToolsUseCase = defineAuthorizedWorkspaceUseCase({ export interface DiscoverMcpServerToolsInput { workspaceId: string serverId: string + /** + * The user whose MCP credentials apply. See `ExecuteMcpToolInput.credentialUserId`: + * discovery resolves the same third-party credentials, so an actorless run's + * surface supplies the workflow's own user, and the principal's subject wins + * whenever it has one. + */ + credentialUserId?: string refresh?: boolean } @@ -151,7 +168,7 @@ export const discoverMcpServerToolsUseCase = defineAuthorizedWorkspaceUseCase({ } const tools = await mcpService.discoverServerTools( - requirePrincipalSubjectUserId(principal), + requireMcpCredentialUserId(principal, input.credentialUserId), context.server.id, context.workspaceId, /** diff --git a/apps/sim/lib/table/application/rows.ts b/apps/sim/lib/table/application/rows.ts index 7746b60db91..4ca671cdc4b 100644 --- a/apps/sim/lib/table/application/rows.ts +++ b/apps/sim/lib/table/application/rows.ts @@ -2,8 +2,8 @@ import { isDeepStrictEqual } from 'node:util' import { AuditAction, AuditResourceType } from '@sim/audit' import { type Principal, - requirePrincipalSubjectUserId, resolvePrincipalAttribution, + resolvePrincipalSubjectUserId, } from '@sim/auth/principal' import { db } from '@sim/db' import { getRequestContext } from '@sim/logger' @@ -496,7 +496,10 @@ export const queryTableRows = defineAuthorizedTableUseCase({ const orgId = await getWorkspaceOrganizationId(context.workspaceId) if ( !(await isFeatureEnabled('tables-v2-api', { - userId: requirePrincipalSubjectUserId(principal), + // An actorless run has no user to match a per-user rule against, and a + // missing one resolves the admin clause to `false` without a query — so + // the gate only ever narrows here, never widens. + userId: resolvePrincipalSubjectUserId(principal), orgId, })) ) { diff --git a/apps/sim/lib/workflows/application/deployments.ts b/apps/sim/lib/workflows/application/deployments.ts index 067f847d0bf..dfc5eadc1d3 100644 --- a/apps/sim/lib/workflows/application/deployments.ts +++ b/apps/sim/lib/workflows/application/deployments.ts @@ -243,6 +243,7 @@ export const revertWorkflowVersion = defineAuthorizedWorkflowUseCase({ operation: workflowOperations.revertVersion, resolveContext: resolveWorkflowContext, async execute({ principal, input, context }) { + // actorless-unsupported: reverting a version is an authored edit and records who made it; no tool exposes it to a run const userId = requirePrincipalSubjectUserId(principal) await requireMutableWorkflow(context.workflowId) const result = await performRevertToVersion({ diff --git a/apps/sim/lib/workspace-files/application/share-workspace-file.ts b/apps/sim/lib/workspace-files/application/share-workspace-file.ts index e28789a8846..247af5d1198 100644 --- a/apps/sim/lib/workspace-files/application/share-workspace-file.ts +++ b/apps/sim/lib/workspace-files/application/share-workspace-file.ts @@ -72,6 +72,7 @@ export const updateWorkspaceFileShare = defineAuthorizedWorkspaceFileUseCase({ return { ...canonical, file } }, async execute({ principal, input, context }): Promise { + // actorless-unsupported: a share link records the person who published it const subjectUserId = requirePrincipalSubjectUserId(principal) const existingShare = await getShareForResource('file', context.fileId) diff --git a/package.json b/package.json index 35e5619bd11..115b8854347 100644 --- a/package.json +++ b/package.json @@ -14,12 +14,13 @@ "dev:sockets": "cd apps/realtime && bun run dev", "dev:full": "bunx concurrently -n \"App,Realtime\" -c \"cyan,magenta\" \"cd apps/sim && bun run dev\" \"cd apps/realtime && bun run dev\"", "dev:full:capped": "bunx concurrently -n \"App,Realtime\" -c \"cyan,magenta\" \"cd apps/sim && bun run dev:capped\" \"cd apps/realtime && bun run dev\"", - "test": "bun run test:setup && bun run test:npm-package-versions && bun run test:icon-path-precision && bun run test:tool-registry-boundary && bun run test:tool-request-boundary && bun run test:migrations-safety && bun run test:generators && turbo run test", + "test": "bun run test:setup && bun run test:npm-package-versions && bun run test:icon-path-precision && bun run test:tool-registry-boundary && bun run test:tool-request-boundary && bun run test:actorless-executor-operations && bun run test:migrations-safety && bun run test:generators && turbo run test", "test:setup": "bun run --cwd packages/sim-setup test", "test:npm-package-versions": "bunx vitest run scripts/bump-npm-package-versions.test.ts", "test:icon-path-precision": "bunx vitest run scripts/check-icon-path-precision.test.ts", "test:tool-registry-boundary": "bunx vitest run scripts/check-tool-registry-boundary.test.ts", "test:tool-request-boundary": "bunx vitest run scripts/check-tool-request-boundary.test.ts", + "test:actorless-executor-operations": "bunx vitest run scripts/check-actorless-executor-operations.test.ts", "test:migrations-safety": "bunx vitest run scripts/check-migrations-safety.test.ts", "test:generators": "bunx vitest run scripts/generate-v2-cli-api.test.ts scripts/generate-cli-docs.test.ts scripts/generate-docs.test.ts", "format": "turbo run format", @@ -44,6 +45,7 @@ "check:api-validation:strict": "bun run scripts/check-api-validation-contracts.ts --check --enforce-boundary-baseline", "check:realtime-prune": "bun run scripts/check-realtime-prune-graph.ts", "check:tool-request-boundary": "bun run scripts/check-tool-request-boundary.ts", + "check:actorless-executor-operations": "bun run scripts/check-actorless-executor-operations.ts", "check:tool-registry-boundary": "bun run scripts/check-tool-registry-boundary.ts --check", "check:trigger-block-cycle": "bun run scripts/check-trigger-block-cycle.ts", "check:import-specifiers": "bun run scripts/check-import-specifiers.ts", diff --git a/packages/auth/src/principal.ts b/packages/auth/src/principal.ts index 0b435434183..3336e5bb624 100644 --- a/packages/auth/src/principal.ts +++ b/packages/auth/src/principal.ts @@ -115,10 +115,25 @@ export class PrincipalSubjectUserRequiredError extends Error { } } +/** + * The Sim user a principal represents, or `undefined` when it represents none. + * + * Actorless callers are ordinary, not exceptional: a scheduled or webhook run, a + * workspace API key, and a Credential Group enrollment all act with real authority + * and no human behind them. Use this wherever the user is attribution — a name to + * record a read or write under — and {@link requirePrincipalSubjectUserId} only + * where the operation's meaning genuinely collapses without one, so that choice is + * visible at the call site instead of hidden in a ternary. + */ +export function resolvePrincipalSubjectUserId(principal: Principal): string | undefined { + const subject = resolvePrincipalSubject(principal) + return subject?.kind === 'sim_user' ? subject.userId : undefined +} + /** Resolves the real human subject represented by a principal or fails fast. */ export function requirePrincipalSubjectUserId(principal: Principal): string { - const subject = resolvePrincipalSubject(principal) - if (subject?.kind === 'sim_user') return subject.userId + const userId = resolvePrincipalSubjectUserId(principal) + if (userId !== undefined) return userId throw new PrincipalSubjectUserRequiredError(principal.kind) } diff --git a/scripts/check-actorless-executor-operations.test.ts b/scripts/check-actorless-executor-operations.test.ts new file mode 100644 index 00000000000..778efbf612b --- /dev/null +++ b/scripts/check-actorless-executor-operations.test.ts @@ -0,0 +1,138 @@ +import { describe, expect, it } from 'vitest' +import { + auditSubjectRequirements, + parseOperationPolicies, + referencedOperations, +} from './check-actorless-executor-operations' + +describe('operation policy parsing', () => { + it('reads an inline delegatedServices list', () => { + const policies = parseOperationPolicies(` + export const logOperations = { + list: defineWorkspaceOperation({ + id: 'logs.list', + delegatedServices: ['copilot', 'executor'], + }), + readStats: defineWorkspaceOperation({ + id: 'logs.read_stats', + delegatedServices: ['copilot'], + }), + } as const + `) + + expect(policies.get('logOperations.list')).toBe(true) + expect(policies.get('logOperations.readStats')).toBe(false) + }) + + it('resolves a policy spread into the definition', () => { + const policies = parseOperationPolicies(` + const READER_POLICY = { + principalKinds: ['session', 'delegated'], + delegatedServices: ['copilot', 'executor'], + } as const + export const logOperations = { + readDetail: defineWorkspaceOperation({ id: 'logs.read_detail', ...READER_POLICY }), + } as const + `) + + expect(policies.get('logOperations.readDetail')).toBe(true) + }) + + it('resolves an operation declared through a same-file factory', () => { + const policies = parseOperationPolicies(` + const TOOL_POLICY = { delegatedServices: ['copilot', 'executor'] } as const + const UI_POLICY = { delegatedServices: ['copilot'] } as const + function toolReadOperation(id: Id) { + return defineWorkspaceOperation({ id, minimumRole: 'read', ...TOOL_POLICY }) + } + function readOperation(id: Id) { + return defineWorkspaceOperation({ id, minimumRole: 'read', ...UI_POLICY }) + } + export const tableOperations = { + queryRows: toolReadOperation('tables.rows.query'), + listTables: readOperation('tables.list'), + } as const + `) + + expect(policies.get('tableOperations.queryRows')).toBe(true) + expect(policies.get('tableOperations.listTables')).toBe(false) + }) + + it('treats an operation with no delegated services as executor-free', () => { + const policies = parseOperationPolicies(` + export const workspaceOperations = { + read: defineWorkspaceOperation({ id: 'workspaces.read', minimumRole: 'read' }), + } as const + `) + + expect(policies.get('workspaceOperations.read')).toBe(false) + }) +}) + +describe('operation references', () => { + it('collects only declared operations', () => { + const referenced = referencedOperations( + ` + const useCase = defineAuthorizedWorkspaceUseCase({ + operation: logOperations.readDetail, + execute: () => input.signal?.throwIfAborted(), + }) + `, + new Set(['logOperations.readDetail', 'logOperations.list']) + ) + + expect(referenced).toEqual(['logOperations.readDetail']) + }) +}) + +describe('subject requirement audit', () => { + const call = ' const userId = requirePrincipalSubjectUserId(principal)' + + it('flags an unannotated call', () => { + const findings = auditSubjectRequirements(`function run() {\n${call}\n}`, ['ops.thing']) + + expect(findings).toEqual([ + { file: '', line: 2, reason: 'unannotated', operations: ['ops.thing'] }, + ]) + }) + + it('accepts an annotated call', () => { + const source = `function run() {\n // actorless-unsupported: skills belong to a person\n${call}\n}` + + expect(auditSubjectRequirements(source, [])).toEqual([]) + }) + + it('tolerates context comments above the annotation', () => { + const source = [ + 'function run() {', + ' // The library is per-user.', + ' // actorless-unsupported: skills belong to a person', + call, + '}', + ].join('\n') + + expect(auditSubjectRequirements(source, [])).toEqual([]) + }) + + it('rejects an annotation with no reason', () => { + const source = `function run() {\n // actorless-unsupported:\n${call}\n}` + + expect(auditSubjectRequirements(source, [])).toEqual([ + { file: '', line: 3, reason: 'empty-reason', operations: [] }, + ]) + }) + + it('ignores an annotation separated from the call by code', () => { + const source = [ + 'function run() {', + ' // actorless-unsupported: not attached to the call below', + ' const workspaceId = context.workspaceId', + call, + '}', + ].join('\n') + + expect(auditSubjectRequirements(source, [])).toEqual([ + { file: '', line: 4, reason: 'unannotated', operations: [] }, + ]) + }) +}) diff --git a/scripts/check-actorless-executor-operations.ts b/scripts/check-actorless-executor-operations.ts new file mode 100644 index 00000000000..91a49b46f8d --- /dev/null +++ b/scripts/check-actorless-executor-operations.ts @@ -0,0 +1,275 @@ +#!/usr/bin/env bun +/** + * Fails when an operation an actorless run can reach demands a human subject. + * + * A workflow executes under a `Principal`, and several triggers have no person + * behind them: a schedule, the public API, a webhook carrying no external subject. + * Those runs still hold real authority — `workspace-authorization.ts` admits an + * actorless executor delegation whose current workflow is a deployment — so they + * are authorized callers with no `subjectUserId`. Any use case they can reach that + * calls `requirePrincipalSubjectUserId` therefore throws, and because + * `PrincipalSubjectUserRequiredError` is not an `OrchestrationError` it surfaces as + * an opaque 500 rather than anything a workflow author can act on. + * + * That is not hypothetical: the Logs detail tools broke for every scheduled run + * this way, and the failure reached production because nothing connected "this + * operation admits `delegatedServices: ['executor']`" to "this use case requires a + * person". This audit connects them. + * + * A subject is genuinely required often enough that the rule is an annotation, not + * a ban. Write `// actorless-unsupported: ` above the call to declare that + * the operation has no meaning without a person — the annotation turns a silent + * 500 into a documented gap that a reviewer can weigh. + */ +import { readdirSync, readFileSync, statSync } from 'node:fs' +import { dirname, join, relative, resolve } from 'node:path' +import { fileURLToPath } from 'node:url' + +const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url)) +const ROOT = resolve(SCRIPT_DIR, '..') +const SCAN_ROOTS = ['apps/sim/lib', 'apps/sim/app'] +const REQUIRE_CALL = 'requirePrincipalSubjectUserId(' +const ANNOTATION = 'actorless-unsupported:' +const MAX_ANNOTATION_LOOKBACK = 3 + +/** + * `apps/sim/lib/internal/**` is the in-process tool surface: every handler under it + * mints an executor delegation, so its modules are executor-reachable whether or + * not they bind a named operation. + */ +const EXECUTOR_SURFACE_PREFIX = 'apps/sim/lib/internal/' + +export interface ActorlessFinding { + file: string + line: number + reason: 'unannotated' | 'empty-reason' + operations: string[] +} + +/** How the file was shown to be reachable by an actorless run. */ +type Reachability = 'internal-surface' | 'declared-operation' | 'unproven' + +/** Text of the balanced `(...)` or `{...}` group that starts at `openIndex`. */ +function balancedGroup(source: string, openIndex: number): string { + const open = source[openIndex] + const close = open === '(' ? ')' : '}' + let depth = 0 + for (let index = openIndex; index < source.length; index++) { + const char = source[index] + if (char === open) depth++ + else if (char === close) { + depth-- + if (depth === 0) return source.slice(openIndex, index + 1) + } + } + return source.slice(openIndex) +} + +function admitsExecutor(text: string): boolean { + const match = /delegatedServices\s*:\s*\[([^\]]*)\]/.exec(text) + return match ? /['"]executor['"]/.test(match[1]) : false +} + +/** + * Maps every `.` declared in an operations module to whether its + * policy admits an executor delegation, resolving same-file policy constants that + * are spread into the definition (e.g. `...LOG_READER_PRINCIPAL_POLICY`). + */ +export function parseOperationPolicies(source: string): Map { + const policies = new Map() + + const spreadable = new Map() + const constPattern = /(?:^|\n)\s*(?:export\s+)?const\s+([A-Za-z0-9_$]+)\s*=\s*\{/g + for (let match = constPattern.exec(source); match; match = constPattern.exec(source)) { + const braceIndex = source.indexOf('{', match.index + match[0].length - 1) + spreadable.set(match[1], admitsExecutor(balancedGroup(source, braceIndex))) + } + + /** Whether a `defineWorkspaceOperation({...})` call admits an executor delegation. */ + const definitionAdmitsExecutor = (definition: string): boolean => { + if (admitsExecutor(definition)) return true + return [...definition.matchAll(/\.\.\.([A-Za-z0-9_$]+)/g)].some( + (spread) => spreadable.get(spread[1]) === true + ) + } + + // Several domains declare their operations through same-file factories + // (`toolReadOperation('tables.rows.query')`) rather than inline, so the policy has + // to be resolved through the factory or those operations read as executor-free. + const factories = new Map() + const factoryPattern = /(?:^|\n)\s*(?:export\s+)?function\s+([A-Za-z0-9_$]+)\s*[<(]/g + for (let match = factoryPattern.exec(source); match; match = factoryPattern.exec(source)) { + const bodyIndex = source.indexOf('{', match.index + match[0].length - 1) + if (bodyIndex === -1) continue + const body = balancedGroup(source, bodyIndex) + const defineIndex = body.indexOf('defineWorkspaceOperation') + if (defineIndex === -1) continue + const parenIndex = body.indexOf('(', defineIndex) + factories.set(match[1], definitionAdmitsExecutor(balancedGroup(body, parenIndex))) + } + + const namespacePattern = /(?:^|\n)\s*export\s+const\s+([A-Za-z0-9_$]+)\s*=\s*\{/g + for (let match = namespacePattern.exec(source); match; match = namespacePattern.exec(source)) { + const namespace = match[1] + const braceIndex = source.indexOf('{', match.index + match[0].length - 1) + const body = balancedGroup(source, braceIndex) + + const entryPattern = /([A-Za-z0-9_$]+)\s*:\s*([A-Za-z0-9_$]+)\s*\(/g + for (let entry = entryPattern.exec(body); entry; entry = entryPattern.exec(body)) { + const [, key, callee] = entry + if (callee === 'defineWorkspaceOperation') { + const parenIndex = body.indexOf('(', entry.index + entry[0].length - 1) + policies.set( + `${namespace}.${key}`, + definitionAdmitsExecutor(balancedGroup(body, parenIndex)) + ) + } else if (factories.has(callee)) { + policies.set(`${namespace}.${key}`, factories.get(callee) === true) + } + } + } + + return policies +} + +/** The declared operations a module references, whether to define or to bind them. */ +export function referencedOperations(source: string, known: Set): string[] { + const referenced = new Set() + for (const match of source.matchAll(/([A-Za-z0-9_$]+)\.([A-Za-z0-9_$]+)/g)) { + const id = `${match[1]}.${match[2]}` + if (known.has(id)) referenced.add(id) + } + return [...referenced].sort() +} + +/** + * Flags `requirePrincipalSubjectUserId` calls that are not declared actorless-unsupported. + * Mirrors the placement rule the boundary annotations use: the annotation must sit in one + * of the preceding comment lines, so extra context above it is fine. + */ +export function auditSubjectRequirements(source: string, operations: string[]): ActorlessFinding[] { + const findings: ActorlessFinding[] = [] + const lines = source.split('\n') + + for (const [index, line] of lines.entries()) { + if (!line.includes(REQUIRE_CALL)) continue + + let annotation: string | undefined + for (let back = index - 1; back >= 0 && back >= index - MAX_ANNOTATION_LOOKBACK; back--) { + const candidate = lines[back].trim() + if (candidate === '') continue + if (!candidate.startsWith('//') && !candidate.startsWith('*')) break + const found = candidate.indexOf(ANNOTATION) + if (found !== -1) { + annotation = candidate.slice(found + ANNOTATION.length).trim() + break + } + } + + if (annotation === undefined) { + findings.push({ file: '', line: index + 1, reason: 'unannotated', operations }) + } else if (annotation === '') { + findings.push({ file: '', line: index + 1, reason: 'empty-reason', operations }) + } + } + + return findings +} + +function walk(directory: string, into: string[]): string[] { + for (const entry of readdirSync(directory)) { + if (entry === 'node_modules' || entry === '.next') continue + const full = join(directory, entry) + if (statSync(full).isDirectory()) walk(full, into) + else if (full.endsWith('.ts') && !full.endsWith('.test.ts')) into.push(full) + } + return into +} + +function main(): void { + const sourceFiles = SCAN_ROOTS.flatMap((root) => walk(join(ROOT, root), [])) + + const policies = new Map() + for (const file of sourceFiles) { + if (!file.endsWith('/application/operations.ts')) continue + for (const [id, executor] of parseOperationPolicies(readFileSync(file, 'utf8'))) { + policies.set(id, executor) + } + } + const known = new Set(policies.keys()) + const executorAdmitting = new Set([...policies].filter(([, yes]) => yes).map(([id]) => id)) + + // Domains with at least one executor-admitting operation. A shared use-case + // factory in such a domain names no operation of its own — it takes one as an + // argument — so it cannot be proven executor-free and fails closed here. + const executorDomains = new Set() + for (const file of sourceFiles) { + if (!file.endsWith('/application/operations.ts')) continue + const parsed = parseOperationPolicies(readFileSync(file, 'utf8')) + if ([...parsed.values()].some(Boolean)) { + executorDomains.add(relative(ROOT, dirname(dirname(file)))) + } + } + + const findings: ActorlessFinding[] = [] + const reachabilityByFinding = new Map() + let auditedFiles = 0 + + for (const file of sourceFiles) { + const source = readFileSync(file, 'utf8') + if (!source.includes(REQUIRE_CALL)) continue + + const relativePath = relative(ROOT, file) + const operations = referencedOperations(source, known) + const reachability: Reachability | undefined = relativePath.startsWith(EXECUTOR_SURFACE_PREFIX) + ? 'internal-surface' + : operations.some((id) => policies.get(id) === true) + ? 'declared-operation' + : operations.length === 0 && + [...executorDomains].some((domain) => relativePath.startsWith(`${domain}/`)) + ? 'unproven' + : undefined + if (!reachability) continue + + auditedFiles++ + for (const audited of auditSubjectRequirements(source, operations)) { + const finding = { ...audited, file: relativePath } + findings.push(finding) + reachabilityByFinding.set(finding, reachability) + } + } + + if (findings.length > 0) { + console.error( + 'Operations an actorless run can reach must not silently require a human subject:' + ) + for (const finding of findings) { + const reachability = reachabilityByFinding.get(finding) + const via = + reachability === 'internal-surface' + ? 'in-process tool surface' + : reachability === 'unproven' + ? 'shared use case in a domain with executor-admitting operations' + : `reachable via ${finding.operations.filter((id) => executorAdmitting.has(id)).join(', ')}` + const problem = + finding.reason === 'empty-reason' + ? `${ANNOTATION} needs a reason` + : `unannotated requirePrincipalSubjectUserId (${via})` + console.error(` ${finding.file}:${finding.line} ${problem}`) + } + console.error( + `\nA scheduled, public-API, or subject-less webhook run reaches these with no user, and` + + `\n\`requirePrincipalSubjectUserId\` throws a 500 there rather than anything actionable.` + + `\nEither resolve the user optionally (\`resolvePrincipalSubjectUserId\`) when it is only` + + `\nattribution, or declare the gap with \`// ${ANNOTATION} \` above the call.` + ) + process.exit(1) + } + + console.log( + `✓ no undeclared human-subject requirements on actorless-reachable operations ` + + `(${executorAdmitting.size} executor-admitting operations, ${auditedFiles} files audited)` + ) +} + +if (import.meta.main) main() From 3125c1f8f40ef1aa16e45abce01bb87dc146bac9 Mon Sep 17 00:00:00 2001 From: Theodore Li Date: Fri, 28 Aug 2026 11:47:50 -0700 Subject: [PATCH 2/2] fix(mcp): keep unattended runs connecting as their execution actor Restores the identity MCP used before the Logs and MCP tools moved in-process: the executor minted an internal token from ExecutionContext.userId and the MCP route ran as that user, so a schedule, webhook, or public-API run has always reached MCP as the execution actor. Names it for what it is rather than implying it is the workflow's author, and documents that a workspace-level MCP identity is the real fix. Also types the missing-execution-context error so it is answered as unauthenticated rather than falling through to a generic 500. --- .../lib/internal/logs/execute-tool.test.ts | 18 +++ apps/sim/lib/internal/mcp/discover-tools.ts | 4 +- apps/sim/lib/internal/mcp/execute-tool.ts | 6 +- apps/sim/lib/internal/principals/executor.ts | 3 +- .../tool-operations/identity-faults.ts | 20 ++++ apps/sim/lib/mcp/application/authorization.ts | 41 ++++--- .../lib/mcp/application/execute-tool.test.ts | 106 ++++++++++++------ apps/sim/lib/mcp/application/execute-tool.ts | 17 +-- apps/sim/lib/mcp/application/use-cases.ts | 20 ++-- 9 files changed, 155 insertions(+), 80 deletions(-) diff --git a/apps/sim/lib/internal/logs/execute-tool.test.ts b/apps/sim/lib/internal/logs/execute-tool.test.ts index e7cdae768ca..842d133d9b4 100644 --- a/apps/sim/lib/internal/logs/execute-tool.test.ts +++ b/apps/sim/lib/internal/logs/execute-tool.test.ts @@ -28,6 +28,7 @@ vi.mock('@/lib/internal/logs/operations', () => ({ })) import { executeLogsTool } from '@/lib/internal/logs/execute-tool' +import { ExecutorDelegationOriginRequiredError } from '@/lib/internal/tool-operations/identity-faults' const PRINCIPAL: WorkflowExecutionDelegatedPrincipal = { kind: 'delegated', @@ -173,6 +174,23 @@ describe('executeLogsTool', () => { expect(await invalidResponse.json()).toEqual({ error: 'Failed to fetch log' }) }) + it('answers a missing execution context as unauthenticated, not as a broken tool', async () => { + // A caller with no executor delegation origin never established an identity. + // The error was untyped, so it fell past the classifier into a generic 500. + mocks.createPrincipal.mockRejectedValueOnce(new ExecutorDelegationOriginRequiredError()) + + const response = await executeLogsTool({ + toolId: 'logs_query', + input: {}, + headers: new Headers(), + context: CONTEXT, + requestId: 'request-1', + }) + + expect(response.status).toBe(401) + expect(await response.json()).toEqual({ error: 'Authentication required' }) + }) + it('names the missing identity instead of answering an opaque 500', async () => { // The regression this guards: an operation that still demands a person answered // every scheduled run with `Failed to fetch log`, which says nothing about why. diff --git a/apps/sim/lib/internal/mcp/discover-tools.ts b/apps/sim/lib/internal/mcp/discover-tools.ts index aa2e35452e4..fcf1f709a46 100644 --- a/apps/sim/lib/internal/mcp/discover-tools.ts +++ b/apps/sim/lib/internal/mcp/discover-tools.ts @@ -25,8 +25,8 @@ export async function discoverMcpServerToolsAsExecutor({ signal?.throwIfAborted() const result = await discoverMcpServerToolsUseCase.execute({ principal, - // See `credentialUserId`: an actorless run's credentials are the workflow's own user's. - input: { workspaceId, serverId, credentialUserId: context.userId }, + // See `executionActorUserId`: preserves the pre-in-process behavior for unattended runs. + input: { workspaceId, serverId, executionActorUserId: context.userId }, }) signal?.throwIfAborted() return result.tools diff --git a/apps/sim/lib/internal/mcp/execute-tool.ts b/apps/sim/lib/internal/mcp/execute-tool.ts index dd36e859e65..e271ed3657a 100644 --- a/apps/sim/lib/internal/mcp/execute-tool.ts +++ b/apps/sim/lib/internal/mcp/execute-tool.ts @@ -149,9 +149,9 @@ export const executeMcpTool: InternalToolOperationHandler = async (request) => { input: { workspaceId: request.context.workspaceId, serverId, - // An actorless run (a schedule, a webhook with no external subject) has no - // subject on its principal; the workflow's own user owns the credentials. - credentialUserId: request.context.userId, + // The run's execution actor, exactly what the pre-in-process path minted its + // internal token from. Keeps unattended MCP workflows working as before. + executionActorUserId: request.context.userId, toolName, arguments: args, callChain: request.context.callChain, diff --git a/apps/sim/lib/internal/principals/executor.ts b/apps/sim/lib/internal/principals/executor.ts index 9295ef88fb5..51c5069d736 100644 --- a/apps/sim/lib/internal/principals/executor.ts +++ b/apps/sim/lib/internal/principals/executor.ts @@ -1,6 +1,7 @@ import { type DelegatedPrincipal, resolvePrincipalSubject } from '@sim/auth/principal' import { generateId } from '@sim/utils/id' import { bindInternalExecutorDelegation } from '@/lib/auth/internal-delegation' +import { ExecutorDelegationOriginRequiredError } from '@/lib/internal/tool-operations/identity-faults' import type { InternalToolOperationContext } from '@/lib/internal/tool-operations/types' import type { ExecutorDelegationOrigin } from '@/executor/types' @@ -70,6 +71,6 @@ export async function createExecutorPrincipalFromExecutionContext({ expiresAt, }: CreateExecutorPrincipalFromExecutionContextInput) { const origin = context.executorDelegationOrigin - if (!origin) throw new Error('Executor delegation origin is required') + if (!origin) throw new ExecutorDelegationOriginRequiredError() return bindExecutorPrincipal(origin, audience, resourceScope, expiresAt) } diff --git a/apps/sim/lib/internal/tool-operations/identity-faults.ts b/apps/sim/lib/internal/tool-operations/identity-faults.ts index a0132aaf386..80a03624a7b 100644 --- a/apps/sim/lib/internal/tool-operations/identity-faults.ts +++ b/apps/sim/lib/internal/tool-operations/identity-faults.ts @@ -18,6 +18,25 @@ import { InvalidInternalDelegationBindingError } from '@/lib/auth/internal-deleg */ export type InternalToolIdentityFault = 'unauthenticated' | 'subject_user_required' +/** + * An in-process tool ran without the execution context that proves who called it. + * + * Typed rather than a bare `Error` so this boundary can answer it as the + * unauthenticated failure it is; left untyped it fell past every classifier into a + * generic 500, which reads as "the tool broke" rather than "this caller never + * established an identity". + * + * It lives here rather than beside its thrower because the classifier must not + * import the executor-principal module: nearly every handler test mocks that + * module, and an `instanceof` against a mock that omits the export throws. + */ +export class ExecutorDelegationOriginRequiredError extends Error { + constructor() { + super('Executor delegation origin is required') + this.name = 'ExecutorDelegationOriginRequiredError' + } +} + const FAULTS: Record = { unauthenticated: { status: 401, message: 'Authentication required' }, subject_user_required: { @@ -34,6 +53,7 @@ export function classifyInternalToolIdentityFault( if (error instanceof PrincipalSubjectUserRequiredError) return 'subject_user_required' if ( error instanceof InvalidInternalDelegationBindingError || + error instanceof ExecutorDelegationOriginRequiredError || (error instanceof Error && error.message === 'Authentication required') ) { return 'unauthenticated' diff --git a/apps/sim/lib/mcp/application/authorization.ts b/apps/sim/lib/mcp/application/authorization.ts index 69060a4f1c6..7bd10b74492 100644 --- a/apps/sim/lib/mcp/application/authorization.ts +++ b/apps/sim/lib/mcp/application/authorization.ts @@ -16,26 +16,39 @@ export const mcpServerDelegationPolicy = { /** * The user whose MCP server credentials an operation presents. * - * Every MCP path — execution and both discoveries — connects to a third-party - * server with one person's OAuth credentials and is gated by that person's - * permission group, so unlike an attribution-only read it genuinely cannot run - * with nobody: there is no credential set and the permission gate would be - * skipped rather than satisfied. + * An MCP call connects to a third-party server with one person's stored + * credentials and is gated by that person's permission group, so unlike an + * attribution-only read it cannot proceed with nobody named. * - * The principal's own subject always wins. `fallbackUserId` is for an actorless - * run — a schedule, or a webhook with no external subject — whose surface passes - * the workflow's own user; because it is consulted only when the principal names - * nobody, a caller cannot use it to nominate someone else's credentials. + * `executionActorUserId` preserves the behavior that existed before the Logs and + * MCP tools moved in-process. That path minted an internal token from + * `ExecutionContext.userId` and the MCP route ran as that user, so an unattended + * run has always reached MCP as the execution actor. For a schedule, webhook, or + * anonymous public-API run that actor is the workspace system actor resolved + * during preprocessing — the billing payer — not the workflow's author. Keeping + * it is what stops every unattended MCP workflow from breaking; changing it is a + * product decision, not a refactor, and a workspace-level MCP identity is the + * real fix. * - * Fails as `forbidden` rather than throwing `PrincipalSubjectUserRequiredError`, - * so a run with no user at all gets a 403 that says why instead of an opaque 500. + * The fallback deliberately covers a webhook carrying an `external_user` subject + * too. That subject is a real identity but never a Sim user, so it has no Sim + * credentials of its own, and those runs have always connected as the actor. + * Refusing them here would break working workflows in the name of a boundary the + * old path never drew. + * + * It is NOT an authorization input: workspace reach is decided by the principal + * before this is read, and a principal that names its own subject always wins, so + * a caller cannot use this to nominate someone else's credentials. */ -export function requireMcpCredentialUserId(principal: Principal, fallbackUserId?: string): string { - const userId = resolvePrincipalSubjectUserId(principal) ?? fallbackUserId +export function requireMcpCredentialUserId( + principal: Principal, + executionActorUserId?: string +): string { + const userId = resolvePrincipalSubjectUserId(principal) ?? executionActorUserId if (!userId) { throw new OrchestrationError( 'forbidden', - 'MCP servers are reached with a user\u2019s own credentials, and this run has no user' + 'MCP servers are reached with a user\u2019s own credentials, and this run has none' ) } return userId diff --git a/apps/sim/lib/mcp/application/execute-tool.test.ts b/apps/sim/lib/mcp/application/execute-tool.test.ts index 99e4d1ca99a..b0711c22960 100644 --- a/apps/sim/lib/mcp/application/execute-tool.test.ts +++ b/apps/sim/lib/mcp/application/execute-tool.test.ts @@ -168,54 +168,28 @@ describe('executeMcpToolUseCase', () => { expect(mocks.executeTool).not.toHaveBeenCalled() }) - it('does not invent a user for actorless system execution', async () => { - // An MCP call presents one person's credentials, so with nobody named at all it - // must refuse rather than reach for a stand-in. It refuses as `forbidden` so the - // caller is told why, instead of the unclassified 500 this used to produce. - await expect( - executeMcpToolUseCase.execute({ - principal: ACTORLESS_PRINCIPAL, - input: { - workspaceId: WORKSPACE.workspaceId, - serverId: SERVER.id, - toolName: 'lookup', - }, - }) - ).rejects.toMatchObject({ - code: 'forbidden', - message: - 'MCP servers are reached with a user\u2019s own credentials, and this run has no user', - }) - - expect(mocks.assertPermissionsAllowed).not.toHaveBeenCalled() - expect(mocks.discoverServerTools).not.toHaveBeenCalled() - expect(mocks.executeTool).not.toHaveBeenCalled() - }) - - it('runs an actorless workflow against the credentials its surface names', async () => { - // A scheduled run has no subject on its principal, but the workflow it runs has - // an owner, and the in-process surface passes that user. Without this a schedule - // could not call an MCP block at all. + it('keeps an unattended run connecting as its execution actor', async () => { + // Pre-in-process behavior: the executor minted an internal token from + // ExecutionContext.userId and MCP ran as that user. Preserved deliberately — + // see requireMcpCredentialUserId for why that actor is the payer, not the author. await executeMcpToolUseCase.execute({ principal: ACTORLESS_PRINCIPAL, input: { workspaceId: WORKSPACE.workspaceId, serverId: SERVER.id, toolName: 'lookup', - credentialUserId: 'workflow-owner', arguments: { count: '2', enabled: 'true', tags: 'a,b' }, + executionActorUserId: 'execution-actor', }, }) expect(mocks.assertPermissionsAllowed).toHaveBeenCalledWith( - expect.objectContaining({ userId: 'workflow-owner' }) + expect.objectContaining({ userId: 'execution-actor' }) ) - // Discovery resolves the same person's OAuth credentials, so it must be told - // the same user the permission gate was. - expect(mocks.discoverServerTools.mock.calls[0][0]).toBe('workflow-owner') + expect(mocks.discoverServerTools.mock.calls[0][0]).toBe('execution-actor') }) - it('ignores a named user when the principal already has one', async () => { + it('lets an authenticated subject win over the execution actor', async () => { // The property that keeps the fallback from becoming an impersonation handle: // it is consulted only when the principal names nobody. await executeMcpToolUseCase.execute({ @@ -224,8 +198,8 @@ describe('executeMcpToolUseCase', () => { workspaceId: WORKSPACE.workspaceId, serverId: SERVER.id, toolName: 'lookup', - credentialUserId: 'someone-else', arguments: { count: '2', enabled: 'true', tags: 'a,b' }, + executionActorUserId: 'someone-else', }, }) @@ -234,6 +208,68 @@ describe('executeMcpToolUseCase', () => { ) }) + it('keeps an external-subject webhook connecting as the execution actor', async () => { + // A webhook's external_user subject is a real identity but never a Sim user, so + // it has no Sim credentials of its own and these runs have always connected as + // the actor. Refusing here would break workflows that worked before the tools + // moved in-process, so the fallback deliberately covers this case. + const externalSubjectPrincipal = { + ...ACTORLESS_PRINCIPAL, + delegationContext: { + ...ACTORLESS_PRINCIPAL.delegationContext, + principal: { + kind: 'system' as const, + serviceId: 'webhook' as const, + workspaceId: WORKSPACE.workspaceId, + workflowId: 'workflow-1', + webhookId: 'webhook-1', + provider: 'slack', + subject: { + kind: 'external_user' as const, + provider: 'slack', + tenantId: 'T1', + subjectId: 'U1', + }, + }, + }, + } + + await executeMcpToolUseCase.execute({ + principal: externalSubjectPrincipal, + input: { + workspaceId: WORKSPACE.workspaceId, + serverId: SERVER.id, + toolName: 'lookup', + arguments: { count: '2', enabled: 'true', tags: 'a,b' }, + executionActorUserId: 'execution-actor', + }, + }) + + expect(mocks.assertPermissionsAllowed).toHaveBeenCalledWith( + expect.objectContaining({ userId: 'execution-actor' }) + ) + }) + + it('refuses when the run names no user and carries no actor either', async () => { + await expect( + executeMcpToolUseCase.execute({ + principal: ACTORLESS_PRINCIPAL, + input: { + workspaceId: WORKSPACE.workspaceId, + serverId: SERVER.id, + toolName: 'lookup', + }, + }) + ).rejects.toMatchObject({ + code: 'forbidden', + message: 'MCP servers are reached with a user\u2019s own credentials, and this run has none', + }) + + expect(mocks.assertPermissionsAllowed).not.toHaveBeenCalled() + expect(mocks.discoverServerTools).not.toHaveBeenCalled() + expect(mocks.executeTool).not.toHaveBeenCalled() + }) + it('does not execute when schema validation fails', async () => { await expect( executeMcpToolUseCase.execute({ diff --git a/apps/sim/lib/mcp/application/execute-tool.ts b/apps/sim/lib/mcp/application/execute-tool.ts index b3ab5aab610..8f71be4582d 100644 --- a/apps/sim/lib/mcp/application/execute-tool.ts +++ b/apps/sim/lib/mcp/application/execute-tool.ts @@ -27,19 +27,10 @@ export interface ExecuteMcpToolInput { workspaceId: string serverId: string /** - * The user whose MCP credentials and permission group apply. - * - * NOT an authorization input — workspace reach is decided by the principal, and - * this is only consulted after that. It exists because an MCP call connects to a - * third-party server with a specific person's OAuth credentials, so unlike a log - * read it cannot proceed with nobody: there is no credential set to present. - * - * An actorless run — a schedule, or a webhook with no external subject — has no - * subject on its principal, so its surface supplies the workflow's own user here. - * Ignored when the principal already names a subject, which is what stops a - * caller from nominating someone else's credentials. + * The run's execution actor. See {@link requireMcpCredentialUserId}: preserves + * the pre-in-process behavior for runs whose principal names no Sim user. */ - credentialUserId?: string + executionActorUserId?: string toolName: string arguments?: Record callChain?: string[] @@ -148,7 +139,7 @@ export const executeMcpToolUseCase = defineAuthorizedWorkspaceUseCase({ authorizationOptions: { delegation: mcpServerDelegationPolicy }, async execute({ principal, input, context }): Promise { input.signal?.throwIfAborted() - const userId = requireMcpCredentialUserId(principal, input.credentialUserId) + const userId = requireMcpCredentialUserId(principal, input.executionActorUserId) await assertPermissionsAllowed({ userId, workspaceId: context.workspaceId, diff --git a/apps/sim/lib/mcp/application/use-cases.ts b/apps/sim/lib/mcp/application/use-cases.ts index 5fed534bf4f..18085334775 100644 --- a/apps/sim/lib/mcp/application/use-cases.ts +++ b/apps/sim/lib/mcp/application/use-cases.ts @@ -92,12 +92,10 @@ export const listMcpServersUseCase = defineAuthorizedWorkspaceUseCase({ export interface DiscoverMcpToolsInput { workspaceId: string /** - * The user whose MCP credentials apply. See `ExecuteMcpToolInput.credentialUserId`: - * discovery resolves the same third-party credentials, so an actorless run's - * surface supplies the workflow's own user, and the principal's subject wins - * whenever it has one. + * The run's execution actor. See {@link requireMcpCredentialUserId}: preserves + * the pre-in-process behavior for runs whose principal names no Sim user. */ - credentialUserId?: string + executionActorUserId?: string refresh?: boolean } @@ -108,7 +106,7 @@ export const discoverMcpToolsUseCase = defineAuthorizedWorkspaceUseCase({ authorizationOptions, async execute({ principal, input, context }) { const tools = await mcpService.discoverTools( - requireMcpCredentialUserId(principal, input.credentialUserId), + requireMcpCredentialUserId(principal, input.executionActorUserId), context.workspaceId, /** * A public `refresh` skips the positive cache but keeps the failure @@ -125,12 +123,10 @@ export interface DiscoverMcpServerToolsInput { workspaceId: string serverId: string /** - * The user whose MCP credentials apply. See `ExecuteMcpToolInput.credentialUserId`: - * discovery resolves the same third-party credentials, so an actorless run's - * surface supplies the workflow's own user, and the principal's subject wins - * whenever it has one. + * The run's execution actor. See {@link requireMcpCredentialUserId}: preserves + * the pre-in-process behavior for runs whose principal names no Sim user. */ - credentialUserId?: string + executionActorUserId?: string refresh?: boolean } @@ -168,7 +164,7 @@ export const discoverMcpServerToolsUseCase = defineAuthorizedWorkspaceUseCase({ } const tools = await mcpService.discoverServerTools( - requireMcpCredentialUserId(principal, input.credentialUserId), + requireMcpCredentialUserId(principal, input.executionActorUserId), context.server.id, context.workspaceId, /**