fix(files): count the document body against the export limit (#6006)

* fix(files): count the document body against the export limit The 250 MB export cap measured only the embedded assets' declared sizes. The markdown body was downloaded with no limit and never counted, so a large document with modest attachments cleared the check and still produced a zip well over the stated limit, materialized whole in memory. The body is the largest single entry in most bundles, so excluding it left the limit unenforced against the item most able to exceed it. It is now capped on read and counted alongside its assets, and the message names both. Follow-up to #5995, which introduced the asset caps without extending them to the body. * test(v1): cover the file download's rendered-bytes behavior The route had no tests, and #5995 changed what it serves: rendered bytes, the resolved content type rather than the record's source MIME, Content-Length from the rendered length, and a retryable 409 while an artifact compiles. One test pins the filename/content-type relationship. A review flagged the download as naming a rendered file with a source extension, but the renderer picks its output format from the file name — getE2BDocFormat and COMPILABLE_FORMATS both key on it — so a .docx renders to a docx and the two cannot disagree. The test makes that argument executable rather than a comment. * fix(files): report an oversized document body as a size rejection Capping the body read meant an oversized document threw PayloadSizeLimitError, which nothing caught, so the caller got a generic 500 — hiding the very limit message the cap was added to produce. It now returns the same 400 as the bundle check, naming the export limit. * chore(files): drop the unreachable export asset-count cap extractEmbeddedFileRefs stops collecting at MAX_EMBEDDED_IMAGES, so the list this route receives is already bounded before it arrives and the count check could never fire. Its test only passed because it mocked the extractor, so it asserted a branch production cannot reach. The byte ceilings are the real bound and stay. A comment records where the count is actually enforced, so the next reader does not add a second one.
2026-09-24 15:45:35 +08:00 · 2026-07-28 10:59:14 -07:00
parent 3df01e7de0
commit ec3156f1bc
3 changed files with 202 additions and 37 deletions
@@ -4,6 +4,7 @@
 import { createMockRequest } from '@sim/testing'
 import JSZip from 'jszip'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'

 const {
  mockCheckAuth,
@@ -79,19 +80,6 @@ describe('markdown export bundling', () => {
    mockExtractEmbeddedImageIds.mockReturnValue([])
  })

-  it('rejects a document embedding more assets than an export may bundle', async () => {
-    mockExtractEmbeddedImageIds.mockReturnValue(
-      Array.from({ length: 501 }, (_, index) => `img-${index}`)
-    )
-
-    const response = await GET(request(), context)
-
-    expect(response.status).toBe(400)
-    expect((await response.json()).error).toContain('501')
-    // Rejected on the embed count alone: nothing was resolved or downloaded.
-    expect(mockGetFileMetadataById).toHaveBeenCalledTimes(1)
-  })
-
  it('rejects on declared asset bytes before downloading any of them', async () => {
    mockExtractEmbeddedImageIds.mockReturnValue(['a', 'b', 'c'])
    mockGetFileMetadataById.mockImplementation(async (id: string) =>
@@ -116,6 +104,39 @@ describe('markdown export bundling', () => {
    expect(mockDownloadFile).toHaveBeenCalledTimes(1)
  })

+  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'])
+    mockDownloadFile.mockResolvedValue(Buffer.alloc(250 * MB))
+
+    const response = await GET(request(), context)
+
+    expect(response.status).toBe(400)
+    expect((await response.json()).error).toContain('document and its embedded files')
+  })
+
+  it('caps the document body read rather than loading it unbounded', async () => {
+    mockExtractEmbeddedImageIds.mockReturnValue([])
+
+    await GET(request(), context)
+
+    const bodyCall = mockDownloadFile.mock.calls.find(([options]) => options.key.endsWith('doc.md'))
+    expect(bodyCall?.[0].maxBytes).toBe(250 * MB)
+  })
+
+  it('reports an oversized body as a size rejection, not a server error', async () => {
+    mockExtractEmbeddedImageIds.mockReturnValue([])
+    mockDownloadFile.mockRejectedValue(
+      new PayloadSizeLimitError({ label: 'storage file download', maxBytes: 1 })
+    )
+
+    const response = await GET(request(), context)
+
+    // The cap exists to produce a clear limit message; a 500 would hide it.
+    expect(response.status).toBe(400)
+    expect((await response.json()).error).toContain('export limit')
+  })
+
  it('caps each asset download rather than trusting its declared size', async () => {
    mockExtractEmbeddedImageIds.mockReturnValue(['a'])

@@ -10,6 +10,7 @@ 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'
 import { captureServerEvent } from '@/lib/posthog/server'
 import type { StorageContext } from '@/lib/uploads/config'
@@ -23,16 +24,14 @@ import { encodeFilenameForHeader } from '@/app/api/files/utils'
 const logger = createLogger('FilesExportAPI')

 /**
- * Bundling caps. The embed list comes from scanning the document body, so its length
- * and the bytes behind it are whatever the author put there — without these the export
- * would materialize an unbounded number of unbounded assets in one request.
+ * Byte ceilings for a bundled export. The bytes behind an embed list are whatever the
+ * author put there, so without these the export would materialize unbounded assets in
+ * one request. They match the bulk-download route, so the two export surfaces reject at
+ * the same size.
 *
- * The byte ceilings are the real bound and match the bulk-download route, so the two
- * export surfaces reject at the same size. The count is only a guard on the metadata
- * lookups that precede the byte check, so it sits far above any hand-authored document
- * rather than at a number a screenshot-heavy doc could plausibly reach.
+ * There is deliberately no count cap here: `extractEmbeddedFileRefs` already stops at
+ * `MAX_EMBEDDED_IMAGES`, so the list this route receives is bounded before it arrives.
 */
-const MAX_EXPORT_ASSETS = 500
 const MAX_EXPORT_ASSET_BYTES = 25 * 1024 * 1024
 const MAX_EXPORT_TOTAL_BYTES = 250 * 1024 * 1024

@@ -128,10 +127,26 @@ export const GET = withRouteHandler(
      return NextResponse.redirect(new URL(servePath, request.url), { status: 302 })
    }

-    const mdBuffer = await downloadFile({
-      key: record.key,
-      context: record.context as StorageContext,
-    })
+    // Capped like everything else in the bundle: the document body is usually the
+    // largest single entry, so leaving it unbounded left the export limit unenforced
+    // against the one item most able to exceed it. A body that alone exceeds the limit
+    // is a size rejection, so it reports as one rather than as a server error.
+    let mdBuffer: Buffer
+    try {
+      mdBuffer = await downloadFile({
+        key: record.key,
+        context: record.context as StorageContext,
+        maxBytes: MAX_EXPORT_TOTAL_BYTES,
+      })
+    } catch (error) {
+      if (!isPayloadSizeLimitError(error)) throw error
+      return NextResponse.json(
+        {
+          error: `This document exceeds the ${formatFileSize(MAX_EXPORT_TOTAL_BYTES)} export limit.`,
+        },
+        { status: 400 }
+      )
+    }
    let mdContent = mdBuffer.toString('utf-8')

    const imageIds = extractEmbeddedImageIds(mdContent)
@@ -152,15 +167,6 @@ export const GET = withRouteHandler(
      })
    }

-    if (imageIds.length > MAX_EXPORT_ASSETS) {
-      return NextResponse.json(
-        {
-          error: `This document embeds ${imageIds.length} files, more than the ${MAX_EXPORT_ASSETS} an export can bundle.`,
-        },
-        { status: 400 }
-      )
-    }
-
    // Metadata first: declared sizes bound the download before a byte is read, and the
    // authorization check costs nothing to run here.
    const assetTargets = (
@@ -180,11 +186,14 @@ export const GET = withRouteHandler(
      })
    ).filter((target): target is NonNullable<typeof target> => target !== null)

-    const declaredAssetBytes = assetTargets.reduce((sum, target) => sum + target.record.size, 0)
-    if (declaredAssetBytes > MAX_EXPORT_TOTAL_BYTES) {
+    // The body counts against the same budget as its assets — the zip holds both, so a
+    // limit that measured only the attachments would not describe the archive produced.
+    const bundleBytes =
+      mdBuffer.length + assetTargets.reduce((sum, target) => sum + target.record.size, 0)
+    if (bundleBytes > MAX_EXPORT_TOTAL_BYTES) {
      return NextResponse.json(
        {
-          error: `Embedded files total ${formatFileSize(declaredAssetBytes)}, which exceeds the ${formatFileSize(MAX_EXPORT_TOTAL_BYTES)} export limit.`,
+          error: `This document and its embedded files total ${formatFileSize(bundleBytes)}, which exceeds the ${formatFileSize(MAX_EXPORT_TOTAL_BYTES)} export limit.`,
        },
        { status: 400 }
      )
@@ -0,0 +1,135 @@
+/**
+ * @vitest-environment node
+ */
+import { createMockRequest } from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const {
+  mockCheckRateLimit,
+  mockValidateWorkspaceAccess,
+  mockGetWorkspaceFile,
+  mockFetchServableWorkspaceFileBuffer,
+} = vi.hoisted(() => ({
+  mockCheckRateLimit: vi.fn(),
+  mockValidateWorkspaceAccess: vi.fn(),
+  mockGetWorkspaceFile: vi.fn(),
+  mockFetchServableWorkspaceFileBuffer: vi.fn(),
+}))
+
+vi.mock('@/app/api/v1/middleware', () => ({
+  checkRateLimit: mockCheckRateLimit,
+  createRateLimitResponse: () => new Response('rate limited', { status: 429 }),
+  validateWorkspaceAccess: mockValidateWorkspaceAccess,
+}))
+vi.mock('@/lib/uploads/contexts/workspace', () => ({
+  getWorkspaceFile: mockGetWorkspaceFile,
+  fetchServableWorkspaceFileBuffer: mockFetchServableWorkspaceFileBuffer,
+}))
+vi.mock('@/lib/workspace-files/orchestration', () => ({
+  performDeleteWorkspaceFileItems: vi.fn(),
+}))
+vi.mock('@sim/audit', () => ({
+  recordAudit: vi.fn(),
+  AuditAction: { FILE_DOWNLOADED: 'file.downloaded', FILE_DELETED: 'file.deleted' },
+  AuditResourceType: { FILE: 'file' },
+}))
+vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: vi.fn() }))
+
+import { DocCompileUserError } from '@/lib/copilot/tools/server/files/doc-compile'
+import { GET } from '@/app/api/v1/files/[fileId]/route'
+
+const WORKSPACE_ID = 'ws-1'
+const FILE_ID = 'file-1'
+const context = { params: Promise.resolve({ fileId: FILE_ID }) }
+
+const DOCX_MIME = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'
+
+function request() {
+  return createMockRequest(
+    'GET',
+    undefined,
+    {},
+    `http://localhost:3000/api/v1/files/${FILE_ID}?workspaceId=${WORKSPACE_ID}`
+  )
+}
+
+/** A generated document: the name carries the target extension, the type the source. */
+function generatedDocument(name = 'report.docx') {
+  return {
+    id: FILE_ID,
+    workspaceId: WORKSPACE_ID,
+    name,
+    key: `workspace/${WORKSPACE_ID}/${FILE_ID}`,
+    path: `/serve/${FILE_ID}`,
+    size: 6_242,
+    type: 'text/x-docxjs',
+    uploadedBy: 'user-1',
+    uploadedAt: new Date('2026-01-01'),
+    updatedAt: new Date('2026-01-01'),
+  }
+}
+
+describe('v1 file download', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    mockCheckRateLimit.mockResolvedValue({ allowed: true, userId: 'user-1' })
+    mockValidateWorkspaceAccess.mockResolvedValue(null)
+    mockGetWorkspaceFile.mockResolvedValue(generatedDocument())
+    mockFetchServableWorkspaceFileBuffer.mockResolvedValue({
+      buffer: Buffer.from('PKrendered'),
+      contentType: DOCX_MIME,
+    })
+  })
+
+  it('serves the rendered bytes and the rendered content type', async () => {
+    const response = await GET(request(), context)
+
+    expect(response.status).toBe(200)
+    // Not the record's `text/x-docxjs`, which describes the stored source.
+    expect(response.headers.get('Content-Type')).toBe(DOCX_MIME)
+    expect(Buffer.from(await response.arrayBuffer()).toString()).toContain('rendered')
+  })
+
+  it('names the download with an extension matching the served content type', async () => {
+    // The renderer picks its output format from the file name, so the two cannot
+    // disagree: a `.docx` renders to a docx. This pins that invariant.
+    const response = await GET(request(), context)
+
+    const disposition = response.headers.get('Content-Disposition') ?? ''
+    expect(disposition).toContain('report.docx')
+    expect(response.headers.get('Content-Type')).toBe(DOCX_MIME)
+  })
+
+  it('reports Content-Length from the rendered bytes, not the declared source size', async () => {
+    const rendered = Buffer.alloc(50_000)
+    mockFetchServableWorkspaceFileBuffer.mockResolvedValue({
+      buffer: rendered,
+      contentType: DOCX_MIME,
+    })
+
+    const response = await GET(request(), context)
+
+    expect(response.headers.get('Content-Length')).toBe(String(rendered.length))
+  })
+
+  it('returns a retryable 409 while the artifact is still compiling', async () => {
+    mockFetchServableWorkspaceFileBuffer.mockRejectedValue(
+      new DocCompileUserError('Document is still being generated')
+    )
+
+    const response = await GET(request(), context)
+
+    // A 500 would give the caller no reason to try again.
+    expect(response.status).toBe(409)
+    expect((await response.json()).error).toContain('still being generated')
+  })
+
+  it('404s a file that does not exist', async () => {
+    mockGetWorkspaceFile.mockResolvedValue(null)
+
+    const response = await GET(request(), context)
+
+    expect(response.status).toBe(404)
+    expect(mockFetchServableWorkspaceFileBuffer).not.toHaveBeenCalled()
+  })
+})