Skip to content

Commit edfcb0d

Browse files
committed
fix(files): check transformed bytes against the ceiling and bound the guard's own walk
Review round 1. Bounding the source read did not bound the response: every branch that replaces the source — a compiled document artifact fetched separately, a page with its images inlined, a transcoded image derivative — could turn a source under the ceiling into a response over it, on the anonymous share route included. The check now sits where those branches converge rather than on the page branch alone. The local read enforced its ceiling on a size measured before the read, so it described the file only as of the stat. It now reads through the same bounded stream reader the S3/Blob/GCS downloads use. The expansion guard held one stack frame per pending node, so proving a wide document too large allocated in proportion to the width it was rejecting. It now holds one frame per level of nesting and consumes each container through a lazy generator, bounding its own working set by depth. A double serializes to up to 24 characters, so numbers are charged their serialized length rather than the flat 16-byte allowance they could outgrow.
1 parent 3ede80e commit edfcb0d

7 files changed

Lines changed: 303 additions & 42 deletions

File tree

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,22 @@ describe('GET /api/files/public/[token]/content', () => {
9393
})
9494
})
9595

96+
it('413s when a compiled artifact outgrows the ceiling its source fit inside', async () => {
97+
mockValidateDeploymentAuth.mockResolvedValueOnce({ authorized: true })
98+
// The source read is bounded, but the artifact is fetched separately — a small
99+
// generation source can resolve to a document far larger than the source ever was.
100+
mockDownloadFile.mockResolvedValueOnce(Buffer.from('generation source'))
101+
mockResolveServableDoc.mockResolvedValueOnce({
102+
kind: 'artifact',
103+
buffer: Buffer.alloc(MAX_BUFFERED_TRANSFER_BYTES + 1),
104+
contentType: 'application/pdf',
105+
})
106+
107+
const res = await GET(request(), params())
108+
109+
expect(res.status).toBe(413)
110+
})
111+
96112
it('answers 413 rather than 500 when the shared file is too large to serve resident', async () => {
97113
mockValidateDeploymentAuth.mockResolvedValueOnce({ authorized: true })
98114
mockDownloadFile.mockRejectedValueOnce(

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,9 +117,6 @@ export const GET = withRouteHandler(
117117
}),
118118
'utf8'
119119
)
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')
123120
contentType = 'text/html'
124121
} else if (preview) {
125122
// Only for a render request: the Download button omits `preview`, so a saved
@@ -128,6 +125,12 @@ export const GET = withRouteHandler(
128125
if (image) ({ buffer, contentType } = image)
129126
}
130127

128+
// Bounding the source read does not bound the response: each branch above can
129+
// replace it with bytes fetched or produced separately — a compiled artifact, a
130+
// page with its images inlined, a transcoded derivative. This is an anonymous
131+
// route, so the bytes it actually returns are what has to fit.
132+
assertKnownSizeWithinLimit(buffer.length, MAX_BUFFERED_TRANSFER_BYTES, 'served file render')
133+
131134
logger.info('Public shared file served', { token, key: file.key, size: buffer.length })
132135

133136
// Anonymous access: null actor (owner-as-actor would misread as a self-download).

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,28 @@ describe('File Serve API Route', () => {
215215
)
216216
})
217217

