Skip to content

Commit ce88d8a

Browse files
authored
fix(files): stop saving fetched URLs to Files and look names up by the unique index (#8327)
- The File block's URL fetch no longer saves every fetched URL into workspace Files. Nothing ever read the saved copy: the parser keeps its own execution file, and the reuse the save once served was removed earlier. Repeated fetches were piling up `name (N).html` copies in the Files root - Renamed `fetchExternalUrlToWorkspace` to `fetchExternalUrl` and removed its save, permission, and upload branch - Name existence checks (`fileNameExistsInWorkspaceFolder`, `getWorkspaceFileByName`) now spell the folder predicate as `coalesce(folder_id, '') = $folder`, matching the unique `(workspace_id, coalesce(folder_id, ''), original_name)` index, so each check is a point lookup. The old `folder_id IS NULL` form made root lookups scan the folder-id index - `allocateUniqueWorkspaceFileName` probes the base name plus `(1)`…`(20)`, then falls back to a short-id suffix instead of probing up to 1,000 candidates and then failing. The unique index and the existing conflict retry remain the authority
1 parent 10282f2 commit ce88d8a

15 files changed

Lines changed: 318 additions & 273 deletions

File tree

‎.agents/skills/memory-load-check/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ Read these when doing a deeper pass:
4545
- dispatch concrete chunks (`workspaceIds`, retention, label) instead of one giant scope
4646
- prefer Trigger.dev queue/concurrency keys when available
4747
- execute inline fallback chunks sequentially, not with unbounded `Promise.all`
48-
- File parse pattern in `apps/sim/lib/internal/file/parser.ts` and `apps/sim/lib/uploads/contexts/workspace/fetch-external-url.ts`
48+
- File parse pattern in `apps/sim/lib/internal/file/parser.ts` and `apps/sim/lib/uploads/utils/fetch-external-url.server.ts`
4949
- cap downloads and parsed output separately
5050
- preserve partial results when a later item exceeds the cap
5151
- never read untrusted response bodies without a byte cap

‎apps/sim/ee/workspace-forking/lib/copy/copy-files.test.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
workspaceFileSecretProvenanceMock,
1515
workspaceFileSecretProvenanceMockFns,
1616
} from '@sim/testing/mocks/workspace-file-secret-provenance.mock'
17+
import { generateShortId } from '@sim/utils/id'
1718
import { beforeEach, describe, expect, it, vi } from 'vitest'
1819

1920
/** The `workspace_files` columns {@link fileRows} enforces its unique indexes on. */
@@ -36,7 +37,7 @@ interface WorkspaceFileRow {
3637
*/
3738
const { fileRows, allocateFromFileRows } = vi.hoisted(() => {
3839
const fileRows: WorkspaceFileRow[] = []
39-
const withCopySuffix = (name: string, n: number) => {
40+
const withCopySuffix = (name: string, n: number | string) => {
4041
const lastDot = name.lastIndexOf('.')
4142
return lastDot > 0 && lastDot < name.length - 1
4243
? `${name.slice(0, lastDot)} (${n})${name.slice(lastDot)}`
@@ -63,11 +64,11 @@ const { fileRows, allocateFromFileRows } = vi.hoisted(() => {
6364
row.originalName === name
6465
)
6566
if (!taken(baseName)) return baseName
66-
for (let n = 1; n <= 1000; n++) {
67+
for (let n = 1; n <= 20; n++) {
6768
const candidate = withCopySuffix(baseName, n)
6869
if (!taken(candidate)) return candidate
6970
}
70-
throw new Error(`A file named "${baseName}" already exists in this workspace`)
71+
return withCopySuffix(baseName, generateShortId(8))
7172
},
7273
}
7374
})

‎apps/sim/lib/internal/file/parser.test.ts‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,7 @@ import {
2727
} from '@sim/testing/mocks/uploads-execution.mock'
2828
import { uploadsMetadataMock } from '@sim/testing/mocks/uploads-metadata.mock'
2929
import { uploadsSetupMock } from '@sim/testing/mocks/uploads-setup.mock'
30-
import {
31-
workspaceFileManagerMock,
32-
workspaceFileManagerMockFns,
33-
} from '@sim/testing/mocks/workspace-file-manager.mock'
30+
import { workspaceFileManagerMock } from '@sim/testing/mocks/workspace-file-manager.mock'
3431
import {
3532
workspaceFileSecretProvenanceMock,
3633
workspaceFileSecretProvenanceMockFns,
@@ -181,21 +178,8 @@ vi.mock('fs/promises', () => ({
181178
}))
182179

183180
const { mockGetStorageProvider, mockIsUsingCloudStorage } = uploadsMockFns
184-
const { mockUploadWorkspaceFile } = workspaceFileManagerMockFns
185181
const { mockGetBoundWorkspaceFileSecretProvenance } = workspaceFileSecretProvenanceMockFns
186182

187-
mockUploadWorkspaceFile.mockImplementation(
188-
async (workspaceId: string, _userId: string, _buffer: Buffer, fileName: string) => ({
189-
id: 'wf_test',
190-
name: fileName,
191-
size: 0,
192-
type: 'application/octet-stream',
193-
url: `/api/files/serve/${workspaceId}/${fileName}`,
194-
key: `${workspaceId}/${fileName}`,
195-
context: 'workspace',
196-
})
197-
)
198-
199183
import { fileParseBodySchema } from '@/lib/api/contracts/storage-transfer'
200184
import { executeFileParserOperation } from '@/lib/internal/file/parser'
201185
import { createWorkspaceFileDelegatedPrincipal } from '@/lib/workspace-files/application/delegated-principal'
@@ -658,7 +642,6 @@ describe('file parser operation', () => {
658642
})
659643
)
660644
mockIsSupportedFileType.mockReturnValue(false)
661-
permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('write')
662645

663646
const req = createMockRequest('POST', {
664647
filePath: [
@@ -686,7 +669,6 @@ describe('file parser operation', () => {
686669
'203.0.113.10',
687670
expect.any(Object)
688671
)
689-
expect(mockUploadWorkspaceFile).toHaveBeenCalledTimes(2)
690672
expect(storageServiceMockFns.mockDownloadFile).not.toHaveBeenCalled()
691673
})
692674

‎apps/sim/lib/internal/file/parser.ts‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,16 @@ import {
3535
} from '@/lib/internal/file/operations'
3636
import { isUsingCloudStorage, StorageService } from '@/lib/uploads'
3737
import { uploadExecutionFile } from '@/lib/uploads/contexts/execution'
38-
import {
39-
ExternalUrlValidationError,
40-
fetchExternalUrlToWorkspace,
41-
} from '@/lib/uploads/contexts/workspace'
4238
import {
4339
getBoundWorkspaceFileSecretProvenance,
4440
type WorkspaceFileSecretProvenance,
4541
} from '@/lib/uploads/contexts/workspace/workspace-file-secret-provenance'
4642
import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server'
4743
import { isWorkspaceScopedContext } from '@/lib/uploads/shared/types'
44+
import {
45+
ExternalUrlValidationError,
46+
fetchExternalUrl,
47+
} from '@/lib/uploads/utils/fetch-external-url.server'
4848
import {
4949
extractCleanFilename,
5050
extractStorageKey,
@@ -512,7 +512,7 @@ function assertParsedContentWithinLimit(content: string, maxBytes?: number): str
512512
* Validate file path for security - prevents null byte injection and path traversal attacks.
513513
*
514514
* External URLs (`http`/`https`) are fetched over HTTP — with SSRF protection applied
515-
* downstream in `fetchExternalUrlToWorkspace` (DNS resolution + private/reserved IP blocking)
515+
* downstream in `fetchExternalUrl` (DNS resolution + private/reserved IP blocking)
516516
* — and are never resolved against the filesystem, so `..`/`~` are legal URL content and must
517517
* not be rejected. Providers such as Slack routinely emit slugs containing a literal `...`.
518518
*
@@ -560,8 +560,8 @@ function validateFilePath(filePath: string): { isValid: boolean; error?: string
560560
*
561561
* Always fetches the URL fresh — there is no filename-based dedup. Distinct URLs
562562
* commonly share a path tail (e.g. every Slack clipboard paste is `image.png`),
563-
* so keying a cache by filename returns stale bytes. `fetchExternalUrlToWorkspace`
564-
* delegates to `uploadWorkspaceFile`, which suffix-disambiguates collisions on save.
563+
* so keying a cache by filename returns stale bytes. The fetched bytes are never saved
564+
* to workspace Files; with an execution context they are kept as an execution file only.
565565
*
566566
* URLs for our workspace and execution storage resolve through the authorized canonical
567567
* read path, keeping stored provenance bound to the same bytes the parser reads.
@@ -647,11 +647,8 @@ async function handleExternalUrl(
647647
)
648648
}
649649

650-
const { filename, buffer, mimeType } = await fetchExternalUrlToWorkspace({
650+
const { filename, buffer, mimeType } = await fetchExternalUrl({
651651
url,
652-
userId,
653-
workspaceId: workspaceId || undefined,
654-
saveToWorkspace: Boolean(workspaceId),
655652
headers,
656653
signal,
657654
maxDownloadBytes,

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ function craftCentralDirectory(records: number, extraPerRecord: number): Buffer
114114
return buffer
115115
}
116116

117-
/** Mirrors `allocateUniqueWorkspaceFileName`'s " (n)" suffixing. */
117+
/** Numbered " (n)" suffixing in the style of `allocateUniqueWorkspaceFileName`'s first candidates. */
118118
function allocateUniqueName(folderKey: string, name: string): string {
119119
const dot = name.lastIndexOf('.')
120120
const base = dot > 0 ? name.slice(0, dot) : name
Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
/** Real PostgreSQL name allocation and name lookups for workspace files, plus the URL fetch path. */
2+
import { mkdtempSync } from 'node:fs'
3+
import { rm } from 'node:fs/promises'
4+
import { tmpdir } from 'node:os'
5+
import path from 'node:path'
6+
import { db, dbFor } from '@sim/db'
7+
import { organization, user, workspace, workspaceFiles } from '@sim/db/schema'
8+
import { generateId } from '@sim/utils/id'
9+
import { and, eq, inArray, isNull, sql } from 'drizzle-orm'
10+
import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'
11+
12+
const fixtureStorage = vi.hoisted(() => ({ root: '' }))
13+
vi.mock('@/lib/uploads/core/setup.server', () => ({
14+
get UPLOAD_DIR_SERVER() {
15+
return fixtureStorage.root
16+
},
17+
}))
18+
19+
import { fileParseBodySchema } from '@/lib/api/contracts/storage-transfer'
20+
import * as inputValidation from '@/lib/core/security/input-validation.server'
21+
import { executeFileParserOperation } from '@/lib/internal/file/parser'
22+
import {
23+
createKnowledgeAclFixtureIds,
24+
seedKnowledgeAclFixture,
25+
} from '@/lib/knowledge/__integration__/seed-source-access-fixture'
26+
import {
27+
createWorkspaceFileFolder,
28+
fileNameExistsInWorkspaceFolder,
29+
workspaceFileNameFolderCondition,
30+
} from '@/lib/uploads/contexts/workspace/workspace-file-folder-manager'
31+
import {
32+
getWorkspaceFileByName,
33+
uploadWorkspaceFile,
34+
} from '@/lib/uploads/contexts/workspace/workspace-file-manager'
35+
import { createWorkspaceFileDelegatedPrincipal } from '@/lib/workspace-files/application/delegated-principal'
36+
37+
describe('workspace file names in PostgreSQL', () => {
38+
const fixtures: ReturnType<typeof createKnowledgeAclFixtureIds>[] = []
39+
40+
beforeAll(() => {
41+
fixtureStorage.root = mkdtempSync(path.join(tmpdir(), 'sim-file-names-'))
42+
})
43+
44+
afterAll(async () => {
45+
vi.restoreAllMocks()
46+
for (const ids of fixtures) {
47+
await db.delete(workspace).where(eq(workspace.id, ids.workspaceId))
48+
await db.delete(organization).where(eq(organization.id, ids.organizationId))
49+
await db.delete(user).where(inArray(user.id, [ids.aliceId, ids.bobId]))
50+
}
51+
await rm(fixtureStorage.root, { recursive: true, force: true })
52+
await Promise.all([db.$client.end(), dbFor('cleanup').$client.end()])
53+
})
54+
55+
async function seedWorkspace() {
56+
const ids = createKnowledgeAclFixtureIds()
57+
fixtures.push(ids)
58+
await seedKnowledgeAclFixture(ids)
59+
return ids
60+
}
61+
62+
function upload(workspaceId: string, userId: string, name: string, folderId?: string | null) {
63+
return uploadWorkspaceFile(workspaceId, userId, Buffer.from(name), name, 'text/plain', {
64+
folderId,
65+
notifyWorkspaceChange: false,
66+
})
67+
}
68+
69+
async function parseExternalUrl(executionId?: string) {
70+
const fixture = await seedWorkspace()
71+
const url = 'https://example.com/page.txt'
72+
vi.spyOn(inputValidation, 'validateUrlWithDNS').mockResolvedValue({
73+
isValid: true,
74+
resolvedIP: '203.0.113.10',
75+
originalHostname: new URL(url).hostname,
76+
})
77+
vi.spyOn(inputValidation, 'secureFetchWithPinnedIP').mockImplementation(async () => {
78+
const response = new Response('fetched page body')
79+
return {
80+
ok: response.ok,
81+
status: response.status,
82+
statusText: response.statusText,
83+
headers: new inputValidation.SecureFetchHeaders({ 'content-type': 'text/plain' }),
84+
body: response.body,
85+
text: () => response.text(),
86+
json: () => response.json(),
87+
arrayBuffer: () => response.arrayBuffer(),
88+
}
89+
})
90+
91+
const response = await executeFileParserOperation(
92+
fileParseBodySchema.parse({ filePath: url, workspaceId: fixture.workspaceId }),
93+
{
94+
principal: createWorkspaceFileDelegatedPrincipal({
95+
serviceId: 'executor',
96+
subjectUserId: fixture.aliceId,
97+
workspaceId: fixture.workspaceId,
98+
delegationId: generateId(),
99+
executionId,
100+
}),
101+
workspaceId: fixture.workspaceId,
102+
workflowId: generateId(),
103+
executionId,
104+
attributedUserId: fixture.aliceId,
105+
fileAccessUserId: fixture.aliceId,
106+
}
107+
)
108+
const body = await response.json()
109+
110+
expect(response.status).toBe(200)
111+
expect(body.output.content).toContain('fetched page body')
112+
return db
113+
.select({ context: workspaceFiles.context })
114+
.from(workspaceFiles)
115+
.where(eq(workspaceFiles.workspaceId, fixture.workspaceId))
116+
}
117+
118+
it('parses an external URL without saving a copy to workspace Files', async () => {
119+
expect(await parseExternalUrl()).toEqual([])
120+
})
121+
122+
it('keeps an external URL parsed during an execution as an execution file only', async () => {
123+
expect(await parseExternalUrl(generateId())).toEqual([{ context: 'execution' }])
124+
})
125+
126+
it('scopes name lookups to root or folder through the unique name index', async () => {
127+
const fixture = await seedWorkspace()
128+
const folder = await createWorkspaceFileFolder({
129+
workspaceId: fixture.workspaceId,
130+
userId: fixture.aliceId,
131+
name: 'Reports',
132+
})
133+
const rootFile = await upload(fixture.workspaceId, fixture.aliceId, 'root.txt')
134+
const folderFile = await upload(fixture.workspaceId, fixture.aliceId, 'nested.txt', folder.id)
135+
136+
expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'root.txt', null)).toBe(true)
137+
expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'root.txt', folder.id)).toBe(
138+
false
139+
)
140+
expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'nested.txt', null)).toBe(
141+
false
142+
)
143+
expect(
144+
await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'nested.txt', folder.id)
145+
).toBe(true)
146+
expect((await getWorkspaceFileByName(fixture.workspaceId, 'root.txt'))?.id).toBe(rootFile.id)
147+
expect(
148+
await getWorkspaceFileByName(fixture.workspaceId, 'root.txt', { folderId: folder.id })
149+
).toBeNull()
150+
expect(
151+
(await getWorkspaceFileByName(fixture.workspaceId, 'nested.txt', { folderId: folder.id }))?.id
152+
).toBe(folderFile.id)
153+
expect(await getWorkspaceFileByName(fixture.workspaceId, 'nested.txt')).toBeNull()
154+
155+
await db.execute(sql`
156+
INSERT INTO ${workspaceFiles} (id, key, user_id, workspace_id, folder_id, context, original_name, content_type)
157+
SELECT 'wf_pad_' || n || '_' || ${fixture.workspaceId}, 'pad/' || n || '/' || ${fixture.workspaceId},
158+
${fixture.aliceId}, ${fixture.workspaceId}, CASE WHEN n % 2 = 0 THEN ${folder.id} END,
159+
'workspace', 'pad-' || n || '.txt', 'text/plain'
160+
FROM generate_series(1, 2000) AS n`)
161+
await db.execute(sql`ANALYZE ${workspaceFiles}`)
162+
163+
for (const folderId of [null, folder.id]) {
164+
const plan = await db.execute(
165+
sql`EXPLAIN (FORMAT JSON) SELECT id FROM ${workspaceFiles} WHERE ${and(
166+
eq(workspaceFiles.workspaceId, fixture.workspaceId),
167+
eq(workspaceFiles.originalName, 'root.txt'),
168+
eq(workspaceFiles.context, 'workspace'),
169+
workspaceFileNameFolderCondition(folderId),
170+
isNull(workspaceFiles.deletedAt)
171+
)}`
172+
)
173+
const scan = JSON.stringify(plan[0]['QUERY PLAN'])
174+
expect(scan).toContain('workspace_files_workspace_folder_name_active_unique')
175+
expect(scan).toMatch(/"Index Cond":"[^"]*COALESCE\(folder_id/)
176+
}
177+
})
178+
179+
it('falls back to a short-id suffix after 20 numbered copies, including under concurrency', async () => {
180+
const fixture = await seedWorkspace()
181+
await upload(fixture.workspaceId, fixture.aliceId, 'page.html')
182+
for (let n = 1; n <= 20; n++) {
183+
await upload(fixture.workspaceId, fixture.aliceId, `page (${n}).html`)
184+
}
185+
186+
const next = await upload(fixture.workspaceId, fixture.aliceId, 'page.html')
187+
const concurrent = await Promise.all(
188+
Array.from({ length: 8 }, () => upload(fixture.workspaceId, fixture.aliceId, 'page.html'))
189+
)
190+
191+
const shortIdSuffixed = /^page \([A-Za-z0-9_-]{8}\)\.html$/
192+
expect(next.name).toMatch(shortIdSuffixed)
193+
const names = concurrent.map((file) => file.name)
194+
for (const name of names) expect(name).toMatch(shortIdSuffixed)
195+
expect(new Set([next.name, ...names]).size).toBe(names.length + 1)
196+
})
197+
})

0 commit comments

Comments
 (0)