-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Added support for busboy configuration parameters; 'defCharset', 'defParamCharset', 'highWaterMark' and 'fileHwm' #1102
Open
RopoMen
wants to merge
1
commit into
expressjs:master
Choose a base branch
from
RopoMen:add-busboy-configurations
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 islatin1
?You are trying to override
latin1
to beutf-8
correct ( in your case ) ?If the existing default is
latin1
, and you are changing the default toutf-8
then this would be a breaking change I believeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 tofalse
by Multer to ensure consistent behaviour. Although with that parameter I don't think that Busboy would change it to betrue
. 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.There was a problem hiding this comment.
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 useslatin1
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 thisfilename*=utf-8
it still uses same decoder, that is defined here and ifdefParamCharset
is not given, decoder will belatin1
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 configuringdefParamCharset: 'utf8'
into Busboy, either Multer provides configuration option for that charset or hardcodes one into make-middleware.jsWe also tested yesterday that Chrome is not adding encoding like this
filename*=utf-8
into form. (we are usingFormData
)I just checked and Busboy 1.4.0 has worked ok with utf-8 filenames, but after that the breaking commit is added
There was a problem hiding this comment.
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 asfilename=
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.There was a problem hiding this comment.
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 100Filename looks like that and Multer returns
req.file.originalname
astest-åäöèøßð.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 Multerreq.file.originalname
istest-aÌaÌoÌeÌøÃð.pdf
. This can be fixed by settingdefParamCharset = utf-8
into Busboy.