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.

Sign up for GitHub

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

Fix file exists but file name does not exist #1216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix file exists but file name does not exist #1216

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/make-middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,15 @@ function makeMiddleware (setup) {
// handle files
busboy.on('file', function (fieldname, fileStream, filename, encoding, mimetype) {
// don't attach to the files object, if there is no file
if (!filename) return fileStream.resume()
if (!filename) {
// @link https://github.com/mscdex/busboy/blob/master/lib/types/multipart.js
// If the type is 'application/octet-stream', it is also identified as a file
if(mimetype==='application/octet-stream'){
filename = 'unknownFile'
}else{
return fileStream.resume()
}
}
Comment on lines +99 to +107
Copy link

@ljwagerfield ljwagerfield Jan 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team!

I've been thinking about the current implementation of this if statement. It might be beneficial for us to consider allowing undefined filenames. This change would give developers more flexibility in handling cases where a client does or does not provide a filename field. This can be pretty important for certain applications.

I wanted to share an interesting point that @Relsoul brought up regarding RFC 7578. The RFC mentions that the filename in the content disposition is recommended but not mandatory. It states:

For form data that represents the content of a file, a name for the file SHOULD be supplied as well, by using a 'filename' parameter of the Content-Disposition header field.
...
The file name isn't mandatory for cases where the file name isn't available or is meaningless or private

With this in mind, it seems quite likely that developers using Multer would appreciate the ability to handle RFC-compliant uploads in a way that best suits their needs, especially when a filename isn't provided. For example, in our system, we prefer storing NULL in the database for the "original file name" if the uploader doesn't provide it, instead of assigning a default value.

Interestingly, busboy allows for undefined filenames, which is a great feature for flexibility. I think it might be a good idea for multer to adopt a similar approach. What do you all think?

To gain parity with busboy, it would simply be a case of removing this entire if statement, and acknowledging that the originalname field may be undefined to reflect RFC 7578:

Suggested change
if (!filename) {
// @link https://github.com/mscdex/busboy/blob/master/lib/types/multipart.js
// If the type is 'application/octet-stream', it is also identified as a file
if(mimetype==='application/octet-stream'){
filename = 'unknownFile'
}else{
return fileStream.resume()
}
}


// Work around bug in Busboy (https://github.com/mscdex/busboy/issues/6)
if (limits && Object.prototype.hasOwnProperty.call(limits, 'fieldNameSize')) {
Expand Down