Skip to content

Commit 21291d7

Browse files
icecrasher321icecrasher321
authored
fix(files): hydrate chat attachments and reject blank download URLs (#7870)
* fix(files): hydrate chat attachments and reject blank download URLs * fix(files): preserve concealed canonical file failures
1 parent 8e14e4d commit 21291d7

11 files changed

Lines changed: 744 additions & 49 deletions

File tree

‎apps/sim/app/(interfaces)/chat/components/message/components/file-download.test.tsx‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,13 @@ describe('ChatFileDownload', () => {
8585
)
8686
})
8787

88+
it.each(['', ' \t\n'])('uses the serve route to preview files with a blank URL (%j)', (url) => {
89+
const container = renderFile({ ...imageFile, base64: undefined, url })
90+
expect(container.querySelector('img')?.getAttribute('src')).toBe(
91+
'/api/files/serve/execution%2Fgenerated.png?context=execution'
92+
)
93+
})
94+
8895
it('keeps a download available when an image preview fails', () => {
8996
const container = renderFile(imageFile)
9097
act(() => container.querySelector('img')!.dispatchEvent(new Event('error')))
@@ -154,6 +161,26 @@ describe('chat file downloads', () => {
154161
expect(downloadedNames).toEqual(['generated.png'])
155162
})
156163

164+
it.each(['', ' \t\n'].flatMap((url) => [false, true].map((stored) => ({ url, stored }))))(
165+
'never downloads the chat page for a blank URL (%j)',
166+
async ({ url, stored }) => {
167+
if (stored) fetchMock.mockResolvedValueOnce(new Response(null, { status: 401 }))
168+
fetchMock.mockResolvedValue(new Response('<html>Chat page</html>'))
169+
const container = renderFile({
170+
...imageFile,
171+
base64: undefined,
172+
key: stored ? imageFile.key : 'url/external',
173+
url,
174+
})
175+
await clickDownload(container)
176+
expect(fetchMock).toHaveBeenCalledTimes(stored ? 1 : 0)
177+
expect(createObjectURL).not.toHaveBeenCalled()
178+
expect(downloadedNames).toEqual([])
179+
expect(container.querySelector('a')).toBeNull()
180+
expect(container.querySelector('[role="alert"]')?.textContent).toContain('Unable to download')
181+
}
182+
)
183+
157184
it.each([false, true])(
158185
'offers a safe browser download when an external host blocks CORS (stored=%s)',
159186
async (stored) => {

‎apps/sim/app/(interfaces)/chat/components/message/components/file-download.tsx‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,17 @@ function isImageFile(mimeType: string): boolean {
6868
return mimeType.startsWith('image/')
6969
}
7070

71+
function getExternalFileUrl(file: ChatFile): string | null {
72+
const url = file.url?.trim()
73+
return url && isSafeHttpUrl(url) ? url : null
74+
}
75+
7176
function getFileUrl(file: ChatFile): string {
7277
if (file.base64) return `data:${file.type};base64,${file.base64}`
73-
if (isSafeHttpUrl(file.url)) return file.url
74-
return `/api/files/serve/${encodeURIComponent(file.key)}?context=${file.context || 'execution'}`
78+
return (
79+
getExternalFileUrl(file) ??
80+
`/api/files/serve/${encodeURIComponent(file.key)}?context=${file.context || 'execution'}`
81+
)
7582
}
7683

7784
async function triggerDownload(file: ChatFile): Promise<void> {
@@ -88,11 +95,10 @@ async function triggerDownload(file: ChatFile): Promise<void> {
8895

8996
const storageContext = tryInferContextFromKey(file.key)
9097
const hasStorageKey = storageContext !== null
98+
const externalUrl = getExternalFileUrl(file)
9199
const url = hasStorageKey
92100
? `/api/files/serve/${encodeURIComponent(file.key)}?context=${encodeURIComponent(storageContext)}`
93-
: isSafeHttpUrl(file.url)
94-
? file.url
95-
: null
101+
: externalUrl
96102
if (!url) throw new Error('File has no download URL')
97103

98104
/** The same serve route as execution logs resolves current storage access on each click. */
@@ -103,10 +109,10 @@ async function triggerDownload(file: ChatFile): Promise<void> {
103109
} else {
104110
response = await fetchExternalFile(url)
105111
}
106-
if (hasStorageKey && response.status === 401 && isSafeHttpUrl(file.url)) {
112+
if (hasStorageKey && response.status === 401 && externalUrl) {
107113
await response.body?.cancel()
108114
/** Public chat visitors may only have the file access already delivered in the response. */
109-
response = await fetchExternalFile(file.url)
115+
response = await fetchExternalFile(externalUrl)
110116
}
111117
if (!response.ok) {
112118
await response.body?.cancel()

‎apps/sim/executor/handlers/agent/memory-harness.postgres.test.ts‎

Lines changed: 66 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ import {
7272
workspaceFiles,
7373
} from '@sim/db/schema'
7474
import { hashDurableSecretProvenanceValue } from '@/lib/execution/durable-secret-provenance'
75+
import { StorageService } from '@/lib/uploads'
7576
import { uploadExecutionFile } from '@/lib/uploads/contexts/execution/execution-file-manager'
7677
import {
7778
EXACT_EMPTY_WORKSPACE_FILE_SECRET_PROVENANCE,
@@ -84,7 +85,13 @@ import { AgentBlockHandler } from '@/executor/handlers/agent/agent-handler'
8485
import type { AgentInputs, Message } from '@/executor/handlers/agent/types'
8586
import type { ExecutionContext, StreamingExecution, UserFile } from '@/executor/types'
8687
import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
88+
import { INLINE_ATTACHMENT_THRESHOLD_BYTES } from '@/providers/attachments'
89+
import {
90+
attachLargeFileRemoteUrls,
91+
uploadLargeFilesToProvider,
92+
} from '@/providers/file-attachments.server'
8793
import { createAgentStreamPump } from '@/providers/stream-pump'
94+
import type { ProviderRequest } from '@/providers/types'
8895
import type { SerializedBlock } from '@/serializer/types'
8996

9097
const databaseUrl = process.env.AGENT_MEMORY_TEST_DATABASE_URL
@@ -382,12 +389,14 @@ describe.skipIf(!databaseUrl)(
382389
})
383390

384391
it.each(
385-
(['openai', 'anthropic'] as const).flatMap((provider) =>
386-
[false, true].map((streaming) => ({ provider, streaming }))
392+
(['workspace', 'mothership'] as const).flatMap((storageContext) =>
393+
(['openai', 'anthropic'] as const).flatMap((provider) =>
394+
[false, true].map((streaming) => ({ storageContext, provider, streaming }))
395+
)
387396
)
388397
)(
389-
'deployed chat reads remembered workspace files with $provider, streaming=$streaming',
390-
async ({ provider, streaming }) => {
398+
'deployed chat reads remembered $storageContext files with $provider, streaming=$streaming',
399+
async ({ storageContext, provider, streaming }) => {
391400
if (!fixture.database || !connection) throw new Error('Missing harness database')
392401
vi.stubGlobal('fetch', interceptFetch)
393402
outbound = []
@@ -414,7 +423,7 @@ describe.skipIf(!databaseUrl)(
414423
key,
415424
userId: scope.userId,
416425
workspaceId: scope.workspaceId,
417-
context: 'workspace',
426+
context: storageContext,
418427
originalName: 'result.pdf',
419428
contentType: 'application/pdf',
420429
size: buffer.length,
@@ -487,6 +496,57 @@ describe.skipIf(!databaseUrl)(
487496
input: { key, assertedWorkspaceId: scope.workspaceId },
488497
})
489498
).rejects.toThrow('Principal kind system')
499+
500+
const strictWorkspaceRead = readWorkspaceFileRecordByKey.execute({
501+
principal: {
502+
kind: 'workspace_api_key',
503+
workspaceId: scope.workspaceId,
504+
keyId: 'harness-key',
505+
},
506+
input: { key, assertedWorkspaceId: scope.workspaceId },
507+
})
508+
if (storageContext === 'mothership') {
509+
await expect(strictWorkspaceRead).rejects.toMatchObject({ code: 'not_found' })
510+
} else {
511+
await expect(strictWorkspaceRead).resolves.toMatchObject({ file: { id: record.id } })
512+
}
513+
514+
/** Exercise large-file authorization with real metadata and delegation before model dispatch. */
515+
const largeFile = { ...file, size: INLINE_ATTACHMENT_THRESHOLD_BYTES + 1 }
516+
const largeRequest: ProviderRequest = {
517+
model: models[provider],
518+
apiKey: apiKey(provider),
519+
userId: scope.userId,
520+
messages: [{ role: 'user', content: 'Read the attachment', files: [largeFile] }],
521+
}
522+
const cloudStorage = vi.spyOn(StorageService, 'hasCloudStorage').mockReturnValue(true)
523+
const presign = vi
524+
.spyOn(StorageService, 'generatePresignedDownloadUrl')
525+
.mockResolvedValue('https://storage.example.com/signed')
526+
try {
527+
await attachLargeFileRemoteUrls(largeRequest, provider, firstContext)
528+
expect(presign).toHaveBeenCalledWith(key, 'workspace', 3600)
529+
expect(largeFile.remoteUrl).toBe('https://storage.example.com/signed')
530+
if (provider === 'openai') {
531+
const upload = vi.fn(async (url: string, init?: RequestInit) => {
532+
expect(url).toBe('https://api.openai.com/v1/files')
533+
expect(init?.body).toBeInstanceOf(FormData)
534+
const body = init!.body as FormData
535+
const uploaded = body.get('file') as File
536+
expect(Buffer.from(await uploaded.arrayBuffer())).toEqual(buffer)
537+
return Response.json({ id: 'file-harness' })
538+
})
539+
vi.stubGlobal('fetch', upload)
540+
await uploadLargeFilesToProvider(largeRequest, provider, firstContext)
541+
expect(upload).toHaveBeenCalledOnce()
542+
expect(largeFile.providerFileId).toBe('file-harness')
543+
}
544+
} finally {
545+
cloudStorage.mockRestore()
546+
presign.mockRestore()
547+
vi.stubGlobal('fetch', interceptFetch)
548+
}
549+
490550
expect(await executeTurn(firstContext, { ...inputs, files: [file] })).toBe('READY')
491551
expect(requestFiles(outbound[0])).toEqual([buffer.toString('base64')])
492552
const stored = await readConversation(conversationId)
@@ -529,7 +589,7 @@ describe.skipIf(!databaseUrl)(
529589
report.push({
530590
provider,
531591
streaming,
532-
workspaceAttachment: true,
592+
storageContext,
533593
stored,
534594
controls: {
535595
missingOrigin: 'blocked before HTTP',

‎apps/sim/lib/execution/payloads/file-secret-provenance.test.ts‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,15 @@ const { metadata, readWorkspaceFile } = vi.hoisted(() => ({
77
}))
88

99
vi.mock('@/lib/uploads/server/metadata', () => ({ getFileMetadataByKey: metadata }))
10-
vi.mock('@/lib/workspace-files/application/read-workspace-file-content-by-key', () => ({
11-
readWorkspaceFileRecordByKey: { execute: readWorkspaceFile },
12-
}))
10+
vi.mock(
11+
'@/lib/workspace-files/application/read-stored-workspace-file-record-by-key',
12+
async (importOriginal) => ({
13+
...(await importOriginal<
14+
typeof import('@/lib/workspace-files/application/read-stored-workspace-file-record-by-key')
15+
>()),
16+
readStoredWorkspaceFileRecordByKey: { execute: readWorkspaceFile },
17+
})
18+
)
1319

1420
import { resolveStoredFileProvenanceSource } from '@/lib/execution/payloads/file-secret-provenance'
1521

0 commit comments

Comments
 (0)