Skip to content

Commit 22277c1

Browse files
committed
fix(execution): resolve stored file references only for workspace members
Workflow file inputs can reference stored files by id, key, or internal URL, and every resolved file is added to the run's explicit file grants. Any caller could use this, so an anonymous public-API, public MCP, chat, or webhook caller could pull another workflow's run files or workspace files into a run and receive a presigned URL for them. Execution now derives a stored-file reference scope from the run's principal: authorized member principals (session, personal/workspace API key, OAuth token, delegated) resolve workspace-wide; system principals may only reference files already stored under the current execution, which keeps chat and webhook uploads working. The default is the restricted scope, so a new caller fails closed. Input-format defaults are workflow-authored and still resolve workspace-wide. Outside workspace scope, an upload whose URL is an internal file URL is resolved as the stored reference it names rather than downloaded with the run user's access, and a key from another execution is refused before it is looked up, so the refusal does not reveal whether the file exists.
1 parent f734ba5 commit 22277c1

5 files changed

Lines changed: 448 additions & 49 deletions

File tree

‎apps/sim/lib/execution/files.test.ts‎

Lines changed: 220 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
/** @vitest-environment node */
2+
import type { WorkflowExecutionPrincipal } from '@sim/auth/principal'
23
import { beforeEach, describe, expect, it, vi } from 'vitest'
34
import type { SerializedBlock } from '@/serializer/types'
45

