Skip to content

Commit aa47166

Browse files
committed
fix(uploads): budget every key built from a caller-supplied name
Auditing the rest of the codebase for the shape that broke the upload-session PUT found five more key builders that put an unbounded name into a path component local storage writes directly. Three are on the same route as the original bug: `table_import`, `profile_picture`, and `workspace_logo` built their key inline with `sanitizeFileName`, which maps characters and never truncates, while their sibling purposes went through `buildStorageKeySegment`. A 255-character name broke `table_import` at the metadata sidecar and the other two at the object write itself. The other two are local-storage writers reached from elsewhere: knowledge-base connector sync capped the document title at 200 and then appended a timestamp, a uuid and `.txt` on top of the cap, landing at exactly 255 with no room for the sidecar; the Mistral-OCR staging and chunk keys inlined the sanitizer with no bound at all; and inbound email attachments went into a key with neither sanitizer nor bound, on a file name an outside sender chooses. All now derive their component through `buildStorageKeySegment`, so the reservation is stated once. The upload-session test asserts it for every purpose the contract admits, which is what keeps a newly added purpose from reintroducing the hand-built form.
1 parent 46c8fcb commit aa47166

5 files changed

Lines changed: 64 additions & 16 deletions

File tree

apps/sim/lib/knowledge/connectors/sync-engine.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import type { DocumentData } from '@/lib/knowledge/documents/service'
2222
import { hardDeleteDocuments, processDocumentsWithQueue } from '@/lib/knowledge/documents/service'
2323
import { refreshAccessTokenIfNeeded } from '@/lib/oauth/credential-service'
2424
import { StorageService } from '@/lib/uploads'
25+
import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key'
2526
import { deleteFile } from '@/lib/uploads/core/storage-service'
2627
import { deleteFileMetadata } from '@/lib/uploads/server/metadata'
2728
import { extractStorageKey } from '@/lib/uploads/utils/file-utils'
@@ -1472,7 +1473,7 @@ async function addDocument(
14721473
const documentId = generateId()
14731474
const contentBuffer = Buffer.from(extDoc.content, 'utf-8')
14741475
const safeTitle = sanitizeStorageTitle(extDoc.title)
1475-
const customKey = `kb/${Date.now()}-${documentId}-${safeTitle}.txt`
1476+
const customKey = `kb/${buildStorageKeySegment(`${Date.now()}-${documentId}-`, `${safeTitle}.txt`)}`
14761477

14771478
const fileInfo = await StorageService.uploadFile({
14781479
file: contentBuffer,
@@ -1561,7 +1562,7 @@ async function updateDocument(
15611562

15621563
const contentBuffer = Buffer.from(extDoc.content, 'utf-8')
15631564
const safeTitle = sanitizeStorageTitle(extDoc.title)
1564-
const customKey = `kb/${Date.now()}-${existingDocId}-${safeTitle}.txt`
1565+
const customKey = `kb/${buildStorageKeySegment(`${Date.now()}-${existingDocId}-`, `${safeTitle}.txt`)}`
15651566

15661567
const fileInfo = await StorageService.uploadFile({
15671568
file: contentBuffer,

apps/sim/lib/knowledge/documents/document-processor.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import {
2525
getKnowledgeOpaqueModelInputRegistry,
2626
} from '@/lib/knowledge/model-input-provenance'
2727
import { StorageService } from '@/lib/uploads'
28+
import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key'
2829
import { isInternalFileUrl } from '@/lib/uploads/utils/file-utils'
2930
import { downloadFileFromUrl } from '@/lib/uploads/utils/file-utils.server'
3031
import { MAX_FILE_SIZE } from '@/lib/uploads/utils/validation'
@@ -362,8 +363,7 @@ async function handleFileForOCR(
362363

363364
const timestamp = Date.now()
364365
const uniqueId = randomBytes(8).toString('hex')
365-
const safeFileName = filename.replace(/[^a-zA-Z0-9.-]/g, '_')
366-
const customKey = `kb/${timestamp}-${uniqueId}-${safeFileName}`
366+
const customKey = `kb/${buildStorageKeySegment(`${timestamp}-${uniqueId}-`, filename)}`
367367

368368
const cloudResult = await StorageService.uploadFile({
369369
file: buffer,
@@ -659,8 +659,10 @@ async function processChunk(
659659
try {
660660
const timestamp = Date.now()
661661
const uniqueId = randomBytes(8).toString('hex')
662-
const safeFileName = filename.replace(/[^a-zA-Z0-9.-]/g, '_')
663-
const chunkKey = `kb/${timestamp}-${uniqueId}-chunk${chunkIndex + 1}-${safeFileName}`
662+
const chunkKey = `kb/${buildStorageKeySegment(
663+
`${timestamp}-${uniqueId}-chunk${chunkIndex + 1}-`,
664+
filename
665+
)}`
664666

665667
// No metadata: these chunks are ephemeral OCR artifacts (deleted in the
666668
// finally below) that are fetched via a direct presigned URL, never through

apps/sim/lib/mothership/inbox/executor.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import * as agentmail from '@/lib/mothership/inbox/agentmail-client'
2424
import { formatEmailAsMessage } from '@/lib/mothership/inbox/format'
2525
import { sendInboxResponse } from '@/lib/mothership/inbox/response'
2626
import type { AgentMailAttachment } from '@/lib/mothership/inbox/types'
27+
import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key'
2728
import { uploadFile } from '@/lib/uploads/core/storage-service'
2829
import { createFileContent, type MessageContent } from '@/lib/uploads/utils/file-utils'
2930
import { checkWorkspaceAccess, getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
@@ -482,7 +483,10 @@ async function downloadAttachmentContents(
482483
const fileContent = createFileContent(buffer, attachment.content_type)
483484
if (!fileContent) return null
484485

485-
const storageKey = `copilot/${Date.now()}-${attachment.attachment_id}-${attachment.filename}`
486+
const storageKey = `copilot/${buildStorageKeySegment(
487+
`${Date.now()}-${attachment.attachment_id}-`,
488+
attachment.filename
489+
)}`
486490
const uploaded = await uploadFile({
487491
file: buffer,
488492
fileName: attachment.filename,

apps/sim/lib/uploads/upload-session/service.test.ts

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,15 @@ vi.mock('@/lib/billing/storage', () => ({
3434
resolveStorageBillingContext: mockResolveBillingContext,
3535
}))
3636

37-
vi.mock('@/lib/uploads/contexts/workspace', () => ({
38-
generateWorkspaceFileKey: vi.fn(
39-
(workspaceId: string, fileName: string) => `workspace/${workspaceId}/final-${fileName}`
40-
),
41-
}))
37+
vi.mock('@/lib/uploads/contexts/workspace', async () => {
38+
const { buildStorageKeySegment } = await import('@/lib/uploads/core/storage-key')
39+
return {
40+
generateWorkspaceFileKey: vi.fn(
41+
(workspaceId: string, fileName: string) =>
42+
`workspace/${workspaceId}/${buildStorageKeySegment('final-', fileName)}`
43+
),
44+
}
45+
})
4246

4347
vi.mock('@/lib/uploads/upload-session/cleanup', () => ({
4448
maybeCleanupLocalUploadArtifacts: vi.fn().mockResolvedValue({ scanned: 0, removed: 0 }),
@@ -56,6 +60,7 @@ vi.mock('@/lib/uploads/upload-session/provider', () => ({
5660
uploadStorageProvider: vi.fn(() => 's3'),
5761
}))
5862

63+
import { LOCAL_UPLOAD_METADATA_SUFFIX } from '@/lib/uploads/core/storage-key'
5964
import {
6065
abortUploadSession,
6166
assertUploadSessionAuthBinding,
@@ -137,6 +142,42 @@ describe('upload sessions', () => {
137142
})
138143
})
139144

145+
// Local storage stores an object's metadata sidecar beside it, under the
146+
// object's own name, so the whole key + suffix must fit one path component.
147+
// Three purposes built their key by hand and admitted a 255-character name
148+
// straight into it: the session was created, its transfer URL issued, and
149+
// every request against it then failed with an unclassifiable 500.
150+
it.each([
151+
['workspace_file', {}],
152+
['knowledge_document', { knowledgeBaseId: 'kb-1' }],
153+
['table_import', {}],
154+
['profile_picture', {}],
155+
['workspace_logo', {}],
156+
['mothership_attachment', {}],
157+
['execution_attachment', { workflowId: 'workflow-1', executionId: 'execution-1' }],
158+
])('bounds the %s key so its local sidecar still fits', async (purpose, extra) => {
159+
dbChainMockFns.returning.mockResolvedValue([uploadRow({ purpose })])
160+
161+
await createUploadSession({
162+
id: 'upload-1',
163+
workspaceId: WORKSPACE_ID,
164+
userId: 'user-1',
165+
principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' },
166+
purpose: purpose as Parameters<typeof createUploadSession>[0]['purpose'],
167+
fileName: `${'a'.repeat(251)}.txt`,
168+
contentType: 'text/plain',
169+
fileSize: 4,
170+
localOrigin: 'http://localhost:3000',
171+
...extra,
172+
} as Parameters<typeof createUploadSession>[0])
173+
174+
const { finalKey } = dbChainMockFns.values.mock.calls[0][0]
175+
const lastComponent = finalKey.slice(finalKey.lastIndexOf('/') + 1)
176+
expect(
177+
Buffer.byteLength(`${lastComponent}${LOCAL_UPLOAD_METADATA_SUFFIX}`, 'utf-8')
178+
).toBeLessThanOrEqual(255)
179+
})
180+
140181
it('allocates distinct keys for same-named execution attachments', async () => {
141182
dbChainMockFns.returning
142183
.mockResolvedValueOnce([

apps/sim/lib/uploads/upload-session/service.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { OrchestrationError } from '@/lib/core/orchestration/types'
1515
import { generateUniqueExecutionFileKey } from '@/lib/uploads/contexts/execution/utils'
1616
import { generateKnowledgeBaseFileKey } from '@/lib/uploads/contexts/knowledge-base/knowledge-base-file-manager'
1717
import { generateWorkspaceFileKey } from '@/lib/uploads/contexts/workspace'
18+
import { buildStorageKeySegment } from '@/lib/uploads/core/storage-key'
1819
import {
1920
MAX_KNOWLEDGE_DOCUMENT_FILE_SIZE,
2021
MAX_WORKSPACE_FILE_SIZE,
@@ -41,7 +42,6 @@ import type {
4142
UploadStorageProvider,
4243
UploadTransferMethod,
4344
} from '@/lib/uploads/upload-session/types'
44-
import { sanitizeFileName } from '@/executor/constants'
4545

4646
export const UPLOAD_SESSION_PUT_MAX_BYTES = 50 * 1024 * 1024
4747
export const UPLOAD_SESSION_PART_SIZE = 8 * 1024 * 1024
@@ -1200,7 +1200,7 @@ function resolveUploadStorage(
12001200
case 'table_import':
12011201
return {
12021202
storageContext: 'table-import',
1203-
finalKey: `table-import/${params.workspaceId}/${id}/${sanitizeFileName(params.fileName)}`,
1203+
finalKey: `table-import/${params.workspaceId}/${id}/${buildStorageKeySegment('', params.fileName)}`,
12041204
}
12051205
case 'knowledge_document':
12061206
return {
@@ -1210,12 +1210,12 @@ function resolveUploadStorage(
12101210
case 'profile_picture':
12111211
return {
12121212
storageContext: 'profile-pictures',
1213-
finalKey: `profile-pictures/${id}-${sanitizeFileName(params.fileName)}`,
1213+
finalKey: `profile-pictures/${buildStorageKeySegment(`${id}-`, params.fileName)}`,
12141214
}
12151215
case 'workspace_logo':
12161216
return {
12171217
storageContext: 'workspace-logos',
1218-
finalKey: `workspace-logos/${params.workspaceId}/${id}-${sanitizeFileName(params.fileName)}`,
1218+
finalKey: `workspace-logos/${params.workspaceId}/${buildStorageKeySegment(`${id}-`, params.fileName)}`,
12191219
}
12201220
case 'mothership_attachment':
12211221
return {

0 commit comments

Comments
 (0)