url: fix surrogate handling in encodeAuth() by TimothyGu · Pull Request #11161 · nodejs/node

TimothyGu · 2017-02-04T06:37:35Z

Currently, the legacy URL stringifier miscalculates the offset of an extra surrogate, causing the high surrogate to be included unescaped:

> url.format({ auth: '\uD83D\uDE00', hostname: 'a' })
'%F0%9F%98%80�@a'
> Array.from(url.format({ auth: '\uD83D\uDE00', hostname: 'a' })).map(ch => ch.charCodeAt(0).toString(16)).join(' ')
'25 46 30 25 39 46 25 39 38 25 38 30 de00 40 61'
//%  F  0  %  9  F  %  9  8  %  8  0    �  @  a

Also included in the PR is a commit that moves the function, which is only used in the legacy url module, to that file. Certain generic percent encoding handling functions and tables are also moved to a separate lib/internal/querystring.js.

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

url, querystring

joyeecheung · 2017-02-04T07:47:16Z

lib/querystring.js

How about moving other private functions to internal/querystring too?

I only moved this array because it is used in multiple places (querystring and url). The only other entity in querystring that is somewhat shared is ParsedQueryString, a copy of which is in url (as ParsedQueryString as well) and in internal/url (as StorageObject). I've moved this as well. (There is also an analog of it in events as EventHandlers, but I kept it as it is since it'd be weird for that module to import from internal/querystring).

I think placing private bits intointernal would make the boundaries clearer, whether its shared or not, like BufferList for _stream_readable.js (only used once there).

mscdex · 2017-02-04T08:04:04Z

lib/url.js

I know this originally came from querystring.escape(), but that function's implementation has recently improved performance-wise and we should probably incorporate those changes here (specifically the ASCII value checking -- it uses a lookup table now).

Done. 3% improvement on url/legacy-vs-whatwg-url-serialize.js with type=auth method=legacy n=5e5.

mscdex · 2017-02-04T08:06:46Z

lib/querystring.js

I think we should be more consistent about how we import single items like this. Here we're using the pre-ES6 style, but in other places we are destructuring like const { hexTable } = require('internal/querystring');

I initially followed the style of existing imports in the file (see Buffer above), but since I'm importing multiple items from this module now I started using destructuring.

TimothyGu · 2017-02-04T08:38:53Z

@joyeecheung and @mscdex, comments addressed.

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

@mscdex, I also factored out the entire percent encoding function so that it can be shared between querystring's qsEscape() and url's encodeAuth. Only trivial performance drop is observed in querystring.

Benchmark results

                                                                            improvement confidence      p.value
 querystring/querystring-stringify.js n=300000 type="encodelast"                -1.59 %            0.3240629986
 querystring/querystring-stringify.js n=300000 type="encodemany"                -0.23 %            0.6514424297
 querystring/querystring-stringify.js n=300000 type="noencode"                  -2.94 %          * 0.0165429734
 url/legacy-vs-whatwg-url-serialize.js n=500000 method="legacy" type="auth"      3.80 %        *** 0.0006922195

TimothyGu · 2017-02-04T08:55:02Z

New(er) CI: https://ci.nodejs.org/job/node-test-pull-request/6214/

joyeecheung · 2017-02-04T09:02:49Z

lib/internal/querystring.js

Off-topic: what does this TODO mean? Take a buffer and escape its content?

I assume it means using a user-provided buffer. However, at the same time, this comment goes all the way back to 57d8172 (v0.3.2), so whether this TODO is still applicable isn't clear by any means.

Anyone is welcome to initiate a formal deprecation process of these exports in another PR.

joyeecheung · 2017-02-04T09:12:47Z

lib/internal/querystring.js

Nit: s/noEscape/noEscapeTable would be more intuitive, noEscape sounds like a boolean..

Fixed, thanks.

joyeecheung · 2017-02-04T09:27:39Z

lib/querystring.js

Off-topic: unescapeBuffer, encode and decode are all removed from the docs, mind adding a comment?
(Not sure about which stage of deprecation they are in)

encode and decode were added in 4ce100f (v0.1.99). unescapeBuffer is slightly newer, but by not much, being added in 6ff12c4 (v0.3.2). None of them have ever been documented.

Hmm..mind adding a comment about that they have never been documented, so technically not public APIs, just exposed by accident?

I'd almost rather just document them IMHO, at least unescapeBuffer() since it's not an alias to an already documented method. I've personally had uses for unescapeBuffer() in my own projects. *shrug*

Not related to deprecations or anything, just now that we are refactoring things, might as well :P

mscdex · 2017-02-06T01:39:09Z

I just benchmarked the current changes in this PR and there is a 3-4% performance regression (with high "confidence" ratings) in the querystring-stringify benchmarks. With that in mind, I think we should avoid extracting the logic into a separate function (percentEscape()), unless something can be done to avoid the regressions. I don't know if binding the function with the table once in each module will make any difference or not, but it's another idea to try I suppose.

jasnell · 2017-02-06T01:50:12Z

Ouch, yeah, with numbers like that I have to agree.

TimothyGu · 2017-02-12T02:41:43Z

@mscdex, okay for now I left percent encoding alone in their respective files.

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

mscdex · 2017-02-12T05:44:34Z

lib/internal/querystring.js

I think this can be moved back to lib/querystring.js as well, since it's not used anywhere else, along with unhexTable.

