url: Improve WHATWG URLSearchParams spec compliance by TimothyGu · Pull Request #9484 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added (WHATWG URL interface is still experimental and undocumented)
commit message follows commit guidelines

Affected core subsystem(s)

url, test

Description of change

This PR aims to improve the general compatibility of URLSearchParams class with WHATWG's URL Standard. Specifically, the PR consists of the following parts:

Make URLSearchParams constructor signature the same as the spec
Strip leading ? in URL#search's setter
Spec-compliant iterable interface
- Implementation of keys, entries, and forEach methods
- Use a dedicated URLSearchParamsIterator object instead of a generator function for @@iterator, per spec
Add tests from W3C's Web Platform Tests.
- Some tests, e.g. the ones related to %20 vs +, are still commented out.

TODOs and points of discussion:

Make URLSearchParams public
Crediting W3C for the tests
It might be better for iterator creation to be made generic and shared for the entire code base.
Investigate the applicability of a "security check" as mandated by the spec for @@iterator, keys, values, and entries
- It is not applicable
Multiple deviations of querystring's behaviors with the WHATWG URL Standard
- querystring does not parse empty values properly
- Currently, querystring uses an implementation of encodeURIComponent to encode the query string. However, the application/x-www-form-urlencoded byte serializer disagrees on certain characters such as 0x20 (space; encodeURIComponent escapes it as %20 while querystring should serialize it to +), 0x27 (left paren; encodeURIComponent does not escape but querystring should), etc.
Is there a reason against making all values in the hashmap arrays?

Fixes: #9302

/cc @jasnell

Awesome! I won't have a chance to review in depth until Monday but it's awesome to see you jump in on this. Thank you!

This line is more for consistency with the constructor and cosmetics than anything else: it's a bit odd to see the property being read in its setter.

About making URLSearchParams public: it was indeed proposed in the EP document:

Accessible via require('url').URLSearchParams

In addition to the %20 vs + issue reported above, another bug/nonconformity in querystring is exemplified by this test case:

While the standard says to:

4.Let sequences be the result of splitting input on &.

...

6.For each byte sequence bytes in sequences, run these substeps:

If bytes is the empty byte sequence, run these substeps for the next byte sequence.

which essentially means to skip all instances of leading, trailing, or consecutive ampersands, querystring treats it as =:

querystring.parse('')
// => ✓ {}
querystring.parse('&')
// => ✗ { '': '' }
// => Expected: {}
querystring.parse('=')
// => ✓ { '': '' }
querystring.parse('&=')
// => ✗ { '': [ '', '' ] }
// => Expected: { '': '' }
querystring.parse('&&')
// => ✗ { '': [ '', '' ] }
// => Expected: {}
querystring.parse('&&=')
// => ✗ { '': [ '', '', '' ] }
// => Expected: { '': '' }

// In Chrome 55 with #enable-experimental-web-platform-features
[...new URLSearchParams('')]
// => ✓ []
[...new URLSearchParams('&')]
// => ✓ []
[...new URLSearchParams('=')]
// => ✓ [ [ '', '' ] ]
[...new URLSearchParams('&=')]
// => ✓ [ [ '', '' ] ]
[...new URLSearchParams('&&')]
// => ✓ []
[...new URLSearchParams('&&=')]
// => ✓ [ [ '', '' ] ]

Another thing I realized is that currently the conversion from ECMAScript String to IDL's USVString isn't strictly done in the JS-based URLSearchParams code (while it is being done in the C++ code for URL parsing, presumably from the construction of a Utf8Value), which leads to cases such as the following:

const { URL } = require('url')

// '\dc00' is an unpaired trail Unicode surrogate
const url = new URL('https://asdf.com/?a=\udc00asdf')

url.search
// => '?a=%EF%BF%BDasdf'
// This is correct: `%EF%BF%BD` corresponds to U+FFFD REPLACEMENT CHARACTER,
// as stipulated by the Web IDL spec:
// https://heycam.github.io/webidl/#dfn-obtain-unicode

url.searchParams.set('a', '\udc00asdf')

url.search
// => '?a=%F0%90%81%A1sdf'
// Here a URL with an invalid Unicode string results, as there isn't a check for
// unpaired surrogates in the userland JS code.

url.search = 'a=\udc00asdf'

url.search
// => '?a=%EF%BF%BDasdf'
// search's setter forces a reparse, so C++ code's Unicode
// sanitation kicks in here

So in addition to making the querystring module conformant to WHATWG standards, this issue would have to be fixed somehow, either by doing some string wizardry in JS or add a C++ binding for sanitizing.

Yep, you're absolutely right. In the initial implementation I decided to punt on 100% compliance with the URLSearchParams in order to get it landed, knowing that we could continue to iterate on it while the feature was still experimental. My plan has been to move the processing into the node_url.cc native code to echo the main URL parsing.

@jasnell, have you gotten a chance to look at this PR?

I have not and I am sorry. I have been in Tokyo for nodefest and have been
a bit distracted. I will review tomorrow when I am back in the States

