Skip to content

Commit 74c4281

Browse files
committed
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.
1 parent 8783ba8 commit 74c4281

4 files changed

Lines changed: 54 additions & 80 deletions

File tree

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

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -13,16 +13,13 @@ import type { SubjectDelegatedPrincipal } from '@sim/auth/principal'
1313
import { db } from '@sim/db'
1414
import { auditLog, organization, user, workspace, workspaceFiles } from '@sim/db/schema'
1515
import { deleteWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
16+
import { createDelegatedPrincipal } from '@sim/testing'
17+
import { setUploadDirServer, uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock'
1618
import { generateId } from '@sim/utils/id'
1719
import { and, eq, inArray } from 'drizzle-orm'
1820
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
1921

20-
const fixtureStorage = vi.hoisted(() => ({ root: '' }))
21-
vi.mock('@/lib/uploads/core/setup.server', () => ({
22-
get UPLOAD_DIR_SERVER() {
23-
return fixtureStorage.root
24-
},
25-
}))
22+
vi.mock('@/lib/uploads/core/setup.server', () => uploadsSetupMock)
2623

2724
import { executeFileTool } from '@/lib/internal/file/execute-tool'
2825
import {
@@ -44,18 +41,12 @@ type Fixture = Awaited<ReturnType<typeof seed>>
4441

4542
/** The chat principal `tools execute` hands to the tool layer. */
4643
function toolExecutionPrincipal(subjectUserId: string, workspaceId: string) {
47-
const issuedAt = new Date()
48-
return {
49-
kind: 'delegated',
50-
serviceId: 'copilot',
44+
return createDelegatedPrincipal({
5145
subjectUserId,
5246
workspaceId,
53-
delegationId: generateId(),
5447
audience: 'sim:tool-execution',
55-
issuedAt,
56-
expiresAt: new Date(issuedAt.getTime() + 60_000),
5748
resourceScope: { chatId: generateId() },
58-
} satisfies SubjectDelegatedPrincipal
49+
})
5950
}
6051

6152
/** A generated image as `image_generate` stores it outside a workflow run. */
@@ -99,8 +90,10 @@ async function writeCopy(
9990
return { status: response.status, saved }
10091
}
10192

93+
const uploadRoot = mkdtempSync(path.join(tmpdir(), 'sim-delegated-caller-write-'))
94+
10295
beforeAll(() => {
103-
fixtureStorage.root = mkdtempSync(path.join(tmpdir(), 'sim-delegated-caller-write-'))
96+
setUploadDirServer(uploadRoot)
10497
})
10598

10699
afterAll(async () => {
@@ -114,7 +107,7 @@ afterAll(async () => {
114107
await db.delete(organization).where(eq(organization.id, ids.organizationId))
115108
await db.delete(user).where(inArray(user.id, [ids.aliceId, ids.bobId]))
116109
}
117-
await rm(fixtureStorage.root, { recursive: true, force: true })
110+
await rm(uploadRoot, { recursive: true, force: true })
118111
} finally {
119112
await db.$client.end()
120113
}
@@ -137,7 +130,7 @@ describe('a File write run directly from a Mothership chat', () => {
137130

138131
const result = await writeCopy(ids, toolExecutionPrincipal(ids.bobId, ids.workspaceId), image)
139132

140-
expect(result.status).toBeGreaterThanOrEqual(400)
133+
expect(result.status).toBe(403)
141134
expect(result.saved).toBeUndefined()
142135
})
143136

@@ -147,7 +140,7 @@ describe('a File write run directly from a Mothership chat', () => {
147140

148141
const result = await writeCopy(ids, toolExecutionPrincipal(ids.aliceId, generateId()), image)
149142

150-
expect(result.status).toBeGreaterThanOrEqual(400)
143+
expect(result.status).toBe(403)
151144
expect(result.saved).toBeUndefined()
152145
})
153146
})

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

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -85,11 +85,9 @@ const fileSearchInputSchema = z
8585
.strict()
8686

8787
/**
88-
* A delegated caller reaches a file tool through a use case that authorized it for
89-
* its own audience (`sim:tool-execution` for a direct tool call), while every
90-
* workspace-file use case admits only `sim:workspace-files`. Rebinding keeps the
91-
* subject, workspace, expiry ceiling, and resource scope, so the file use cases
92-
* still check the subject's current workspace access.
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.
9391
*/
9492
function bindWorkspaceFilePrincipal(
9593
principal: Principal,
@@ -98,7 +96,8 @@ function bindWorkspaceFilePrincipal(
9896
): Principal {
9997
if (
10098
principal.kind !== 'delegated' ||
101-
principal.audience === WORKSPACE_FILES_DELEGATION_AUDIENCE
99+
principal.audience === WORKSPACE_FILES_DELEGATION_AUDIENCE ||
100+
principal.workspaceId !== workspaceId
102101
) {
103102
return principal
104103
}
@@ -107,9 +106,9 @@ function bindWorkspaceFilePrincipal(
107106
principal,
108107
workspaceId,
109108
delegationId: `file-tool:${requestId}`,
110-
...(scope?.fileId ? { fileId: scope.fileId } : {}),
111-
...(scope?.chatId ? { chatId: scope.chatId } : {}),
112-
...(scope?.executionId ? { executionId: scope.executionId } : {}),
109+
fileId: scope?.fileId,
110+
chatId: scope?.chatId,
111+
executionId: scope?.executionId,
113112
})
114113
}
115114

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

Lines changed: 31 additions & 52 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,57 +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-
imageFile: fileInfo,
658-
fileName: safeFileName,
659-
contentType: imageResult.contentType,
634+
content: imageFile.url,
635+
imageUrl: imageFile.url,
636+
imageFile,
637+
fileName,
638+
contentType,
660639
provider: imageResult.provider,
661640
model: imageResult.model,
662641
metadata: {
@@ -666,7 +645,7 @@ async function storeGeneratedImage(
666645
revisedPrompt: imageResult.revisedPrompt,
667646
seed: imageResult.seed,
668647
jobId: imageResult.jobId,
669-
contentType: imageResult.contentType,
648+
contentType,
670649
},
671650
__falaiCostDollars: imageResult.falaiCost?.costDollars,
672651
__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)