I think one reason to keep it in internal is that unescapeBuffer is exported to user land by accident, putting it in internal should make it clear(if there is no performance regression caused by this though).

I still think we should just document unescapeBuffer(), since it can be useful (and I've used it in some of my projects before for decoding binary/latin1 strings).

Hmm, so probably move it back to lib/querystring.js for now, and let another PR sort this issue out, no matter we want to make it really internal or start to document it?

I would say yes.

Just to be clear, this PR does not modify the exports on querystring: unescapeBuffer() is still exported on require('querystring'), just the physical location is changed.

That said, I thought this function would be of interest for other modules like url as well, which was why I moved it to internal/url.js in the first place. As it doesn't seem like this decision is popular, I've reverted it. PTAL.

mscdex · 2017-02-12T05:46:10Z

lib/internal/querystring.js

Can this be switched to the module.exports = { ... } syntax instead (for faster property access)?

TimothyGu · 2017-02-12T19:32:49Z

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

TimothyGu · 2017-02-14T04:58:44Z

@mscdex, any more comments? Note, I am not against documenting qs.unescapeBuffer(), but it falls outside the scope of this PR which is really about fixing a corner case in url.format().

mscdex · 2017-02-14T07:48:52Z

@TimothyGu I recognize that, I was just advocating for returning it back to lib/querystring.js.

Current changes LGTM.

Also factor out common parts in querystring and url. PR-URL: nodejs#11161 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

TimothyGu · 2017-02-14T20:15:12Z

Landed in c6b12d0.

Also factor out common parts in querystring and url. Backport-of: nodejs#11161

Also factor out common parts in querystring and url. Backport-of: #11161

@A

Currently, the legacy URL stringifier miscalculates the offset of an extra surrogate, causing the high surrogate to be included unescaped: > url.format({ auth: '\uD83D\uDE00', hostname: 'a' }) '//%F0%9F%98%80�@A' PR-URL: nodejs#11387 Backport-of: nodejs#11161

@A

Currently, the legacy URL stringifier miscalculates the offset of an extra surrogate, causing the high surrogate to be included unescaped: > url.format({ auth: '\uD83D\uDE00', hostname: 'a' }) '//%F0%9F%98%80�@A' PR-URL: #11387 Backport-of: #11161

@A

Currently, the legacy URL stringifier miscalculates the offset of an extra surrogate, causing the high surrogate to be included unescaped: > url.format({ auth: '\uD83D\uDE00', hostname: 'a' }) '//%F0%9F%98%80�@A' PR-URL: #11387 Backport-of: #11161

joyeecheung reviewed Feb 4, 2017

View reviewed changes

mscdex reviewed Feb 4, 2017

View reviewed changes

TimothyGu force-pushed the url-fix branch from 3c7f82f to a607334 Compare February 4, 2017 08:54

joyeecheung reviewed Feb 4, 2017

View reviewed changes

joyeecheung approved these changes Feb 4, 2017

View reviewed changes

jasnell approved these changes Feb 4, 2017

View reviewed changes

url: fix surrogate handling in encodeAuth()

8368389

TimothyGu force-pushed the url-fix branch from 9c83828 to cb39f1b Compare February 12, 2017 02:38

mscdex reviewed Feb 12, 2017

View reviewed changes

querystring, url: factor out common parts

cc46363

TimothyGu force-pushed the url-fix branch from cb39f1b to cc46363 Compare February 12, 2017 19:23

TimothyGu removed the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Feb 14, 2017

TimothyGu closed this Feb 14, 2017

TimothyGu deleted the url-fix branch February 14, 2017 20:15

TimothyGu mentioned this pull request Feb 14, 2017

(v6.x backport) url: fix surrogate handling in encodeAuth() #11387

Closed

3 tasks

TimothyGu added a commit to TimothyGu/node that referenced this pull request Feb 18, 2017

url: fix surrogate handling in encodeAuth()

acb8b2b

Also factor out common parts in querystring and url. Backport-of: nodejs#11161

TimothyGu mentioned this pull request Feb 18, 2017

(v7.x backport) URL-related changes #11457

Closed

4 tasks

italoacasas added the dont-land-on-v7.x label Feb 20, 2017

italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017

url: fix surrogate handling in encodeAuth()

18ea379

Also factor out common parts in querystring and url. Backport-of: nodejs#11161

italoacasas pushed a commit that referenced this pull request Feb 22, 2017

url: fix surrogate handling in encodeAuth()

8dbd562

Also factor out common parts in querystring and url. Backport-of: #11161

Uh oh!

Conversation

TimothyGu commented Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Feb 4, 2017

Uh oh!

TimothyGu commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Feb 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Feb 6, 2017

Uh oh!

TimothyGu commented Feb 12, 2017

Uh oh!

mscdex Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Feb 12, 2017

Uh oh!

TimothyGu commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex commented Feb 14, 2017

Uh oh!

TimothyGu commented Feb 14, 2017

TimothyGu commented Feb 4, 2017 •

edited

Loading

TimothyGu Feb 4, 2017 •

edited

Loading

mscdex Feb 4, 2017 •

edited

Loading

mscdex commented Feb 6, 2017 •

edited

Loading

mscdex Feb 12, 2017 •

edited

Loading

joyeecheung Feb 12, 2017 •

edited

Loading

TimothyGu commented Feb 14, 2017 •

edited

Loading