On Mon, Nov 14, 2016 at 7:32 AM Timothy Gu notifications@github.com wrote:

@jasnell https://github.com/jasnell, have you gotten a chance to look
at this PR?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9484 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eVZF4GtP8CNucE4vgaizwrPj_o0Eks5q95AJgaJpZM4KqbjZ
.

@jasnell, ping.

Just a heads up, in case anyone is interested I've reimplemented the core parsing and serializing routine in C++, in TimothyGu/node@urlsearchparams...urlsearchparams-cpp. I will submit a PR for that after this one is merged.

@TimothyGu ... I've taken an initial look at things look good but have not yet had the opportunity to do a proper complete review. Haven't forgotten about it tho!

- Make URLSearchParams constructor spec-compliant - Strip leading `?` in URL#search's setter - Spec-compliant iterable interface - More precise handling of update steps as mandated by the spec - Add class strings to URLSearchParams objects and their prototype - Make sure `this instanceof URLSearchParams` in methods Also included are relevant tests from W3C's Web Platform Tests (https://github.com/w3c/web-platform-tests/tree/master/url). Fixes: nodejs#9302

@jasnell, ping 3x...

CI: https://ci.nodejs.org/job/node-test-pull-request/5177/

LGTM if CI is green! Thank you very much!

@TimothyGu ... things look good except for a couple of linter nits. Can you run make lint and fix the errors.

New CI: https://ci.nodejs.org/job/node-test-pull-request/5181/

- Make URLSearchParams constructor spec-compliant - Strip leading `?` in URL#search's setter - Spec-compliant iterable interface - More precise handling of update steps as mandated by the spec - Add class strings to URLSearchParams objects and their prototype - Make sure `this instanceof URLSearchParams` in methods Also included are relevant tests from W3C's Web Platform Tests (https://github.com/w3c/web-platform-tests/tree/master/url). Fixes: #9302 PR-URL: #9484 Reviewed-By: James M Snell <jasnell@gmail.com>

Not in 7.2.1? It's not in the changelog, anyway.

- Make URLSearchParams constructor spec-compliant - Strip leading `?` in URL#search's setter - Spec-compliant iterable interface - More precise handling of update steps as mandated by the spec - Add class strings to URLSearchParams objects and their prototype - Make sure `this instanceof URLSearchParams` in methods Also included are relevant tests from W3C's Web Platform Tests (https://github.com/w3c/web-platform-tests/tree/master/url). Fixes: #9302 PR-URL: #9484 Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2.fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 PR-URL: #10277

Notable changes SEMVER-MINOR - url: - add inspect function to TupleOrigin (Safia Abdalla) #10039 - crypto: - allow adding extra certs to well-known CAs (Sam Roberts) #9139 SEMVER-PATCH - buffer: - fix single-character string filling (Anna Henningsen) #9837 - handle UCS2 .fill() properly on BE (Anna Henningsen) #9837 - url: - including base argument in originFor (joyeecheung) #10021 - improve URLSearchParams spec compliance (Timothy Gu) #9484 - http: - remove stale timeout listeners (Karl Böhlmark) #9440 - build: - fix node_g target (Daniel Bevenius) #10153 - fs: - remove unused argument from copyObject() (Ethan Arrowood) #10041 - timers: - fix handling of cleared immediates (hveldstra) #9759 PR-URL: #10277

Notable changes: * **crypto**: - Allow adding extra certificates to well-known CAs. (Sam Roberts) [#9139](#9139) * **buffer**: - Fix single-character string filling. (Anna Henningsen) [#9837](#9837) - Handle UCS2 `.fill()` properly on BE. (Anna Henningsen) [#9837](#9837) * **url**: - Add inspect function to TupleOrigin. (Safia Abdalla) [#10039](#10039) - Including base argument in originFor. (joyeecheung) [#10021](#10021) - Improve URLSearchParams spec compliance. (Timothy Gu) [#9484](#9484) * **http**: - Remove stale timeout listeners. (Karl Böhlmark) [#9440](#9440) * **build**: - Fix node_g target. (Daniel Bevenius) [#10153](#10153) * **fs**: - Remove unused argument from copyObject(). (EthanArrowood) [#10041](#10041) * **timers**: - Fix handling of cleared immediates. (hveldstra) [#9759](#9759) * **src**: - Add wrapper for process.emitWarning(). (SamRoberts) [#9139](#9139) - Fix string format mistake for 32 bit node.(Alex Newman) [#10082](#10082)

Notable changes: * **crypto**: The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) [#9139](#9139) * **buffer**: buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) [#9837](#9837) * **url**: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) [#10021](#10021) - Improve URLSearchParams to meet specification compliance. (Timothy Gu) [#9484](#9484) * **http**: Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) [#9440](#9440)

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) nodejs#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs#9484 PR-URL: nodejs#10277

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) #9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) #10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) #9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Böhlmark) #9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) #10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) #10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) #9484 PR-URL: #10277

