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

Jump to bottom

Added support for busboy configuration parameters; 'defCharset', 'defParamCharset', 'highWaterMark' and 'fileHwm' #1102

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

Added support for busboy configuration parameters; 'defCharset', 'defParamCharset', 'highWaterMark' and 'fileHwm' #1102

Show file tree
Hide file tree
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
14 changes: 9 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,14 @@ The following are the options that can be passed to Multer.

Key | Description
--- | ---
`dest` or `storage` | Where to store the files
`fileFilter` | Function to control which files are accepted
`limits` | Limits of the uploaded data
`preservePath` | Keep the full path of files instead of just the base name
`dest` or `storage` | Where to store the files.
`fileFilter` | Function to control which files are accepted.
`limits` | Limits of the uploaded data. **Defaults:** check [busboy's page](https://github.com/mscdex/busboy#exports)
`preservePath` | Keep the full path of files instead of just the base name. **Default:** check [busboy's page](https://github.com/mscdex/busboy#exports)
`charset` | Character set to use when one isn't defined. Sets busboy's option `defCharset`. **Default:** `utf-8`. List of [available charsets](https://github.com/mscdex/busboy/blob/master/lib/utils.js#L384)
`paramCharset` | For multipart forms, the default character set to use for values of part header parameters (e.g. filename) that are not extended parameters (that contain an explicit charset). Sets busboy's option `defParamCharset`. **Default:** `utf-8`. List of [available charsets](https://github.com/mscdex/busboy/blob/master/lib/utils.js#L384).
Copy link

@zacharytyhacz zacharytyhacz Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't the default charset be latin1 if the existing default is latin1 ?

You are trying to override latin1 to be utf-8 correct ( in your case ) ?

If the existing default is latin1, and you are changing the default to utf-8 then this would be a breaking change I believe

Copy link
Author

@RopoMen RopoMen Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You are trying to override latin1 to be utf-8 correct ( in your case ) ?

Yes, and the reason for that is that Busboy has parsed filenames as utf8 before this commit 3 months ago mscdex/busboy@d7e4e2d

My opinion is that Multer should set its own defaults to ensure consistent behaviour and not rely on busboys defaults, because those may change without notice. This could be dealt with fixed version pinnings in package.json, but that may not be the best option.

Also preservePath should be set to false by Multer to ensure consistent behaviour. Although with that parameter I don't think that Busboy would change it to be true. Reasoning for this is that it depend on the client whether or not it sends the path of the file or not. And most of the clients don't send it.

And for the parameter handling; I didn't want to use same parameter names as Busboy because I see that Multer could someday relay on some other solution than Busboy and for example highwatermark params were named inconsistently in Busboy.

Copy link
Author

Choose a reason for hiding this comment

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

Busboy currently is not interested if filename field is said to be utf-8, it still uses latin1 decoder at least in Busboy 1.6.0.

Here Busboy takes filename from multipart form. It does not matter whether or not filename has encoding set like this filename*=utf-8 it still uses same decoder, that is defined here and if defParamCharset is not given, decoder will be latin1

And because of this knowledge, I think that this unit test in lts branch is broken https://github.com/expressjs/multer/blob/lts/test/unicode.js#L37-L44 and proper fix would be configuring defParamCharset: 'utf8' into Busboy, either Multer provides configuration option for that charset or hardcodes one into make-middleware.js

We also tested yesterday that Chrome is not adding encoding like this filename*=utf-8 into form. (we are using FormData)

I just checked and Busboy 1.4.0 has worked ok with utf-8 filenames, but after that the breaking commit is added
Screenshot 2022-06-03 at 7 29 31

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps I should comment this Unicode unit test again https://github.com/expressjs/multer/blob/lts/test/unicode.js#L37-L44

Because Busboy is handling filename*=utf-8 in a same way as filename= AND because Busboy >=1.4.0 parsed filenames always as utf-8; that test passed always. With Busboy 1.5.0+ that test should fail.

Copy link
Author

Choose a reason for hiding this comment

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

Tested, it didn't fail. So I'm missing now something here. 🤔

We had Multer 1.4.2 that installed Busboy 0.2.14. Filename test-åäöèøßð.pdf is uploaded from Chrome 100
Screenshot 2022-06-03 at 9 45 23

Filename looks like that and Multer returns req.file.originalname as test-åäöèøßð.pdf

Because of this npm audit issue GHSA-wm7h-9275-46v2 we updated Multer to 1.4.5-lts.1 that installs Busboy 1.6.0

Now, browser sends file as it did previosly test-åäöèøßð.pdf, but Multer req.file.originalname is test-aÌaÌoÌeÌøÃð.pdf. This can be fixed by setting defParamCharset = utf-8 into Busboy.

`writableHVM` | Node Stream highWaterMark to use for the parser stream. Sets busboy's option `highWaterMark`. **Default:** check [busboy's page](https://github.com/mscdex/busboy#exports)
`readableHVM` | Node Stream highWaterMark to use for individual file streams. Sets busboy's option `fileHwm`. **Default:** check [busboy's page](https://github.com/mscdex/busboy#exports)

In an average web app, only `dest` might be required, and configured as shown in
the following example.
Expand Down Expand Up @@ -260,7 +264,7 @@ memory storage is used.

### `limits`

An object specifying the size limits of the following optional properties. Multer passes this object into busboy directly, and the details of the properties can be found on [busboy's page](https://github.com/mscdex/busboy#busboy-methods).
An object specifying the size limits of the following optional properties. Multer passes this object into busboy directly, and the details of the properties can be found on [busboy's page](https://github.com/mscdex/busboy#exports).

The following integer values are available:

Expand Down
8 changes: 8 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ function Multer (options) {
}

this.limits = options.limits
this.charset = typeof options.charset === 'string' ? options.charset : 'utf-8'
this.paramCharset = typeof options.paramCharset === 'string' ? options.paramCharset : 'utf-8'
this.writableHVM = options.writableHVM
this.readableHVM = options.readableHVM
this.preservePath = options.preservePath
this.fileFilter = options.fileFilter || allowAll
}
Expand Down Expand Up @@ -48,6 +52,10 @@ Multer.prototype._makeMiddleware = function (fields, fileStrategy) {
limits: this.limits,
preservePath: this.preservePath,
storage: this.storage,
charset: this.charset,
paramCharset: this.paramCharset,
writableHVM: this.writableHVM,
readableHVM: this.readableHVM,
fileFilter: wrappedFileFilter,
fileStrategy: fileStrategy
}
Expand Down
15 changes: 13 additions & 2 deletions lib/make-middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,24 @@ function makeMiddleware (setup) {
var fileFilter = options.fileFilter
var fileStrategy = options.fileStrategy
var preservePath = options.preservePath

var charset = options.charset
var paramCharset = options.paramCharset
var writableHVM = options.writableHVM
var readableHVM = options.readableHVM
req.body = Object.create(null)

var busboy

try {
busboy = new Busboy({ headers: req.headers, limits: limits, preservePath: preservePath })
busboy = new Busboy({
headers: req.headers,
limits: limits,
preservePath: preservePath,
defCharset: charset,
defParamCharset: paramCharset,
fileHwm: readableHVM,
highWaterMark: writableHVM
})
} catch (err) {
return next(err)
}
Expand Down