Skip to content

Commit 94fa43b

Browse files
waleedlatif1waleedlatif1
committed
fix(uploads): restore archive extraction folder parity
Archive extraction into workspace files/ was rewritten onto the authorized application-operation boundary, and three behavioral regressions came with that move. Together they broke every archive containing a subdirectory, and 100% of copilot extract() calls (materialize-file always passes rootFolderSegments: [baseName], and its catch only handles ArchiveError). 1. Non-canonical folder path. The extractor joined the folder segments with "/" and passed the result as `path` to createWorkspaceFileFolderOperation. That path reaches requireNonRootFolderPath -> parseFolderPath, which requires a leading "/" and byte-for-byte canonical per-segment encoding, so "bundle/data" threw FolderPathError before anything was written — and a folder name containing a space or a reserved character would still have thrown after merely prefixing a slash. 2. exactName: true. createWorkspaceFileFromBuffer was told to demand the exact leaf name, which sets maxAttempts = 1 and raises FileConflictError when the name already exists. The extractor's rollback then deleted every file written so far, so one colliding name destroyed the whole extraction. Reachable today for flat archives through the unzip action of POST /api/tools/file/manage. Restored to auto-suffixing via allocateUniqueWorkspaceFileName. 3. Wrong folder primitive. createWorkspaceFileFolderAtPath creates exactly one leaf, conflicts on an existing path, and requires the parent to exist already. The extractor never creates intermediates and caches by full path, so the first nested entry asked for a folder whose parent was never created. The correct semantics are ensureWorkspaceFileFolderPath: walk every segment, reuse what exists, create only what is missing. Rather than bypass the operation boundary by calling the manager primitive directly, this adds ensureWorkspaceFileFolderPathOperation — an authorized application use case under files.folders.create that expresses "ensure this whole chain exists" — and routes the extractor through it with raw decoded segments, so no path string is built and no encoding can be malformed. archive.test.ts previously mocked the folder operation and asserted the broken shape (path: 'bundle'), which is why this shipped. The suite now fakes the workspace-file store in memory while enforcing the real rules: folder paths run through the production parseFolderPath family, the create-one-leaf operation conflicts and requires a parent, and exactName governs conflict vs auto-suffix. Nested, reuse, encoded-name, and collision cases are covered and each fails against the pre-fix code.
1 parent 6abc1b4 commit 94fa43b

3 files changed

Lines changed: 246 additions & 27 deletions

File tree

‎apps/sim/lib/uploads/archive.test.ts‎

Lines changed: 193 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,34 @@ import { Buffer } from 'buffer'
55
import JSZip from 'jszip'
66
import { beforeEach, describe, expect, it, vi } from 'vitest'
77

