diff --git a/apps/sim/app/api/files/export/[id]/route.test.ts b/apps/sim/app/api/files/export/[id]/route.test.ts index 069e1fc30a9..8b6e7b53d24 100644 --- a/apps/sim/app/api/files/export/[id]/route.test.ts +++ b/apps/sim/app/api/files/export/[id]/route.test.ts @@ -11,23 +11,29 @@ const { mockGetFileMetadataById, mockVerifyFileAccess, mockDownloadFile, - mockExtractEmbeddedImageIds, + mockExtractEmbeddedFileRefs, } = vi.hoisted(() => ({ mockCheckAuth: vi.fn(), mockGetFileMetadataById: vi.fn(), mockVerifyFileAccess: vi.fn(), mockDownloadFile: vi.fn(), - mockExtractEmbeddedImageIds: vi.fn(), + mockExtractEmbeddedFileRefs: vi.fn(), })) +/** `embedded-image-refs.test.ts` covers the grammar itself. */ +function embeds(...ids: string[]) { + mockExtractEmbeddedFileRefs.mockReturnValue({ keys: [], ids }) +} + vi.mock('@/lib/auth/hybrid', () => ({ checkSessionOrInternalAuth: mockCheckAuth })) vi.mock('@/lib/uploads/server/metadata', () => ({ getFileMetadataById: mockGetFileMetadataById, })) vi.mock('@/app/api/files/authorization', () => ({ verifyFileAccess: mockVerifyFileAccess })) vi.mock('@/lib/uploads/core/storage-service', () => ({ downloadFile: mockDownloadFile })) -vi.mock('@/lib/copilot/tools/server/files/embedded-image-refs', () => ({ - extractEmbeddedImageIds: mockExtractEmbeddedImageIds, +vi.mock('@/lib/uploads/server/embedded-image-refs', () => ({ + extractEmbeddedFileRefs: mockExtractEmbeddedFileRefs, + storedFileId: (spelledId: string) => decodeURIComponent(spelledId), })) vi.mock('@sim/audit', () => ({ recordAudit: vi.fn(), @@ -58,43 +64,35 @@ function assetRecord(id: string, size: number) { } } -describe('markdown export bundling', () => { - beforeEach(() => { - vi.clearAllMocks() - mockCheckAuth.mockResolvedValue({ success: true, userId: 'user-1' }) - mockVerifyFileAccess.mockResolvedValue(true) - mockGetFileMetadataById.mockImplementation(async (id: string) => - id === DOC_ID - ? { - id: DOC_ID, - key: 'workspace/ws-1/doc.md', - originalName: 'doc.md', - contentType: 'text/markdown', - context: 'workspace', - size: 1024, - workspaceId: 'ws-1', - } - : assetRecord(id, 1 * MB) - ) - mockDownloadFile.mockResolvedValue(Buffer.from('# Doc\n')) - mockExtractEmbeddedImageIds.mockReturnValue([]) - }) +const DOC_RECORD = { + id: DOC_ID, + key: 'workspace/ws-1/doc.md', + originalName: 'doc.md', + contentType: 'text/markdown', + context: 'workspace', + size: 1024, + workspaceId: 'ws-1', +} + +function assetsResolveTo(assetFor: (id: string) => unknown) { + mockGetFileMetadataById.mockImplementation(async (id: string) => + id === DOC_ID ? DOC_RECORD : assetFor(id) + ) +} + +beforeEach(() => { + vi.clearAllMocks() + mockCheckAuth.mockResolvedValue({ success: true, userId: 'user-1' }) + mockVerifyFileAccess.mockResolvedValue(true) + assetsResolveTo((id) => assetRecord(id, 1 * MB)) + mockDownloadFile.mockResolvedValue(Buffer.from('# Doc\n')) + embeds() +}) +describe('markdown export bundling', () => { it('rejects on declared asset bytes before downloading any of them', async () => { - mockExtractEmbeddedImageIds.mockReturnValue(['a', 'b', 'c']) - mockGetFileMetadataById.mockImplementation(async (id: string) => - id === DOC_ID - ? { - id: DOC_ID, - key: 'workspace/ws-1/doc.md', - originalName: 'doc.md', - contentType: 'text/markdown', - context: 'workspace', - size: 1024, - workspaceId: 'ws-1', - } - : assetRecord(id, 100 * MB) - ) + embeds('a', 'b', 'c') + assetsResolveTo((id) => assetRecord(id, 100 * MB)) const response = await GET(request(), context) @@ -106,7 +104,7 @@ describe('markdown export bundling', () => { it('counts the document body against the export limit, not just its assets', async () => { // Assets alone sit under the cap; the body is what carries the bundle over it. - mockExtractEmbeddedImageIds.mockReturnValue(['a']) + embeds('a') mockDownloadFile.mockResolvedValue(Buffer.alloc(250 * MB)) const response = await GET(request(), context) @@ -116,7 +114,7 @@ describe('markdown export bundling', () => { }) it('caps the document body read rather than loading it unbounded', async () => { - mockExtractEmbeddedImageIds.mockReturnValue([]) + embeds() await GET(request(), context) @@ -125,7 +123,7 @@ describe('markdown export bundling', () => { }) it('reports an oversized body as a size rejection, not a server error', async () => { - mockExtractEmbeddedImageIds.mockReturnValue([]) + embeds() mockDownloadFile.mockRejectedValue( new PayloadSizeLimitError({ label: 'storage file download', maxBytes: 1 }) ) @@ -138,7 +136,7 @@ describe('markdown export bundling', () => { }) it('caps each asset download rather than trusting its declared size', async () => { - mockExtractEmbeddedImageIds.mockReturnValue(['a']) + embeds('a') await GET(request(), context) @@ -149,7 +147,7 @@ describe('markdown export bundling', () => { }) it('drops an unreadable asset instead of failing the whole export', async () => { - mockExtractEmbeddedImageIds.mockReturnValue(['good', 'bad']) + embeds('good', 'bad') mockDownloadFile.mockImplementation(async ({ key }: { key: string }) => { if (key.endsWith('doc.md')) return Buffer.from('# Doc\n![x](/api/files/view/good)\n') if (key.endsWith('bad')) throw new Error('storage down') @@ -164,8 +162,31 @@ describe('markdown export bundling', () => { expect(zip.file('assets/bad.png')).toBeNull() }) + /** + * The two id representations have to stay distinct: metadata resolves by the stored id, while the + * rewrite finds the embed by the spelling the document used. Collapsing them either drops the + * asset or bundles it behind a link still pointing at the API. + */ + it('resolves and rewrites an embed whose id is percent-encoded in the document', async () => { + embeds('wf%5Fa') + assetsResolveTo((id) => (id === 'wf_a' ? assetRecord(id, 1 * MB) : null)) + mockDownloadFile.mockImplementation(async ({ key }: { key: string }) => + key.endsWith('doc.md') + ? Buffer.from('# Doc\n![x](/api/files/view/wf%5Fa)\n') + : Buffer.from('png-bytes') + ) + + const response = await GET(request(), context) + + const zip = await JSZip.loadAsync(Buffer.from(await response.arrayBuffer())) + expect(zip.file('assets/wf_a.png')).not.toBeNull() + const md = await zip.file('doc.md')?.async('string') + expect(md).toContain('./assets/wf_a.png') + expect(md).not.toContain('/api/files/view/') + }) + it('skips an asset the caller cannot read', async () => { - mockExtractEmbeddedImageIds.mockReturnValue(['secret']) + embeds('secret') mockVerifyFileAccess.mockImplementation(async (key: string) => !key.endsWith('secret')) const response = await GET(request(), context) @@ -177,3 +198,47 @@ describe('markdown export bundling', () => { ) }) }) + +describe('markdown export format', () => { + async function expectPlainMarkdown(response: Response) { + expect(response.status).toBe(200) + expect(response.headers.get('Content-Type')).toBe('text/markdown; charset=utf-8') + expect(response.headers.get('Content-Disposition')).toContain('doc.md') + expect(await response.text()).toBe('# Doc\n') + } + + it('returns the document itself when it embeds nothing', async () => { + await expectPlainMarkdown(await GET(request(), context)) + }) + + /** + * The reported bug: a document that references files which no longer resolve downloaded as a zip + * whose `assets/` folder was empty. The format follows what was bundled, not what was referenced. + */ + it('returns the document itself when no embed resolves to a file', async () => { + embeds('gone', 'also-gone') + assetsResolveTo(() => null) + + await expectPlainMarkdown(await GET(request(), context)) + }) + + it('returns the document itself when every embed fails to download', async () => { + embeds('a') + mockDownloadFile.mockImplementation(async ({ key }: { key: string }) => { + if (key.endsWith('doc.md')) return Buffer.from('# Doc\n') + throw new Error('storage down') + }) + + await expectPlainMarkdown(await GET(request(), context)) + }) + + it('bundles a zip once at least one embed resolves', async () => { + embeds('a') + + const response = await GET(request(), context) + + expect(response.headers.get('Content-Type')).toBe('application/zip') + const zip = await JSZip.loadAsync(Buffer.from(await response.arrayBuffer())) + expect(zip.file('assets/a.png')).not.toBeNull() + }) +}) diff --git a/apps/sim/app/api/files/export/[id]/route.ts b/apps/sim/app/api/files/export/[id]/route.ts index 0e578ada87b..ceb2d6d118e 100644 --- a/apps/sim/app/api/files/export/[id]/route.ts +++ b/apps/sim/app/api/files/export/[id]/route.ts @@ -8,7 +8,6 @@ import { NextResponse } from 'next/server' import { fileExportContract } from '@/lib/api/contracts/storage-transfer' import { parseRequest } from '@/lib/api/server' import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid' -import { extractEmbeddedImageIds } from '@/lib/copilot/tools/server/files/embedded-image-refs' import { MATERIALIZE_CONCURRENCY, mapWithConcurrency } from '@/lib/core/utils/concurrency' import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' @@ -16,6 +15,7 @@ import { captureServerEvent } from '@/lib/posthog/server' import type { StorageContext } from '@/lib/uploads/config' import { getServeStoragePrefix } from '@/lib/uploads/config' import { downloadFile } from '@/lib/uploads/core/storage-service' +import { extractEmbeddedFileRefs, storedFileId } from '@/lib/uploads/server/embedded-image-refs' import { getFileMetadataById } from '@/lib/uploads/server/metadata' import { formatFileSize } from '@/lib/uploads/utils/file-utils' import { verifyFileAccess } from '@/app/api/files/authorization' @@ -149,30 +149,18 @@ export const GET = withRouteHandler( } let mdContent = mdBuffer.toString('utf-8') - const imageIds = extractEmbeddedImageIds(mdContent) + // Ids only: a serve-URL embed names a storage key, which the bundler has no id to rewrite the + // markdown against, so those images stay pointed at their original URL. + const { ids: imageIds } = extractEmbeddedFileRefs(mdContent) logger.info('Exporting markdown', { id, imageCount: imageIds.length }) - if (imageIds.length === 0) { - const mdName = safeFilename(record.originalName) - const mdBytes = Buffer.from(mdContent, 'utf-8') - auditExport('markdown', 0) - return new NextResponse(new Uint8Array(mdBytes), { - status: 200, - headers: { - 'Content-Type': 'text/markdown; charset=utf-8', - 'Content-Disposition': `attachment; ${encodeFilenameForHeader(mdName)}`, - 'Content-Length': String(mdBytes.length), - }, - }) - } - // Metadata first: declared sizes bound the download before a byte is read, and the // authorization check costs nothing to run here. const assetTargets = ( await mapWithConcurrency(imageIds, MATERIALIZE_CONCURRENCY, async (imageId) => { try { - const imgRecord = await getFileMetadataById(imageId) + const imgRecord = await getFileMetadataById(storedFileId(imageId)) if (!imgRecord) return null if (!(await verifyFileAccess(imgRecord.key, userId))) return null return { imageId, record: imgRecord } @@ -234,6 +222,21 @@ export const GET = withRouteHandler( assetMap.set(imageId, { filename, buffer }) } + // Format follows what was bundled, not what was referenced: an embed can point at a file that is + // missing, unreadable, or oversized, and an empty `assets/` zip is a worse answer than the + // document itself. `mdContent` is still unrewritten here, so `mdBuffer` holds exactly its bytes. + if (assetMap.size === 0) { + auditExport('markdown', 0) + return new NextResponse(new Uint8Array(mdBuffer), { + status: 200, + headers: { + 'Content-Type': 'text/markdown; charset=utf-8', + 'Content-Disposition': `attachment; ${encodeFilenameForHeader(safeFilename(record.originalName))}`, + 'Content-Length': String(mdBuffer.length), + }, + }) + } + for (const [imageId, asset] of assetMap) { const escapedId = imageId.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') const replacement = `./assets/${asset.filename}` diff --git a/apps/sim/app/api/files/public/[token]/inline/route.ts b/apps/sim/app/api/files/public/[token]/inline/route.ts index a777b953ba5..85405a74712 100644 --- a/apps/sim/app/api/files/public/[token]/inline/route.ts +++ b/apps/sim/app/api/files/public/[token]/inline/route.ts @@ -4,16 +4,13 @@ import type { NextRequest } from 'next/server' import { NextResponse } from 'next/server' import { getPublicInlineFileContract } from '@/lib/api/contracts/public-shares' import { parseRequest } from '@/lib/api/server' -import { - extractEmbeddedImageIds, - extractEmbeddedImageKeys, -} from '@/lib/copilot/tools/server/files/embedded-image-refs' import { validateDeploymentAuth } from '@/lib/core/security/deployment-auth' import { generateRequestId } from '@/lib/core/utils/request' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' import { enforcePublicFileRateLimit } from '@/lib/public-shares/rate-limit' import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager' import { downloadFile } from '@/lib/uploads/core/storage-service' +import { extractEmbeddedFileRefs } from '@/lib/uploads/server/embedded-image-refs' import { resolveWorkspaceInlineImage } from '@/lib/uploads/server/inline-image' import { serveInlineImage } from '@/app/api/files/serve-inline-image' import { createErrorResponse, FileNotFoundError } from '@/app/api/files/utils' @@ -29,8 +26,9 @@ const logger = createLogger('PublicInlineFileAPI') * instead of broken icons. The share grants the document bytes; this route extends that grant to the * document's referenced images only, behind three gates that together hold the security boundary: * - * 1. Referenced-by-doc — the requested key/id must appear in the shared document's current bytes. The - * token is a capability for the document and its embeds, never an arbitrary workspace file. + * 1. Referenced-by-doc — the requested key/id must be embedded as an image by the shared document's + * current bytes. The token is a capability for the document and its embeds, never an arbitrary + * workspace file, and never one the document merely links to or mentions in prose. * 2. Same-workspace — the referenced file must be a `workspace` file in the document's own workspace * ({@link resolveWorkspaceInlineImage}). This blocks any cross-workspace reference (which an author * can write but must never resolve) from loading. @@ -74,9 +72,8 @@ export const GET = withRouteHandler( // Referenced-by-doc gate: the share grants exactly the images the document embeds. const docText = (await downloadFile({ key: doc.key, context: 'workspace' })).toString('utf-8') - const referenced = ref.fileId - ? extractEmbeddedImageIds(docText).includes(ref.fileId) - : extractEmbeddedImageKeys(docText).includes(ref.key as string) + const { keys, ids } = extractEmbeddedFileRefs(docText) + const referenced = ref.fileId ? ids.includes(ref.fileId) : keys.includes(ref.key as string) if (!referenced) { throw new FileNotFoundError('Not found') } diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.test.ts index fff5f035a81..15f4f1fbd28 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.test.ts @@ -11,7 +11,6 @@ import { import { createMarkdownEditorExtensions } from './editor-extensions' import { extractImageFiles, - extractImgSrcs, findHostedImageAttrs, hasHostedImageHtml, htmlReferencesSrc, @@ -151,19 +150,6 @@ describe('hasHostedImageHtml', () => { }) }) -describe('extractImgSrcs', () => { - it('extracts every img src in document order, including duplicates', () => { - expect( - extractImgSrcs('

text

') - ).toEqual(['/a.png', '/b.png', '/a.png']) - }) - - it('returns an empty array for html with no img', () => { - expect(extractImgSrcs('

hello

')).toEqual([]) - expect(extractImgSrcs('')).toEqual([]) - }) -}) - describe('shouldSkipFileUpload (shared by paste and drop)', () => { const isHosted = (src: string) => src.startsWith('/api/files/view/') const hostedHtml = '' diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.ts index c3edc938661..9bd35bfc1fa 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/image-paste.ts @@ -1,3 +1,5 @@ +import { extractImgSrcs } from '@/lib/uploads/utils/embedded-image-ref' + /** * Extract image `File` objects from a paste/drop payload. Reads `files` first, then falls back to * `items` — many browsers expose a pasted or copied image (e.g. a screenshot) only through @@ -13,13 +15,6 @@ export function extractImageFiles(transfer: DataTransfer | null): File[] { .filter((file): file is File => file !== null) } -/** - * Matches `` `src` attribute values: double-quoted, single-quoted, or (validly) unquoted per - * the HTML spec — the browser's own clipboard serialization always quotes it, but other producers - * of `text/html` are not obligated to. - */ -const IMG_SRC_RE = /]*\bsrc\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s"'>]+))/gi - /** Query params under which the inline route addresses a workspace file. */ const INLINE_ROUTE_QUERY_KEYS = new Set(['key', 'fileId']) @@ -80,18 +75,6 @@ export function isInlineRouteSrc(src: string, origin = runtimeOrigin()): boolean } } -/** - * Extracts every `` `src` value found in `html`, in document order (may contain duplicates). - */ -export function extractImgSrcs(html: string): string[] { - const srcs: string[] = [] - for (const match of html.matchAll(IMG_SRC_RE)) { - const src = match[1] ?? match[2] ?? match[3] - if (src) srcs.push(src) - } - return srcs -} - /** * True when `html` contains an `` whose `src` is already one of our own hosted workspace file * references. Copying a rendered `` that's already on the page (e.g. Cmd+C after clicking it to diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx index 0a53b21cce5..92e05442fac 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/rich-markdown-editor.tsx @@ -14,7 +14,7 @@ import { truncateSelectionText, } from '@/lib/copilot/chat/selection-context' import type { WorkspaceFileRecord } from '@/lib/uploads/contexts/workspace' -import { extractEmbeddedFileRef } from '@/lib/uploads/utils/embedded-image-ref' +import { extractEmbeddedFileRef, extractImgSrcs } from '@/lib/uploads/utils/embedded-image-ref' import { isUntitledName } from '@/app/workspace/[workspaceId]/files/untitled-title' import { useUploadWorkspaceFile } from '@/hooks/queries/workspace-files' import { useAddToChat } from '@/hooks/use-add-to-chat' @@ -40,12 +40,7 @@ import { useFileDocCollaboration } from './collaboration/use-file-doc-collaborat import { createMarkdownEditorExtensions } from './editor-extensions' import { findHeadingPos } from './heading-anchors' import { moveDraggedImageNode } from './image-drag-move' -import { - extractImageFiles, - extractImgSrcs, - findHostedImageAttrs, - shouldSkipFileUpload, -} from './image-paste' +import { extractImageFiles, findHostedImageAttrs, shouldSkipFileUpload } from './image-paste' import { applyFrontmatter, normalizeLinkHref, diff --git a/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.test.ts b/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.test.ts index d4c33793f3f..c1c0c0c64ea 100644 --- a/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.test.ts +++ b/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.test.ts @@ -1,43 +1,62 @@ -import { describe, expect, it } from 'vitest' -import { - extractEmbeddedImageIds, - extractEmbeddedImageKeys, -} from '@/lib/copilot/tools/server/files/embedded-image-refs' - -const KEY = 'workspace/W1/1700000000000-deadbeefdeadbeef-photo.png' - -describe('extractEmbeddedImageIds', () => { - it('extracts unique ids from view-url and in-app-path embeds (wf_ and uuid)', () => { - const a = 'wf_YwDXi8eWOkTxn0sbgChlB' - const b = '4bdaf6c4-072e-464e-891d-b6af3b5fe2cc' - const content = `![x](/api/files/view/${a}) ![y](/workspace/W1/files/${b}) ![dup](/api/files/view/${a})` - expect(extractEmbeddedImageIds(content).sort()).toEqual([b, a].sort()) - }) +/** + * @vitest-environment node + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockGetFileMetadataById } = vi.hoisted(() => ({ + mockGetFileMetadataById: vi.fn(), +})) + +vi.mock('@/lib/uploads/server/metadata', () => ({ + getFileMetadataById: mockGetFileMetadataById, +})) + +import { findUnembeddableImageRefs } from '@/lib/copilot/tools/server/files/embedded-image-refs' + +const WORKSPACE_ID = 'W1' - it('ignores serve-url, external, and plain content', () => { - expect( - extractEmbeddedImageIds(`![a](/api/files/serve/${encodeURIComponent(KEY)}) plain`) - ).toEqual([]) +describe('findUnembeddableImageRefs', () => { + beforeEach(() => { + vi.clearAllMocks() }) - it('caps the result at 50 ids', () => { - const content = Array.from( - { length: 60 }, - (_, i) => `/api/files/view/wf_${String(i).padStart(6, '0')}` - ).join(' ') - expect(extractEmbeddedImageIds(content)).toHaveLength(50) + it('flags embeds that are not workspace files in this workspace', async () => { + mockGetFileMetadataById.mockImplementation(async (id: string) => { + if (id === 'wf_here') return { context: 'workspace', workspaceId: WORKSPACE_ID } + if (id === 'wf_elsewhere') return { context: 'workspace', workspaceId: 'W2' } + if (id === 'wf_chat') return { context: 'mothership', workspaceId: WORKSPACE_ID } + return null + }) + + const content = `![a](/api/files/view/wf_here) ![b](/api/files/view/wf_elsewhere) + ![c](/api/files/view/wf_chat) ![d](/api/files/view/wf_missing)` + + expect((await findUnembeddableImageRefs(content, WORKSPACE_ID)).sort()).toEqual([ + 'wf_chat', + 'wf_elsewhere', + 'wf_missing', + ]) }) -}) -describe('extractEmbeddedImageKeys', () => { - it('extracts decoded workspace keys from serve-url embeds (encoded + s3/blob prefixed)', () => { - const content = `![a](/api/files/serve/${encodeURIComponent(KEY)}?context=workspace) ![b](/api/files/serve/s3/${encodeURIComponent(KEY)})` - expect(extractEmbeddedImageKeys(content)).toEqual([KEY]) + it('never warns about a url the document only mentions', async () => { + const content = 'Call `/api/files/view/{id}`; see [the docs](/api/files/view/wf_linked).' + + expect(await findUnembeddableImageRefs(content, WORKSPACE_ID)).toEqual([]) + expect(mockGetFileMetadataById).not.toHaveBeenCalled() }) - it('drops non-workspace keys (e.g. public profile pictures) and view-url embeds', () => { - const content = - '![a](/api/files/serve/profile-pictures%2Fu1%2Favatar.png) ![b](/api/files/view/wf_abc)' - expect(extractEmbeddedImageKeys(content)).toEqual([]) + /** + * The export bundler resolves an embed by its stored id, so reporting the same embed as one that + * will not survive an export would contradict what the export actually does with it. + */ + it('resolves a percent-encoded embed by its stored id, like the export does', async () => { + mockGetFileMetadataById.mockImplementation(async (id: string) => + id === 'wf_abc' ? { context: 'workspace', workspaceId: WORKSPACE_ID } : null + ) + + expect(await findUnembeddableImageRefs('![a](/api/files/view/wf%5Fabc)', WORKSPACE_ID)).toEqual( + [] + ) + expect(mockGetFileMetadataById).toHaveBeenCalledWith('wf_abc') }) }) diff --git a/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.ts b/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.ts index f5f7b9fbbad..569ef320f00 100644 --- a/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.ts +++ b/apps/sim/lib/copilot/tools/server/files/embedded-image-refs.ts @@ -1,47 +1,28 @@ +import { extractEmbeddedFileRefs, storedFileId } from '@/lib/uploads/server/embedded-image-refs' import { getFileMetadataById } from '@/lib/uploads/server/metadata' -import { extractEmbeddedFileRefs } from '@/lib/uploads/utils/embedded-image-ref' - -/** View-URL embed (`/api/files/view/`) — the only form the file agent writes; see {@link findUnembeddableImageRefs}. */ -const VIEW_EMBED_RE = /\/api\/files\/view\/([A-Za-z0-9_-]+)/g - -/** - * De-duplicated workspace file **ids** embedded in `content` (view URL or in-app workspace path). - * Shares the {@link extractEmbeddedFileRefs} grammar with the frontend renderer so the referenced-by-doc - * gate authorizes exactly what the client links. Resolution and access are checked by the caller. - */ -export function extractEmbeddedImageIds(content: string): string[] { - return extractEmbeddedFileRefs(content).ids -} - -/** - * De-duplicated workspace storage **keys** (`workspace//…`) embedded in `content` via the serve URL. - * Same shared grammar as {@link extractEmbeddedImageIds}. - */ -export function extractEmbeddedImageKeys(content: string): string[] { - return extractEmbeddedFileRefs(content).keys -} /** - * Returns the ids of `/api/files/view/` image embeds in `content` that will not render or survive a - * workspace export. An embed is valid only when its id resolves to a workspace file in this same - * workspace — the only thing the view route serves and an export can bundle. Every other case (missing, - * archived, a different workspace, or a non-`workspace` upload such as a chat-scoped `mothership` file) - * is flagged by id alone, without disclosing the referenced file's real context or owning workspace, so + * Returns the ids of image embeds in `content` that will not render or survive a workspace export. + * An embed is valid only when its id resolves to a workspace file in this same workspace — the only + * thing the view route serves and an export can bundle. Every other case (missing, archived, a + * different workspace, or a non-`workspace` upload such as a chat-scoped `mothership` file) is + * flagged by id alone, without disclosing the referenced file's real context or owning workspace, so * the result can't be used to probe files outside this workspace. Best-effort and never throws, so a * content write is never blocked by this validation. + * + * Resolved through {@link storedFileId}, like the export bundler: an embed the export would resolve + * and bundle must not be reported here as one that will not survive it. */ export async function findUnembeddableImageRefs( content: string, workspaceId: string ): Promise { - const ids = new Set() - for (const match of content.matchAll(VIEW_EMBED_RE)) ids.add(match[1]) - if (ids.size === 0) return [] + const { ids } = extractEmbeddedFileRefs(content) const checked = await Promise.all( - [...ids].map(async (id): Promise => { + ids.map(async (id): Promise => { try { - const record = await getFileMetadataById(id) + const record = await getFileMetadataById(storedFileId(id)) const embeddable = record?.context === 'workspace' && record.workspaceId === workspaceId return embeddable ? null : id } catch { diff --git a/apps/sim/lib/uploads/server/embedded-image-refs.test.ts b/apps/sim/lib/uploads/server/embedded-image-refs.test.ts new file mode 100644 index 00000000000..8c4f2a60ce4 --- /dev/null +++ b/apps/sim/lib/uploads/server/embedded-image-refs.test.ts @@ -0,0 +1,66 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { extractEmbeddedFileRefs } from '@/lib/uploads/server/embedded-image-refs' + +const KEY = 'workspace/W1/1700000000000-deadbeefdeadbeef-photo.png' +const ENCODED = encodeURIComponent(KEY) + +describe('extractEmbeddedFileRefs', () => { + it('collects de-duplicated keys and ids from the images a document embeds', () => { + const content = [ + `![a](/api/files/serve/${ENCODED}?context=workspace)`, + '![b](/api/files/view/wf_abc)', + '![c](/workspace/W1/files/4bdaf6c4-072e-464e-891d-b6af3b5fe2cc)', + `![dup](/api/files/serve/s3/${ENCODED})`, + '![ext](https://cdn.example.com/x.png)', + '![pub](/api/files/serve/profile-pictures%2Fu1%2Favatar.png)', + ].join('\n\n') + const { keys, ids } = extractEmbeddedFileRefs(content) + expect(keys).toEqual([KEY]) + expect(ids.sort()).toEqual(['4bdaf6c4-072e-464e-891d-b6af3b5fe2cc', 'wf_abc'].sort()) + }) + + it('resolves reference-style images and raw tags', () => { + const content = [ + '![a][ref]', + 'b', + 'inline in a sentence', + '[ref]: /api/files/view/wf_reference', + ].join('\n\n') + expect(extractEmbeddedFileRefs(content).ids.sort()).toEqual([ + 'wf_html', + 'wf_inline', + 'wf_reference', + ]) + }) + + it('ignores a document that only mentions embed urls without displaying them', () => { + const content = [ + 'The `/api/files/serve/{key}` and `/api/files/view/{id}` endpoints return 401.', + 'A bare url like /api/files/view/wf_mentioned is prose, not an embed.', + '[a link](/api/files/view/wf_linked) is navigated to, not displayed.', + '', + '```http', + `GET /api/files/serve/${ENCODED}`, + 'GET /workspace/W1/files/wf_fenced', + '```', + '', + '
', + ].join('\n') + expect(extractEmbeddedFileRefs(content)).toEqual({ keys: [], ids: [] }) + }) + + it('caps total references (keys + ids) at 50 combined', () => { + const images = [ + ...Array.from({ length: 40 }, (_, i) => `![](/api/files/view/wf_${i})`), + ...Array.from( + { length: 40 }, + (_, i) => `![](/api/files/serve/${encodeURIComponent(`workspace/W1/k${i}.png`)})` + ), + ] + const { keys, ids } = extractEmbeddedFileRefs(images.join('\n\n')) + expect(keys.length + ids.length).toBe(50) + }) +}) diff --git a/apps/sim/lib/uploads/server/embedded-image-refs.ts b/apps/sim/lib/uploads/server/embedded-image-refs.ts new file mode 100644 index 00000000000..fb3574e20df --- /dev/null +++ b/apps/sim/lib/uploads/server/embedded-image-refs.ts @@ -0,0 +1,98 @@ +/** + * Finds the workspace files a markdown document embeds as images. + * + * Lives under `server/` rather than beside its pure sibling in `lib/uploads/utils` so that `marked` + * stays out of the client bundles that import the single-`src` grammar. + */ +import { Marked, type Token } from 'marked' +import { extractEmbeddedFileRef, extractImgSrcs } from '@/lib/uploads/utils/embedded-image-ref' + +/** Hard cap on embedded images resolved from one document — bounds export bundles and the share cascade. */ +export const MAX_EMBEDDED_IMAGES = 50 + +/** + * The stored id behind the spelling a document used. {@link extractEmbeddedFileRefs} returns ids + * exactly as the document writes them, because that is what the export bundler searches for when it + * rewrites an embed — but storage is keyed by the decoded id. Every consumer that resolves one of + * these ids goes through here, so a document's spelling and the stored id stay distinct without + * either side re-deriving the other. + * + * Only for ids read out of document text. Ids arriving as request input are already constrained to + * the plain id charset by their route contract, so they need no decoding. + */ +export function storedFileId(spelledId: string): string { + try { + return decodeURIComponent(spelledId) + } catch { + return spelledId + } +} + +/** + * A parser of this module's own, not the `marked` singleton: the public share's referenced-by-doc + * gate authorizes against what this returns, and a global `marked.use()` elsewhere in the process + * must not be able to redefine an authorization boundary. + */ +const markdown = new Marked() + +/** Children hang off `tokens`, except on tables (per cell) and lists (per item). */ +function childrenOf(token: Token): Token[] { + if (token.type === 'table') { + return [...token.header, ...token.rows.flat()].flatMap((cell) => cell.tokens) + } + if (token.type === 'list') return token.items + return 'tokens' in token && token.tokens ? token.tokens : [] +} + +/** + * The de-duplicated workspace keys and file ids `content` embeds **as images**, bounded to + * {@link MAX_EMBEDDED_IMAGES} references combined. Covers markdown images (`![alt](src)`, including + * the reference form the lexer resolves) and `` tags in raw HTML. + * + * Parsed with the markdown lexer rather than scanned as text, because only a parser can tell an + * embed from a mention. A document *about* the files API — prose, an inline `` `/api/files/view/{id}` ``, + * a fenced request sample — is full of strings that look like embed URLs but display nothing, and + * counting them as assets made every such document export as a zip with an empty `assets/` folder. + * Links are excluded for the same reason: a link is navigated to, not displayed, so it is neither an + * exportable asset nor something a document's public share should cascade to. + */ +export function extractEmbeddedFileRefs(content: string): { keys: string[]; ids: string[] } { + const keys = new Set() + const ids = new Set() + const atCap = () => keys.size + ids.size >= MAX_EMBEDDED_IMAGES + + const record = (src: string) => { + if (atCap()) return + const ref = extractEmbeddedFileRef(src) + if (!ref) return + if ('key' in ref) keys.add(ref.key) + else ids.add(ref.fileId) + } + + let tokens: Token[] + try { + tokens = markdown.lexer(content) + } catch { + // Best effort: a document that fails to lex must never block an export or a share. + return { keys: [], ids: [] } + } + + // Walked explicitly rather than with `marked.walkTokens`, which concatenates its callback's return + // value into an accumulator once per token and so costs O(n²): a 254 KB document measured 5.4s of + // blocked event loop versus 14ms here, on a path anonymous share traffic reaches. + const stack = [...tokens].reverse() + while (stack.length > 0 && !atCap()) { + const token = stack.pop() as Token + if (token.type === 'image') record(token.href) + else if (token.type === 'html') { + // `
`/`