Skip to content

Commit 3ede80e

Browse files
committed
fix(files): bound YAML expansion and buffered reads on the file-serve path
Two unbounded resource paths reachable from the anonymous public share routes. YAML alias expansion: the page compiler parsed sim: fence payloads with no ceiling on what the parsed value expands to. Aliases are shared references, so `columns: &c [...]` plus a row list of aliases to it renders N^2 cells from ~13N source bytes — 25 KB of source cost 3.6s of CPU and 38 MB of HTML per request. The expansion guard already in the file parser is now a shared primitive taking caller-supplied limits, and the compiler charges every fence and the frontmatter to one per-compile budget so splitting across blocks buys no extra rendering. Buffered reads: the Files-module serve path already capped reads at MAX_BUFFERED_TRANSFER_BYTES via fetchWorkspaceFileBuffer, but five sibling paths serving the same objects did not, including both unauthenticated share routes. A workspace object is admitted at 5 GB, so a share link was the one way to make an anonymous request hold gigabytes resident in the shared process.
1 parent 8e9aeb9 commit 3ede80e

13 files changed

Lines changed: 643 additions & 185 deletions

File tree

apps/sim/app/api/files/public/[token]/content/route.test.ts

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
*/
44
import { NextRequest } from 'next/server'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
7+
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
68

79
const {
810
mockResolveActiveShareByToken,
@@ -78,13 +80,31 @@ describe('GET /api/files/public/[token]/content', () => {
7880
expect(mockDownloadFile).not.toHaveBeenCalled()
7981
})
8082

81-
it('serves the bytes once authorized', async () => {
83+
it('serves the bytes once authorized, bounded by the shared transfer ceiling', async () => {
8284
mockValidateDeploymentAuth.mockResolvedValueOnce({ authorized: true })
8385
const res = await GET(request(), params())
8486
expect(res.status).toBe(200)
87+
// The ceiling matters most here: this is the only surface that reads a workspace
88+
// object for a caller with no session, and the object is admitted at 5 GB.
8589
expect(mockDownloadFile).toHaveBeenCalledWith({
8690
key: passwordShare.file.key,
8791
context: 'workspace',
92+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
8893
})
8994
})
95+
96+
it('answers 413 rather than 500 when the shared file is too large to serve resident', async () => {
97+
mockValidateDeploymentAuth.mockResolvedValueOnce({ authorized: true })
98+
mockDownloadFile.mockRejectedValueOnce(
99+
new PayloadSizeLimitError({
100+
label: 'storage download',
101+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
102+
observedBytes: 5 * 1024 * 1024 * 1024,
103+
})
104+
)
105+
106+
const res = await GET(request(), params())
107+
108+
expect(res.status).toBe(413)
109+
})
90110
})

apps/sim/app/api/files/public/[token]/content/route.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,13 @@ import { parseRequest } from '@/lib/api/server'
77
import { resolveServableDoc } from '@/lib/copilot/tools/server/files/doc-compile'
88
import { validateDeploymentAuth } from '@/lib/core/security/deployment-auth'
99
import { generateRequestId } from '@/lib/core/utils/request'
10+
import { assertKnownSizeWithinLimit } from '@/lib/core/utils/stream-limits'
1011
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1112
import { enforcePublicFileRateLimit } from '@/lib/public-shares/rate-limit'
1213
import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager'
1314
import { downloadFile } from '@/lib/uploads/core/storage-service'
1415
import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative'
16+
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
1517
import { isSimPageSource, SIM_PAGE_CONTENT_TYPE } from '@/lib/workspace-files/page-compile'
1618
import { renderSimPageDocumentWithAssets } from '@/lib/workspace-files/page-document.server'
1719
import {
@@ -69,7 +71,14 @@ export const GET = withRouteHandler(
6971
}
7072

7173
const { file } = resolved
72-
const raw = await downloadFile({ key: file.key, context: 'workspace' })
74+
// The same ceiling the authenticated serve route reads this object under
75+
// (`fetchWorkspaceFileBuffer`). Without it a share link is the one way to ask
76+
// an unauthenticated caller's request to hold a 5 GB workspace file resident.
77+
const raw = await downloadFile({
78+
key: file.key,
79+
context: 'workspace',
80+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
81+
})
7382

7483
const servable = file.workspaceId
7584
? await resolveServableDoc(file.workspaceId, raw, file.originalName)
@@ -108,6 +117,9 @@ export const GET = withRouteHandler(
108117
}),
109118
'utf8'
110119
)
120+
// Rendering inlines referenced workspace images, so a source comfortably under
121+
// the read ceiling can resolve to a document well over it.
122+
assertKnownSizeWithinLimit(buffer.length, MAX_BUFFERED_TRANSFER_BYTES, 'served page render')
111123
contentType = 'text/html'
112124
} else if (preview) {
113125
// Only for a render request: the Download button omits `preview`, so a saved

apps/sim/app/api/files/public/[token]/inline/route.test.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
*/
44
import { NextRequest } from 'next/server'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
import { PayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
7+
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
68

79
const { mockResolveShare, mockRateLimit, mockValidateAuth, mockDownloadFile, mockResolveImage } =
810
vi.hoisted(() => ({
@@ -122,4 +124,37 @@ describe('GET /api/files/public/[token]/inline', () => {
122124
expect(res.status).toBe(404)
123125
expect(mockDownloadFile).not.toHaveBeenCalled()
124126
})
127+
128+
it('bounds both reads: the doc scan tightly, the served image at the transfer ceiling', async () => {
129+
await GET(req(`fileId=${FILE_ID}`), params)
130+
131+
const [docRead, imageRead] = mockDownloadFile.mock.calls.map(([args]) => args)
132+
// The doc is scanned and discarded (and decoded to UTF-16 on top of the buffer),
133+
// so it must not inherit the ceiling of a file this route actually serves.
134+
expect(docRead.key).toBe(DOC_KEY)
135+
expect(docRead.maxBytes).toBeGreaterThan(0)
136+
expect(docRead.maxBytes).toBeLessThan(MAX_BUFFERED_TRANSFER_BYTES)
137+
expect(imageRead.key).toBe(IMG_KEY)
138+
expect(imageRead.maxBytes).toBe(MAX_BUFFERED_TRANSFER_BYTES)
139+
})
140+
141+
it('fails the referenced-by-doc gate closed when the document is too large to scan', async () => {
142+
mockDownloadFile.mockImplementation(({ key }: { key: string }) =>
143+
key === DOC_KEY
144+
? Promise.reject(
145+
new PayloadSizeLimitError({
146+
label: 'storage download',
147+
maxBytes: 10 * 1024 * 1024,
148+
observedBytes: 5 * 1024 * 1024 * 1024,
149+
})
150+
)
151+
: Promise.resolve(PNG)
152+
)
153+
154+
const res = await GET(req(`fileId=${FILE_ID}`), params)
155+
156+
expect(res.status).toBe(404)
157+
// The gate could not be verified, so the image must never be read at all.
158+
expect(mockDownloadFile).toHaveBeenCalledTimes(1)
159+
})
125160
})

apps/sim/app/api/files/public/[token]/inline/route.ts

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { getPublicInlineFileContract } from '@/lib/api/contracts/public-shares'
66
import { parseRequest } from '@/lib/api/server'
77
import { validateDeploymentAuth } from '@/lib/core/security/deployment-auth'
88
import { generateRequestId } from '@/lib/core/utils/request'
9+
import { isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
910
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
1011
import { enforcePublicFileRateLimit } from '@/lib/public-shares/rate-limit'
1112
import { resolveActiveShareByToken } from '@/lib/public-shares/share-manager'
@@ -20,6 +21,17 @@ export const dynamic = 'force-dynamic'
2021

2122
const logger = createLogger('PublicInlineFileAPI')
2223

24+
/**
25+
* Ceiling on the shared document read for the referenced-by-doc gate below.
26+
*
27+
* Far tighter than the ceiling on a file this route SERVES, because these bytes are
28+
* never served — they are scanned for image references and discarded, and scanning
29+
* decodes them to UTF-16 on top of the buffer, so the resident cost is roughly double
30+
* the read. A share can point at any workspace file, admitted at 5 GB, and this route
31+
* is anonymous; nothing a person writes as a document approaches even this bound.
32+
*/
33+
const MAX_INLINE_REF_SCAN_BYTES = 10 * 1024 * 1024
34+
2335
/**
2436
* GET /api/files/public/[token]/inline?key=<cloudKey>|fileId=<id>
2537
*
@@ -72,7 +84,21 @@ export const GET = withRouteHandler(
7284
}
7385

7486
// Referenced-by-doc gate: the share grants exactly the images the document embeds.
75-
const docText = (await downloadFile({ key: doc.key, context: 'workspace' })).toString('utf-8')
87+
// A document too large to scan fails the gate like any other unverifiable
88+
// reference — the grant cannot be extended to an embed we were unable to confirm.
89+
let docText: string
90+
try {
91+
const docBuffer = await downloadFile({
92+
key: doc.key,
93+
context: 'workspace',
94+
maxBytes: MAX_INLINE_REF_SCAN_BYTES,
95+
})
96+
docText = docBuffer.toString('utf-8')
97+
} catch (error) {
98+
if (!isPayloadSizeLimitError(error)) throw error
99+
logger.info('Shared document too large to scan for embedded references', { token })
100+
throw new FileNotFoundError('Not found')
101+
}
76102
const { keys, ids } = extractEmbeddedFileRefs(docText)
77103
const referenced = ref.fileId
78104
? ids.some((id) => storedFileId(id) === ref.fileId)

apps/sim/app/api/files/serve-inline-image.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { createLogger } from '@sim/logger'
22
import type { NextResponse } from 'next/server'
33
import { downloadFile } from '@/lib/uploads/core/storage-service'
44
import type { ResolvedInlineImage } from '@/lib/uploads/server/inline-image'
5+
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
56
import { sniffImageContentType } from '@/lib/uploads/utils/validation'
67
import { createFileResponse, FileNotFoundError } from '@/app/api/files/utils'
78

@@ -25,7 +26,11 @@ export async function serveInlineImage(
2526
image: ResolvedInlineImage,
2627
{ sniff }: { sniff: boolean }
2728
): Promise<NextResponse> {
28-
const buffer = await downloadFile({ key: image.key, context: 'workspace' })
29+
const buffer = await downloadFile({
30+
key: image.key,
31+
context: 'workspace',
32+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
33+
})
2934

3035
let contentType = image.contentType
3136
if (sniff) {

apps/sim/app/api/files/serve/[...path]/route.test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import { hybridAuthMockFns, storageServiceMock, storageServiceMockFns } from '@sim/testing'
77
import { NextRequest } from 'next/server'
88
import { beforeEach, describe, expect, it, vi } from 'vitest'
9+
import { MAX_BUFFERED_TRANSFER_BYTES } from '@/lib/uploads/shared/types'
910

1011
vi.mock('@sim/logger', () => ({
1112
createLogger: vi.fn(() => serveLogger),
@@ -27,6 +28,7 @@ const {
2728
mockResolveServableDocBytes,
2829
mockGetContentType,
2930
mockFindLocalFile,
31+
mockReadLocalFileWithinLimit,
3032
mockCreateFileResponse,
3133
mockCreateErrorResponse,
3234
FileNotFoundError,
@@ -52,6 +54,7 @@ const {
5254
mockResolveServableDocBytes: vi.fn(),
5355
mockGetContentType: vi.fn(),
5456
mockFindLocalFile: vi.fn(),
57+
mockReadLocalFileWithinLimit: vi.fn(),
5558
mockCreateFileResponse: vi.fn(),
5659
mockCreateErrorResponse: vi.fn(),
5760
FileNotFoundError: FileNotFoundErrorClass,
@@ -119,6 +122,7 @@ vi.mock('@/app/api/files/utils', () => ({
119122
extractStorageKey: vi.fn().mockImplementation((path: string) => path.split('/').pop()),
120123
extractFilename: vi.fn().mockImplementation((path: string) => path.split('/').pop()),
121124
findLocalFile: mockFindLocalFile,
125+
readLocalFileWithinLimit: mockReadLocalFileWithinLimit,
122126
}))
123127

124128
import { GET } from '@/app/api/files/serve/[...path]/route'
@@ -162,6 +166,9 @@ describe('File Serve API Route', () => {
162166
)
163167
mockGetContentType.mockReturnValue('text/plain')
164168
mockFindLocalFile.mockReturnValue('/test/uploads/test-file.txt')
169+
mockReadLocalFileWithinLimit.mockImplementation(async (filePath: string) =>
170+
mockReadFile(filePath)
171+
)
165172
mockCreateFileResponse.mockImplementation(
166173
(file: { buffer: Buffer; contentType: string; filename: string }) => {
167174
return new Response(file.buffer, {
@@ -181,6 +188,60 @@ describe('File Serve API Route', () => {
181188
})
182189
})
183190

191+
it('bounds every buffered read at the shared transfer ceiling', async () => {
192+
mockIsUsingCloudStorage.mockReturnValue(true)
193+
mockResolveStoredFileContext.mockResolvedValue('copilot')
194+
mockInferContextFromKey.mockReturnValue('copilot')
195+
mockDownloadCopilotFile.mockResolvedValue(Buffer.from('bytes'))
196+
197+
await GET(new NextRequest('http://localhost:3000/api/files/serve/copilot/doc.txt'), {
198+
params: Promise.resolve({ path: ['copilot', 'doc.txt'] }),
199+
})
200+
201+
expect(mockDownloadCopilotFile).toHaveBeenCalledWith('copilot/doc.txt', {
202+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
203+
})
204+
})
205+
206+
it('bounds the local read rather than trusting the stored size', async () => {
207+
await GET(new NextRequest('http://localhost:3000/api/files/serve/workspace/ws/test-file.txt'), {
208+
params: Promise.resolve({ path: ['workspace', 'ws', 'test-file.txt'] }),
209+
})
210+
211+
expect(mockReadLocalFileWithinLimit).toHaveBeenCalledWith(
212+
'/test/uploads/test-file.txt',
213+
MAX_BUFFERED_TRANSFER_BYTES,
214+
expect.any(String)
215+
)
216+
})
217+
218+
it('answers 413 rather than 500 when a file is too large to serve resident', async () => {
219+
const { PayloadSizeLimitError } = await import('@/lib/core/utils/stream-limits')
220+
mockReadLocalFileWithinLimit.mockRejectedValue(
221+
new PayloadSizeLimitError({
222+
label: 'served file',
223+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
224+
observedBytes: MAX_BUFFERED_TRANSFER_BYTES + 1,
225+
})
226+
)
227+
// The real createErrorResponse owns the status mapping; mirror it here so the
228+
// route's own error path is what decides, not the mock's default 500.
229+
mockCreateErrorResponse.mockImplementation(
230+
(error: Error) =>
231+
new Response(JSON.stringify({ error: error.name }), {
232+
status: error.name === 'PayloadSizeLimitError' ? 413 : 500,
233+
})
234+
)
235+
236+
const response = await GET(
237+
new NextRequest('http://localhost:3000/api/files/serve/workspace/ws/huge.bin'),
238+
{ params: Promise.resolve({ path: ['workspace', 'ws', 'huge.bin'] }) }
239+
)
240+
241+
expect(response.status).toBe(413)
242+
expect(serveLogger.error).not.toHaveBeenCalled()
243+
})
244+
184245
it('should serve local file successfully', async () => {
185246
const req = new NextRequest(
186247
'http://localhost:3000/api/files/serve/workspace/test-workspace-id/test-file.txt'
@@ -232,6 +293,7 @@ describe('File Serve API Route', () => {
232293
expect(storageServiceMockFns.mockDownloadFile).toHaveBeenCalledWith({
233294
key: 'workspace/test-workspace-id/1234567890-image.png',
234295
context: 'mothership',
296+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
235297
})
236298
})
237299

@@ -318,6 +380,7 @@ describe('File Serve API Route', () => {
318380
expect(storageServiceMockFns.mockDownloadFile).toHaveBeenCalledWith({
319381
key: 'workspace/test-workspace-id/1234567890-photo.png',
320382
context: 'mothership',
383+
maxBytes: MAX_BUFFERED_TRANSFER_BYTES,
321384
})
322385
})
323386

0 commit comments

Comments
 (0)