8-
const { mockEnsureFolder, mockUpload, mockDelete } = vi.hoisted(() => ({
9-
mockEnsureFolder: vi.fn(),
8+
/**
9+
* The workspace-file store is faked in memory rather than stubbed, because the
10+
* defects this suite guards against live in the *contract* between the extractor
11+
* and the folder/file layers, not in the extractor's own arithmetic. The fake
12+
* therefore enforces the real rules:
13+
*
14+
* - folder paths are validated with the production {@link parseFolderPath} family,
15+
* so a non-canonical path (no leading slash, unencoded segment) throws exactly
16+
* as `requireNonRootFolderPath` does in `workspace-file-folder-manager`;
17+
* - `createWorkspaceFileFolderOperation` creates ONE leaf and rejects an existing
18+
* path or a missing parent, mirroring `createWorkspaceFileFolderAtPath`;
19+
* - `exactName: true` throws `FileConflictError` on a duplicate leaf name, while
20+
* `exactName: false` auto-suffixes, mirroring `uploadWorkspaceFile`.
21+
*/
22+
const { store, mockUpload, mockDelete, mockCreateFolder, mockEnsureFolder } = vi.hoisted(() => ({
23+
store: {
24+
folderIdByPath: new Map<string, string>(),
25+
fileKeys: new Set<string>(),
26+
sequence: 0,
27+
},
1028
mockUpload: vi.fn(),
1129
mockDelete: vi.fn(),
30+
mockCreateFolder: vi.fn(),
31+
mockEnsureFolder: vi.fn(),
1232
}))
1333
vi.mock('@/lib/workspace-files/application/workspace-file-folders', () => ({
14-
createWorkspaceFileFolderOperation: {
15-
execute: mockEnsureFolder,
16-
},
34+
createWorkspaceFileFolderOperation: { execute: mockCreateFolder },
35+
ensureWorkspaceFileFolderPathOperation: { execute: mockEnsureFolder },
1736
}))
1837
vi.mock('@/lib/workspace-files/application/create-workspace-file', () => ({
1938
createWorkspaceFileFromBuffer: {
@@ -26,6 +45,7 @@ vi.mock('@/lib/workspace-files/application/delete-workspace-file', () => ({
2645
},
2746
}))
2847

48+
import { buildFolderPath, requireNonRootFolderPath } from '@/lib/folders/paths'
2949
import {
3050
decompressArchiveBufferToWorkspaceFiles,
3151
MAX_ARCHIVE_CENTRAL_DIR_EXTRA_BYTES,
@@ -83,21 +103,88 @@ function craftCentralDirectory(records: number, extraPerRecord: number): Buffer
83103
return buffer
84104
}
85105

106+
/** Mirrors `allocateUniqueWorkspaceFileName`'s " (n)" suffixing. */
107+
function allocateUniqueName(folderKey: string, name: string): string {
108+
const dot = name.lastIndexOf('.')
109+
const base = dot > 0 ? name.slice(0, dot) : name
110+
const extension = dot > 0 ? name.slice(dot) : ''
111+
for (let attempt = 1; ; attempt++) {
112+
const candidate = `${base} (${attempt})${extension}`
113+
if (!store.fileKeys.has(`${folderKey}|${candidate}`)) return candidate
114+
}
115+
}
116+
117+
/** Pre-seeds an already-existing workspace file so a later leaf name collides. */
118+
function seedExistingFile(folderId: string | null, name: string): void {
119+
store.fileKeys.add(`${folderId ?? ''}|${name}`)
120+
}
121+
86122
beforeEach(() => {
87123
vi.clearAllMocks()
88-
mockEnsureFolder.mockResolvedValue({ folder: { id: 'folder_1' } })
124+
store.folderIdByPath.clear()
125+
store.fileKeys.clear()
126+
store.sequence = 0
127+
128+
mockCreateFolder.mockImplementation(async ({ input }: { input: { path: string } }) => {
129+
const segments = requireNonRootFolderPath(input.path)
130+
if (store.folderIdByPath.has(input.path)) {
131+
throw new Error(`A folder named "${segments[segments.length - 1]}" already exists`)
132+
}
133+
const parentPath = buildFolderPath(segments.slice(0, -1))
134+
if (parentPath !== '/' && !store.folderIdByPath.has(parentPath)) {
135+
throw new Error('Parent folder not found')
136+
}
137+
const id = `folder_${++store.sequence}`
138+
store.folderIdByPath.set(input.path, id)
139+
return { folder: { id, path: input.path } }
140+
})
141+
142+
mockEnsureFolder.mockImplementation(async ({ input }: { input: { pathSegments: string[] } }) => {
143+
let folderId: string | null = null
144+
const walked: string[] = []
145+
for (const segment of input.pathSegments) {
146+
walked.push(segment)
147+
const path = buildFolderPath(walked)
148+
const existing = store.folderIdByPath.get(path)
149+
if (existing) {
150+
folderId = existing
151+
continue
152+
}
153+
folderId = `folder_${++store.sequence}`
154+
store.folderIdByPath.set(path, folderId)
155+
}
156+
return { folderId }
157+
})
158+
89159
mockDelete.mockResolvedValue(undefined)
90160
mockUpload.mockImplementation(
91-
async ({ input }: { input: { content: Buffer; name: string } }) => ({
92-
file: {
93-
id: `f_${input.name}`,
94-
name: input.name,
95-
url: `/api/files/serve/${input.name}`,
96-
key: `workspace/ws/${input.name}`,
97-
size: input.content.length,
98-
type: 'text/plain',
99-
},
100-
})
161+
async ({
162+
input,
163+
}: {
164+
input: { content: Buffer; name: string; folderId?: string | null; exactName: boolean }
165+
}) => {
166+
const folderKey = input.folderId ?? ''
167+
let name = input.name
168+
if (store.fileKeys.has(`${folderKey}|${name}`)) {
169+
if (input.exactName) {
170+
const conflict = new Error(`A file named "${name}" already exists`)
171+
conflict.name = 'FileConflictError'
172+
throw conflict
173+
}
174+
name = allocateUniqueName(folderKey, name)
175+
}
176+
store.fileKeys.add(`${folderKey}|${name}`)
177+
return {
178+
file: {
179+
id: `f_${name}`,
180+
name,
181+
url: `/api/files/serve/${name}`,
182+
key: `workspace/ws/${name}`,
183+
size: input.content.length,
184+
type: 'text/plain',
185+
},
186+
}
187+
}
101188
)
102189
})
103190

@@ -118,11 +205,99 @@ describe('decompressArchiveBufferToWorkspaceFiles', () => {
118205
expect(leafNames).toEqual(['report.txt', 'sheet.csv'])
119206
// Entries are rooted under the archive's folder; nested paths are preserved.
120207
expect(mockEnsureFolder).toHaveBeenCalledWith(
121-
expect.objectContaining({ input: { workspaceId: 'ws', path: 'bundle' } })
208+
expect.objectContaining({ input: { workspaceId: 'ws', pathSegments: ['bundle'] } })
122209
)
123210
expect(mockEnsureFolder).toHaveBeenCalledWith(
124-
expect.objectContaining({ input: { workspaceId: 'ws', path: 'bundle/data' } })
211+
expect.objectContaining({ input: { workspaceId: 'ws', pathSegments: ['bundle', 'data'] } })
125212
)
213+
// Every folder in the chain is materialized, intermediates included.
214+
expect([...store.folderIdByPath.keys()].sort()).toEqual(['/bundle', '/bundle/data'])
215+
})
216+
217+
it('creates intermediate folders for a deeply nested archive', async () => {
218+
// `createWorkspaceFileFolderAtPath` semantics would ask for the full leaf path
219+
// whose parents were never created and fail with "Parent folder not found";
220+
// extraction must ensure the whole chain instead.
221+
const buffer = await buildZip({ 'src/deep/nested/leaf.txt': 'x' })
222+
223+
const result = await decompressArchiveBufferToWorkspaceFiles(buffer, {
224+
workspaceId: 'ws',
225+
principal: TEST_PRINCIPAL,
226+
rootFolderSegments: ['bundle'],
227+
})
228+
229+
expect(result.extracted).toHaveLength(1)
230+
expect([...store.folderIdByPath.keys()].sort()).toEqual([
231+
'/bundle',
232+
'/bundle/src',
233+
'/bundle/src/deep',
234+
'/bundle/src/deep/nested',
235+
])
236+
expect(mockUpload.mock.calls[0][0].input.folderId).toBe(
237+
store.folderIdByPath.get('/bundle/src/deep/nested')
238+
)
239+
})
240+
241+
it('reuses a folder that already exists instead of failing on conflict', async () => {
242+
// Two entries in the same directory, plus a directory that a previous
243+
// extraction already created — neither may raise a folder conflict.
244+
store.folderIdByPath.set('/bundle', 'preexisting-folder')
245+
const buffer = await buildZip({ 'top.txt': 't', 'docs/a.txt': 'a', 'docs/b.txt': 'b' })
246+
247+
const result = await decompressArchiveBufferToWorkspaceFiles(buffer, {
248+
workspaceId: 'ws',
249+
principal: TEST_PRINCIPAL,
250+
rootFolderSegments: ['bundle'],
251+
})
252+
253+
expect(result.extracted).toHaveLength(3)
254+
expect([...store.folderIdByPath.keys()].sort()).toEqual(['/bundle', '/bundle/docs'])
255+
expect(store.folderIdByPath.get('/bundle')).toBe('preexisting-folder')
256+
const folderIdByFileName = new Map(
257+
mockUpload.mock.calls.map(([args]) => [args.input.name, args.input.folderId])
258+
)
259+
expect(folderIdByFileName.get('top.txt')).toBe('preexisting-folder')
260+
expect(folderIdByFileName.get('a.txt')).toBe(store.folderIdByPath.get('/bundle/docs'))
261+
expect(folderIdByFileName.get('b.txt')).toBe(store.folderIdByPath.get('/bundle/docs'))
262+
})
263+
264+
it('extracts into a folder whose name needs path encoding', async () => {
265+
// A space (and other reserved characters) must never be handed to the folder
266+
// layer as a raw path segment — `parseFolderPath` round-trip-checks the
267+
// encoding and rejects "my report" while accepting "my%20report".
268+
const buffer = await buildZip({ 'my report/q1 & q2.txt': 'x' })
269+
270+
const result = await decompressArchiveBufferToWorkspaceFiles(buffer, {
271+
workspaceId: 'ws',
272+
principal: TEST_PRINCIPAL,
273+
rootFolderSegments: ['bundle v2'],
274+
})
275+
276+
expect(result.extracted).toHaveLength(1)
277+
expect([...store.folderIdByPath.keys()].sort()).toEqual([
278+
'/bundle%20v2',
279+
'/bundle%20v2/my%20report',
280+
])
281+
expect(mockUpload.mock.calls[0][0].input.name).toBe('q1 & q2.txt')
282+
})
283+
284+
it('auto-suffixes a leaf whose name already exists instead of rolling back', async () => {
285+
// One colliding name must not destroy an otherwise valid extraction: the
286+
// upload layer allocates a unique name, nothing is deleted, and every entry
287+
// still lands.
288+
seedExistingFile(null, 'report.txt')
289+
const buffer = await buildZip({ 'report.txt': 'hi', 'other.txt': 'yo' })
290+
291+
const result = await decompressArchiveBufferToWorkspaceFiles(buffer, {
292+
workspaceId: 'ws',
293+
principal: TEST_PRINCIPAL,
294+
})
295+
296+
expect(result.extracted.map((file) => file.name).sort()).toEqual([
297+
'other.txt',
298+
'report (1).txt',
299+
])
300+
expect(mockDelete).not.toHaveBeenCalled()
126301
})
127302

128303
it('marks extracted files unknown when an archive has secret provenance', async () => {

‎apps/sim/lib/uploads/archive.ts‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type { WorkspaceFileSecretProvenance } from '@/lib/uploads/contexts/works
77
import { getFileExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils'
88
import { createWorkspaceFileFromBuffer } from '@/lib/workspace-files/application/create-workspace-file'
99
import { deleteWorkspaceFileOperation } from '@/lib/workspace-files/application/delete-workspace-file'
10-
import { createWorkspaceFileFolderOperation } from '@/lib/workspace-files/application/workspace-file-folders'
10+
import { ensureWorkspaceFileFolderPathOperation } from '@/lib/workspace-files/application/workspace-file-folders'
1111
import type { UserFile } from '@/executor/types'
1212

1313
/**
@@ -362,15 +362,15 @@ export async function decompressArchiveBufferToWorkspaceFiles(
362362
const folderKey = folderSegments.join('/')
363363
let folderId = folderIdCache.get(folderKey)
364364
if (folderId === undefined) {
365-
if (folderSegments.length === 0) {
366-
folderId = null
367-
} else {
368-
const result = await createWorkspaceFileFolderOperation.execute({
365+
// Ensure-semantics, not create-semantics: an archive addresses every folder
366+
// by its full chain, so intermediates must be materialized and any folder
367+
// that already exists (from a sibling entry or an earlier extraction) reused.
368+
folderId = (
369+
await ensureWorkspaceFileFolderPathOperation.execute({
369370
principal,
370-
input: { workspaceId, path: folderSegments.join('/') },
371+
input: { workspaceId, pathSegments: folderSegments },
371372
})
372-
folderId = result.folder.id
373-
}
373+
).folderId
374374
folderIdCache.set(folderKey, folderId)
375375
}
376376

@@ -384,7 +384,9 @@ export async function decompressArchiveBufferToWorkspaceFiles(
384384
name: leafName,
385385
contentType: mimeType,
386386
folderId,
387-
exactName: true,
387+
// Auto-suffix on collision: one leaf name that already exists must not
388+
// roll back an otherwise valid extraction.
389+
exactName: false,
388390
secretProvenance: extractedSecretProvenance,
389391
},
390392
})

‎apps/sim/lib/workspace-files/application/workspace-file-folders.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
createWorkspaceFileFolder,
1010
createWorkspaceFileFolderAtPath,
1111
deleteWorkspaceFileFolderByPath,
12+
ensureWorkspaceFileFolderPath,
1213
listWorkspaceFileFolders,
1314
loadWorkspaceFileOperationContext,
1415
relocateWorkspaceFileFolderByPath,
@@ -46,6 +47,17 @@ export interface CreateWorkspaceFileFolderResult {
4647
folder: WorkspaceFileFolderRecord
4748
}
4849

50+
export interface EnsureWorkspaceFileFolderPathInput {
51+
workspaceId: string
52+
/** Decoded folder names, outermost first. An empty list resolves to the root. */
53+
pathSegments: string[]
54+
}
55+
56+
export interface EnsureWorkspaceFileFolderPathResult {
57+
/** Id of the deepest folder, or `null` when the path resolves to the root. */
58+
folderId: string | null
59+
}
60+
4961
export interface UpdateWorkspaceFileFolderInput {
5062
workspaceId: string
5163
folderId?: string
@@ -148,6 +160,22 @@ async function executeCreateWorkspaceFileFolder(args: {
148160
return { folder }
149161
}
150162

163+
async function executeEnsureWorkspaceFileFolderPath(args: {
164+
principal: Parameters<typeof resolvePrincipalAttribution>[0]
165+
input: EnsureWorkspaceFileFolderPathInput
166+
context: FolderOperationContext
167+
}): Promise<EnsureWorkspaceFileFolderPathResult> {
168+
const attribution = resolvePrincipalAttribution(args.principal, {
169+
workspaceBillingOwnerUserId: args.context.billedAccountUserId,
170+
})
171+
const folderId = await ensureWorkspaceFileFolderPath({
172+
workspaceId: args.context.workspaceId,
173+
userId: attribution.attributedUserId,
174+
pathSegments: args.input.pathSegments,
175+
})
176+
return { folderId }
177+
}
178+
151179
async function executeUpdateWorkspaceFileFolder(args: {
152180
input: UpdateWorkspaceFileFolderInput
153181
context: FolderOperationContext
@@ -240,6 +268,20 @@ export const createWorkspaceFileFolderOperation = defineAuthorizedWorkspaceFileU
240268
},
241269
})
242270

271+
/**
272+
* Idempotently materializes a whole folder chain, reusing every folder that already
273+
* exists and creating only the missing ones. Unlike {@link createWorkspaceFileFolderOperation}
274+
* — which creates exactly one leaf and fails on an existing path or a missing parent —
275+
* this is the primitive for writers that materialize a tree (archive extraction, workspace
276+
* import), where intermediate folders and repeat runs are expected rather than exceptional.
277+
*/
278+
export const ensureWorkspaceFileFolderPathOperation = defineAuthorizedWorkspaceFileUseCase({
279+
operation: fileOperations.createFolder,
280+
resolveContext: (args: { input: EnsureWorkspaceFileFolderPathInput }) =>
281+
resolveFolderContext(args),
282+
execute: executeEnsureWorkspaceFileFolderPath,
283+
})
284+
243285
export const updateWorkspaceFileFolderOperation = defineAuthorizedWorkspaceFileUseCase({
244286
operation: fileOperations.updateFolder,
245287
resolveContext: (args: { input: UpdateWorkspaceFileFolderInput }) => resolveFolderContext(args),

0 commit comments

Comments
 (0)