Skip to content

fix: report the decoded filename on LIMIT_FILE_SIZE - #1478

Open
MaxFreedomPollard wants to merge 1 commit into
expressjs:mainfrom
MaxFreedomPollard:fix-error-filename-decoding
Open

fix: report the decoded filename on LIMIT_FILE_SIZE#1478
MaxFreedomPollard wants to merge 1 commit into
expressjs:mainfrom
MaxFreedomPollard:fix-error-filename-decoding

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown

A LIMIT_FILE_SIZE error reports the filename with the WHATWG %0A, %0D and %22 escapes still in it, while file.originalname and LIMIT_UNEXPECTED_FILE report the unescaped name. Upload a file called file".ext that is over limits.fileSize and you get err.filename === 'file%22.ext'; send the same file to a field the middleware does not expect and you get err.filename === 'file".ext'. The README says errors about a specific file expose the client-supplied name in filename and to treat it as untrusted input "like file.originalname", so those two should be the same string.

The cause is that decodeFormDataName is called in exactly one place, lib/make-middleware.js:298, where the file object is built. The two abortWithCode('LIMIT_FILE_SIZE', ...) calls, at line 282 in the limit listener and line 324 in the fileFilter callback, pass busboy's raw filename instead. The decoding landed in fix: decode WHATWG-escaped characters in file originalname and err.filename landed just after in feat: add filename to file errors, which picked up the variable that was already in scope.

The fix decodes once at the top of the file handler into originalname and uses that for file.originalname and for both errors. busboy passes filename as undefined for a part that carries no file, so the decode is guarded; that part returns at the existing if (!filename) check and never reaches an error that names a file.

Verification on Node 22.23.2, macOS. npx mocha --exit test/filename-decoding.js on unmodified main with only the new tests applied fails both of them, + 'file%22.ext' against - 'file".ext' and + 'a%0D%0Ab.ext' against - 'a\r\nb.ext', and passes with the change. npm test goes from 132 passing to 134 passing with 2 pending and no failures. npm run lint is clean. The second test uses an async fileFilter so it exercises the other call site, the one where limit fires while the filter is still pending.

A LIMIT_FILE_SIZE error carried the raw client-supplied filename, still
carrying the WHATWG %0A, %0D and %22 escapes, while file.originalname and
LIMIT_UNEXPECTED_FILE carried the unescaped name. Uploading `file%22.ext`
over the size limit produced err.filename === 'file%22.ext' where
LIMIT_UNEXPECTED_FILE for the same upload produced 'file".ext'.

decodeFormDataName was applied only where file.originalname is built, so the
two abortWithCode('LIMIT_FILE_SIZE', ...) calls in lib/make-middleware.js
passed busboy's raw `filename` instead. Decode once at the top of the file
handler and use that value for file.originalname and for both errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant