fix(files): stop saving fetched URLs to Files and look names up by th… · simstudioai/sim@ce88d8a · GitHub
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

‎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

0 commit comments

Comments
 (0)