Skip to content

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

Description

@syranide

webpack/webpack-dev-server#240 (comment)

console.log(require('url').parse('http://:::1').href) // "http://:1/::"

Activity

  1. Trott commented on Sep 17, 2015

    @Trott
    Member

    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
    Author

    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. added
    urlIssues and PRs related to the legacy built-in url module.
    on Sep 17, 2015
  4. Trott commented on Sep 17, 2015

    @Trott
    Member

    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
    Member

    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
    Author

    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
    Member

    Another result from Chrome: new URL('http://:::1') => Uncaught TypeError: Failed to construct 'URL': Invalid URL

  8. Trott commented on Sep 18, 2015

    @Trott
    Member

    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
    Member

    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
    Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

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

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions