url: make WHATWG URL implementation more spec compliant by targos · Pull Request #10317 · nodejs/node

targos · 2016-12-17T13:23:01Z

Checklist

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

Affected core subsystem(s)

url

Description of change

Return a long from ParseNumber to prevent overflow for valid big numbers
Don't throw when there are more than 4 parts (it cannot be an IP
address)
Correctly interpret the address and don't always throw when there are
numbers > 255

Ref: https://url.spec.whatwg.org/#concept-ipv4-parser
Fixes: #10306

targos · 2016-12-17T13:24:03Z

cpplint is not happy:

src/node_url.cc:264: Use int16/int64/etc, rather than the C type long [runtime/int] [4]
src/node_url.cc:308: Use int16/int64/etc, rather than the C type long [runtime/int] [4]
src/node_url.cc:321: Use int16/int64/etc, rather than the C type long [runtime/int] [4]

How can I fix this?

cjihrig · 2016-12-17T16:46:50Z

I think it's asking you to do something like this: https://github.com/targos/node/blob/b35a5f84e89e021f27920ffc0c3562e84151d6fe/src/node_url.cc#L120

targos · 2016-12-17T16:55:27Z

I tried with int32_t and the result is different

cjihrig · 2016-12-17T17:06:55Z

If you're using long, you could try int64_t.

mscdex · 2016-12-17T18:42:47Z

/cc @jasnell

targos · 2016-12-17T23:02:08Z

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

jasnell · 2016-12-17T23:50:00Z

I won't be able to review until Monday but as long as the fix adheres to the WHATWG spec then +1 and thank you!

sam-github · 2016-12-19T16:16:12Z

Doesn't this imply that the WhatWG test suite is incomplete? Should the test changes be upstreamed to them?

jasnell · 2016-12-19T19:03:47Z

@sam-github ... that is entirely possible.

targos · 2016-12-19T19:18:13Z

@sam-github good idea. I opened a PR here: web-platform-tests/wpt#4365

targos · 2016-12-19T19:23:14Z

@jasnell Just wondering if you remember what this condition was for? Maybe it was a mistake to remove it.

not off the top of my head... I'll be able to take a look in the next day or two. (sorry, company in town for the holiday is taking up quite a bit of my time this week)

targos · 2016-12-20T07:31:23Z

My PR was merged and I updated our copy in the fixtures. Now the tests don't pass because of a spec change:

I'll work on it when I have some time

New changes

targos · 2016-12-22T07:45:05Z

I had to add 3 other changes so that the updated tests can pass. This is ready to be reviewed again!

targos · 2016-12-22T07:46:08Z

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

targos · 2016-12-23T16:13:16Z

cc @nodejs/collaborators

targos · 2016-12-30T19:36:30Z

Rebased. PTAL.

targos · 2016-12-31T09:30:15Z

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

targos · 2016-12-31T09:31:40Z

Should I squash all the commits together? If I don't, there will be commits with failing tests.

benjamingr · 2016-12-31T15:28:36Z

I'm not familiar with the spec intimately, but I went through the changes and spec and they all LGTM

gibfahn · 2016-12-31T19:45:52Z

Should I squash all the commits together? If I don't, there will be commits with failing tests.

Definitely worth squashing so there aren't any commits with failing tests.

jasnell · 2017-01-01T07:24:10Z

+1 to squashing

This patch contains the following changes: url: make IPv4 parser more spec compliant * Return int64_t from ParseNumber to prevent overflow for valid big numbers * Don't throw when there are more than 4 parts (it cannot be an IP address) * Correctly interpret the address and don't always throw when there are numbers > 255 Ref: https://url.spec.whatwg.org/#concept-ipv4-parserFixes: nodejs#10306 url: percent encode fragment to follow spec change Ref: whatwg/url#150 Ref: whatwg/url@373dbed url: fix URL#search setter The check for empty string must be done before removing the leading '?'. Ref: https://url.spec.whatwg.org/#dom-url-search url: set port to null if an empty string is given This is to follow a spec change. Ref: whatwg/url#113 url: fix parsing of paths with Windows drive letter test: update WHATWG URL test fixtures PR-URL: nodejs#10317 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos · 2017-01-01T10:03:34Z

Landed in db18dd8

This patch contains the following changes: url: make IPv4 parser more spec compliant * Return int64_t from ParseNumber to prevent overflow for valid big numbers * Don't throw when there are more than 4 parts (it cannot be an IP address) * Correctly interpret the address and don't always throw when there are numbers > 255 Ref: https://url.spec.whatwg.org/#concept-ipv4-parserFixes: #10306 url: percent encode fragment to follow spec change Ref: whatwg/url#150 Ref: whatwg/url@373dbed url: fix URL#search setter The check for empty string must be done before removing the leading '?'. Ref: https://url.spec.whatwg.org/#dom-url-search url: set port to null if an empty string is given This is to follow a spec change. Ref: whatwg/url#113 url: fix parsing of paths with Windows drive letter test: update WHATWG URL test fixtures PR-URL: #10317 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 17, 2016

