Skip to content

Commit 4b22934

Browse files
committed
fix(files): stop saving fetched URLs to Files and look names up by the unique index
1 parent a0c93d6 commit 4b22934

9 files changed

Lines changed: 236 additions & 186 deletions

File tree

‎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: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,7 @@ 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'
38+
import { ExternalUrlValidationError, fetchExternalUrl } from '@/lib/uploads/contexts/workspace'
4239
import {
4340
getBoundWorkspaceFileSecretProvenance,
4441
type WorkspaceFileSecretProvenance,
@@ -512,7 +509,7 @@ function assertParsedContentWithinLimit(content: string, maxBytes?: number): str
512509
* Validate file path for security - prevents null byte injection and path traversal attacks.
513510
*
514511
* External URLs (`http`/`https`) are fetched over HTTP — with SSRF protection applied
515-
* downstream in `fetchExternalUrlToWorkspace` (DNS resolution + private/reserved IP blocking)
512+
* downstream in `fetchExternalUrl` (DNS resolution + private/reserved IP blocking)
516513
* — and are never resolved against the filesystem, so `..`/`~` are legal URL content and must
517514
* not be rejected. Providers such as Slack routinely emit slugs containing a literal `...`.
518515
*
@@ -560,8 +557,8 @@ function validateFilePath(filePath: string): { isValid: boolean; error?: string
560557
*
561558
* Always fetches the URL fresh — there is no filename-based dedup. Distinct URLs
562559
* 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.
560+
* so keying a cache by filename returns stale bytes. The fetched bytes are never saved
561+
* to workspace Files; with an execution context they are kept as an execution file only.
565562
*
566563
* URLs for our workspace and execution storage resolve through the authorized canonical
567564
* read path, keeping stored provenance bound to the same bytes the parser reads.
@@ -647,11 +644,8 @@ async function handleExternalUrl(
647644
)
648645
}
649646

650-
const { filename, buffer, mimeType } = await fetchExternalUrlToWorkspace({
647+
const { filename, buffer, mimeType } = await fetchExternalUrl({
651648
url,
652-
userId,
653-
workspaceId: workspaceId || undefined,
654-
saveToWorkspace: Boolean(workspaceId),
655649
headers,
656650
signal,
657651
maxDownloadBytes,
Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
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+
it('parses an external URL without saving a copy to workspace Files', async () => {
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+
}),
100+
workspaceId: fixture.workspaceId,
101+
workflowId: generateId(),
102+
attributedUserId: fixture.aliceId,
103+
fileAccessUserId: fixture.aliceId,
104+
}
105+
)
106+
const body = await response.json()
107+
108+
expect(response.status).toBe(200)
109+
expect(body.output.content).toContain('fetched page body')
110+
const rows = await db
111+
.select({ id: workspaceFiles.id })
112+
.from(workspaceFiles)
113+
.where(eq(workspaceFiles.workspaceId, fixture.workspaceId))
114+
expect(rows).toEqual([])
115+
})
116+
117+
it('scopes name lookups to root or folder through the unique name index', async () => {
118+
const fixture = await seedWorkspace()
119+
const folder = await createWorkspaceFileFolder({
120+
workspaceId: fixture.workspaceId,
121+
userId: fixture.aliceId,
122+
name: 'Reports',
123+
})
124+
const rootFile = await upload(fixture.workspaceId, fixture.aliceId, 'root.txt')
125+
const folderFile = await upload(fixture.workspaceId, fixture.aliceId, 'nested.txt', folder.id)
126+
127+
expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'root.txt', null)).toBe(true)
128+
expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'root.txt', folder.id)).toBe(
129+
false
130+
)
131+
expect(await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'nested.txt', null)).toBe(
132+
false
133+
)
134+
expect(
135+
await fileNameExistsInWorkspaceFolder(fixture.workspaceId, 'nested.txt', folder.id)
136+
).toBe(true)
137+
expect((await getWorkspaceFileByName(fixture.workspaceId, 'root.txt'))?.id).toBe(rootFile.id)
138+
expect(
139+
await getWorkspaceFileByName(fixture.workspaceId, 'root.txt', { folderId: folder.id })
140+
).toBeNull()
141+
expect(
142+
(await getWorkspaceFileByName(fixture.workspaceId, 'nested.txt', { folderId: folder.id }))?.id
143+
).toBe(folderFile.id)
144+
expect(await getWorkspaceFileByName(fixture.workspaceId, 'nested.txt')).toBeNull()
145+
146+
await db.execute(sql`
147+
INSERT INTO ${workspaceFiles} (id, key, user_id, workspace_id, folder_id, context, original_name, content_type)
148+
SELECT 'wf_pad_' || n || '_' || ${fixture.workspaceId}, 'pad/' || n || '/' || ${fixture.workspaceId},
149+
${fixture.aliceId}, ${fixture.workspaceId}, CASE WHEN n % 2 = 0 THEN ${folder.id} END,
150+
'workspace', 'pad-' || n || '.txt', 'text/plain'
151+
FROM generate_series(1, 2000) AS n`)
152+
await db.execute(sql`ANALYZE ${workspaceFiles}`)
153+
154+
for (const folderId of [null, folder.id]) {
155+
const plan = await db.execute(
156+
sql`EXPLAIN (FORMAT JSON) SELECT id FROM ${workspaceFiles} WHERE ${and(
157+
eq(workspaceFiles.workspaceId, fixture.workspaceId),
158+
eq(workspaceFiles.originalName, 'root.txt'),
159+
eq(workspaceFiles.context, 'workspace'),
160+
workspaceFileNameFolderCondition(folderId),
161+
isNull(workspaceFiles.deletedAt)
162+
)}`
163+
)
164+
const scan = JSON.stringify(plan[0]['QUERY PLAN'])
165+
expect(scan).toContain('workspace_files_workspace_folder_name_active_unique')
166+
expect(scan).toMatch(/"Index Cond":"[^"]*COALESCE\(folder_id/)
167+
}
168+
})
169+
170+
it('falls back to a short-id suffix after 20 numbered copies, including under concurrency', async () => {
171+
const fixture = await seedWorkspace()
172+
await upload(fixture.workspaceId, fixture.aliceId, 'page.html')
173+
for (let n = 1; n <= 20; n++) {
174+
await upload(fixture.workspaceId, fixture.aliceId, `page (${n}).html`)
175+
}
176+
177+
const next = await upload(fixture.workspaceId, fixture.aliceId, 'page.html')
178+
const concurrent = await Promise.all(
179+
Array.from({ length: 8 }, () => upload(fixture.workspaceId, fixture.aliceId, 'page.html'))
180+
)
181+
182+
const shortIdSuffixed = /^page \([A-Za-z0-9_-]{8}\)\.html$/
183+
expect(next.name).toMatch(shortIdSuffixed)
184+
const names = concurrent.map((file) => file.name)
185+
for (const name of names) expect(name).toMatch(shortIdSuffixed)
186+
expect(new Set([next.name, ...names]).size).toBe(names.length + 1)
187+
})
188+
})

0 commit comments

Comments
 (0)