Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not getting the full type when parsing multipart/form-data #2283

Closed
jimmywarting opened this issue Sep 23, 2023 · 9 comments · Fixed by #3683
Closed

Not getting the full type when parsing multipart/form-data #2283

jimmywarting opened this issue Sep 23, 2023 · 9 comments · Fixed by #3683
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented Sep 23, 2023

Bug Description

Not getting the full type when parsing multipart/form-data

Reproducible By

var b = new Blob(['123'], { type: 'text/plain;charset=utf-8'})
var fd = new FormData()
fd.set('x', b)
var res = new Response(fd)
res.clone().text().then(body => 
  // Just making sure that it contains ;charset=utf-8 (which it dose)
  console.log(body.includes('utf-8'))
)

new Response(fd).formData().then(fd => {
  // returns just 'text/plain'
  console.log(fd.get('x').type)
})

Expected Behavior

To get the full type, text/plain;charset=utf-8

Logs & Screenshots

Screenshot of chrome:
image

Screenshot of NodeJS
image

Environment

Not using undici directly - using Node.js v20.6.1 built in fetch

Additional context

presumable this revolves around the usage around Busboy...
Maybe the type should be extracted from the headers instead...? to get the raw content-type.

@jimmywarting jimmywarting added the bug Something isn't working label Sep 23, 2023
@KhafraDev
Copy link
Member

would you like to send in a PR?

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 23, 2023

would you like to send in a PR?

Not this time... this time i think i will pass.
Have plenty of other things to do.

@KhafraDev KhafraDev added the good first issue Good for newcomers label Sep 24, 2023
@barronw
Copy link

barronw commented Sep 29, 2023

Hi @jimmywarting and @KhafraDev. Can I work on this issue?

@jimmywarting
Copy link
Contributor Author

fine by me

@barronw
Copy link

barronw commented Sep 29, 2023

The fix seems to require a change to busboy, and there is a pull request (#2211) to replace busboy with @fastify/busboy. I'll wait to see if that lands before submitting a fix.

@IlyasShabi
Copy link
Contributor

@barronw Indeed, it comes from busboy, and even @fastify/busboy returns text/plain as the mimeType

@KhafraDev
Copy link
Member

@fastify/busboy is a fork of busboy, the options are probably the same

@IlyasShabi
Copy link
Contributor

IlyasShabi commented Sep 30, 2023

@KhafraDev @barronw I think that we can emit charset also boy.emit('file', fieldname, file, filename, encoding, contype, charset) https://github.com/fastify/busboy/blob/master/lib/types/multipart.js#L193 and then handle the logic from in undici like:
busboy.on('file', (name, value, filename, encoding, mimeType, charset) https://github.com/nodejs/undici/pull/2211/files#diff-bd6db9a8bceb4e9fce3f93ba5284c81f46a44a1187909e8609006e1efeab2570R399

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 30, 2023

@IlyasShabi
charset should not be the only thing we should worry about...

if i would like then i could just send this

var type = 'multipart/form-data; boundary=----WebKitFormBoundaryv9GiqHf1i48Tx1aI'
var otherFormData = new Blob(['123'], { type })
console.assert(otherFormData.type === type)
var fd = new FormData()
fd.set('embeded-formdata', otherFormData)

// later
fd = await request.formData()
console.assert(fd.get('embeded-formdata').type === type)

i should be able to put in a file that have completely nonsense type

new Blob(data, { type: 'x/y; foo=bar; fuz=123' })

and get back exactly the same thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants