Malformed boundary does not throw

[gurgunday]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

X

Plugin version

X

Node.js version

20

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

Latest

Description

I want to merge @fastify/busboy into undici, but a specific test will not currently pass:

test('busboy emit error', async (t) => {
  t.plan(1)
  const formData = new FormData()
  formData.append('field1', 'value1')

  const tempRes = new Response(formData)
  const formRaw = await tempRes.text()

  const server = createServer((req, res) => {
    res.setHeader('content-type', 'multipart/form-data; boundary=wrongboundary')
    res.write(formRaw)
    res.end()
  })
  t.teardown(server.close.bind(server))

  const listen = promisify(server.listen.bind(server))
  await listen(0)

  const res = await fetch(`http://localhost:${server.address().port}`)
  await t.rejects(res.formData(), 'Unexpected end of multipart data')
})

Busboy ignores this input rather than throwing for malformed boundary

I found within the code a part that should throw in this case (if I'm not mistaken), but it's not working in practice

I will take a look at this myself, but if anyone could help, it would be highly appreciated

Steps to Reproduce

Run this test after replacing busboy with @fastify/busboy in undici:

test('busboy emit error', async (t) => {
  t.plan(1)
  const formData = new FormData()
  formData.append('field1', 'value1')

  const tempRes = new Response(formData)
  const formRaw = await tempRes.text()

  const server = createServer((req, res) => {
    res.setHeader('content-type', 'multipart/form-data; boundary=wrongboundary')
    res.write(formRaw)
    res.end()
  })
  t.teardown(server.close.bind(server))

  const listen = promisify(server.listen.bind(server))
  await listen(0)

  const res = await fetch(`http://localhost:${server.address().port}`)
  await t.rejects(res.formData(), 'Unexpected end of multipart data')
})

Expected Behavior

Test should pass

[gurgunday]

Ref PR:

nodejs/undici#2211

[Uzlopak]

Why should it actually throw?

[gurgunday]

That's what the undici unit test demands, and more importantly what the browser behavior is

Normal busboy does throw there, so I'm inclined to believe one of our patches prevents/ignores the error

[Uzlopak]

undici throws, ok. Where is it defined that a non matching boundary should throw in the first place?

Anyhow: It should be actually no problem to patch it accordingly to throw.

[gurgunday]

Where is it defined that a non matching boundary should throw in the first place?

I actually couldn't spot it in the multipart formdata RFC, maybe they mimicked browser behavior (i.e., Safari)

[KhafraDev]

It's part of the fetch spec (https://fetch.spec.whatwg.org/#dom-body-formdata):

[...] Parse bytes, using the value of the `boundary` parameter from mimeType, per the rules set forth in Returning Values from Forms: multipart/form-data.
[...] If that fails for some reason, then [throw](https://webidl.spec.whatwg.org/#dfn-throw) a [TypeError](https://webidl.spec.whatwg.org/#exceptiondef-typeerror).

Returning a value from a malformed body doesn't seem like correct behavior to me - the body can't be parsed if the boundary is mismatched, but the parsing "succeeds" in the sense that an object is returned.

[KhafraDev]

In the RFC:

   In the case of multipart entities, in which one or more different
   sets of data are combined in a single body, a "multipart" media type
   field must appear in the entity's header.  The body must then contain
   one or more body parts, each preceded by a boundary delimiter line,
   and the last one followed by a closing boundary delimiter line.

note that the RFC says the body must contain > 1 parts and must be delimited with the boundary (new lines, etc.) https://www.rfc-editor.org/rfc/rfc2046#section-5.1

[Uzlopak]

Ok, you convinced me.

Unfortunately i am currently not that deep in the codebase. But if you could provide a PR, i would review it asap.

[gurgunday]

Now all we need is a semver major release 🤠

I do wanna do some refactoring too but nothing involving behavior change

[Uzlopak]

I have to bump @kibertoad to release a new major. I dont think I have npm publish rights.

Also I want to change to node-tap, so that tests, are faster

Also come to discord if possible.. have to coordinate with you.

[gurgunday]

I’d love to! Where can I find the Discord server

[Uzlopak]

Click here:

[gurgunday]

@climba03003 @Eomm

Although breaking, this was an important change that aligned busboy with the RFC

It will also let us merge this more performant version into undici

Can we make a release? I don't want to take initiative without asking 😁

[gurgunday]

Latest results:

{ cpu: { brand: 'M1', speed: '2.4 GHz' } }
| Node    | Option         | Msecs/op       | Ops/sec  | V8                    |
| ------- | -------------- | -------------- | -------- | --------------------- |
| 20.6.0  | fastify-busboy | 0.110915 msecs | 9015.906 | V8 11.3.244.8-node.14 |
| 20.6.0  | busboy         | 0.207797 msecs | 4812.393 | V8 11.3.244.8-node.14 |

[gurgunday]

@mcollina can you move this under the plugins team?

[kibertoad]

@gurgunday I can push out a release today

[gurgunday]

Alright, thanks!

Just to restate the obvious, this was a breaking change

[kibertoad]

@gurgunday Released 2.0.0