Skip to content

Commit c4ccdde

Browse files
waleedlatif1waleedlatif1
authored
fix(files): let Mothership tool runs write workspace files (#9685)
* fix(files): let Mothership tool runs write workspace files `tools execute` authorizes the chat's delegated principal for `sim:tool-execution` and hands it to the tool layer unchanged, but every workspace-file use case admits only `sim:workspace-files`. Every File tool run directly from a Mothership chat therefore failed with a 403 "Delegated workspace access is no longer valid", which the agent relayed to the user as lost workspace access. The File tool handler now rebinds a delegated caller to the workspace-files audience (keeping subject, workspace, expiry ceiling and resource scope), as function-execute and payload materialization already do; the file use cases still check the subject's current workspace role. `image_generate` outside a workflow run returned only a URL for its copilot-storage upload, so the file-output processor downloaded and re-uploaded it, storing every image twice under two keys and returning `imageUrl` and `image` that disagreed. It now returns the stored file. * improvement(files): sniff generated images once and refuse foreign delegations with 403 Generated images are now content-sniffed before they are stored, so every output field references the same verified object on both the workflow and chat paths; skipping the re-upload had otherwise skipped the sniff it did. A delegation minted for another workspace is no longer rebound (which threw and surfaced a 500); the workspace-file use cases refuse it with their ordinary 403. The regression test now uses the shared principal factory and uploads mock. * chore(files): import the principal factory directly in the delegated write test
1 parent e5b4e3f commit c4ccdde

4 files changed

Lines changed: 220 additions & 54 deletions

File tree

‎apps/sim/lib/internal/file/delegated-caller-write.integration.ts‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
/**
2+
* Mothership runs a file tool through `tools execute`, whose use case authorizes the chat's
3+
* delegated principal for `sim:tool-execution`. The File write that follows must still reach the
4+
* workspace-file use cases, which admit only `sim:workspace-files`, and must still refuse a
5+
* subject or workspace the original delegation does not cover. Exercises the real handler,
6+
* application authorization, metadata and local storage against Postgres.
7+
*/
8+
import { mkdtempSync } from 'node:fs'
9+
import { rm } from 'node:fs/promises'
10+
import { tmpdir } from 'node:os'
11+
import path from 'node:path'
12+
import type { SubjectDelegatedPrincipal } from '@sim/auth/principal'
13+
import { db } from '@sim/db'
14+
import { auditLog, organization, user, workspace, workspaceFiles } from '@sim/db/schema'
15+
import { deleteWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
16+
import { createDelegatedPrincipal } from '@sim/testing/factories/principal.factory'
17+
import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock'
18+
import { generateId } from '@sim/utils/id'
19+
import { and, eq, inArray } from 'drizzle-orm'
20+
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
21+
22+
vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock)
23+
24+
import { executeFileTool } from '@/lib/internal/file/execute-tool'
25+
import {
26+
createKnowledgeAclFixtureIds,
27+
seedKnowledgeAclFixture,
28+
} from '@/lib/knowledge/__integration__/seed-source-access-fixture'
29+
import { uploadCopilotFile } from '@/lib/uploads/contexts/copilot/file-manager'
30+
31+
const fixtures: ReturnType<typeof createKnowledgeAclFixtureIds>[] = []
32+
33+
async function seed() {
34+
const ids = createKnowledgeAclFixtureIds()
35+
fixtures.push(ids)
36+
await seedKnowledgeAclFixture(ids)
37+
return ids
38+
}
39+
40+
type Fixture = Awaited<ReturnType<typeof seed>>
41+
42+
/** The chat principal `tools execute` hands to the tool layer. */
43+
function toolExecutionPrincipal(subjectUserId: string, workspaceId: string) {
44+
return createDelegatedPrincipal({
45+
subjectUserId,
46+
workspaceId,
47+
audience: 'sim:tool-execution',
48+
resourceScope: { chatId: generateId() },
49+
})
50+
}
51+
52+
/** A generated image as `image_generate` stores it outside a workflow run. */
53+
async function generatedImage(ownerId: string) {
54+
return uploadCopilotFile({
55+
buffer: Buffer.from(`image bytes ${generateId()}`),
56+
fileName: `${generateId()}.webp`,
57+
contentType: 'image/webp',
58+
userId: ownerId,
59+
})
60+
}
61+
62+
async function writeCopy(
63+
ids: Fixture,
64+
principal: SubjectDelegatedPrincipal,
65+
fileInput: Awaited<ReturnType<typeof generatedImage>>
66+
) {
67+
const fileName = `saved-${generateId()}.webp`
68+
const response = await executeFileTool({
69+
toolId: 'file_write',
70+
input: { operation: 'write', workspaceId: ids.workspaceId, fileName, fileInput },
71+
headers: new Headers(),
72+
requestId: generateId(),
73+
context: {
74+
workflowId: '',
75+
workspaceId: ids.workspaceId,
76+
userId: principal.subjectUserId,
77+
callerPrincipal: principal,
78+
},
79+
})
80+
const [saved] = await db
81+
.select({ id: workspaceFiles.id, sizeBytes: workspaceFiles.sizeBytes })
82+
.from(workspaceFiles)
83+
.where(
84+
and(
85+
eq(workspaceFiles.workspaceId, ids.workspaceId),
86+
eq(workspaceFiles.originalName, fileName),
87+
eq(workspaceFiles.context, 'workspace')
88+
)
89+
)
90+
return { status: response.status, saved }
91+
}
92+
93+
const uploadRoot = mkdtempSync(path.join(tmpdir(), 'sim-delegated-caller-write-'))
94+
95+
beforeAll(() => {
96+
setUploadDirServer(uploadRoot)
97+
})
98+
99+
afterAll(async () => {
100+
try {
101+
for (const ids of fixtures) {
102+
await db.delete(auditLog).where(eq(auditLog.workspaceId, ids.workspaceId))
103+
await db
104+
.delete(workspaceFiles)
105+
.where(inArray(workspaceFiles.userId, [ids.aliceId, ids.bobId]))
106+
await deleteWorkspaceFixture(db, eq(workspace.id, ids.workspaceId))
107+
await db.delete(organization).where(eq(organization.id, ids.organizationId))
108+
await db.delete(user).where(inArray(user.id, [ids.aliceId, ids.bobId]))
109+
}
110+
await rm(uploadRoot, { recursive: true, force: true })
111+
} finally {
112+
await db.$client.end()
113+
}
114+
})
115+
116+
describe('a File write run directly from a Mothership chat', () => {
117+
it('saves a generated image into Workspace Files', async () => {
118+
const ids = await seed()
119+
const image = await generatedImage(ids.aliceId)
120+
121+
const result = await writeCopy(ids, toolExecutionPrincipal(ids.aliceId, ids.workspaceId), image)
122+
123+
expect(result.status).toBe(200)
124+
expect(result.saved?.sizeBytes).toBe(image.size)
125+
})
126+
127+
it('still refuses a member whose role cannot write', async () => {
128+
const ids = await seed()
129+
const image = await generatedImage(ids.bobId)
130+
131+
const result = await writeCopy(ids, toolExecutionPrincipal(ids.bobId, ids.workspaceId), image)
132+
133+
expect(result.status).toBe(403)
134+
expect(result.saved).toBeUndefined()
135+
})
136+
137+
it('still refuses a delegation minted for another workspace', async () => {
138+
const ids = await seed()
139+
const image = await generatedImage(ids.aliceId)
140+
141+
const result = await writeCopy(ids, toolExecutionPrincipal(ids.aliceId, generateId()), image)
142+
143+
expect(result.status).toBe(403)
144+
expect(result.saved).toBeUndefined()
145+
})
146+
})

‎apps/sim/lib/internal/file/execute-tool.ts‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
import { resolvePrincipalAttribution, resolvePrincipalSubject } from '@sim/auth/principal'
1+
import {
2+
type Principal,
3+
resolvePrincipalAttribution,
4+
resolvePrincipalSubject,
5+
} from '@sim/auth/principal'
26
import { createLogger } from '@sim/logger'
37
import { getErrorMessage, toError } from '@sim/utils/errors'
48
import { z } from 'zod'
@@ -25,6 +29,7 @@ import {
2529
import { parseInternalToolInput } from '@/lib/internal/tool-operations/parse-input'
2630
import type { InternalToolOperationHandler } from '@/lib/internal/tool-operations/types'
2731
import { WORKSPACE_FILES_DELEGATION_AUDIENCE } from '@/lib/workspace-files/application/authorization'
32+
import { rebindWorkspaceFileDelegatedPrincipal } from '@/lib/workspace-files/application/delegated-principal'
2833
import { searchWorkspaceFileContent } from '@/lib/workspace-files/application/search-workspace-file-content'
2934
import {
3035
FILE_SEARCH_DEFAULT_MAX_RESULTS,
@@ -79,6 +84,34 @@ const fileSearchInputSchema = z
7984
})
8085
.strict()
8186

87+
/**
88+
* A delegated caller arrives with its calling use case's audience (`sim:tool-execution`),
89+
* but workspace-file use cases admit only `sim:workspace-files`; they still check the
90+
* subject's access. A delegation for another workspace is left for them to refuse.
91+
*/
92+
function bindWorkspaceFilePrincipal(
93+
principal: Principal,
94+
workspaceId: string,
95+
requestId: string
96+
): Principal {
97+
if (
98+
principal.kind !== 'delegated' ||
99+
principal.audience === WORKSPACE_FILES_DELEGATION_AUDIENCE ||
100+
principal.workspaceId !== workspaceId
101+
) {
102+
return principal
103+
}
104+
const scope = principal.resourceScope
105+
return rebindWorkspaceFileDelegatedPrincipal({
106+
principal,
107+
workspaceId,
108+
delegationId: `file-tool:${requestId}`,
109+
fileId: scope?.fileId,
110+
chatId: scope?.chatId,
111+
executionId: scope?.executionId,
112+
})
113+
}
114+
82115
export const executeFileTool: InternalToolOperationHandler = async (request) => {
83116
request.signal?.throwIfAborted()
84117
if (!FILE_MANAGE_TOOL_IDS.has(request.toolId)) {
@@ -117,7 +150,11 @@ export const executeFileTool: InternalToolOperationHandler = async (request) =>
117150
try {
118151
const principal =
119152
request.context.callerPrincipal && !request.context.executorDelegationOrigin
120-
? request.context.callerPrincipal
153+
? bindWorkspaceFilePrincipal(
154+
request.context.callerPrincipal,
155+
workspaceId,
156+
request.requestId
157+
)
121158
: await createExecutorPrincipalFromExecutionContext({
122159
context: request.context,
123160
audience: WORKSPACE_FILES_DELEGATION_AUDIENCE,

‎apps/sim/lib/internal/image/operations.ts‎

Lines changed: 31 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { createLogger } from '@sim/logger'
22
import { getErrorMessage } from '@sim/utils/errors'
33
import { interruptibleSleep } from '@sim/utils/helpers'
4-
import { isRecordLike } from '@sim/utils/object'
4+
import { isRecordLike, omit } from '@sim/utils/object'
55
import { getUserFacingErrorMessage } from '@/lib/core/errors/database-query-error'
66
import { getMaxExecutionTimeout } from '@/lib/core/execution-limits'
77
import {
@@ -23,6 +23,7 @@ import { recordServiceMeteringFailure } from '@/lib/mothership/billing/service-o
2323
import { type FalAICostMetadata, getFalAICostMetadata } from '@/lib/tools/falai-pricing'
2424
import { uploadCopilotFile } from '@/lib/uploads/contexts/copilot'
2525
import { uploadExecutionFile } from '@/lib/uploads/contexts/execution'
26+
import { resolveStoredFileMetadata } from '@/lib/uploads/utils/stored-file-metadata'
2627

2728
const logger = createLogger('ImageOperations')
2829
const MAX_IMAGE_BYTES = MAX_REMOTE_IMAGE_BYTES
@@ -595,8 +596,11 @@ async function storeGeneratedImage(
595596
context: ImageOperationContext
596597
): Promise<StoredImageResponse> {
597598
context.signal?.throwIfAborted()
598-
const timestamp = Date.now()
599-
const safeFileName = imageResult.fileName || `image-${imageResult.provider}-${timestamp}.png`
599+
const { fileName, mimeType: contentType } = resolveStoredFileMetadata(
600+
imageResult.fileName || `image-${imageResult.provider}-${Date.now()}.png`,
601+
imageResult.contentType,
602+
imageResult.buffer
603+
)
600604
const executionContext =
601605
context.workspaceId && context.workflowId && context.executionId
602606
? {
@@ -606,56 +610,32 @@ async function storeGeneratedImage(
606610
}
607611
: null
608612

609-
if (executionContext) {
610-
const imageFile = await uploadExecutionFile(
611-
executionContext,
612-
imageResult.buffer,
613-
safeFileName,
614-
imageResult.contentType,
615-
context.userId
616-
)
617-
context.signal?.throwIfAborted()
618-
619-
return {
620-
content: imageFile.url,
621-
imageUrl: imageFile.url,
622-
imageFile,
623-
fileName: safeFileName,
624-
contentType: imageResult.contentType,
625-
provider: imageResult.provider,
626-
model: imageResult.model,
627-
metadata: {
628-
provider: imageResult.provider,
629-
model: imageResult.model,
630-
description: imageResult.description,
631-
revisedPrompt: imageResult.revisedPrompt,
632-
seed: imageResult.seed,
633-
jobId: imageResult.jobId,
634-
contentType: imageResult.contentType,
635-
},
636-
__falaiCostDollars: imageResult.falaiCost?.costDollars,
637-
__falaiBilling: imageResult.falaiCost,
638-
}
639-
}
640-
641-
const fileInfo = await uploadCopilotFile({
642-
buffer: imageResult.buffer,
643-
fileName: safeFileName,
644-
contentType: imageResult.contentType,
645-
userId: context.userId,
646-
})
613+
// A stored file output is kept as is; a bare URL would be downloaded and stored again.
614+
const imageFile = executionContext
615+
? await uploadExecutionFile(
616+
executionContext,
617+
imageResult.buffer,
618+
fileName,
619+
contentType,
620+
context.userId
621+
)
622+
: omit(
623+
await uploadCopilotFile({
624+
buffer: imageResult.buffer,
625+
fileName,
626+
contentType,
627+
userId: context.userId,
628+
}),
629+
['mimeType']
630+
)
647631
context.signal?.throwIfAborted()
648-
const imageUrl = fileInfo.url
649-
logger.info(`[${context.requestId}] Stored generated image fallback`, {
650-
fileName: safeFileName,
651-
size: imageResult.buffer.length,
652-
})
653632

654633
return {
655-
content: imageUrl,
656-
imageUrl,
657-
fileName: safeFileName,
658-
contentType: imageResult.contentType,
634+
content: imageFile.url,
635+
imageUrl: imageFile.url,
636+
imageFile,
637+
fileName,
638+
contentType,
659639
provider: imageResult.provider,
660640
model: imageResult.model,
661641
metadata: {
@@ -665,7 +645,7 @@ async function storeGeneratedImage(
665645
revisedPrompt: imageResult.revisedPrompt,
666646
seed: imageResult.seed,
667647
jobId: imageResult.jobId,
668-
contentType: imageResult.contentType,
648+
contentType,
669649
},
670650
__falaiCostDollars: imageResult.falaiCost?.costDollars,
671651
__falaiBilling: imageResult.falaiCost,

‎apps/sim/lib/internal/tool-operations/types.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ export interface InternalToolOperationContext {
2222
executorDelegationOrigin?: ExecutorDelegationOrigin
2323
/** Trusted source block for saved MCP operation restrictions. */
2424
mcpBlockId?: string
25-
/** Authenticated direct caller, supplied by an authorized use case, never tool input. */
25+
/**
26+
* Authenticated direct caller, supplied by an authorized use case, never tool input. A
27+
* delegated principal carries that use case's audience; rebind it before another use case.
28+
*/
2629
callerPrincipal?: Principal
2730
copilotToolExecution?: boolean
2831
copilotInteractionMode?: 'interactive' | 'headless'

0 commit comments

Comments
 (0)