Skip to content

Commit fcd000a

Browse files
committed
fix(files): address the paged file reads' third review round
- Type labels and extension MIME types read their tables by own key only, so a type or extension such as `constructor` or `__proto__` never resolves to an `Object.prototype` member; the parity test covers them. - A serve URL's fragment, like its query, is no part of its storage key. - Workflow search-replace offers only files answering what is typed now — the recent files with nothing typed, else the settled matches for the current query — never a previous query's (`currentFiles`).
1 parent 6cd7537 commit fcd000a

9 files changed

Lines changed: 59 additions & 12 deletions

File tree

‎apps/sim/hooks/queries/workflow-search-replace.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -712,10 +712,15 @@ export function useWorkflowSearchFileReplacementOptions(
712712
[matches]
713713
)
714714
const enabled = Boolean(workspaceId && fileGroups.length > 0)
715-
const { files: offered } = useWorkspaceFilePickerOptions(workspaceId ?? '', options.query, {
716-
limit: RECENT_REPLACEMENT_FILE_LIMIT,
717-
enabled,
718-
})
715+
/** Never a previous query's matches: a choice offered must answer what is typed now. */
716+
const { currentFiles: offered } = useWorkspaceFilePickerOptions(
717+
workspaceId ?? '',
718+
options.query,
719+
{
720+
limit: RECENT_REPLACEMENT_FILE_LIMIT,
721+
enabled,
722+
}
723+
)
719724
const chosenKeys = useMemo(
720725
() =>
721726
options.chosen.flatMap((value) => {

‎apps/sim/hooks/queries/workspace-files.test.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -850,6 +850,7 @@ describe('useWorkspaceFilePickerOptions', () => {
850850
await vi.waitFor(() => expect(hook.result().files?.map((file) => file.id)).toEqual(['match']))
851851
expect(hook.result().matchedQuery).toBe('mat')
852852
expect(hook.result().isPending).toBe(false)
853+
expect(hook.result().currentFiles?.map((file) => file.id)).toEqual(['match'])
853854
} finally {
854855
hook.unmount()
855856
}
@@ -873,6 +874,8 @@ describe('useWorkspaceFilePickerOptions', () => {
873874
await vi.waitFor(() => expect(mockRequestJson).toHaveBeenCalledTimes(3))
874875
expect(hook.result().files?.map((file) => file.id)).toEqual(['match'])
875876
expect(hook.result().matchedQuery).toBe('')
877+
/** What is offered as-is never holds the previous query's matches. */
878+
expect(hook.result().currentFiles).toBeUndefined()
876879
} finally {
877880
hook.unmount()
878881
}

‎apps/sim/hooks/queries/workspace-files.ts‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -499,6 +499,13 @@ export interface WorkspaceFilePickerOptions {
499499
* `files` are the recent files, or a previous query's matches held while the next loads.
500500
*/
501501
matchedQuery: string
502+
/**
503+
* `files` only while they answer exactly what is typed now: the recent files with nothing
504+
* typed, the server's matches once they are in for the current query — never a previous
505+
* query's matches, nor the recent files while a query settles. For a caller that offers the
506+
* files as they are rather than filtering them.
507+
*/
508+
currentFiles: WorkspaceFileRecord[] | undefined
502509
/** True until `files` answers what was typed: a menu still empty here is not "no match" yet. */
503510
isPending: boolean
504511
}
@@ -526,9 +533,13 @@ export function useWorkspaceFilePickerOptions(
526533
{ q: settledQuery, limit: WORKSPACE_FILE_PICKER_LIMIT },
527534
{ enabled }
528535
)
536+
const files = settledQuery ? matching.data : recent.data?.pages[0]?.files
537+
const matchedQuery =
538+
settledQuery && matching.data && !matching.isPlaceholderData ? settledQuery : ''
529539
return {
530-
files: settledQuery ? matching.data : recent.data?.pages[0]?.files,
531-
matchedQuery: settledQuery && matching.data && !matching.isPlaceholderData ? settledQuery : '',
540+
files,
541+
matchedQuery,
542+
currentFiles: !typed || matchedQuery === typed ? files : undefined,
532543
isPending: settledQuery
533544
? typed !== settledQuery || matching.isFetching
534545
: recent.isPending || typed.length > 0,

‎apps/sim/lib/uploads/utils/file.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,15 @@ describe('storageKeyOfStoredValue', () => {
196196
)
197197
})
198198

199+
it("drops a serve URL's fragment as well as its query", () => {
200+
expect(storageKeyOfStoredValue('/api/files/serve/workspace%2Fws-1%2Fa.png#page=2')).toBe(
201+
'workspace/ws-1/a.png'
202+
)
203+
expect(
204+
storageKeyOfStoredValue('/api/files/serve/workspace%2Fws-1%2Fa.png?context=workspace#x')
205+
).toBe('workspace/ws-1/a.png')
206+
})
207+
199208
it('keeps any other value whole, `?` and `#` included', () => {
200209
expect(storageKeyOfStoredValue('workspace/ws-1/what?.md')).toBe('workspace/ws-1/what?.md')
201210
expect(storageKeyOfStoredValue('notes #1?.md')).toBe('notes #1?.md')

‎apps/sim/lib/uploads/utils/file.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,9 @@ function identifiesFormat(declared: string | undefined): declared is string {
413413
* Get MIME type from file extension (fallback if not provided)
414414
*/
415415
export function getMimeTypeFromExtension(extension: string): string {
416-
return EXTENSION_TO_MIME[extension.toLowerCase()] || GENERIC_MIME_TYPE
416+
const key = extension.toLowerCase()
417+
// Own keys only: an extension such as `constructor` must not read an `Object.prototype` member.
418+
return (Object.hasOwn(EXTENSION_TO_MIME, key) && EXTENSION_TO_MIME[key]) || GENERIC_MIME_TYPE
417419
}
418420

419421
/**
@@ -435,7 +437,9 @@ export function resolveEffectiveMimeType(
435437
if (identifiesFormat(declared)) return declared
436438

437439
const extension = getFileExtension(filename)
438-
return DUAL_CONTAINER_MIME[extension] ?? getMimeTypeFromExtension(extension)
440+
return Object.hasOwn(DUAL_CONTAINER_MIME, extension)
441+
? DUAL_CONTAINER_MIME[extension]
442+
: getMimeTypeFromExtension(extension)
439443
}
440444

441445
const MEDIA_FALLBACK_MIME = { audio: 'audio/mpeg', video: 'video/mp4' } as const
@@ -694,7 +698,8 @@ export function extractStorageKey(filePath: string): string {
694698
export function storageKeyOfStoredValue(value: string): string | undefined {
695699
if (!isInternalFileUrl(value)) return value
696700
try {
697-
return extractStorageKey(value)
701+
// The key is the path alone: a URL's query and fragment are no part of it.
702+
return extractStorageKey(value.split(/[?#]/, 1)[0])
698703
} catch {
699704
return undefined
700705
}

‎apps/sim/lib/workspace-files/__integration__/file-listing.integration.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -704,6 +704,10 @@ describe('bounded workspace file reads in PostgreSQL', () => {
704704
{ name: 'clip.bin', contentType: 'video/ogg' },
705705
{ name: 'song.bin', contentType: 'Audio/X-Unlisted; rate=44100' },
706706
{ name: 'clip.vid', contentType: 'video/x-unlisted' },
707+
{ name: 'file.constructor', contentType: '' },
708+
{ name: 'file.__proto__', contentType: 'application/octet-stream' },
709+
{ name: 'noext', contentType: 'constructor' },
710+
{ name: 'noext', contentType: 'toString' },
707711
{ name: 'file.foo', contentType: 'application/octet-stream' },
708712
{ name: 'file.foo', contentType: '' },
709713
]

‎apps/sim/lib/workspace-files/browser.test.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,3 +31,12 @@ describe('fileBrowserKinds', () => {
3131
expect(fileBrowserKinds('application/octet-stream', 'notes.md').has('document')).toBe(true)
3232
})
3333
})
34+
35+
describe('table lookups by name', () => {
36+
it('never reads an Object.prototype member as a label or a MIME type', () => {
37+
expect(fileBrowserTypeLabel('constructor', 'noext')).toBe('constructor')
38+
expect(fileBrowserTypeLabel('toString', 'noext')).toBe('toString')
39+
expect(fileBrowserMimeType('', 'file.constructor')).toBe(fileBrowserMimeType('', 'file.zzz'))
40+
expect(fileBrowserTypeLabel('', 'file.__proto__')).toBe('__PROTO__')
41+
})
42+
})

‎apps/sim/lib/workspace-files/browser.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ export function fileBrowserKinds(storedType: string, filename: string): Set<File
9898
*/
9999
export function fileBrowserTypeLabel(storedType: string, filename: string): string {
100100
const mimeType = fileBrowserMimeType(storedType, filename)
101-
const label = FILE_BROWSER_MIME_LABELS[mimeType]
102-
if (label) return label
101+
// Own keys only, as the SQL twin's `jsonb ->>` reads them: `constructor` is no label.
102+
if (Object.hasOwn(FILE_BROWSER_MIME_LABELS, mimeType)) return FILE_BROWSER_MIME_LABELS[mimeType]
103103
if (mimeType.startsWith('audio/')) return 'Audio'
104104
if (mimeType.startsWith('video/')) return 'Video'
105105
if (mimeType.startsWith('image/')) return 'Image'

‎packages/testing/src/mocks/file-utils.mock.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,8 @@ function extractStorageKey(filePath: string): string {
377377
function storageKeyOfStoredValue(value: string): string | undefined {
378378
if (!isInternalFileUrl(value)) return value
379379
try {
380-
return extractStorageKey(value)
380+
// The key is the path alone: a URL's query and fragment are no part of it.
381+
return extractStorageKey(value.split(/[?#]/, 1)[0])
381382
} catch {
382383
return undefined
383384
}

0 commit comments

Comments
 (0)