Skip to content

Commit 2deac27

Browse files
fix(files): preserve principals when serving documents
1 parent 738006d commit 2deac27

17 files changed

Lines changed: 656 additions & 36 deletions

apps/sim/app/api/files/serve/[...path]/route.test.ts

Lines changed: 94 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,10 @@ const {
2020
mockIsUsingCloudStorage,
2121
mockDownloadCopilotFile,
2222
mockInferContextFromKey,
23+
mockParseWorkspaceFileKey,
24+
mockAuthenticateWorkspaceFile,
25+
mockReadWorkspaceFileContentByKey,
26+
mockResolveServableDocBytes,
2327
mockGetContentType,
2428
mockFindLocalFile,
2529
mockCreateFileResponse,
@@ -40,6 +44,10 @@ const {
4044
mockIsUsingCloudStorage: vi.fn(),
4145
mockDownloadCopilotFile: vi.fn(),
4246
mockInferContextFromKey: vi.fn(),
47+
mockParseWorkspaceFileKey: vi.fn(),
48+
mockAuthenticateWorkspaceFile: vi.fn(),
49+
mockReadWorkspaceFileContentByKey: vi.fn(),
50+
mockResolveServableDocBytes: vi.fn(),
4351
mockGetContentType: vi.fn(),
4452
mockFindLocalFile: vi.fn(),
4553
mockCreateFileResponse: vi.fn(),
@@ -82,7 +90,19 @@ vi.mock('@/lib/execution/sandbox/run-task', () => ({
8290
}))
8391

8492
vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({
85-
parseWorkspaceFileKey: vi.fn().mockReturnValue(undefined),
93+
parseWorkspaceFileKey: mockParseWorkspaceFileKey,
94+
}))
95+
96+
vi.mock('@/lib/workspace-files/api', () => ({
97+
internalSessionOrExecutorAuth: { authenticate: mockAuthenticateWorkspaceFile },
98+
}))
99+
100+
vi.mock('@/lib/workspace-files/application/read-workspace-file-content-by-key', () => ({
101+
readWorkspaceFileContentByKey: { execute: mockReadWorkspaceFileContentByKey },
102+
}))
103+
104+
vi.mock('@/lib/copilot/tools/server/files/doc-compile', () => ({
105+
resolveServableDocBytes: mockResolveServableDocBytes,
86106
}))
87107

88108
vi.mock('@/app/api/files/utils', () => ({
@@ -109,7 +129,27 @@ describe('File Serve API Route', () => {
109129
mockReadFile.mockResolvedValue(Buffer.from('test content'))
110130
mockIsUsingCloudStorage.mockReturnValue(false)
111131
storageServiceMockFns.mockHasCloudStorage.mockReturnValue(true)
112-
mockInferContextFromKey.mockReturnValue('workspace')
132+
mockInferContextFromKey.mockReturnValue('mothership')
133+
mockParseWorkspaceFileKey.mockReturnValue(undefined)
134+
mockAuthenticateWorkspaceFile.mockResolvedValue({
135+
kind: 'session',
136+
userId: 'test-user-id',
137+
sessionId: 'session-1',
138+
})
139+
mockReadWorkspaceFileContentByKey.mockResolvedValue({
140+
file: {
141+
id: 'file-1',
142+
workspaceId: 'test-workspace-id',
143+
name: 'report.pdf',
144+
},
145+
content: Buffer.from('generated source'),
146+
})
147+
mockResolveServableDocBytes.mockImplementation(
148+
async ({ rawBuffer, fileName }: { rawBuffer: Buffer; fileName: string }) => ({
149+
buffer: rawBuffer,
150+
contentType: mockGetContentType(fileName),
151+
})
152+
)
113153
mockGetContentType.mockReturnValue('text/plain')
114154
mockFindLocalFile.mockReturnValue('/test/uploads/test-file.txt')
115155
mockCreateFileResponse.mockImplementation(
@@ -181,8 +221,59 @@ describe('File Serve API Route', () => {
181221

182222
expect(storageServiceMockFns.mockDownloadFile).toHaveBeenCalledWith({
183223
key: 'workspace/test-workspace-id/1234567890-image.png',
184-
context: 'workspace',
224+
context: 'mothership',
225+
})
226+
})
227+
228+
it('serves a workspace document through the authorized use case and preserves the Principal', async () => {
229+
const principal = {
230+
kind: 'delegated' as const,
231+
serviceId: 'executor' as const,
232+
subjectUserId: 'test-user-id',
233+
workspaceId: 'test-workspace-id',
234+
delegationId: 'delegation-1',
235+
audience: 'sim:workspace-files',
236+
issuedAt: new Date('2026-08-01T00:00:00Z'),
237+
expiresAt: new Date('2026-08-01T01:00:00Z'),
238+
delegationContext: {
239+
kind: 'workflow_execution' as const,
240+
workflowId: 'workflow-1',
241+
},
242+
}
243+
mockInferContextFromKey.mockReturnValue('workspace')
244+
mockParseWorkspaceFileKey.mockReturnValue('test-workspace-id')
245+
mockAuthenticateWorkspaceFile.mockResolvedValue(principal)
246+
mockResolveServableDocBytes.mockResolvedValue({
247+
buffer: Buffer.from('%PDF-compiled'),
248+
contentType: 'application/pdf',
185249
})
250+
251+
const req = new NextRequest(
252+
'http://localhost:3000/api/files/serve/workspace/test-workspace-id/report.pdf'
253+
)
254+
const response = await GET(req, {
255+
params: Promise.resolve({
256+
path: ['workspace', 'test-workspace-id', 'report.pdf'],
257+
}),
258+
})
259+
260+
expect(response.status).toBe(200)
261+
expect(mockReadWorkspaceFileContentByKey).toHaveBeenCalledWith({
262+
principal,
263+
input: {
264+
key: 'workspace/test-workspace-id/report.pdf',
265+
assertedWorkspaceId: 'test-workspace-id',
266+
},
267+
request: req,
268+
})
269+
expect(mockResolveServableDocBytes).toHaveBeenCalledWith(
270+
expect.objectContaining({
271+
workspaceId: 'test-workspace-id',
272+
filePrincipal: principal,
273+
})
274+
)
275+
expect(hybridAuthMockFns.mockCheckSessionOrInternalAuth).not.toHaveBeenCalled()
276+
expect(mockVerifyFileAccess).not.toHaveBeenCalled()
186277
})
187278

188279
it('should return 404 when file not found', async () => {

apps/sim/app/api/files/serve/[...path]/route.ts

Lines changed: 86 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,26 @@
11
import { readFile } from 'fs/promises'
2+
import { type Principal, requirePrincipalSubjectUserId } from '@sim/auth/principal'
23
import { createLogger } from '@sim/logger'
34
import type { NextRequest } from 'next/server'
45
import { NextResponse } from 'next/server'
56
import { fileServeParamsSchema, fileServeQuerySchema } from '@/lib/api/contracts/storage-transfer'
7+
import {
8+
concealCrossTenantResourceError,
9+
InternalUnauthenticatedError,
10+
} from '@/lib/api/server/routes'
611
import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
712
import { resolveServableDocBytes } from '@/lib/copilot/tools/server/files/doc-compile'
813
import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile-error'
14+
import { asOrchestrationError } from '@/lib/core/orchestration/types'
915
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1016
import { CopilotFiles, isUsingCloudStorage } from '@/lib/uploads'
1117
import type { StorageContext } from '@/lib/uploads/config'
1218
import { parseWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
1319
import { downloadFile } from '@/lib/uploads/core/storage-service'
1420
import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative'
1521
import { inferContextFromKey } from '@/lib/uploads/utils/file-utils'
22+
import { internalSessionOrExecutorAuth } from '@/lib/workspace-files/api'
23+
import { readWorkspaceFileContentByKey } from '@/lib/workspace-files/application/read-workspace-file-content-by-key'
1624
import { verifyFileAccess } from '@/app/api/files/authorization'
1725
import {
1826
createErrorResponse,
@@ -66,9 +74,11 @@ async function resolveServableBytes(params: {
6674
workspaceId: string | undefined
6775
options: ServeOptions
6876
ownerKey: string | undefined
77+
filePrincipal?: Principal
6978
signal: AbortSignal | undefined
7079
}): Promise<{ buffer: Buffer; contentType: string }> {
71-
const { buffer, filename, storageKey, workspaceId, options, ownerKey, signal } = params
80+
const { buffer, filename, storageKey, workspaceId, options, ownerKey, filePrincipal, signal } =
81+
params
7282
if (options.raw) return { buffer, contentType: getContentType(filename) }
7383

7484
if (options.preview) {
@@ -82,6 +92,7 @@ async function resolveServableBytes(params: {
8292
rawBuffer: buffer,
8393
fileName: filename,
8494
workspaceId,
95+
filePrincipal,
8596
ownerKey,
8697
signal,
8798
})
@@ -154,6 +165,23 @@ export const GET = withRouteHandler(
154165
return await handleLocalFilePublic(fullPath)
155166
}
156167

168+
const storageContext = inferContextFromKey(cloudKey)
169+
const workspacePrincipal =
170+
storageContext === 'workspace'
171+
? await internalSessionOrExecutorAuth.authenticate(request, { path })
172+
: undefined
173+
const legacyAuthResult = workspacePrincipal
174+
? undefined
175+
: await checkSessionOrInternalAuth(request, { requireWorkflowId: false })
176+
177+
if (legacyAuthResult && (!legacyAuthResult.success || !legacyAuthResult.userId)) {
178+
logger.warn('Unauthorized file access attempt', {
179+
path,
180+
error: legacyAuthResult.error || 'Missing userId',
181+
})
182+
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
183+
}
184+
157185
const query = fileServeQuerySchema.parse({
158186
raw: request.nextUrl.searchParams.get('raw'),
159187
preview: request.nextUrl.searchParams.get('preview'),
@@ -165,24 +193,24 @@ export const GET = withRouteHandler(
165193
versioned: query.v != null,
166194
}
167195

168-
const authResult = await checkSessionOrInternalAuth(request, { requireWorkflowId: false })
169-
170-
if (!authResult.success || !authResult.userId) {
171-
logger.warn('Unauthorized file access attempt', {
172-
path,
173-
error: authResult.error || 'Missing userId',
174-
})
175-
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
196+
if (workspacePrincipal) {
197+
return await handleWorkspaceFile(cloudKey, workspacePrincipal, options, request)
176198
}
177199

178-
const userId = authResult.userId
200+
const userId = legacyAuthResult?.userId
201+
if (!userId) throw new Error('Authenticated file serve request is missing a user ID')
179202

180203
if (isUsingCloudStorage()) {
181204
return await handleCloudProxy(cloudKey, userId, options, request.signal)
182205
}
183206

184207
return await handleLocalFile(cloudKey, userId, options, request.signal)
185208
} catch (error) {
209+
if (error instanceof InternalUnauthenticatedError) {
210+
logger.warn('Unauthorized file access attempt', { error: error.message })
211+
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
212+
}
213+
186214
// An in-progress/incomplete doc source fails to compile — this is expected
187215
// mid-generation, not a server fault. Return 409 (not 500) so it isn't an
188216
// alarming error; the client re-fetches once the doc finishes (the serve
@@ -194,6 +222,15 @@ export const GET = withRouteHandler(
194222
return NextResponse.json({ error: 'Document is still being generated' }, { status: 409 })
195223
}
196224

225+
const orchestrationError = asOrchestrationError(
226+
concealCrossTenantResourceError(error, 'File not found')
227+
)
228+
if (orchestrationError?.code === 'not_found') {
229+
const notFound = new FileNotFoundError('File not found')
230+
logServeFailure('Error serving file:', notFound)
231+
return createErrorResponse(notFound)
232+
}
233+
197234
logServeFailure('Error serving file:', error)
198235

199236
if (error instanceof FileNotFoundError) {
@@ -205,6 +242,45 @@ export const GET = withRouteHandler(
205242
}
206243
)
207244

245+
async function handleWorkspaceFile(
246+
key: string,
247+
principal: Principal,
248+
options: ServeOptions,
249+
request: NextRequest
250+
): Promise<NextResponse> {
251+
const workspaceId = getWorkspaceIdForCompile(key)
252+
if (!workspaceId) throw new FileNotFoundError(`File not found: ${key}`)
253+
254+
const { file, content } = await readWorkspaceFileContentByKey.execute({
255+
principal,
256+
input: { key, assertedWorkspaceId: workspaceId },
257+
request,
258+
})
259+
const ownerKey = `user:${requirePrincipalSubjectUserId(principal)}`
260+
const resolved = await resolveServableBytes({
261+
buffer: content,
262+
filename: file.name,
263+
storageKey: key,
264+
workspaceId,
265+
options,
266+
ownerKey,
267+
filePrincipal: principal,
268+
signal: request.signal,
269+
})
270+
271+
logger.info('Workspace file served', {
272+
fileId: file.id,
273+
workspaceId,
274+
size: resolved.buffer.length,
275+
})
276+
return createFileResponse({
277+
buffer: resolved.buffer,
278+
contentType: resolved.contentType,
279+
filename: file.name,
280+
cacheControl: resolveServeCacheControl(options.versioned, 'workspace'),
281+
})
282+
}
283+
208284
async function handleLocalFile(
209285
filename: string,
210286
userId: string,

apps/sim/lib/copilot/tools/handlers/function-execute.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import { queryRows } from '@/lib/table/rows/service'
3030
import { getTableById, listTables } from '@/lib/table/service'
3131
import { getOrCreateTableSnapshot, SNAPSHOT_MAX_BYTES } from '@/lib/table/snapshot-cache'
3232
import {
33-
fetchServableWorkspaceFileBuffer,
3433
findWorkspaceFileRecord,
3534
getSandboxWorkspaceFilePath,
3635
type WorkspaceFileRecord,
@@ -42,6 +41,7 @@ import {
4241
hasCloudStorage,
4342
} from '@/lib/uploads/core/storage-service'
4443
import { isGeneratedDocumentSourceType } from '@/lib/uploads/utils/file-utils'
44+
import { fetchAuthorizedServableWorkspaceFileBuffer } from '@/lib/workspace-files/application/fetch-servable-workspace-file-buffer'
4545
import { listAllWorkspaceFiles } from '@/lib/workspace-files/application/list-workspace-files'
4646
import { readWorkspaceFileContent } from '@/lib/workspace-files/application/read-workspace-file-content'
4747
import { downloadWorkspaceFileRecord } from '@/lib/workspace-files/application/read-workspace-file-record'
@@ -202,7 +202,7 @@ async function pushWorkspaceFileMount(
202202
}
203203

204204
const { buffer, contentType } = rendersFromSource
205-
? await fetchServableWorkspaceFileBuffer(record, {
205+
? await fetchAuthorizedServableWorkspaceFileBuffer(record, principal, {
206206
maxBytes: Math.min(MAX_FILE_SIZE, remainingBudget),
207207
}).catch((error) => {
208208
if (!isPayloadSizeLimitError(error)) throw error

apps/sim/lib/copilot/tools/server/files/doc-compile.test.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,15 @@ const {
88
executeInSandboxMock,
99
executeShellInSandboxMock,
1010
loadCompiledDocMock,
11+
publishCompiledDocArtifactMock,
1112
readWorkspaceFileContentMock,
1213
readWorkspaceFileMetadataMock,
1314
storeCompiledDocMock,
1415
} = vi.hoisted(() => ({
1516
executeInSandboxMock: vi.fn(),
1617
executeShellInSandboxMock: vi.fn(),
1718
loadCompiledDocMock: vi.fn(),
19+
publishCompiledDocArtifactMock: vi.fn(),
1820
readWorkspaceFileContentMock: vi.fn(),
1921
readWorkspaceFileMetadataMock: vi.fn(),
2022
storeCompiledDocMock: vi.fn(),
@@ -35,6 +37,8 @@ vi.mock('@/lib/workspace-files/application/read-workspace-file-metadata', () =>
3537
}))
3638
vi.mock('./doc-compiled-store', () => ({
3739
loadCompiledDoc: loadCompiledDocMock,
40+
loadPublishedCompiledDoc: vi.fn(),
41+
publishCompiledDocArtifact: publishCompiledDocArtifactMock,
3842
storeCompiledDoc: storeCompiledDocMock,
3943
}))
4044

@@ -245,5 +249,11 @@ describe('collectReferencedFileIds', () => {
245249
)
246250
expect(readWorkspaceFileContentMock).not.toHaveBeenCalled()
247251
expect(executeInSandboxMock).not.toHaveBeenCalled()
252+
expect(publishCompiledDocArtifactMock).toHaveBeenCalledWith(
253+
'workspace-1',
254+
`image = await getFileBase64('${ID}')`,
255+
'pdf',
256+
expect.stringContaining(ID)
257+
)
248258
})
249259
})

0 commit comments

Comments
 (0)