urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326

papandreou · 2018-07-31T21:05:06Z

Fixes #194

Further elaborations here: ljharb/qs#268

Supports both the simple and extended parsers as outlined in ljharb/qs#268 (comment) -- and doesn't use the equivalent capabilities of the qs module.

~~Note: This will fail in node.js 0.8 and 0.10 as the capability to specify a custom decodeURIComponent implementation to use wasn't added until 0.12.~~

…d also accept iso-8859-1 as a default encoding.

Works in both extended and simple mode.

ljharb/qs#268 (comment)

dougwilson · 2018-08-09T13:37:18Z

Note: This will fail in node.js 0.8 and 0.10 as the capability to specify a custom decodeURIComponent implementation to use wasn't added until 0.12.

We can hold the PR for the 2.0 release, which will drop support for those versions 👍

papandreou · 2018-08-09T22:59:55Z

@dougwilson, that sounds fair! Does it look good otherwise?

Lurow · 2018-09-10T08:39:11Z

Hi there,

Any news on v2.0 release date with the pr?

Cheers

thopaw · 2019-01-09T19:21:55Z

Any news to iso-8859-1 support?

papandreou · 2019-08-13T18:34:22Z

Fixed conflict with master.

@dougwilson, this has been sitting for some time now 😅

I'm not really in a hurry, but how about getting this released soon? I can remove those ancient node versions from .travis.yml if that would be a help? (I guess it might be a nice occasion to drop more old versions)

dougwilson · 2019-08-13T18:38:29Z

The reason it has been sitting is just because it cannot be merged into the 1.x line due to the incompatibility. Because this module is a part of express, it effectively inherits it's support policy. The 1.x series of this module won't end up out of support for some time, and the count down starts with the next express release.

In order to reduce the support burden by maintaining multiple major versions of this module, I am just waiting to make the 2.0 in coordination with express 5.

If this could be made to work in those node.js versions I would of course be happy to land right away.

papandreou · 2019-08-15T22:43:55Z

Okay, thanks for the update, that makes sense!

If this could be made to work in those node.js versions I would of course be happy to land right away.

Unfortunately I think this would involve us to stop relying on the built-in querystring module in simple mode. Either by copying in a newer implementation than the one in 0.10, or by figuring out a way to use the qs module in simple mode as well, disabling the advanced behaviors. That seems a bit far-reaching, unless you were already planning to go in that direction?

ljharb · 2019-08-15T22:47:14Z

@papandreou if there's manageable changes i could make in qs that would allow body-parser to switch to it for everything, i'm willing to make those.

dougwilson · 2019-08-16T00:37:57Z

And I would be happy to use qs for everything here as well. The downside to using querystring for the simple parser is that since it ships as part of Node.js core, the results of this module will vary by both the version of this module and the version of Node.js (vs only varying by the version of this module).

ljharb · 2019-08-16T02:32:33Z

@dougwilson if you can save me a few minutes and link me to a branch that uses qs for the simple parser, but is failing tests, i can use npm link and see what options and changes it would take to get it working :-D 🙏

dougwilson · 2019-08-16T02:44:17Z

@ljharb this is likely not even correct for params to qs (I passed depth: -1 since 0 is not the right behavior, but idk), but it's at least something that should help :)

Branch: https://github.com/expressjs/body-parser/tree/urlencoded-qs-simple
Test: npm i && npm test -- --grep=bodyParser.urlencoded

ljharb · 2019-08-16T04:31:39Z

give me 48 hours, i'll see what i can do

ljharb · 2019-08-16T21:39:43Z

body-parser tests on that branch now pass with ljharb/qs#326 (comment), and I can merge and release that in qs as soon as I've convinced myself what semver it is.

dougwilson · 2019-08-16T22:00:18Z

Ah, yes, so depth: 0 was supposed to make sense after all :D . Yes, that is a hard decision. But if it helps at all, this module and Express do not directly expose the qs arguments, making whichever semver it is a non issue to us :)

ljharb · 2019-08-16T22:01:03Z

What I'm probably leaning towards is releasing it as a minor, and if users report breakage, reverting the depth 0 part, but keeping the depth false part - so express thus should use depth false to ensure continuity.

