Skip to content

Commit 9b3bf89

Browse files
authored
fix: do not end parts() iteration while busboy still holds undelivered parts (#631)
1 parent 45023e0 commit 9b3bf89

3 files changed

Lines changed: 259 additions & 13 deletions

File tree

index.js

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ const PrototypeViolationError = createError('FST_PROTO_VIOLATION', 'prototype pr
2626
const InvalidMultipartContentTypeError = createError('FST_INVALID_MULTIPART_CONTENT_TYPE', 'the request is not multipart', 406)
2727
const InvalidJSONFieldError = createError('FST_INVALID_JSON_FIELD_ERROR', 'a request field is not a valid JSON as declared by its Content-Type', 406)
2828
const FileBufferNotFoundError = createError('FST_FILE_BUFFER_NOT_FOUND', 'the file buffer was not found', 500)
29+
const PrematureCloseError = createError('FST_MP_PREMATURE_CLOSE', 'the request was closed before the multipart data was fully parsed', 400)
2930
const NoFormData = createError('FST_NO_FORM_DATA', 'FormData is not available', 500)
3031

3132
function setMultipart (req, _payload, done) {
@@ -179,7 +180,8 @@ function fastifyMultipart (fastify, options, done) {
179180
PrototypeViolationError,
180181
InvalidMultipartContentTypeError,
181182
RequestFileTooLargeError,
182-
FileBufferNotFoundError
183+
FileBufferNotFoundError,
184+
PrematureCloseError
183185
})
184186

185187
fastify.addContentTypeParser('multipart/form-data', setMultipart)
@@ -243,23 +245,16 @@ function fastifyMultipart (fastify, options, done) {
243245
const parts = () => {
244246
return new Promise((resolve, reject) => {
245247
handle((val) => {
246-
if (val instanceof Error) {
247-
if (val.message === 'Unexpected end of multipart data') {
248-
// Stop parsing without throwing an error
249-
resolve(null)
250-
} else {
251-
reject(val)
252-
}
253-
} else {
254-
resolve(val)
255-
}
248+
if (val instanceof Error) return reject(val)
249+
resolve(val)
256250
})
257251
})
258252
}
259253

260254
const body = {}
261255
let lastError = null
262256
let currentFile = null
257+
let sawData = false
263258
const request = this.raw
264259
const busboyOptions = deepmergeAll(
265260
{ headers: request.headers },
@@ -270,7 +265,7 @@ function fastifyMultipart (fastify, options, done) {
270265
this.log.trace({ busboyOptions }, 'Providing options to busboy')
271266
const bb = busboy(busboyOptions)
272267

273-
request.on('close', cleanup)
268+
request.on('close', onRequestClose)
274269
request.on('error', cleanup)
275270

276271
bb
@@ -279,7 +274,7 @@ function fastifyMultipart (fastify, options, done) {
279274
.on('end', cleanup)
280275
.on('finish', cleanup)
281276
.on('close', cleanup)
282-
.on('error', cleanup)
277+
.on('error', onBusboyError)
283278

284279
bb.on('partsLimit', function () {
285280
const err = new PartsLimitError()
@@ -299,8 +294,23 @@ function fastifyMultipart (fastify, options, done) {
299294
process.nextTick(() => cleanup(err))
300295
})
301296

297+
request.once('data', onFirstData)
302298
request.pipe(bb)
303299

300+
function onFirstData () {
301+
sawData = true
302+
}
303+
304+
function onBusboyError (err) {
305+
// an empty body is treated as an empty form rather than as
306+
// truncated multipart data
307+
if (!sawData && err.message === 'Unexpected end of multipart data') {
308+
cleanup()
309+
} else {
310+
cleanup(err)
311+
}
312+
}
313+
304314
function onField (name, fieldValue, fieldnameTruncated, valueTruncated, encoding, contentType) {
305315
// don't overwrite prototypes
306316
if (name in Object.prototype) {
@@ -412,6 +422,15 @@ function fastifyMultipart (fastify, options, done) {
412422
// Other errors are expected to be handled by the consumer of the stream
413423
})
414424

425+
// If the consumer destroys the file stream without reading it to the
426+
// end, busboy stops parsing and will never emit 'finish', so the
427+
// iteration has to end here (with the last error, if there is one)
428+
file.on('close', function () {
429+
if (!file.readableEnded) {
430+
cleanup()
431+
}
432+
})
433+
415434
if (throwFileSizeLimit) {
416435
file.on('limit', function () {
417436
const err = new RequestFileTooLargeError()
@@ -436,6 +455,16 @@ function fastifyMultipart (fastify, options, done) {
436455
currentFile = null
437456
}
438457

458+
function onRequestClose () {
459+
// 'close' on a completed request says nothing about parts busboy is
460+
// still holding: they must keep flowing and iteration ends on busboy's
461+
// own end events. A premature close has to end iteration here though,
462+
// as busboy will never emit 'finish' for a truncated stream.
463+
if (!request.readableEnded) {
464+
cleanup(lastError || new PrematureCloseError())
465+
}
466+
}
467+
439468
function cleanup (err) {
440469
request.unpipe(bb)
441470

test/fix-630.test.js

Lines changed: 216 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,216 @@
1+
'use strict'
2+
3+
const test = require('node:test')
4+
const Fastify = require('fastify')
5+
const multipart = require('..')
6+
const http = require('node:http')
7+
const net = require('node:net')
8+
const { once } = require('node:events')
9+
const { setTimeout: sleep } = require('node:timers/promises')
10+
11+
const BOUNDARY = 'fix630boundary'
12+
13+
function filePart (fieldname, filename, payload) {
14+
return Buffer.concat([
15+
Buffer.from(
16+
`--${BOUNDARY}\r\n` +
17+
`Content-Disposition: form-data; name="${fieldname}"; filename="${filename}"\r\n` +
18+
'Content-Type: application/octet-stream\r\n\r\n'
19+
),
20+
payload,
21+
Buffer.from('\r\n')
22+
])
23+
}
24+
25+
function fieldPart (fieldname, value) {
26+
return Buffer.from(
27+
`--${BOUNDARY}\r\n` +
28+
`Content-Disposition: form-data; name="${fieldname}"\r\n\r\n` +
29+
`${value}\r\n`
30+
)
31+
}
32+
33+
const closingBoundary = Buffer.from(`--${BOUNDARY}--\r\n`)
34+
35+
// Reproduces https://github.com/fastify/fastify-multipart/issues/630
36+
// The request emits 'close' once its body is fully piped into busboy,
37+
// which can happen while the consumer is still slowly reading an earlier
38+
// file part and busboy is still holding undelivered parts. Those trailing
39+
// parts must still be yielded instead of being cut off by the end marker.
40+
test('parts() delivers trailing parts even when the request closes while busboy still buffers them', async function (t) {
41+
t.plan(2)
42+
43+
const fastify = Fastify()
44+
t.after(() => fastify.close())
45+
46+
// a large busboy highWaterMark lets the whole request body drain into
47+
// busboy's writable buffer at once, so the request emits 'close' while
48+
// busboy still holds undelivered parts. The same ordering happens with
49+
// the default highWaterMark when a proxy re-segments the stream.
50+
fastify.register(multipart, { highWaterMark: 2 * 1024 * 1024 })
51+
52+
fastify.post('/', async function (req) {
53+
const seen = []
54+
for await (const part of req.parts()) {
55+
if (part.type === 'file') {
56+
let size = 0
57+
for await (const chunk of part.file) {
58+
size += chunk.length
59+
// simulate a slow storage write so the request finishes piping
60+
// (and emits 'close') while we are still consuming this file
61+
await sleep(10)
62+
}
63+
seen.push({ type: 'file', fieldname: part.fieldname, size })
64+
} else {
65+
seen.push({ type: 'field', fieldname: part.fieldname })
66+
}
67+
}
68+
return { seen }
69+
})
70+
71+
await fastify.listen({ port: 0 })
72+
73+
const image = Buffer.alloc(200 * 1024, 'a')
74+
const mask = Buffer.alloc(6 * 1024, 'b')
75+
const body = Buffer.concat([
76+
filePart('file', 'image.bin', image),
77+
filePart('mask', 'mask.bin', mask),
78+
fieldPart('clientJobId', 'job-1'),
79+
fieldPart('format', 'png'),
80+
fieldPart('quality', '90'),
81+
closingBoundary
82+
])
83+
84+
const req = http.request({
85+
protocol: 'http:',
86+
hostname: 'localhost',
87+
port: fastify.server.address().port,
88+
path: '/',
89+
method: 'POST',
90+
headers: {
91+
'content-type': `multipart/form-data; boundary=${BOUNDARY}`,
92+
'content-length': body.length
93+
}
94+
})
95+
req.end(body)
96+
97+
const [res] = await once(req, 'response')
98+
t.assert.strictEqual(res.statusCode, 200)
99+
100+
const chunks = []
101+
for await (const chunk of res) {
102+
chunks.push(chunk)
103+
}
104+
const { seen } = JSON.parse(Buffer.concat(chunks))
105+
t.assert.deepStrictEqual(seen, [
106+
{ type: 'file', fieldname: 'file', size: image.length },
107+
{ type: 'file', fieldname: 'mask', size: mask.length },
108+
{ type: 'field', fieldname: 'clientJobId' },
109+
{ type: 'field', fieldname: 'format' },
110+
{ type: 'field', fieldname: 'quality' }
111+
])
112+
})
113+
114+
test('parts() rejects when the multipart data is truncated instead of ending cleanly', async function (t) {
115+
t.plan(3)
116+
117+
const fastify = Fastify()
118+
t.after(() => fastify.close())
119+
120+
fastify.register(multipart)
121+
122+
fastify.post('/', async function (req, reply) {
123+
try {
124+
for await (const part of req.parts()) {
125+
if (part.file) {
126+
await part.toBuffer()
127+
}
128+
}
129+
} catch (err) {
130+
t.assert.strictEqual(err.message, 'Unexpected end of multipart data')
131+
return reply.code(400).send({ error: 'truncated' })
132+
}
133+
return reply.send({ error: 'iteration ended cleanly' })
134+
})
135+
136+
await fastify.listen({ port: 0 })
137+
138+
// a complete first part but no closing boundary: the request body ends
139+
// normally at the HTTP layer while the multipart data is truncated
140+
const body = Buffer.concat([
141+
filePart('file', 'image.bin', Buffer.alloc(1024, 'a')),
142+
fieldPart('clientJobId', 'job-1')
143+
])
144+
145+
const req = http.request({
146+
protocol: 'http:',
147+
hostname: 'localhost',
148+
port: fastify.server.address().port,
149+
path: '/',
150+
method: 'POST',
151+
headers: {
152+
'content-type': `multipart/form-data; boundary=${BOUNDARY}`,
153+
'content-length': body.length
154+
}
155+
})
156+
req.end(body)
157+
158+
const [res] = await once(req, 'response')
159+
t.assert.strictEqual(res.statusCode, 400)
160+
res.resume()
161+
await once(res, 'end')
162+
t.assert.ok('request completed')
163+
})
164+
165+
test('parts() rejects when the client aborts mid-upload instead of ending cleanly', async function (t) {
166+
t.plan(1)
167+
168+
const fastify = Fastify()
169+
t.after(() => fastify.close())
170+
171+
fastify.register(multipart)
172+
173+
let handled
174+
const handledPromise = new Promise((resolve) => { handled = resolve })
175+
176+
fastify.post('/', async function (req, reply) {
177+
try {
178+
for await (const part of req.parts()) {
179+
if (part.file) {
180+
for await (const chunk of part.file) {
181+
chunk.toString()
182+
}
183+
}
184+
}
185+
handled({ outcome: 'clean end' })
186+
} catch (err) {
187+
handled({ outcome: 'error', message: err.message })
188+
}
189+
return reply.send()
190+
})
191+
192+
await fastify.listen({ port: 0 })
193+
194+
const body = Buffer.concat([
195+
filePart('file', 'image.bin', Buffer.alloc(64 * 1024, 'a')),
196+
fieldPart('clientJobId', 'job-1'),
197+
closingBoundary
198+
])
199+
200+
const socket = net.connect(fastify.server.address().port, 'localhost')
201+
await once(socket, 'connect')
202+
socket.write(
203+
'POST / HTTP/1.1\r\n' +
204+
'Host: localhost\r\n' +
205+
`Content-Type: multipart/form-data; boundary=${BOUNDARY}\r\n` +
206+
`Content-Length: ${body.length}\r\n` +
207+
'\r\n'
208+
)
209+
// send only part of the body, then abort the connection
210+
socket.write(body.subarray(0, 16 * 1024))
211+
await sleep(100)
212+
socket.destroy()
213+
214+
const result = await handledPromise
215+
t.assert.strictEqual(result.outcome, 'error', `iteration must reject on abort, got: ${JSON.stringify(result)}`)
216+
})

types/index.d.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ interface MultipartErrors {
6262
PrototypeViolationError: FastifyErrorConstructor;
6363
InvalidMultipartContentTypeError: FastifyErrorConstructor;
6464
RequestFileTooLargeError: FastifyErrorConstructor;
65+
PrematureCloseError: FastifyErrorConstructor;
6566
}
6667

6768
declare namespace fastifyMultipart {

0 commit comments

Comments
 (0)