Notable changes: * buffer: - buffer.fill() now works properly for the UCS2 encoding on Big-Endian machines. (Anna Henningsen) nodejs/node#9837 * cluster: - disconnect() now returns a reference to the disconnected worker. (Sean Villars) nodejs/node#10019 * crypto: - The built-in list of Well-Known CAs (Certificate Authorities) can now be extended via a NODE_EXTRA_CA_CERTS environment variable. (Sam Roberts) nodejs/node#9139 * http: - Remove stale timeout listeners in order to prevent a memory leak when using keep alive. (Karl Bohlmark) nodejs/node#9440 * tls: - Allow obvious key/passphrase combinations. (Sam Roberts) nodejs/node#10294 * url: - Including base argument in URL.originFor() to meet specification compliance. (joyeecheung) nodejs/node#10021 - Improve URLSearchParams to meet specification compliance. (Timothy Gu) nodejs/node#9484 PR-URL: nodejs/node#10277 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

nodejs-github-bot added the url Issues and PRs related to the legacy built-in url module. label Nov 6, 2016

TimothyGu force-pushed the urlsearchparams branch from 20a610e to 3c099fa Compare November 6, 2016 01:30

TimothyGu commented Nov 6, 2016

View reviewed changes

TimothyGu changed the title Improve URLSearchParams spec compliance url: Improve WHATWG URLSearchParams spec compliance Nov 7, 2016

TimothyGu mentioned this pull request Nov 13, 2016

query string parameters on GET node-fetch/node-fetch#189

Closed

TimothyGu force-pushed the urlsearchparams branch from 3c099fa to 9795284 Compare November 13, 2016 22:24

TimothyGu force-pushed the urlsearchparams branch 2 times, most recently from d04241e to fe08218 Compare November 25, 2016 00:40

jasnell self-assigned this Nov 25, 2016

TimothyGu force-pushed the urlsearchparams branch 4 times, most recently from 9754f70 to 796213f Compare November 27, 2016 05:08

TimothyGu force-pushed the urlsearchparams branch from 796213f to 8ea0a65 Compare November 27, 2016 05:24

jasnell approved these changes Dec 4, 2016

View reviewed changes

Fix linting error

5d73072

italoacasas closed this Dec 7, 2016

TimothyGu deleted the urlsearchparams branch December 7, 2016 17:23

addaleax added the dont-land-on-v6.x label Dec 7, 2016

italoacasas mentioned this pull request Dec 15, 2016

v7.3.0 proposal #10277

Merged

This was referenced Jan 14, 2017

url: export URLSearchParams #10801

Closed

url: track status of a spec-compliant URLSearchParams #10821

Closed

Add opaque hosts whatwg/url#185

Merged

TimothyGu mentioned this pull request Jan 24, 2017

querystring, url: repeated '&' in a paramsString will be skipped #10967

Closed

3 tasks

joyeecheung mentioned this pull request Jan 31, 2017

url, test: synchronize web-platform-test url tests #11079

Closed

3 tasks

hiroppy added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Apr 21, 2017

This was referenced Nov 11, 2024

Update Node.js data for URLSearchParams mdn/browser-compat-data#24998

Merged

Update nodejs data for URL mdn/browser-compat-data#24997

Merged

doc: fix history info for URLSearchParams #55837

Closed

doc: fix history info for URLSearchParams #55840

Closed

url: Improve WHATWG URLSearchParams spec compliance by TimothyGu · Pull Request #9484 · nodejs/node · GitHub

Uh oh!

Uh oh!

url: Improve WHATWG URLSearchParams spec compliance - #9484

url: Improve WHATWG URLSearchParams spec compliance#9484TimothyGu wants to merge 2 commits intonodejs:masternodejs/node:masterfrom TimothyGu:urlsearchparamsTimothyGu/node:urlsearchparamsCopy head branch name to clipboard

Conversation

TimothyGu commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdex commented Nov 6, 2016

Uh oh!

jasnell commented Nov 6, 2016

Uh oh!

TimothyGu Nov 6, 2016

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Nov 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Nov 7, 2016

Uh oh!

TimothyGu commented Nov 13, 2016

Uh oh!

jasnell commented Nov 13, 2016

Uh oh!

TimothyGu commented Nov 22, 2016

Uh oh!

TimothyGu commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Nov 25, 2016

Uh oh!

TimothyGu commented Dec 4, 2016

Uh oh!

jasnell commented Dec 4, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 4, 2016

Uh oh!

jasnell commented Dec 4, 2016

Uh oh!

stevenvachon commented Dec 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

url: Improve WHATWG URLSearchParams spec compliance#9484
TimothyGu wants to merge 2 commits into
nodejs:masterfrom
TimothyGu:urlsearchparams

TimothyGu commented Nov 6, 2016 •
edited
Loading

TimothyGu commented Nov 7, 2016 •
edited
Loading

TimothyGu commented Nov 25, 2016 •
edited
Loading