Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid Url when domain contains between 4 and 9 digits #10306

Closed
Janpot opened this issue Dec 16, 2016 · 2 comments
Closed

Invalid Url when domain contains between 4 and 9 digits #10306

Janpot opened this issue Dec 16, 2016 · 2 comments
Labels
confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@Janpot
Copy link

Janpot commented Dec 16, 2016

  • Version: v7.2.1
  • Platform: Darwin Jans-MacBook-Pro-2.local 16.3.0 Darwin Kernel Version 16.3.0: Thu Nov 17 20:23:58 PST 2016; root:xnu-3789.31.2~1/RELEASE_X86_64 x86_64
  • Subsystem: url

Executing:

const { URL } = require('url');
new URL('http://1234.com');
new URL('http://123456789.com');

all throw

TypeError: Invalid URL
    at Object.URL.binding.parse (internal/url.js:85:17)
    at new URL (internal/url.js:81:13)
    at repl:1:1
    at realRunInThisContextScript (vm.js:22:35)
    at sigintHandlersWrap (vm.js:98:12)
    at ContextifyScript.Script.runInThisContext (vm.js:24:12)
    at REPLServer.defaultEval (repl.js:346:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.onLine (repl.js:545:10)

all following work without a problem:

const { URL } = require('url');
new URL('http://123.com');
new URL('http://1234567890.com');

The bug doesn't happen in the browser. It looks like it tries to parse an IP address or something?

@toasohcah
Copy link

Note that it affects every integer in the range [256, 999999999]:

> new (require("url").URL)('http://255.com/')
URL {
> new (require("url").URL)('http://256.com/')
TypeError: Invalid URL
> new (require("url").URL)('http://999999999.com/')
TypeError: Invalid URL
> new (require("url").URL)('http://1000000000.com/')
URL {

@targos targos added url Issues and PRs related to the legacy built-in url module. confirmed-bug Issues with confirmed bugs. labels Dec 16, 2016
@targos
Copy link
Member

targos commented Dec 16, 2016

It looks like it tries to parse an IP address or something?

Correct. I'm working on a fix

targos added a commit to targos/node that referenced this issue Jan 1, 2017
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-parser
Fixes: 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>
evanlucas pushed a commit that referenced this issue Jan 3, 2017
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-parser
Fixes: #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>
evanlucas pushed a commit that referenced this issue Jan 4, 2017
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-parser
Fixes: #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>
@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

No branches or pull requests

4 participants