targos requested a review from jasnell December 17, 2016 13:23

targos added the url Issues and PRs related to the legacy built-in url module. label Dec 17, 2016

targos force-pushed the fix-10306 branch from 4b71b58 to b35a5f8Compare December 17, 2016 13:25

targos force-pushed the fix-10306 branch from b35a5f8 to 9b86e2aCompare December 17, 2016 23:00

targos force-pushed the fix-10306 branch from 9b86e2a to 367a3bfCompare December 17, 2016 23:43

targos mentioned this pull request Dec 19, 2016
test/arm reported as failure when it passed for a PR nodejs/build#572
Closed

jasnell previously approved these changes Dec 19, 2016
View reviewed changes

targos mentioned this pull request Dec 19, 2016
Add URL tests for IPv4 addresses web-platform-tests/wpt#4365
Merged

targos commented Dec 19, 2016
View reviewed changes

targos force-pushed the fix-10306 branch from 367a3bf to 997b8cfCompare December 20, 2016 07:30

Janpot mentioned this pull request Dec 20, 2016
Use native Node URL parsing after Node 7.2.1 errors resolved GoogleChrome/lighthouse#1186
Closed

targos force-pushed the fix-10306 branch from 997b8cf to 80f6921Compare December 22, 2016 07:41

targos changed the title ~~url: make IPv4 parser more spec compliant~~url: make WHATWG URL implementation more spec compliantDec 22, 2016

joyeecheung mentioned this pull request Dec 22, 2016
url: make WHATWG URL properties spec compliant #10408
Closed
3 tasks

targos added dont-land-on-v4.x labels Dec 28, 2016

targos mentioned this pull request Dec 28, 2016
Cannot empty URL port #10508
Closed

targos force-pushed the fix-10306 branch from e0c1f1b to 4dc02bbCompare December 30, 2016 19:35

nodejs-github-bot added the dont-land-on-v7.x label Dec 30, 2016

jasnell approved these changes Dec 30, 2016
View reviewed changes

targos force-pushed the fix-10306 branch from 4dc02bb to ef8da06Compare December 31, 2016 09:29

nodejs-github-bot added the dont-land-on-v7.x label Dec 31, 2016

benjamingr approved these changes Dec 31, 2016
View reviewed changes

targos force-pushed the fix-10306 branch from ef8da06 to db18dd8Compare January 1, 2017 10:01

nodejs-github-bot added the dont-land-on-v7.x label Jan 1, 2017

targos merged commit db18dd8 into nodejs:masterJan 1, 2017

targos added test Issues and PRs related to the tests. and removed dont-land-on-v7.x labels Jan 1, 2017

targos deleted the fix-10306 branch January 1, 2017 10:03

targos removed the test Issues and PRs related to the tests. label Jan 1, 2017

evanlucas mentioned this pull request Jan 3, 2017
v7.4.0 release proposal #10589
Merged

joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017

Uh oh!

Uh oh!

Conversation

targos commented Dec 17, 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!

targos commented Dec 17, 2016

Uh oh!

cjihrig commented Dec 17, 2016

Uh oh!

targos commented Dec 17, 2016

Uh oh!

cjihrig commented Dec 17, 2016

Uh oh!

mscdex commented Dec 17, 2016

Uh oh!

targos commented Dec 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Dec 17, 2016

Uh oh!

sam-github commented Dec 19, 2016

Uh oh!

jasnell commented Dec 19, 2016

Uh oh!

targos commented Dec 19, 2016

Uh oh!

targosDec 19, 2016

Choose a reason for hiding this comment

Uh oh!

jasnellDec 22, 2016

Choose a reason for hiding this comment

Uh oh!

targos commented Dec 20, 2016

Uh oh!

targos commented Dec 22, 2016

Uh oh!

targos commented Dec 22, 2016

Uh oh!

targos commented Dec 23, 2016

Uh oh!

targos commented Dec 30, 2016

Uh oh!

targos commented Dec 31, 2016

Uh oh!

targos commented Dec 31, 2016

Uh oh!

benjamingr commented Dec 31, 2016

Uh oh!

gibfahn commented Dec 31, 2016

Uh oh!

jasnell commented Jan 1, 2017

Uh oh!

targos commented Jan 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

targos commented Dec 17, 2016•
edited
Loading

targos commented Dec 17, 2016•
edited
Loading