fix: normalize the defaultCharset option to lower case - #764
Conversation
|
I reviewed the exact head The normalization fixes mixed-case charset names before downstream charset validation while preserving an explicit request The change is focused and the added unit test covers the regression. GitHub does not allow my account to submit a formal approval on this repository, so I am recording the verification evidence here for the maintainers. |
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
I verified this change against exact head cd72cdf with real HTTP requests.
Base/head comparison:
- On the base, json({ defaultCharset: 'UTF-8' }) returns 415 for JSON without an explicit charset; the head parses the same UTF-8 payload successfully.
- Mixed-case UtF-16Le correctly decodes a UTF-16LE JSON body on the head.
- urlencoded({ defaultCharset: 'IsO-8859-1' }) correctly parses name=%E9 as é on the head; the base rejects the uppercase spelling at construction.
- An explicit Content-Type charset=UTF-8 still overrides an ISO-8859-1 default and parses successfully.
I also ran the complete test suite with the exact head implementation: 273 passing, 4 pending, 0 failures.
I checked non-string and falsy option boundaries as well. Truthy non-string values now fail earlier at construction, but those values are outside the documented string contract; null, undefined, empty string, false, and zero retain the existing UTF-8 fallback behavior. I did not find a functional regression in the supported inputs.
|
Correction to my review above: I reran |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Applied, thanks. 274 passing, lint clean. One behaviour note so the choice is explicit: the two forms differ only on non-string input. |
|
I think we decided to introduce stricter option validation in the next major release, but I'm not entirely sure anymore. Right now we're in an awkward middle ground. We're neither fully strict nor fully lenient. I'm not sure how we should move forward here. Any opinions, @UlisesGascon @bjohansebas? |
Charset names are case-insensitive (RFC 2978). body-parser already lower-cases the charset parsed from the request's
Content-Type(getCharset()), but the developer-configureddefaultCharsetoption is stored verbatim, so a differently-cased but valid value breaks:json({ defaultCharset: 'UTF-8' })rejects a normalapplication/jsonrequest (no charset param) with415 unsupported charset "UTF-8", becauseisValidCharsetcheckscharset.slice(0, 4) === 'utf-'.urlencoded({ defaultCharset: 'UTF-8' })throws at construction:option defaultCharset must be either utf-8 or iso-8859-1.Lower-casing
defaultCharsetinnormalizeOptionsfixes both call sites, matching how the request-header charset is already normalized.