218+
it('413s when a resolved document outgrows the ceiling its source fit inside', async () => {
219+
// The stored source is a small generation script; the compiled artifact it
220+
// resolves to is fetched separately and is what the response would carry.
221+
mockResolveServableDocBytes.mockResolvedValue({
222+
buffer: Buffer.alloc(MAX_BUFFERED_TRANSFER_BYTES + 1),
223+
contentType: 'application/pdf',
224+
})
225+
mockCreateErrorResponse.mockImplementation(
226+
(error: Error) =>
227+
new Response(JSON.stringify({ error: error.name }), {
228+
status: error.name === 'PayloadSizeLimitError' ? 413 : 500,
229+
})
230+
)
231+
232+
const response = await GET(
233+
new NextRequest('http://localhost:3000/api/files/serve/workspace/ws/report.pdf'),
234+
{ params: Promise.resolve({ path: ['workspace', 'ws', 'report.pdf'] }) }
235+
)
236+
237+
expect(response.status).toBe(413)
238+
})
239+
218240
it('answers 413 rather than 500 when a file is too large to serve resident', async () => {
219241
const { PayloadSizeLimitError } = await import('@/lib/core/utils/stream-limits')
220242
mockReadLocalFileWithinLimit.mockRejectedValue(

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

Lines changed: 45 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,13 @@ interface ServeOptions {
7474
* routes through here. An image derivative is the opposite — the stored bytes are
7575
* the file — so it is served only when the caller asked to preview, never when it
7676
* asked to download.
77+
*
78+
* Every branch that replaces the source bytes is re-checked against the transfer
79+
* ceiling on the way out. Bounding the read alone does not bound the response: a
80+
* page inlines its images, a generated document resolves to a compiled artifact
81+
* fetched separately, and a derivative is transcoded here — so each can turn a
82+
* source under the ceiling into a response over it. One check where the branches
83+
* converge is what makes that impossible to miss when a branch is added.
7784
*/
7885
async function resolveServableBytes(params: {
7986
buffer: Buffer
@@ -99,6 +106,44 @@ async function resolveServableBytes(params: {
99106
signal,
100107
} = params
101108
if (options.raw) return { buffer, contentType: getContentType(filename) }
109+
return withinTransferCeiling(await resolveTransformedBytes(params))
110+
}
111+
112+
/** Rejects a resolved response whose bytes outgrew what one request may hold resident. */
113+
function withinTransferCeiling(resolved: { buffer: Buffer; contentType: string }): {
114+
buffer: Buffer
115+
contentType: string
116+
} {
117+
assertKnownSizeWithinLimit(
118+
resolved.buffer.length,
119+
MAX_BUFFERED_TRANSFER_BYTES,
120+
'served file render'
121+
)
122+
return resolved
123+
}
124+
125+
async function resolveTransformedBytes(params: {
126+
buffer: Buffer
127+
filename: string
128+
storageKey: string
129+
workspaceId: string | undefined
130+
options: ServeOptions
131+
ownerKey: string | undefined
132+
filePrincipal?: Principal
133+
fileType?: string
134+
signal: AbortSignal | undefined
135+
}): Promise<{ buffer: Buffer; contentType: string }> {
136+
const {
137+
buffer,
138+
filename,
139+
storageKey,
140+
workspaceId,
141+
options,
142+
ownerKey,
143+
filePrincipal,
144+
fileType,
145+
signal,
146+
} = params
102147

103148
// The pdf model for pages: a page file stores its SOURCE (frontmatter +
104149
// markdown + sim: fences) and serving compiles it to the rendered document,
@@ -113,9 +158,6 @@ async function resolveServableBytes(params: {
113158
await renderSimPageDocumentWithAssets(text, { workspaceId }),
114159
'utf8'
115160
)
116-
// Rendering inlines referenced workspace images, so a source comfortably under
117-
// the read ceiling can resolve to a document well over it.
118-
assertKnownSizeWithinLimit(rendered.length, MAX_BUFFERED_TRANSFER_BYTES, 'served page render')
119161
return { buffer: rendered, contentType: 'text/html' }
120162
}
121163
}

‎apps/sim/app/api/files/utils.ts‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import { createLogger } from '@sim/logger'
22
import { NextResponse } from 'next/server'
3-
import { assertKnownSizeWithinLimit, isPayloadSizeLimitError } from '@/lib/core/utils/stream-limits'
3+
import {
4+
isPayloadSizeLimitError,
5+
readNodeStreamToBufferWithLimit,
6+
} from '@/lib/core/utils/stream-limits'
47
import { sanitizeFileKey } from '@/lib/uploads/utils/file-utils'
58

69
const logger = createLogger('FilesUtils')
@@ -290,21 +293,30 @@ export function createErrorResponse(error: Error, status = 500): NextResponse {
290293
}
291294

292295
/**
293-
* Reads a local upload into memory only after its on-disk size clears `maxBytes`.
296+
* Reads a local upload into memory under a hard byte ceiling.
294297
*
295298
* The self-hosted mirror of the `maxBytes` every cloud provider download takes:
296299
* a bare `readFile` inherits the 5 GB admission ceiling workspace files are stored
297300
* under and allocates all of it inside the shared app process.
301+
*
302+
* The limit is enforced on the bytes as they arrive, through the same bounded-stream
303+
* reader the S3/Blob/GCS downloads use, rather than by checking `stat` and then
304+
* reading. A declared size only describes the file at the moment it was measured, so
305+
* a stat-then-read pair admits whatever the file becomes in between — the cloud
306+
* providers check `ContentLength` too, but never trust it as the only bound.
298307
*/
299308
export async function readLocalFileWithinLimit(
300309
filePath: string,
301310
maxBytes: number,
302311
label: string
303312
): Promise<Buffer> {
304-
const { readFile, stat } = await import('fs/promises')
305-
const { size } = await stat(filePath)
306-
assertKnownSizeWithinLimit(size, maxBytes, label)
307-
return readFile(filePath)
313+
const { createReadStream } = await import('fs')
314+
const stream = createReadStream(filePath)
315+
try {
316+
return await readNodeStreamToBufferWithLimit(stream, { maxBytes, label })
317+
} finally {
318+
stream.destroy()
319+
}
308320
}
309321

310322
export function createSuccessResponse(data: ApiSuccessResponse): NextResponse {
Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import {
6+
createYamlExpansionBudget,
7+
isYamlExpansionBudgetExhausted,
8+
measureYamlExpansion,
9+
type YamlExpansionLimits,
10+
} from '@/lib/file-parsers/yaml-limits'
11+
12+
const LIMITS: YamlExpansionLimits = {
13+
maxNodes: 1000,
14+
maxSerializedBytes: 64 * 1024,
15+
maxDepth: 10,
16+
}
17+
18+
const limits = (overrides: Partial<YamlExpansionLimits> = {}): YamlExpansionLimits => ({
19+
...LIMITS,
20+
...overrides,
21+
})
22+
23+
describe('measureYamlExpansion', () => {
24+
it('reports the depth of the expanded tree', () => {
25+
expect(measureYamlExpansion('scalar', LIMITS)).toEqual({ within: true, depth: 0 })
26+
expect(measureYamlExpansion([1, 2, 3], LIMITS)).toEqual({ within: true, depth: 1 })
27+
expect(measureYamlExpansion({ a: { b: { c: 1 } } }, LIMITS)).toEqual({ within: true, depth: 3 })
28+
})
29+
30+
it('counts an empty container as a level', () => {
31+
expect(measureYamlExpansion([], LIMITS)).toEqual({ within: true, depth: 1 })
32+
expect(measureYamlExpansion({ a: {} }, LIMITS)).toEqual({ within: true, depth: 2 })
33+
})
34+
35+
it('charges an aliased subtree once per path that reaches it', () => {
36+
const shared = [1, 2, 3, 4, 5]
37+
const aliased = { a: shared, b: shared, c: shared }
38+
39+
const measured = measureYamlExpansion(aliased, limits({ maxNodes: 20 }))
40+
expect(measured).toEqual({ within: true, depth: 2 })
41+
// 18 nodes if every reach is charged; 8 if the shared array were counted once.
42+
expect(measureYamlExpansion(aliased, limits({ maxNodes: 12 })).within).toBe(false)
43+
})
44+
45+
it('terminates on a self-referential anchor instead of recursing forever', () => {
46+
const cyclic: Record<string, unknown> = {}
47+
cyclic.self = cyclic
48+
49+
const measured = measureYamlExpansion(cyclic, LIMITS)
50+
51+
expect(measured.within).toBe(false)
52+
if (!measured.within) expect(measured.reason).toContain('nesting depth')
53+
})
54+
55+
it('rejects a wide fan-out of containers without enumerating it first', () => {
56+
// The traversal holds one frame per level, not one per pending node, so a node
57+
// whose fan-out dwarfs the budget trips the cap part way through rather than
58+
// after building a frame for every sibling.
59+
const wide = Array.from({ length: 100_000 }, () => ({ a: 1 }))
60+
61+
const measured = measureYamlExpansion(wide, limits({ maxNodes: 50 }))
62+
63+
expect(measured.within).toBe(false)
64+
if (!measured.within) expect(measured.reason).toContain('expanded nodes')
65+
})
66+
67+
it('charges a long number its serialized length, not the flat allowance', () => {
68+
// Each serializes to 24 characters; the flat non-string allowance is 16.
69+
const longNumbers = Array.from({ length: 200 }, () => -1.2345678901234567e-308)
70+
const shortNumbers = Array.from({ length: 200 }, () => 1)
71+
72+
// Sits between what 200 short numbers cost (~4.4 KB) and what 200 long ones do
73+
// (~6 KB); under the flat allowance both would land on the same side of it.
74+
const byteCap = limits({ maxSerializedBytes: 5000 })
75+
76+
expect(measureYamlExpansion(shortNumbers, byteCap).within).toBe(true)
77+
expect(measureYamlExpansion(longNumbers, byteCap).within).toBe(false)
78+
})
79+
80+
it('charges object keys, so an aliased object with long keys cannot bypass the cap', () => {
81+
const key = 'k'.repeat(500)
82+
const shared = { [key]: 1 }
83+
const aliased = Array.from({ length: 50 }, () => shared)
84+
85+
const measured = measureYamlExpansion(aliased, limits({ maxSerializedBytes: 10_000 }))
86+
87+
expect(measured.within).toBe(false)
88+
if (!measured.within) expect(measured.reason).toContain('serialized size')
89+
})
90+
91+
it('draws several documents down one shared budget', () => {
92+
const budget = createYamlExpansionBudget(limits({ maxNodes: 30 }))
93+
const doc = Array.from({ length: 10 }, (_, i) => i)
94+
95+
expect(measureYamlExpansion(doc, limits({ maxNodes: 30 }), budget).within).toBe(true)
96+
expect(isYamlExpansionBudgetExhausted(budget)).toBe(false)
97+
expect(measureYamlExpansion(doc, limits({ maxNodes: 30 }), budget).within).toBe(true)
98+
// The third pass runs out: 3 x 11 nodes exceeds the 30 the budget was created with.
99+
expect(measureYamlExpansion(doc, limits({ maxNodes: 30 }), budget).within).toBe(false)
100+
expect(isYamlExpansionBudgetExhausted(budget)).toBe(true)
101+
})
102+
103+
it('leaves a shared budget usable after a depth rejection', () => {
104+
// Depth costs only its own nesting, so one over-deep document must not bankrupt
105+
// the documents that share its budget.
106+
const budget = createYamlExpansionBudget(limits({ maxDepth: 2 }))
107+
const deep = { a: { b: { c: { d: 1 } } } }
108+
109+
expect(measureYamlExpansion(deep, limits({ maxDepth: 2 }), budget).within).toBe(false)
110+
expect(isYamlExpansionBudgetExhausted(budget)).toBe(false)
111+
expect(measureYamlExpansion({ ok: 1 }, limits({ maxDepth: 2 }), budget).within).toBe(true)
112+
})
113+
114+
it('ignores inherited properties when walking an object', () => {
115+
const parent = { inherited: 'x'.repeat(5000) }
116+
const child = Object.create(parent) as Record<string, unknown>
117+
child.own = 1
118+
119+
const measured = measureYamlExpansion(child, limits({ maxSerializedBytes: 200 }))
120+
121+
expect(measured).toEqual({ within: true, depth: 1 })
122+
})
123+
})

0 commit comments

Comments
 (0)