ljharb · 2019-08-17T02:45:42Z

v6.8.0 of qs is published.

DarkBitz · 2022-07-26T15:26:16Z

Is there a specific reason why the pull request is not being merged?

expressjs#326 (comment)

* Always use the qs module, even in simple mode #326 (comment) * Create the simple and extended parser with the same function, reducing duplication * Don't mention the querystring module in the README * Fix lint

wesleytodd · 2024-07-26T21:48:54Z

I merged #387 yesterday, I am trying to catch up on what was going on here but was thinking it might be a better idea to see if you (@papandreou) wanted to get this PR updated with that in mind? Or are we calling this not necessary anymore?

ljharb · 2024-07-26T21:52:33Z

Note: This will fail in node.js 0.8 and 0.10 as the capability to specify a custom decodeURIComponent implementation to use wasn't added until 0.12.

means it should probably wait for a major?

wesleytodd · 2024-07-26T23:09:44Z

A major is what I am doing! So that is why I am circling back on all these PRs.

EDIT: I should have pointed out the "changed the base branch from master to 2.x"

ljharb · 2024-07-26T23:11:33Z

in that case it seems like a great idea

wesleytodd · 2024-07-26T23:14:43Z

With that in mind, I am not up on the specifications involved here and am just doing the mechanical turk job of preparing all these stale branches for prime time, can you give a review and TLDR of what we should be aware of with this and #387 going out along with express@5?

ljharb

Reviewed some of it, but i'm not familiar enough with body-parser to review the rest.

ljharb · 2024-07-26T23:16:15Z

README.md

+##### defaultCharset
+
+The default charset to parse as, if not specified in content-type. Must be
+either `utf-8` or `iso-8859-1`.


this allows changing the charset, which is a nice semver-minor; the default should probably be utf-8

It is. Documented that in 44a6aa9 now.

ljharb · 2024-07-26T23:16:37Z

README.md

+Whether to let the value of the `utf8` parameter take precedence as the charset
+selector. It requires the form to contain a parameter named `utf8` with a value
+of `✓`. Defaults to `false`.


this goes along with defaultCharset

ljharb · 2024-07-26T23:16:48Z

README.md

+##### interpretNumericEntities
+
+Whether to decode numeric entities such as `&#9786;` when parsing an iso-8859-1
+form. Defaults to `false`.


and this goes along with the other defaultCharset value

ljharb · 2024-07-26T23:18:05Z

lib/types/urlencoded.js

+var charsetBySentinel = {
+ // This is what browsers will submit when the ✓ character occurs in an
+ // application/x-www-form-urlencoded body and the encoding of the page containing
+ // the form is iso-8859-1, or when the submitted form has an accept-charset
+ // attribute of iso-8859-1. Presumably also with other charsets that do not contain
+ // the ✓ character, such as us-ascii.
+ '✓': 'iso-8859-1', // encodeURIComponent('&#10003;')
+ // These are the percent-encoded utf-8 octets representing a checkmark, indicating
+ // that the request actually is utf-8 encoded.
+ '✓': 'utf-8' // encodeURIComponent('✓')
+}
+


this does confuse me a bit tho; it kind of seems like code that already exists in qs? meaning that the options can just be passed to qs and it'll handle the logic.

altho if someone's using a different querystring parser then the logic might need replicating

You're correct that these magic values are also found here. I spent some time trying to remember why also having the decoding logic here was warranted. It seems like we can get actually get rid of it and still have the test suite pass!

I wonder if I did it like this because it seemed like a long shot to get changes into the qs module, so I went for body-parser first, because that's what I needed myself. It has been 6 years, though, so I'm not really sure 🤔

