Skip to content

fix: decode escaped field names, not just filenames - #1473

Open
MohammedAlkindi wants to merge 1 commit into
expressjs:mainfrom
MohammedAlkindi:fix/decode-escaped-field-names
Open

fix: decode escaped field names, not just filenames#1473
MohammedAlkindi wants to merge 1 commit into
expressjs:mainfrom
MohammedAlkindi:fix/decode-escaped-field-names

Conversation

@MohammedAlkindi

Copy link
Copy Markdown
Contributor

decodeFormDataName reverses the WHATWG escaping of %0A, %0D and %22, and its own comment says the rule covers "field names and filenames". It was only applied to filename. Busboy leaves both escaped, so req.body keys, file.fieldname and err.field all carried the raw escapes.

Measured against the unpatched tree:

body keys                        -> ["a%22b"]        expected a"b
upload.single('a"b') + that file -> LIMIT_UNEXPECTED_FILE
originalname                     -> q".txt           already correct

So an <input name='a"b' type=file> upload is rejected outright, and the only way to accept it was to register the escaped spelling.

fieldNameSize now measures the name as it arrived, since decoding shortens it and measuring the decoded form would quietly weaken that limit. decodeFormDataName itself is untouched, so the deliberate "never decodeURIComponent" property still holds and 50%.pdf stays intact.

Six new tests, two of which pass before the change as controls (a name with no escapes, and a literal percent sign). The other four fail on the unpatched tree. Each of the two call sites was neutered independently to confirm both are load-bearing.

Suite: 132 passing before, 138 after, 2 pending, no failures either side; standard exits 0.

This is the field-name half of #1421, which fixed the filename half.

The WHATWG serialisation rule that decodeFormDataName reverses covers field names as well as filenames, as its own comment says, but it was only applied to filename. Busboy leaves the escapes in place, so req.body keys, file.fieldname and err.field all carried the raw %22, %0D and %0A.

The practical effect is that upload.single('a"b') rejects a browser upload from an input named a"b with LIMIT_UNEXPECTED_FILE, and the only way to accept it was to register the escaped spelling.

fieldNameSize is measured on the name as it arrived, since decoding shortens it.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified on Windows with Node 24.12.0 at 26c5a6b: npm test passes (138 passing, 2 pending) and standard lint is clean. The new field-name cases cover percent-encoding decode, literal percent preservation, expected-file matching, and decoded limit errors; existing filename and storage/cleanup coverage remains green. I found no correctness issues.

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.

2 participants