@@ -8,6 +9,7 @@ const mocks = vi.hoisted(() => ({
89
byKey: vi.fn(),
910
presign: vi.fn(),
1011
readWorkspace: vi.fn(),
12+
download: vi.fn(),
1113
}))
1214
vi.mock('@/lib/uploads/contexts/execution', () => ({ uploadExecutionFile: mocks.upload }))
1315
vi.mock('@/lib/uploads/server/metadata', () => ({
@@ -17,6 +19,7 @@ vi.mock('@/lib/uploads/server/metadata', () => ({
1719
vi.mock('@/lib/uploads/core/storage-service', () => ({
1820
generatePresignedDownloadUrl: mocks.presign,
1921
}))
22+
vi.mock('@/lib/uploads/utils/file-utils.server', () => ({ downloadFileFromUrl: mocks.download }))
2023
vi.mock('@/lib/core/network/resource-scope.server', () => ({
2124
withResourceOutboundScope: (_: unknown, fn: () => unknown) => fn(),
2225
}))
@@ -31,7 +34,11 @@ vi.mock(
3134
})
3235
)
3336

34-
import { processExecutionFiles, processInputFileFields } from '@/lib/execution/files'
37+
import {
38+
getStoredFileReferenceScope,
39+
processExecutionFiles,
40+
processInputFileFields,
41+
} from '@/lib/execution/files'
3542
import { assertUserFileContentAccess } from '@/lib/execution/payloads/materialization.server'
3643
import { StartBlockPath } from '@/lib/workflows/triggers/triggers'
3744
import { buildStartBlockOutput } from '@/executor/utils/start-block'
@@ -102,7 +109,9 @@ describe('workflow input files', () => {
102109
scope,
103110
'request',
104111
'actor',
105-
'selected'
112+
'selected',
113+
undefined,
114+
'workspace'
106115
)
107116
expect(result).toEqual({
108117
...input,
@@ -127,7 +136,8 @@ describe('workflow input files', () => {
127136
[existing, { type: 'file', name: 'new.txt', data: 'data:text/plain;base64,YWJj' }, existing],
128137
scope,
129138
'request',
130-
'actor'
139+
'actor',
140+
'workspace'
131141
)
132142
expect(files.map((file) => file.id)).toEqual([stored.id, 'new', stored.id])
133143
expect(mocks.upload).toHaveBeenCalledTimes(1)
@@ -140,7 +150,13 @@ describe('workflow input files', () => {
140150
)
141151
})
142152
it('resolves an ID-only reference against active workspace metadata', async () => {
143-
const result = await processExecutionFiles({ id: 'metadata-id' }, scope, 'request', 'actor')
153+
const result = await processExecutionFiles(
154+
{ id: 'metadata-id' },
155+
scope,
156+
'request',
157+
'actor',
158+
'workspace'
159+
)
144160
expect(result[0]).toMatchObject({ id: 'metadata-id', name: 'report.pdf', size: 24 })
145161
expect(mocks.byId).toHaveBeenCalledWith('metadata-id')
146162
})
@@ -150,7 +166,8 @@ describe('workflow input files', () => {
150166
{ ...file, url: `/api/files/serve/s3/${encodeURIComponent(key)}?context=workspace` },
151167
scope,
152168
'request',
153-
'actor'
169+
'actor',
170+
'workspace'
154171
)
155172
expect(mocks.byKey).toHaveBeenCalledWith(key)
156173
expect(mocks.byId).not.toHaveBeenCalled()
@@ -184,7 +201,9 @@ describe('workflow input files', () => {
184201
scope,
185202
'request',
186203
'actor',
187-
'start'
204+
'start',
205+
undefined,
206+
'workspace'
188207
)
189208
const output = buildStartBlockOutput({
190209
resolution: { blockId: block.id, block, path: StartBlockPath.UNIFIED },
@@ -211,7 +230,9 @@ describe('workflow input files', () => {
211230
scope,
212231
'request',
213232
'actor',
214-
'start'
233+
'start',
234+
undefined,
235+
'workspace'
215236
)
216237
expect(
217238
buildStartBlockOutput({
@@ -228,9 +249,9 @@ describe('workflow input files', () => {
228249
{ ...stored, key: 'workspace/foreign/file.pdf' },
229250
])('rejects missing, deleted, and foreign file bindings before signing', async (record) => {
230251
mocks.byKey.mockResolvedValue(record)
231-
await expect(processExecutionFiles([existing], scope, 'request', 'actor')).rejects.toThrow(
232-
'File not found in this workspace'
233-
)
252+
await expect(
253+
processExecutionFiles([existing], scope, 'request', 'actor', 'workspace')
254+
).rejects.toThrow('File not found in this workspace')
234255
expect(mocks.presign).not.toHaveBeenCalled()
235256
})
236257
it('grants only successfully resolved declared and reserved file inputs, not arbitrary JSON', async () => {
@@ -246,7 +267,8 @@ describe('workflow input files', () => {
246267
'request',
247268
'actor',
248269
'start',
249-
granted
270+
granted,
271+
'workspace'
250272
)
251273
expect(granted).toHaveBeenCalledTimes(2)
252274
expect(granted.mock.calls.map(([file]) => file.key)).toEqual([stored.key, stored.key])
@@ -264,15 +286,16 @@ describe('workflow input files', () => {
264286
'request',
265287
'actor',
266288
'start',
267-
granted
289+
granted,
290+
'workspace'
268291
)
269292
).rejects.toThrow('File not found')
270293
expect(granted).not.toHaveBeenCalled()
271294
})
272295

273296
it('normalizes Mothership attachment storage context for downstream materialization', async () => {
274297
mocks.byKey.mockResolvedValue({ ...stored, context: 'mothership' })
275-
const [file] = await processExecutionFiles([existing], scope, 'request', 'actor')
298+
const [file] = await processExecutionFiles([existing], scope, 'request', 'actor', 'workspace')
276299
expect(file).toMatchObject({ id: stored.id, context: 'workspace', key: stored.key })
277300
expect(mocks.presign).toHaveBeenCalledWith(stored.key, 'workspace', 300)
278301
await assertUserFileContentAccess(file, {
@@ -291,7 +314,8 @@ describe('workflow input files', () => {
291314
[{ name: 'new.txt', data: 'YWJj', mimeType: 'text/plain' }],
292315
scope,
293316
'request',
294-
'actor'
317+
'actor',
318+
'workspace'
295319
)
296320
expect(mocks.upload).toHaveBeenCalledWith(
297321
scope,
@@ -309,7 +333,9 @@ describe('workflow input files', () => {
309333
scope,
310334
'request',
311335
'actor',
312-
'start'
336+
'start',
337+
undefined,
338+
'workspace'
313339
)
314340
expect(nested).toMatchObject({
315341
input: {
@@ -354,8 +380,186 @@ describe('workflow input files', () => {
354380
scope,
355381
'request',
356382
'actor',
357-
'start'
383+
'start',
384+
undefined,
385+
'workspace'
358386
)
359387
expect(mocks.byKey).toHaveBeenCalledWith(existing.key)
360388
})
389+
390+
describe('stored references from callers outside the workspace', () => {
391+
const priorRun = {
392+
...stored,
393+
key: 'execution/11111111-1111-4111-8111-111111111111/other-workflow/old-run/secret.txt',
394+
context: 'execution',
395+
}
396+
const references = [
397+
{ key: priorRun.key },
398+
{ id: 'metadata-id' },
399+
{
400+
id: 'client-id',
401+
url: `/api/files/serve/s3/${encodeURIComponent(priorRun.key)}?context=execution`,
402+
},
403+
]
404+
405+
it.each([
406+
['a public API principal', 'execution' as const],
407+
['an omitted scope', undefined],
408+
])(
409+
'rejects key, ID, and internal URL references from %s and grants nothing',
410+
async (_, scopeOption) => {
411+
for (const record of [priorRun, stored]) {
412+
mocks.byKey.mockResolvedValue(record)
413+
mocks.byId.mockResolvedValue(record)
414+
for (const reference of references) {
415+
const granted = vi.fn()
416+
await expect(
417+
processInputFileFields(
418+
{ documents: [reference] },
419+
[trigger('start', 'documents')],
420+
scope,
421+
'request',
422+
'actor',
423+
'start',
424+
granted,
425+
scopeOption
426+
)
427+
).rejects.toThrow('Stored file references require workspace member access')
428+
expect(granted).not.toHaveBeenCalled()
429+
}
430+
}
431+
expect(mocks.presign).not.toHaveBeenCalled()
432+
}
433+
)
434+
435+
it('does not reveal whether a rejected reference exists', async () => {
436+
mocks.byKey.mockResolvedValue(null)
437+
await expect(
438+
processExecutionFiles([{ key: priorRun.key }], scope, 'request')
439+
).rejects.toThrow('Stored file references require workspace member access')
440+
})
441+
442+
it('refuses a key from another execution before looking it up', async () => {
443+
await expect(
444+
processExecutionFiles([{ key: priorRun.key }], scope, 'request')
445+
).rejects.toThrow('Stored file references require workspace member access')
446+
expect(mocks.byKey).not.toHaveBeenCalled()
447+
})
448+
449+
it('treats an internal file URL upload as a stored reference, not a download', async () => {
450+
mocks.download.mockResolvedValue(Buffer.from('secret'))
451+
await expect(
452+
processExecutionFiles(
453+
[
454+
{
455+
type: 'url',
456+
name: 'secret.txt',
457+
data: `/api/files/serve/s3/${encodeURIComponent(priorRun.key)}?context=execution`,
458+
},
459+
],
460+
scope,
461+
'request',
462+
'actor'
463+
)
464+
).rejects.toThrow('Stored file references require workspace member access')
465+
expect(mocks.download).not.toHaveBeenCalled()
466+
expect(mocks.upload).not.toHaveBeenCalled()
467+
})
468+
469+
it('keeps downloading internal file URL uploads for a workspace member', async () => {
470+
mocks.download.mockResolvedValue(Buffer.from('bytes'))
471+
await processExecutionFiles(
472+
[
473+
{
474+
type: 'url',
475+
name: 'secret.txt',
476+
data: `/api/files/serve/s3/${encodeURIComponent(priorRun.key)}?context=execution`,
477+
},
478+
],
479+
scope,
480+
'request',
481+
'actor',
482+
'workspace'
483+
)
484+
expect(mocks.download).toHaveBeenCalledTimes(1)
485+
expect(mocks.upload).toHaveBeenCalledTimes(1)
486+
})
487+
488+
it('still resolves files this execution stored before handing its input on', async () => {
489+
const ownKey = `execution/${scope.workspaceId}/${scope.workflowId}/${scope.executionId}/upload.txt`
490+
mocks.byKey.mockResolvedValue({ ...priorRun, key: ownKey })
491+
const granted = vi.fn()
492+
await processInputFileFields(
493+
{ files: [{ id: 'upload', key: ownKey }] },
494+
[trigger('start', 'other')],
495+
scope,
496+
'request',
497+
'actor',
498+
'start',
499+
granted
500+
)
501+
expect(granted.mock.calls.map(([file]) => file.key)).toEqual([ownKey])
502+
})
503+
504+
it('resolves the same prior-run reference for a workspace member and grants it', async () => {
505+
mocks.byKey.mockResolvedValue(priorRun)
506+
const granted = vi.fn()
507+
await processInputFileFields(
508+
{ documents: [{ key: priorRun.key }] },
509+
[trigger('start', 'documents')],
510+
scope,
511+
'request',
512+
'actor',
513+
'start',
514+
granted,
515+
'workspace'
516+
)
517+
expect(granted.mock.calls.map(([file]) => file.key)).toEqual([priorRun.key])
518+
})
519+
})
520+
521+
describe('getStoredFileReferenceScope', () => {
522+
const workspaceId = scope.workspaceId
523+
const workflowId = scope.workflowId
524+
const delegation = {
525+
workspaceId,
526+
delegationId: 'delegation',
527+
audience: 'workflow-execution',
528+
issuedAt: new Date(),
529+
expiresAt: new Date(),
530+
}
531+
it.each<WorkflowExecutionPrincipal>([
532+
{ kind: 'session', userId: 'user', sessionId: 'session' },
533+
{ kind: 'personal_api_key', userId: 'user', keyId: 'key' },
534+
{
535+
kind: 'oauth_access_token',
536+
userId: 'user',
537+
clientId: 'client',
538+
tokenId: 'token',
539+
scopes: [],
540+
expiresAt: new Date(),
541+
},
542+
{ kind: 'workspace_api_key', workspaceId, keyId: 'key' },
543+
{ kind: 'delegated', serviceId: 'copilot', subjectUserId: 'user', ...delegation },
544+
])('lets authorized member principal $kind reference workspace files', (principal) => {
545+
expect(getStoredFileReferenceScope(principal)).toBe('workspace')
546+
})
547+
it.each<WorkflowExecutionPrincipal>([
548+
{ kind: 'system', serviceId: 'public_api', workspaceId, workflowId },
549+
{ kind: 'system', serviceId: 'internal', workspaceId, workflowId },
550+
{ kind: 'system', serviceId: 'schedule', workspaceId, workflowId },
551+
{ kind: 'system', serviceId: 'table', workspaceId, workflowId },
552+
{ kind: 'system', serviceId: 'chat', workspaceId, workflowId },
553+
{
554+
kind: 'system',
555+
serviceId: 'webhook',
556+
workspaceId,
557+
workflowId,
558+
webhookId: 'webhook',
559+
provider: 'generic',
560+
},
561+
])('limits system principal $serviceId to its own execution files', (principal) => {
562+
expect(getStoredFileReferenceScope(principal)).toBe('execution')
563+
})
564+
})
361565
})

0 commit comments

Comments
 (0)