* 2.x: (98 commits) fix(deps): raw-body@^3.0.0 (expressjs#529) Also use the qs module for the simple parser (expressjs#387) feat!: remove node less than 18 from ci 2.0.0-beta.2 docs: add missing history entry tests: enable strict mode Remove deprecated bodyParser() combination middleware build: remove conditional code coverage deps: [email protected] deps: [email protected] deps: [email protected] 1.20.2 Fix strict json error message on Node.js 19 deps: [email protected] build: [email protected] build: [email protected] build: [email protected] deps: content-type@~1.0.5 build: [email protected] build: [email protected] ...

papandreou · 2024-07-27T10:56:16Z

I merged #387 yesterday, I am trying to catch up on what was going on here but was thinking it might be a better idea to see if you (@papandreou) wanted to get this PR updated with that in mind? Or are we calling this not necessary anymore?

I think it's still a nice-to-have! I've merged the 2.x branch in and resolved the conflicts now.

expressjs#326 (comment)

ljharb

LGTM - altho i don’t actually see how this is breaking? where is a custom decodeURIComponent implementation used?

papandreou · 2024-07-27T21:39:07Z

altho i don’t actually see how this is breaking? where is a custom decodeURIComponent implementation used?

Ah, that referred to the querystring module built into node.js. In node.js 0.12 and above, it supports specifying a custom decodeURIComponent implementation (docs), whereas 0.10 and below didn't (docs).

Before #387, body-parser would use the querystring module when not configured to use the extended parsing mode. I think that meant that without #387, the new features introduced in this PR would only have worked in extended mode in node.js 0.10 and below. Even 6 years ago, node.js 0.12 had been out for 6 years, and it seemed fine to just live with it and do a new major release. And here we (almost) are 😆

wesleytodd · 2024-07-30T21:42:20Z

Well either way I don't think we are doing any extra work to land these things in the current majors. We have needed to clean house on much of this for a while, so as long as this is good to land we will be doing it along with a major no matter what. And with that in mind, here I am clicking the merge button. We will be preparing the v2 release soon.

papandreou and others added 5 commits July 17, 2018 00:27

urlencoded, in extended mode: Support iso-8859-1 encoded requests, an…

9c3860d

…d also accept iso-8859-1 as a default encoding.

urlencoded: Support an utf8 sentinel to detect the charset.

6f5406c

Pass the interpretNumericEntities option through to qs.

cf7807b

Fix lint

22eb75a

Support charsets, sentinels etc. via custom decoders

5c25f5d

Works in both extended and simple mode.

papandreou mentioned this pull request Jul 31, 2018

Add support for iso-8859-1, utf8 "sentinel" and numeric entities ljharb/qs#268

Merged

papandreou added 3 commits July 31, 2018 23:24

Simplify

dab286a

Fix empty parameter issue with utf8Sentinel in simple mode

e3fca5d

Run all the charset/sentinel tests in both extended and simple modes

5b39b86

papandreou force-pushed the feature/iso-8859-1/take2 branch from c056515 to 5b39b86 Compare July 31, 2018 21:25

utf8Sentinel => charsetSentinel

5daeca8

ljharb/qs#268 (comment)

dougwilson added pr enhancement labels Aug 9, 2018

Merge branch 'master' into feature/iso-8859-1/take2

0fb82de

Merge branch 'master' into feature/iso-8859-1/take2

f4a3bca

dougwilson force-pushed the master branch from 70560b1 to b356758 Compare January 28, 2022 05:13

dougwilson force-pushed the master branch from acc026f to b6bfabd Compare February 15, 2022 00:53

dougwilson force-pushed the master branch 3 times, most recently from 0ad1d88 to 2a2f471 Compare April 3, 2022 00:47

papandreou mentioned this pull request Apr 27, 2024

iso-8859-1/windows-1252 support? #194

Open

wesleytodd mentioned this pull request May 17, 2024

Express 5.0 - last push! expressjs/discussions#233

Open

44 tasks

papandreou added a commit to papandreou/body-parser that referenced this pull request Jul 23, 2024

Always use the qs module, even in simple mode

779dffc

expressjs#326 (comment)

papandreou added a commit to papandreou/body-parser that referenced this pull request Jul 23, 2024

Always use the qs module, even in simple mode

11c7d20

expressjs#326 (comment)

wesleytodd changed the base branch from master to 2.x July 26, 2024 21:44

ljharb reviewed Jul 26, 2024

View reviewed changes

papandreou force-pushed the feature/iso-8859-1/take2 branch from 14fb806 to 9a4ebac Compare July 27, 2024 10:54

papandreou added 4 commits July 27, 2024 14:16

Simplify by relying on the qs module's support for detecting the charset

cbf4cec

expressjs#326 (comment)

Simplify further

97d2044

Put back debug option

9d47455

Document that defaultCharset defaults to utf-8

44a6aa9

expressjs#326 (comment)

ljharb reviewed Jul 27, 2024

View reviewed changes

wesleytodd merged commit 6cea6bd into expressjs:2.x Jul 30, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326

urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326

papandreou commented Jul 31, 2018 •

edited

Loading

dougwilson commented Aug 9, 2018

papandreou commented Aug 9, 2018

Lurow commented Sep 10, 2018

thopaw commented Jan 9, 2019

papandreou commented Aug 13, 2019

dougwilson commented Aug 13, 2019

papandreou commented Aug 15, 2019 •

edited

Loading

ljharb commented Aug 15, 2019

dougwilson commented Aug 16, 2019

ljharb commented Aug 16, 2019

dougwilson commented Aug 16, 2019

ljharb commented Aug 16, 2019

ljharb commented Aug 16, 2019

dougwilson commented Aug 16, 2019

ljharb commented Aug 16, 2019

ljharb commented Aug 17, 2019 •

edited

Loading

DarkBitz commented Jul 26, 2022

wesleytodd commented Jul 26, 2024

ljharb commented Jul 26, 2024

wesleytodd commented Jul 26, 2024 •

edited

Loading

ljharb commented Jul 26, 2024

wesleytodd commented Jul 26, 2024

ljharb left a comment

ljharb Jul 26, 2024

papandreou Jul 27, 2024

ljharb Jul 26, 2024

ljharb Jul 26, 2024

ljharb Jul 26, 2024

papandreou Jul 27, 2024 •

edited

Loading

papandreou commented Jul 27, 2024

ljharb left a comment

papandreou commented Jul 27, 2024

wesleytodd commented Jul 30, 2024

urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326

urlencoded: Support iso-8859-1, utf8 sentinel, and numeric entities #326

Conversation

papandreou commented Jul 31, 2018 • edited Loading

dougwilson commented Aug 9, 2018

papandreou commented Aug 9, 2018

Lurow commented Sep 10, 2018

thopaw commented Jan 9, 2019

papandreou commented Aug 13, 2019

dougwilson commented Aug 13, 2019

papandreou commented Aug 15, 2019 • edited Loading

ljharb commented Aug 15, 2019

dougwilson commented Aug 16, 2019

ljharb commented Aug 16, 2019

dougwilson commented Aug 16, 2019

ljharb commented Aug 16, 2019

ljharb commented Aug 16, 2019

dougwilson commented Aug 16, 2019

ljharb commented Aug 16, 2019

ljharb commented Aug 17, 2019 • edited Loading

DarkBitz commented Jul 26, 2022

wesleytodd commented Jul 26, 2024

ljharb commented Jul 26, 2024

wesleytodd commented Jul 26, 2024 • edited Loading

ljharb commented Jul 26, 2024

wesleytodd commented Jul 26, 2024

ljharb left a comment

Choose a reason for hiding this comment

ljharb Jul 26, 2024

Choose a reason for hiding this comment

papandreou Jul 27, 2024

Choose a reason for hiding this comment

ljharb Jul 26, 2024

Choose a reason for hiding this comment

ljharb Jul 26, 2024

Choose a reason for hiding this comment

ljharb Jul 26, 2024

Choose a reason for hiding this comment

papandreou Jul 27, 2024 • edited Loading

Choose a reason for hiding this comment

papandreou commented Jul 27, 2024

ljharb left a comment

Choose a reason for hiding this comment

papandreou commented Jul 27, 2024

wesleytodd commented Jul 30, 2024

papandreou commented Jul 31, 2018 •

edited

Loading

papandreou commented Aug 15, 2019 •

edited

Loading

ljharb commented Aug 17, 2019 •

edited

Loading

wesleytodd commented Jul 26, 2024 •

edited

Loading

papandreou Jul 27, 2024 •

edited

Loading