From 4b22934df2e7483fe13e1f5b757f22193c5b951d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 10:22:54 -0700 Subject: [PATCH 1/3] fix(files): stop saving fetched URLs to Files and look names up by the unique index --- apps/sim/lib/internal/file/parser.test.ts | 20 +- apps/sim/lib/internal/file/parser.ts | 16 +- .../__integration__/file-names.integration.ts | 188 ++++++++++++++++++ .../workspace/fetch-external-url.test.ts | 92 +-------- .../contexts/workspace/fetch-external-url.ts | 67 +------ .../workspace-file-folder-manager.ts | 11 +- .../workspace/workspace-file-manager.ts | 20 +- .../src/mocks/workspace-file-folders.mock.ts | 2 + .../src/mocks/workspace-uploads.mock.ts | 6 +- 9 files changed, 236 insertions(+), 186 deletions(-) create mode 100644 apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts diff --git a/apps/sim/lib/internal/file/parser.test.ts b/apps/sim/lib/internal/file/parser.test.ts index 242d32ed9f3..8ad033d6d27 100644 --- a/apps/sim/lib/internal/file/parser.test.ts +++ b/apps/sim/lib/internal/file/parser.test.ts @@ -27,10 +27,7 @@ import { } from '@sim/testing/mocks/uploads-execution.mock' import { uploadsMetadataMock } from '@sim/testing/mocks/uploads-metadata.mock' import { uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock' -import { - workspaceFileManagerMock, - workspaceFileManagerMockFns, -} from '@sim/testing/mocks/workspace-file-manager.mock' +import { workspaceFileManagerMock } from '@sim/testing/mocks/workspace-file-manager.mock' import { workspaceFileSecretProvenanceMock, workspaceFileSecretProvenanceMockFns, @@ -181,21 +178,8 @@ vi.mock('fs/promises', () => ({ })) const { mockGetStorageProvider, mockIsUsingCloudStorage } = uploadsMockFns -const { mockUploadWorkspaceFile } = workspaceFileManagerMockFns const { mockGetBoundWorkspaceFileSecretProvenance } = workspaceFileSecretProvenanceMockFns -mockUploadWorkspaceFile.mockImplementation( - async (workspaceId: string, _userId: string, _buffer: Buffer, fileName: string) => ({ - id: 'wf_test', - name: fileName, - size: 0, - type: 'application/octet-stream', - url: `/api/files/serve/${workspaceId}/${fileName}`, - key: `${workspaceId}/${fileName}`, - context: 'workspace', - }) -) - import { fileParseBodySchema } from '@/lib/api/contracts/storage-transfer' import { executeFileParserOperation } from '@/lib/internal/file/parser' import { createWorkspaceFileDelegatedPrincipal } from '@/lib/workspace-files/application/delegated-principal' @@ -658,7 +642,6 @@ describe('file parser operation', () => { }) ) mockIsSupportedFileType.mockReturnValue(false) - permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write') const req = createMockRequest('POST', { filePath: [ @@ -686,7 +669,6 @@ describe('file parser operation', () => { '203.0.113.10', expect.any(Object) ) - expect(mockUploadWorkspaceFile).toHaveBeenCalledTimes(2) expect(storageServiceMockFns.mockDownloadFile).not.toHaveBeenCalled() }) diff --git a/apps/sim/lib/internal/file/parser.ts b/apps/sim/lib/internal/file/parser.ts index 43714100d5c..3071159b122 100644 --- a/apps/sim/lib/internal/file/parser.ts +++ b/apps/sim/lib/internal/file/parser.ts @@ -35,10 +35,7 @@ import { } from '@/lib/internal/file/operations' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' import { uploadExecutionFile } from '@/lib/uploads/contexts/execution' -import { - ExternalUrlValidationError, - fetchExternalUrlToWorkspace, -} from '@/lib/uploads/contexts/workspace' +import { ExternalUrlValidationError, fetchExternalUrl } from '@/lib/uploads/contexts/workspace' import { getBoundWorkspaceFileSecretProvenance, type WorkspaceFileSecretProvenance, @@ -512,7 +509,7 @@ function assertParsedContentWithinLimit(content: string, maxBytes?: number): str * Validate file path for security - prevents null byte injection and path traversal attacks. * * External URLs (`http`/`https`) are fetched over HTTP — with SSRF protection applied - * downstream in `fetchExternalUrlToWorkspace` (DNS resolution + private/reserved IP blocking) + * downstream in `fetchExternalUrl` (DNS resolution + private/reserved IP blocking) * — and are never resolved against the filesystem, so `..`/`~` are legal URL content and must * not be rejected. Providers such as Slack routinely emit slugs containing a literal `...`. * @@ -560,8 +557,8 @@ function validateFilePath(filePath: string): { isValid: boolean; error?: string * * Always fetches the URL fresh — there is no filename-based dedup. Distinct URLs * commonly share a path tail (e.g. every Slack clipboard paste is `image.png`), - * so keying a cache by filename returns stale bytes. `fetchExternalUrlToWorkspace` - * delegates to `uploadWorkspaceFile`, which suffix-disambiguates collisions on save. + * so keying a cache by filename returns stale bytes. The fetched bytes are never saved + * to workspace Files; with an execution context they are kept as an execution file only. * * URLs for our workspace and execution storage resolve through the authorized canonical * read path, keeping stored provenance bound to the same bytes the parser reads. @@ -647,11 +644,8 @@ async function handleExternalUrl( ) } - const { filename, buffer, mimeType } = await fetchExternalUrlToWorkspace({ + const { filename, buffer, mimeType } = await fetchExternalUrl({ url, - userId, - workspaceId: workspaceId || undefined, - saveToWorkspace: Boolean(workspaceId), headers, signal, maxDownloadBytes, diff --git a/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts b/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts new file mode 100644 index 00000000000..59f1da8ee4e --- /dev/null +++ b/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts @@ -0,0 +1,188 @@ +/** Real PostgreSQL name allocation and name lookups for workspace files, plus the URL fetch path. */ +import { mkdtempSync } from 'node:fs' +import { rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import path from 'node:path' +import { db, dbFor } from '@sim/db' +import { organization, user, workspace, workspaceFiles } from '@sim/db/schema' +import { generateId } from '@sim/utils/id' +import { and, eq, inArray, isNull, sql } from 'drizzle-orm' +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' + +const fixtureStorage = vi.hoisted(() => ({ root: '' })) +vi.mock('@/lib/uploads/core/setup.server', () => ({ + get UPLOAD_DIR_SERVER() { + return fixtureStorage.root + }, +})) + +import { fileParseBodySchema } from '@/lib/api/contracts/storage-transfer' +import * as inputValidation from '@/lib/core/security/input-validation.server' +import { executeFileParserOperation } from '@/lib/internal/file/parser' +import { + createKnowledgeAclFixtureIds, + seedKnowledgeAclFixture, +} from '@/lib/knowledge/__integration__/seed-source-access-fixture' +import { + createWorkspaceFileFolder, + fileNameExistsInWorkspaceFolder, + workspaceFileNameFolderCondition, +} from '@/lib/uploads/contexts/workspace/workspace-file-folder-manager' +import { + getWorkspaceFileByName, + uploadWorkspaceFile, +} from '@/lib/uploads/contexts/workspace/workspace-file-manager' +import { createWorkspaceFileDelegatedPrincipal } from '@/lib/workspace-files/application/delegated-principal' + +describe('workspace file names in PostgreSQL', () => { + const fixtures: ReturnType[] = [] + + beforeAll(() => { + fixtureStorage.root = mkdtempSync(path.join(tmpdir(), 'sim-file-names-')) + }) + + afterAll(async () => { + vi.restoreAllMocks() + for (const ids of fixtures) { + await db.delete(workspace).where(eq(workspace.id, ids.workspaceId)) + await db.delete(organization).where(eq(organization.id, ids.organizationId)) + await db.delete(user).where(inArray(user.id, [ids.aliceId, ids.bobId])) + } + await rm(fixtureStorage.root, { recursive: true, force: true }) + await Promise.all([db.$client.end(), dbFor('cleanup').$client.end()]) + }) + + async function seedWorkspace() { + const ids = createKnowledgeAclFixtureIds() + fixtures.push(ids) + await seedKnowledgeAclFixture(ids) + return ids + } + + function upload(workspaceId: string, userId: string, name: string, folderId?: string | null) { + return uploadWorkspaceFile(workspaceId, userId, Buffer.from(name), name, 'text/plain', { + folderId, + notifyWorkspaceChange: false, + }) + } + + it('parses an external URL without saving a copy to workspace Files', async () => { + const fixture = await seedWorkspace() + const url = 'https://example.com/page.txt' + vi.spyOn(inputValidation, 'validateUrlWithDNS').mockResolvedValue({ + isValid: true, + resolvedIP: '203.0.113.10', + originalHostname: new URL(url).hostname, + }) + vi.spyOn(inputValidation, 'secureFetchWithPinnedIP').mockImplementation(async () => { + const response = new Response('fetched page body') + return { + ok: response.ok, + status: response.status, + statusText: response.statusText, + headers: new inputValidation.SecureFetchHeaders({ 'content-type': 'text/plain' }), + body: response.body, + text: () => response.text(), + json: () => response.json(), + arrayBuffer: () => response.arrayBuffer(), + } + }) + + const response = await executeFileParserOperation( + fileParseBodySchema.parse({ filePath: url, workspaceId: fixture.workspaceId }), + { + principal: createWorkspaceFileDelegatedPrincipal({ + serviceId: 'executor', + subjectUserId: fixture.aliceId, + workspaceId: fixture.workspaceId, + delegationId: generateId(), + }), + workspaceId: fixture.workspaceId, + workflowId: generateId(), + attributedUserId: fixture.aliceId, + fileAccessUserId: fixture.aliceId, + } + ) + const body = await response.json() + + expect(response.status).toBe(200) + expect(body.output.content).toContain('fetched page body') + const rows = await db + .select({ id: workspaceFiles.id }) + .from(workspaceFiles) + .where(eq(workspaceFiles.workspaceId, fixture.workspaceId)) + expect(rows).toEqual([]) + }) + + it('scopes name lookups to root or folder through the unique name index', async () => { + const fixture = await seedWorkspace() + const folder = await createWorkspaceFileFolder({ + workspaceId: fixture.workspaceId, + userId: fixture.aliceId, + name: 'Reports', + }) + const rootFile = await upload(fixture.workspaceId, fixture.aliceId, 'root.txt') + const folderFile = await upload(fixture.workspaceId, fixture.aliceId, 'nested.txt', folder.id) + + expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'root.txt', null)).toBe(true) + expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'root.txt', folder.id)).toBe( + false + ) + expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'nested.txt', null)).toBe( + false + ) + expect( + await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'nested.txt', folder.id) + ).toBe(true) + expect((await getWorkspaceFileByName(fixture.workspaceId, 'root.txt'))?.id).toBe(rootFile.id) + expect( + await getWorkspaceFileByName(fixture.workspaceId, 'root.txt', { folderId: folder.id }) + ).toBeNull() + expect( + (await getWorkspaceFileByName(fixture.workspaceId, 'nested.txt', { folderId: folder.id }))?.id + ).toBe(folderFile.id) + expect(await getWorkspaceFileByName(fixture.workspaceId, 'nested.txt')).toBeNull() + + await db.execute(sql` + INSERT INTO ${workspaceFiles} (id, key, user_id, workspace_id, folder_id, context, original_name, content_type) + SELECT 'wf_pad_' || n || '_' || ${fixture.workspaceId}, 'pad/' || n || '/' || ${fixture.workspaceId}, + ${fixture.aliceId}, ${fixture.workspaceId}, CASE WHEN n % 2 = 0 THEN ${folder.id} END, + 'workspace', 'pad-' || n || '.txt', 'text/plain' + FROM generate_series(1, 2000) AS n`) + await db.execute(sql`ANALYZE ${workspaceFiles}`) + + for (const folderId of [null, folder.id]) { + const plan = await db.execute( + sql`EXPLAIN (FORMAT JSON) SELECT id FROM ${workspaceFiles} WHERE ${and( + eq(workspaceFiles.workspaceId, fixture.workspaceId), + eq(workspaceFiles.originalName, 'root.txt'), + eq(workspaceFiles.context, 'workspace'), + workspaceFileNameFolderCondition(folderId), + isNull(workspaceFiles.deletedAt) + )}` + ) + const scan = JSON.stringify(plan[0]['QUERY PLAN']) + expect(scan).toContain('workspace_files_workspace_folder_name_active_unique') + expect(scan).toMatch(/"Index Cond":"[^"]*COALESCE\(folder_id/) + } + }) + + it('falls back to a short-id suffix after 20 numbered copies, including under concurrency', async () => { + const fixture = await seedWorkspace() + await upload(fixture.workspaceId, fixture.aliceId, 'page.html') + for (let n = 1; n <= 20; n++) { + await upload(fixture.workspaceId, fixture.aliceId, `page (${n}).html`) + } + + const next = await upload(fixture.workspaceId, fixture.aliceId, 'page.html') + const concurrent = await Promise.all( + Array.from({ length: 8 }, () => upload(fixture.workspaceId, fixture.aliceId, 'page.html')) + ) + + const shortIdSuffixed = /^page \([A-Za-z0-9_-]{8}\)\.html$/ + expect(next.name).toMatch(shortIdSuffixed) + const names = concurrent.map((file) => file.name) + for (const name of names) expect(name).toMatch(shortIdSuffixed) + expect(new Set([next.name, ...names]).size).toBe(names.length + 1) + }) +}) diff --git a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts b/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts index dc8c401dee0..6d0d02218cf 100644 --- a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts +++ b/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts @@ -6,64 +6,38 @@ * cached module reads at call time, so the tests behave identically whether * the module graph is fresh or reused. */ -import { getMockLogger } from '@sim/testing/mocks/logger.mock' import { afterAll, beforeEach, describe, expect, it, type MockInstance, vi } from 'vitest' import * as inputValidation from '@/lib/core/security/input-validation.server' import { ExternalUrlValidationError, - fetchExternalUrlToWorkspace, + fetchExternalUrl, } from '@/lib/uploads/contexts/workspace/fetch-external-url' -import * as workspaceFileManager from '@/lib/uploads/contexts/workspace/workspace-file-manager' -import * as workspacePermissions from '@/lib/workspaces/permissions/utils' let validateUrlWithDNSSpy: MockInstance let secureFetchWithPinnedIPSpy: MockInstance -let getUserEntityPermissionsSpy: MockInstance -let uploadWorkspaceFileSpy: MockInstance beforeEach(() => { validateUrlWithDNSSpy = vi.spyOn(inputValidation, 'validateUrlWithDNS') secureFetchWithPinnedIPSpy = vi.spyOn(inputValidation, 'secureFetchWithPinnedIP') - getUserEntityPermissionsSpy = vi.spyOn(workspacePermissions, 'getUserEntityPermissions') - uploadWorkspaceFileSpy = vi.spyOn(workspaceFileManager, 'uploadWorkspaceFile') }) function makeResponse(body: string, contentType = 'application/octet-stream'): Response { return new Response(body, { status: 200, headers: { 'content-type': contentType } }) } -const warnMock = getMockLogger('FetchExternalUrl').warn - -describe('fetchExternalUrlToWorkspace', () => { +describe('fetchExternalUrl', () => { beforeEach(() => { validateUrlWithDNSSpy.mockReset() secureFetchWithPinnedIPSpy.mockReset() - getUserEntityPermissionsSpy.mockReset() - uploadWorkspaceFileSpy.mockReset() validateUrlWithDNSSpy.mockResolvedValue({ isValid: true, resolvedIP: '203.0.113.10', }) - getUserEntityPermissionsSpy.mockResolvedValue('write') - uploadWorkspaceFileSpy.mockImplementation( - async (workspaceId: string, _userId: string, _buffer: Buffer, fileName: string) => - ({ - id: `wf_${fileName}`, - name: fileName, - size: 0, - type: 'application/octet-stream', - url: `/api/files/serve/${workspaceId}/${fileName}`, - key: `${workspaceId}/${fileName}`, - context: 'workspace', - }) as unknown as Awaited> - ) }) afterAll(() => { validateUrlWithDNSSpy.mockRestore() secureFetchWithPinnedIPSpy.mockRestore() - getUserEntityPermissionsSpy.mockRestore() - uploadWorkspaceFileSpy.mockRestore() }) it('downloads each URL independently — never dedups by path filename', async () => { @@ -71,23 +45,17 @@ describe('fetchExternalUrlToWorkspace', () => { .mockResolvedValueOnce(makeResponse('first bytes', 'image/png')) .mockResolvedValueOnce(makeResponse('different second bytes', 'image/png')) - const first = await fetchExternalUrlToWorkspace({ + const first = await fetchExternalUrl({ url: 'https://files.slack.com/files-pri/T07-FAAA/download/image.png', - userId: 'user-1', - workspaceId: 'workspace-1', }) - const second = await fetchExternalUrlToWorkspace({ + const second = await fetchExternalUrl({ url: 'https://files.slack.com/files-pri/T07-FBBB/download/image.png', - userId: 'user-1', - workspaceId: 'workspace-1', }) expect(first.filename).toBe('image.png') expect(second.filename).toBe('image.png') expect(first.buffer.toString()).toBe('first bytes') expect(second.buffer.toString()).toBe('different second bytes') - expect(secureFetchWithPinnedIPSpy).toHaveBeenCalledTimes(2) - expect(uploadWorkspaceFileSpy).toHaveBeenCalledTimes(2) }) it('throws ExternalUrlValidationError when SSRF validation fails', async () => { @@ -96,55 +64,9 @@ describe('fetchExternalUrlToWorkspace', () => { error: 'Blocked private IP', }) - await expect( - fetchExternalUrlToWorkspace({ - url: 'http://169.254.169.254/secret', - userId: 'user-1', - }) - ).rejects.toBeInstanceOf(ExternalUrlValidationError) + await expect(fetchExternalUrl({ url: 'http://169.254.169.254/secret' })).rejects.toBeInstanceOf( + ExternalUrlValidationError + ) expect(secureFetchWithPinnedIPSpy).not.toHaveBeenCalled() }) - - it('skips workspace save when user lacks write permission', async () => { - secureFetchWithPinnedIPSpy.mockResolvedValue(makeResponse('bytes', 'text/plain')) - getUserEntityPermissionsSpy.mockResolvedValue('read') - - const result = await fetchExternalUrlToWorkspace({ - url: 'https://example.com/file.txt', - userId: 'user-1', - workspaceId: 'workspace-1', - }) - - expect(result.savedWorkspaceFile).toBeUndefined() - expect(uploadWorkspaceFileSpy).not.toHaveBeenCalled() - }) - - it('returns parsed bytes but skips save when user is not a workspace member', async () => { - secureFetchWithPinnedIPSpy.mockResolvedValue(makeResponse('bytes', 'text/plain')) - getUserEntityPermissionsSpy.mockResolvedValue(null) - - const result = await fetchExternalUrlToWorkspace({ - url: 'https://example.com/file.txt', - userId: 'user-1', - workspaceId: 'workspace-1', - }) - - expect(result.buffer.toString()).toBe('bytes') - expect(result.savedWorkspaceFile).toBeUndefined() - expect(uploadWorkspaceFileSpy).not.toHaveBeenCalled() - }) - - it('swallows workspace save errors so parsing can still proceed', async () => { - secureFetchWithPinnedIPSpy.mockResolvedValue(makeResponse('bytes', 'text/plain')) - uploadWorkspaceFileSpy.mockRejectedValueOnce(new Error('disk full')) - - const result = await fetchExternalUrlToWorkspace({ - url: 'https://example.com/file.txt', - userId: 'user-1', - workspaceId: 'workspace-1', - }) - - expect(result.buffer.toString()).toBe('bytes') - expect(result.savedWorkspaceFile).toBeUndefined() - }) }) diff --git a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts b/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts index b290b20b78b..dfe830435c1 100644 --- a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts +++ b/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts @@ -1,7 +1,5 @@ import type { Buffer } from 'buffer' import path from 'path' -import { createLogger } from '@sim/logger' -import { describeError } from '@sim/utils/errors' import { secureFetchWithPinnedIP, validateUrlWithDNS, @@ -11,12 +9,7 @@ import { readResponseTextWithLimit, readResponseToBufferWithLimit, } from '@/lib/core/utils/stream-limits' -import { uploadWorkspaceFile } from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { ensureFileNameExtension, getMimeTypeFromExtension } from '@/lib/uploads/utils/file-utils' -import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' -import type { UserFile } from '@/executor/types' - -const logger = createLogger('FetchExternalUrl') const DEFAULT_TIMEOUT_MS = 30_000 const DEFAULT_MAX_DOWNLOAD_BYTES = 100 * 1024 * 1024 @@ -34,11 +27,6 @@ export class ExternalUrlValidationError extends Error { export interface FetchExternalUrlOptions { url: string - userId: string - /** When provided alongside `saveToWorkspace: true`, the downloaded bytes are persisted as a workspace file. */ - workspaceId?: string - /** Defaults to true when a `workspaceId` is provided. Set false when the URL already points at our own storage. */ - saveToWorkspace?: boolean headers?: Record signal?: AbortSignal maxDownloadBytes?: number @@ -55,32 +43,20 @@ export interface FetchExternalUrlResult { buffer: Buffer /** Content-Type from the response, or inferred from the filename extension. */ mimeType: string - /** - * Saved workspace file record. Undefined when the workspace save was skipped - * (no workspaceId, `saveToWorkspace: false`, missing write permission, or a - * save error — the last is logged, not thrown, so the parse path stays alive). - */ - savedWorkspaceFile?: UserFile } /** - * Fetch an external URL into memory and (optionally) save it as a fresh workspace file. + * Fetch an external URL into memory behind SSRF validation and download limits. * * URL fetches are NEVER deduplicated by filename. Two URLs whose paths end in - * `image.png` are two different fetches that produce two different workspace - * files; `uploadWorkspaceFile` allocates a unique on-disk name (`image.png`, - * `image (1).png`, ...) on the save side. Keying a cache by path tail would - * silently return stale bytes — that was the original bug this helper exists - * to make unrepresentable. + * `image.png` are two different fetches with two different payloads; keying a + * cache by path tail would silently return stale bytes. */ -export async function fetchExternalUrlToWorkspace( +export async function fetchExternalUrl( options: FetchExternalUrlOptions ): Promise { const { url, - userId, - workspaceId, - saveToWorkspace = Boolean(workspaceId), headers, signal, maxDownloadBytes = DEFAULT_MAX_DOWNLOAD_BYTES, @@ -121,38 +97,5 @@ export async function fetchExternalUrlToWorkspace( const mimeType = response.headers.get('content-type') || getMimeTypeFromExtension(extension) const filename = ensureFileNameExtension(pathFilename, mimeType) - let savedWorkspaceFile: UserFile | undefined - if (workspaceId && saveToWorkspace) { - const permission = await getUserEntityPermissions(userId, 'workspace', workspaceId) - if (permission === 'admin' || permission === 'write') { - try { - savedWorkspaceFile = await uploadWorkspaceFile( - workspaceId, - userId, - buffer, - filename, - mimeType - ) - } catch (saveError) { - logger.warn('Failed to save fetched URL to workspace storage', { - workspaceId, - filename, - cause: describeError(saveError), - }) - } - } else if (permission === null) { - logger.warn('Skipping workspace save: user is not a workspace member', { - userId, - workspaceId, - }) - } else { - logger.warn('Skipping workspace save: user lacks write permission', { - userId, - workspaceId, - permission, - }) - } - } - - return { filename, buffer, mimeType, savedWorkspaceFile } + return { filename, buffer, mimeType } } diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts index ac5daf31bf0..afe7dc6135e 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts @@ -229,6 +229,15 @@ function fileFolderCondition(folderId?: string | null) { return normalized ? eq(workspaceFiles.folderId, normalized) : isNull(workspaceFiles.folderId) } +/** + * Folder predicate for active-name lookups, spelled exactly as the + * `workspace_files_workspace_folder_name_active_unique` index expression so the + * planner can use the index as a point lookup at the root as well as in a folder. + */ +export function workspaceFileNameFolderCondition(folderId?: string | null) { + return sql`coalesce(${workspaceFiles.folderId}, '') = ${folderId ?? ''}` +} + async function acquireWorkspaceFileFolderMutationLock(tx: DbOrTx, workspaceId: string) { await acquireFolderMutationLock(tx, workspaceId, FILE_FOLDER_RESOURCE_TYPE) } @@ -884,7 +893,7 @@ export async function fileNameExistsInWorkspaceFolder( eq(workspaceFiles.workspaceId, workspaceId), eq(workspaceFiles.originalName, fileName), eq(workspaceFiles.context, 'workspace'), - fileFolderCondition(folderId), + workspaceFileNameFolderCondition(folderId), isNull(workspaceFiles.deletedAt) ) ) diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts index 8b7519e0189..cae7057d2ed 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts @@ -117,6 +117,7 @@ import { listWorkspaceFileFolders, normalizeWorkspaceFileItemName, resolveWorkspaceFileFolderTarget, + workspaceFileNameFolderCondition, } from './workspace-file-folder-manager' const logger = createLogger('WorkspaceFileStorage') @@ -262,7 +263,12 @@ export function generateWorkspaceFileKey(workspaceId: string, fileName: string): return `workspace/${workspaceId}/${buildStorageKeySegment(`${timestamp}-${random}-`, fileName)}` } -const MAX_COPY_SUFFIX = 1000 +/** + * Numbered ` (n)` candidates probed before falling back to a random suffix. Keeps + * the familiar `a (1).pdf` naming for the common case while bounding every + * allocation to a fixed number of point lookups however many copies exist. + */ +const MAX_NUMBERED_COPY_SUFFIX = 20 const MAX_UPLOAD_UNIQUE_RETRIES = 8 interface WorkspaceFileMetadataInsert { @@ -387,7 +393,7 @@ async function cleanupWorkspaceStorageObject(key: string, reason: string): Promi /** * Inserts ` (n)` before the last extension (e.g. `a.pdf` → `a (1).pdf`), or appends for names without. */ -function withCopySuffix(fileName: string, n: number): string { +function withCopySuffix(fileName: string, n: number | string): string { const lastDot = fileName.lastIndexOf('.') const hasExtension = lastDot > 0 && lastDot < fileName.length - 1 if (hasExtension) { @@ -398,6 +404,9 @@ function withCopySuffix(fileName: string, n: number): string { /** * Picks a display name that does not collide with an active workspace file (`original_name`). + * + * Tries the base name, then `name (1)` … `name (20)`, then a random short-id suffix. The + * result is a hint: the unique index stays the authority, and callers retry on 23505. */ export async function allocateUniqueWorkspaceFileName( workspaceId: string, @@ -407,13 +416,13 @@ export async function allocateUniqueWorkspaceFileName( if (!(await fileExistsInWorkspace(workspaceId, baseName, folderId))) { return baseName } - for (let n = 1; n <= MAX_COPY_SUFFIX; n++) { + for (let n = 1; n <= MAX_NUMBERED_COPY_SUFFIX; n++) { const candidate = withCopySuffix(baseName, n) if (!(await fileExistsInWorkspace(workspaceId, candidate, folderId))) { return candidate } } - throw new FileConflictError(baseName) + return withCopySuffix(baseName, generateShortId(8)) } /** @@ -1263,7 +1272,6 @@ export async function getWorkspaceFileByName( fileName: string, options?: { folderId?: string | null } ): Promise { - const folderId = options?.folderId ?? null const files = await db .select() .from(workspaceFiles) @@ -1272,7 +1280,7 @@ export async function getWorkspaceFileByName( eq(workspaceFiles.workspaceId, workspaceId), eq(workspaceFiles.originalName, fileName), eq(workspaceFiles.context, 'workspace'), - folderId ? eq(workspaceFiles.folderId, folderId) : isNull(workspaceFiles.folderId), + workspaceFileNameFolderCondition(options?.folderId), isNull(workspaceFiles.deletedAt) ) ) diff --git a/packages/testing/src/mocks/workspace-file-folders.mock.ts b/packages/testing/src/mocks/workspace-file-folders.mock.ts index 021a4e0088c..811cec9ce5a 100644 --- a/packages/testing/src/mocks/workspace-file-folders.mock.ts +++ b/packages/testing/src/mocks/workspace-file-folders.mock.ts @@ -92,6 +92,7 @@ export const workspaceFileFoldersMockFns = { mockRelocateWorkspaceFileFolderByPath: vi.fn(), mockDeleteWorkspaceFileFolderByPath: vi.fn(), mockArchiveWorkspaceFileFolderIfEmpty: vi.fn(), + mockWorkspaceFileNameFolderCondition: vi.fn(), } const fns = workspaceFileFoldersMockFns @@ -133,4 +134,5 @@ export const workspaceFileFoldersMock = { relocateWorkspaceFileFolderByPath: fns.mockRelocateWorkspaceFileFolderByPath, deleteWorkspaceFileFolderByPath: fns.mockDeleteWorkspaceFileFolderByPath, archiveWorkspaceFileFolderIfEmpty: fns.mockArchiveWorkspaceFileFolderIfEmpty, + workspaceFileNameFolderCondition: fns.mockWorkspaceFileNameFolderCondition, } diff --git a/packages/testing/src/mocks/workspace-uploads.mock.ts b/packages/testing/src/mocks/workspace-uploads.mock.ts index b731ad037c8..519e53c6931 100644 --- a/packages/testing/src/mocks/workspace-uploads.mock.ts +++ b/packages/testing/src/mocks/workspace-uploads.mock.ts @@ -75,7 +75,7 @@ interface MockFolderPathRow { */ export const workspaceUploadsMockFns = { ...workspaceFileManagerMockFns, - mockFetchExternalUrlToWorkspace: vi.fn(), + mockFetchExternalUrl: vi.fn(), mockLoadWorkspaceFileOperationContext: vi.fn(), mockAssertWorkspaceFileItemsBelongToWorkspace: vi.fn(), mockNormalizeWorkspaceFileItemName: vi.fn((name: string, itemLabel: 'File' | 'Folder') => { @@ -121,6 +121,7 @@ export const workspaceUploadsMockFns = { mockRelocateWorkspaceFileFolderByPath: vi.fn(), mockDeleteWorkspaceFileFolderByPath: vi.fn(), mockArchiveWorkspaceFileFolderIfEmpty: vi.fn(), + mockWorkspaceFileNameFolderCondition: vi.fn(), } const fns = workspaceUploadsMockFns @@ -143,7 +144,7 @@ export const workspaceUploadsMock = { WorkspaceFileFolderConflictError: MockWorkspaceFileFolderConflictError, WorkspaceFileMoveConflictError: MockWorkspaceFileMoveConflictError, WorkspaceFileItemsNotFoundError: MockWorkspaceFileItemsNotFoundError, - fetchExternalUrlToWorkspace: fns.mockFetchExternalUrlToWorkspace, + fetchExternalUrl: fns.mockFetchExternalUrl, loadWorkspaceFileOperationContext: fns.mockLoadWorkspaceFileOperationContext, assertWorkspaceFileItemsBelongToWorkspace: fns.mockAssertWorkspaceFileItemsBelongToWorkspace, normalizeWorkspaceFileItemName: fns.mockNormalizeWorkspaceFileItemName, @@ -165,4 +166,5 @@ export const workspaceUploadsMock = { relocateWorkspaceFileFolderByPath: fns.mockRelocateWorkspaceFileFolderByPath, deleteWorkspaceFileFolderByPath: fns.mockDeleteWorkspaceFileFolderByPath, archiveWorkspaceFileFolderIfEmpty: fns.mockArchiveWorkspaceFileFolderIfEmpty, + workspaceFileNameFolderCondition: fns.mockWorkspaceFileNameFolderCondition, } From b06840e27cad1247deb2221851227b53d484daa0 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 10:35:11 -0700 Subject: [PATCH 2/3] fix(files): index-shape the move conflict probe and cover execution-context URL parses --- .../lib/copy/copy-files.test.ts | 7 ++++--- apps/sim/lib/uploads/archive.test.ts | 2 +- .../__integration__/file-names.integration.ts | 17 +++++++++++++---- .../workspace/workspace-file-folder-manager.ts | 7 +------ 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts b/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts index fabd134b726..b666a36fd29 100644 --- a/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts +++ b/apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts @@ -14,6 +14,7 @@ import { workspaceFileSecretProvenanceMock, workspaceFileSecretProvenanceMockFns, } from '@sim/testing/mocks/workspace-file-secret-provenance.mock' +import { generateShortId } from '@sim/utils/id' import { beforeEach, describe, expect, it, vi } from 'vitest' /** The `workspace_files` columns {@link fileRows} enforces its unique indexes on. */ @@ -36,7 +37,7 @@ interface WorkspaceFileRow { */ const { fileRows, allocateFromFileRows } = vi.hoisted(() => { const fileRows: WorkspaceFileRow[] = [] - const withCopySuffix = (name: string, n: number) => { + const withCopySuffix = (name: string, n: number | string) => { const lastDot = name.lastIndexOf('.') return lastDot > 0 && lastDot < name.length - 1 ? `${name.slice(0, lastDot)} (${n})${name.slice(lastDot)}` @@ -63,11 +64,11 @@ const { fileRows, allocateFromFileRows } = vi.hoisted(() => { row.originalName === name ) if (!taken(baseName)) return baseName - for (let n = 1; n <= 1000; n++) { + for (let n = 1; n <= 20; n++) { const candidate = withCopySuffix(baseName, n) if (!taken(candidate)) return candidate } - throw new Error(`A file named "${baseName}" already exists in this workspace`) + return withCopySuffix(baseName, generateShortId(8)) }, } }) diff --git a/apps/sim/lib/uploads/archive.test.ts b/apps/sim/lib/uploads/archive.test.ts index 4ce0c28185d..83dc60ee911 100644 --- a/apps/sim/lib/uploads/archive.test.ts +++ b/apps/sim/lib/uploads/archive.test.ts @@ -114,7 +114,7 @@ function craftCentralDirectory(records: number, extraPerRecord: number): Buffer return buffer } -/** Mirrors `allocateUniqueWorkspaceFileName`'s " (n)" suffixing. */ +/** Numbered " (n)" suffixing in the style of `allocateUniqueWorkspaceFileName`'s first candidates. */ function allocateUniqueName(folderKey: string, name: string): string { const dot = name.lastIndexOf('.') const base = dot > 0 ? name.slice(0, dot) : name diff --git a/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts b/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts index 59f1da8ee4e..f4baa51c973 100644 --- a/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts +++ b/apps/sim/lib/uploads/contexts/workspace/__integration__/file-names.integration.ts @@ -66,7 +66,7 @@ describe('workspace file names in PostgreSQL', () => { }) } - it('parses an external URL without saving a copy to workspace Files', async () => { + async function parseExternalUrl(executionId?: string) { const fixture = await seedWorkspace() const url = 'https://example.com/page.txt' vi.spyOn(inputValidation, 'validateUrlWithDNS').mockResolvedValue({ @@ -96,9 +96,11 @@ describe('workspace file names in PostgreSQL', () => { subjectUserId: fixture.aliceId, workspaceId: fixture.workspaceId, delegationId: generateId(), + executionId, }), workspaceId: fixture.workspaceId, workflowId: generateId(), + executionId, attributedUserId: fixture.aliceId, fileAccessUserId: fixture.aliceId, } @@ -107,11 +109,18 @@ describe('workspace file names in PostgreSQL', () => { expect(response.status).toBe(200) expect(body.output.content).toContain('fetched page body') - const rows = await db - .select({ id: workspaceFiles.id }) + return db + .select({ context: workspaceFiles.context }) .from(workspaceFiles) .where(eq(workspaceFiles.workspaceId, fixture.workspaceId)) - expect(rows).toEqual([]) + } + + it('parses an external URL without saving a copy to workspace Files', async () => { + expect(await parseExternalUrl()).toEqual([]) + }) + + it('keeps an external URL parsed during an execution as an execution file only', async () => { + expect(await parseExternalUrl(generateId())).toEqual([{ context: 'execution' }]) }) it('scopes name lookups to root or folder through the unique name index', async () => { diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts index afe7dc6135e..f021fd7c622 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts @@ -224,11 +224,6 @@ function folderParentCondition(parentId?: string | null) { return normalized ? eq(folderTable.parentId, normalized) : isNull(folderTable.parentId) } -function fileFolderCondition(folderId?: string | null) { - const normalized = normalizeParentId(folderId) - return normalized ? eq(workspaceFiles.folderId, normalized) : isNull(workspaceFiles.folderId) -} - /** * Folder predicate for active-name lookups, spelled exactly as the * `workspace_files_workspace_folder_name_active_unique` index expression so the @@ -1059,7 +1054,7 @@ export async function moveWorkspaceFileItems(params: { eq(workspaceFiles.workspaceId, params.workspaceId), eq(workspaceFiles.originalName, file.name), eq(workspaceFiles.context, 'workspace'), - fileFolderCondition(targetFolderId), + workspaceFileNameFolderCondition(targetFolderId), isNull(workspaceFiles.deletedAt) ) ) From 4d2c6b3844c781b244fedaef89c8b6dd775841de Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 26 Sep 2026 13:16:07 -0700 Subject: [PATCH 3/3] refactor(uploads): move the external URL fetch out of the workspace files module --- .agents/skills/memory-load-check/SKILL.md | 2 +- apps/sim/lib/internal/file/parser.ts | 5 ++++- apps/sim/lib/uploads/contexts/workspace/index.ts | 1 - .../fetch-external-url.server.test.ts} | 2 +- .../fetch-external-url.server.ts} | 0 packages/testing/src/mocks/index.ts | 1 - .../testing/src/mocks/workspace-uploads.mock.ts | 15 ++------------- 7 files changed, 8 insertions(+), 18 deletions(-) rename apps/sim/lib/uploads/{contexts/workspace/fetch-external-url.test.ts => utils/fetch-external-url.server.test.ts} (97%) rename apps/sim/lib/uploads/{contexts/workspace/fetch-external-url.ts => utils/fetch-external-url.server.ts} (100%) diff --git a/.agents/skills/memory-load-check/SKILL.md b/.agents/skills/memory-load-check/SKILL.md index 157e7a89db8..22c03cb7767 100644 --- a/.agents/skills/memory-load-check/SKILL.md +++ b/.agents/skills/memory-load-check/SKILL.md @@ -45,7 +45,7 @@ Read these when doing a deeper pass: - dispatch concrete chunks (`workspaceIds`, retention, label) instead of one giant scope - prefer Trigger.dev queue/concurrency keys when available - execute inline fallback chunks sequentially, not with unbounded `Promise.all` -- File parse pattern in `apps/sim/lib/internal/file/parser.ts` and `apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts` +- File parse pattern in `apps/sim/lib/internal/file/parser.ts` and `apps/sim/lib/uploads/utils/fetch-external-url.server.ts` - cap downloads and parsed output separately - preserve partial results when a later item exceeds the cap - never read untrusted response bodies without a byte cap diff --git a/apps/sim/lib/internal/file/parser.ts b/apps/sim/lib/internal/file/parser.ts index 3071159b122..d8f20be48f5 100644 --- a/apps/sim/lib/internal/file/parser.ts +++ b/apps/sim/lib/internal/file/parser.ts @@ -35,13 +35,16 @@ import { } from '@/lib/internal/file/operations' import { isUsingCloudStorage, StorageService } from '@/lib/uploads' import { uploadExecutionFile } from '@/lib/uploads/contexts/execution' -import { ExternalUrlValidationError, fetchExternalUrl } from '@/lib/uploads/contexts/workspace' import { getBoundWorkspaceFileSecretProvenance, type WorkspaceFileSecretProvenance, } from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance' import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server' import { isWorkspaceScopedContext } from '@/lib/uploads/shared/types' +import { + ExternalUrlValidationError, + fetchExternalUrl, +} from '@/lib/uploads/utils/fetch-external-url.server' import { extractCleanFilename, extractStorageKey, diff --git a/apps/sim/lib/uploads/contexts/workspace/index.ts b/apps/sim/lib/uploads/contexts/workspace/index.ts index d5c542b512a..b7dbcdee264 100644 --- a/apps/sim/lib/uploads/contexts/workspace/index.ts +++ b/apps/sim/lib/uploads/contexts/workspace/index.ts @@ -1,3 +1,2 @@ -export * from './fetch-external-url' export * from './workspace-file-folder-manager' export * from './workspace-file-manager' diff --git a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts b/apps/sim/lib/uploads/utils/fetch-external-url.server.test.ts similarity index 97% rename from apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts rename to apps/sim/lib/uploads/utils/fetch-external-url.server.test.ts index 6d0d02218cf..480ee37482d 100644 --- a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.test.ts +++ b/apps/sim/lib/uploads/utils/fetch-external-url.server.test.ts @@ -11,7 +11,7 @@ import * as inputValidation from '@/lib/core/security/input-validation.server' import { ExternalUrlValidationError, fetchExternalUrl, -} from '@/lib/uploads/contexts/workspace/fetch-external-url' +} from '@/lib/uploads/utils/fetch-external-url.server' let validateUrlWithDNSSpy: MockInstance let secureFetchWithPinnedIPSpy: MockInstance diff --git a/apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts b/apps/sim/lib/uploads/utils/fetch-external-url.server.ts similarity index 100% rename from apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts rename to apps/sim/lib/uploads/utils/fetch-external-url.server.ts diff --git a/packages/testing/src/mocks/index.ts b/packages/testing/src/mocks/index.ts index ab1fb27bcd0..b0224177ee9 100644 --- a/packages/testing/src/mocks/index.ts +++ b/packages/testing/src/mocks/index.ts @@ -1044,7 +1044,6 @@ export { workspaceForkingMappingStoreMockFns, } from './workspace-forking-mapping-store.mock' export { - MockExternalUrlValidationError, MockWorkspaceFileFolderConflictError, MockWorkspaceFileItemsNotFoundError, MockWorkspaceFileMoveConflictError, diff --git a/packages/testing/src/mocks/workspace-uploads.mock.ts b/packages/testing/src/mocks/workspace-uploads.mock.ts index 519e53c6931..8928f86669d 100644 --- a/packages/testing/src/mocks/workspace-uploads.mock.ts +++ b/packages/testing/src/mocks/workspace-uploads.mock.ts @@ -41,14 +41,6 @@ export class MockWorkspaceFileItemsNotFoundError extends Error { } } -/** Real `ExternalUrlValidationError` stand-in: same `name` and message. */ -export class MockExternalUrlValidationError extends Error { - constructor(message: string) { - super(message) - this.name = 'ExternalUrlValidationError' - } -} - /** Structural stand-in for a folder row passed to `buildWorkspaceFileFolderPathMap`. */ interface MockFolderPathRow { id: string @@ -57,7 +49,7 @@ interface MockFolderPathRow { } /** - * Controllable mock functions for the folder-manager and external-URL halves of the + * Controllable mock functions for the folder-manager half of the * `@/lib/uploads/contexts/workspace` barrel. The file-manager half is * `workspaceFileManagerMockFns` (re-exported here as `...workspaceFileManagerMockFns`). * @@ -75,7 +67,6 @@ interface MockFolderPathRow { */ export const workspaceUploadsMockFns = { ...workspaceFileManagerMockFns, - mockFetchExternalUrl: vi.fn(), mockLoadWorkspaceFileOperationContext: vi.fn(), mockAssertWorkspaceFileItemsBelongToWorkspace: vi.fn(), mockNormalizeWorkspaceFileItemName: vi.fn((name: string, itemLabel: 'File' | 'Folder') => { @@ -128,7 +119,7 @@ const fns = workspaceUploadsMockFns /** * Static mock module for the `@/lib/uploads/contexts/workspace` barrel - * (`workspace-file-manager` + `workspace-file-folder-manager` + `fetch-external-url`). + * (`workspace-file-manager` + `workspace-file-folder-manager`). * * The error classes extend `Error`, not `OrchestrationError`: their `name`, `code` and * message match production, but `instanceof OrchestrationError` is false. @@ -140,11 +131,9 @@ const fns = workspaceUploadsMockFns */ export const workspaceUploadsMock = { ...workspaceFileManagerMock, - ExternalUrlValidationError: MockExternalUrlValidationError, WorkspaceFileFolderConflictError: MockWorkspaceFileFolderConflictError, WorkspaceFileMoveConflictError: MockWorkspaceFileMoveConflictError, WorkspaceFileItemsNotFoundError: MockWorkspaceFileItemsNotFoundError, - fetchExternalUrl: fns.mockFetchExternalUrl, loadWorkspaceFileOperationContext: fns.mockLoadWorkspaceFileOperationContext, assertWorkspaceFileItemsBelongToWorkspace: fns.mockAssertWorkspaceFileItemsBelongToWorkspace, normalizeWorkspaceFileItemName: fns.mockNormalizeWorkspaceFileItemName,