url.parse("http://:::1") is parsed as "http://:1/::" #2929

Activity

1. Trott commented on Sep 17, 2015

   

   Trott

   on Sep 17, 2015

   Member

   More actions

   Literal IPv6 URLs need to be enclosed in [] per RFC 2732.

   console.log(require('url').parse('http://[:::1]').href)
   // -> http://[::1]/
   

   I'm not so sure it shouldn't throw an error or at least a warning on the example you provide, though.
2. syranide commented on Sep 17, 2015

   

   syranide

   on Sep 17, 2015

   Author

   More actions

   Yeah, not saying it's correct use of IPv6, but I would expect it to not mess up the parsing at least (i.e. input ~= output).
3. mscdex

   added

   urlIssues and PRs related to the legacy built-in url module.Issues and PRs related to the legacy built-in url module.

   on Sep 17, 2015
4. Trott commented on Sep 17, 2015

   

   Trott

   on Sep 17, 2015

   Member

   More actions

   I believe the two main rules for the url.parse() code are:

   • Try to emulate how browsers treat URLs as closely as possible
   • OMG do not touch this code if you don't have to because so much of the ecosystem can blow up if an edge case changes!

   Focusing on the first rule for now, here's what I get from parse.url('http://:::1');

   Url {
     protocol: 'http:',
     slashes: true,
     auth: null,
     host: ':1',
     port: '1',
     hostname: '',
     hash: null,
     search: null,
     query: null,
     pathname: '/::',
     path: '/::',
     href: 'http://:1/::' }
   

   For comparison, I did this in Chrome:

   var a = document.createElement('a');
   a.href = 'http://:::1';
   

   Then I inspected a and, filling in the equivalent Node Url object, (and disregarding that null, undefined, and empty string are not actually all equivalent), I get:

   Url {
     protocol: ':',
     slashes: ¯\_(ツ)_/¯,
     auth: null,
     host: ':0',
     port: '0',
     hostname: '',
     hash: null,
     search: null,
     query: null,
     pathname: '',
     path: null,
     href: 'http://:::1/' }
   

   Given all that, I would say it's a mess either way and (given the second bullet point above) probably not worth "fixing". The exception is the href property. It seems like that really shouldn't get mangled quite the way it does in Node.
5. Trott commented on Sep 18, 2015

   

   Trott

   on Sep 18, 2015

   Member

   More actions

   Related: For an impressive list of URLs along with how they are parsed by Chrome: http://src.chromium.org/viewvc/chrome/trunk/src/url/url_parse_unittest.cc
6. syranide commented on Sep 18, 2015

   

   syranide

   on Sep 18, 2015

   Author

   More actions

   It's interesting that it's parsed the same way in chrome... anyway, I'm not really bothered by this myself and while it's not great, as you say more harm can (will) come from trying to fix it than just leaving it as it is.

   Leaving it for you to close (your decision).
7. targos commented on Sep 18, 2015

   

   targos

   on Sep 18, 2015

   Member

   More actions

   Another result from Chrome: new URL('http://:::1') => Uncaught TypeError: Failed to construct 'URL': Invalid URL
8. Trott commented on Sep 18, 2015

   

   Trott

   on Sep 18, 2015

   Member

   More actions

   That result makes a lot more sense to me. Do you know if the implementation is in Blink/Chromium code or in v8? I'm guessing it's not in v8 but if I'm wrong, is it absurdly naive of me to ask if it's possible that Node can hook into it and maybe it can be called by url.parse(), allowing us to ditch a lot of the seemingly convoluted JS code that defines url.parse()? We'd still need a (hopefully thin) wrapper to deal with things like the slashes property. But it sure would be nice to offload that stuff to a canonical implementation devised from v8 and/or the browser. I'm guessing I'm not the first person to think of this and that there's an excellent reason it is not done that way. (Such as: It's not in v8.)
9. bnoordhuis commented on Sep 18, 2015

   

   bnoordhuis

   on Sep 18, 2015

   Member

   More actions

   Do you know if the implementation is in Blink/Chromium code or in v8? I'm guessing it's not in v8

   It's not V8. ;-)
10. Trott commented on Mar 27, 2016

    

    Trott

    on Mar 27, 2016

    Member

    More actions

    Closing due to inactivity and an apparent general consensus that the cure may be worse than the disease.
11. Trott

    closed this as completedon Mar 27, 2016