url: Improve WHATWG URLSearchParams spec compliance

[TimothyGu]

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

[mscdex]

/cc @jasnell

[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!

[TimothyGu]

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.

[TimothyGu]

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:

1. 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.

[jasnell]

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.

[TimothyGu]

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

[jasnell]

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
.

[TimothyGu]

@jasnell, ping.

[TimothyGu]

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.

[jasnell]

@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!

[TimothyGu]

@jasnell, ping 3x...

[jasnell]

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

[jasnell]

LGTM if CI is green! Thank you very much!

[jasnell]

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

[jasnell]

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

[stevenvachon]

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