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

Base URL hostnames are overridden by certain paths when constructing a URL #38963

Closed
jonnydgreen opened this issue Jun 7, 2021 · 14 comments
Closed

Comments

@jonnydgreen
Copy link

  • Version: v16.2.0
  • Platform: Darwin bisterix.local 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64
  • Subsystem: url

What steps will reproduce the bug?

Running new URL('\\\\-', 'http://asd') in Node.js v16.2.0 shell will override the hostname of the arbitrarily chosen base URL.

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior?

The URL is correctly constructed and the hostname is preserved:

URL {
  href: 'http://asd///-',
  origin: 'http://asd',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'asd',
  hostname: 'asd',
  port: '',
  pathname: '///-',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

What do you see instead?

The hostname of the base URL is overridden by the path:

$ node
Welcome to Node.js v16.2.0.
Type ".help" for more information.
> new URL('\\\\-', 'http://asd')
URL {
  href: 'http://-/',
  origin: 'http://-',
  protocol: 'http:',
  username: '',
  password: '',
  host: '-',
  hostname: '-',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

Additional information

This does not occur in Node.js 14.15.4 (I haven't tested in other versions). Happy to help resolve this, although I would need some pointers for where to start :)

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

It's the same behavior in the browser (Chrome 90) — it's probably actually correct functionality per the spec 🤔

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

I believe this is working as intended. According to the spec, the URL constructor take two parameters: url and optional base. If base is not undefined and url has no base (a base is like https://abc or \\host), then base gets applied to url to produce the final URL. To demonstrate:

// `base` (second param) is used here because `url` (first param) has no base (it's just a relative path)
new URL('/hello/world', 'http://def') // -> { href: 'http://def/hello/world' ... }

// `base` is ignored here because `url` already has a base
new URL('http://abc/hello/world', 'http://def') // -> { href: 'http://abc/hello/world' }

You might be getting confused because \\-—which is a normal Windows UNC path like \\hostname—is also interpreted as a base, thus your base parameter (http://asd) is getting ignored.

cc @aduh95

@jonnydgreen
Copy link
Author

Ah that makes sense about the base interpretation, yeah - thanks for the clarification! :) If this is correct functionality per the spec for v16, would that mean it's incorrect for Node.js v14 in that case, since this does not get interpreted as a base?

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

Not sure what you mean about v14 — from v10.0.0 (when URL was introduced) to v16.3.0 (latest) the behavior is the same (I checked the latest of all major versions, 10, 11, 12, 13, 14, 15, 16 🙂).

@jonnydgreen
Copy link
Author

Sorry, I should've provided an example! On v14, I ran the same command but got a different result, i.e. the path '\\\\-' was not interpreted as a base and http://asd was not ignored?

$ node
Welcome to Node.js v14.15.4.
Type ".help" for more information.
> new URL('\\\\-', 'http://asd')
URL {
  href: 'http://asd///-',
  origin: 'http://asd',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'asd',
  hostname: 'asd', <-- Was expecting this to be '-' like in v16
  port: '',
  pathname: '///-',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

Ohhhh — I made a mistake. I didn't realize what you were saying :)

Yeah, something has definitely changed recently 😬 let me investigate.

@jonnydgreen
Copy link
Author

Cool, thanks - happy to provide more testing/debugging output if you need :)

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

I'm guessing that the previous functionality (that which you just demontrated) was a bug, and fixed with v15.6.0 by @RaisinTen in #36613.

cc @jasnell @watilde

@jonnydgreen
Copy link
Author

Thanks for the update, looking at that PR it looks like it's included in the v14.17.0 proposal? I've just run on this version and it works okay!

$ node
Welcome to Node.js v14.17.0.
Type ".help" for more information.
> new URL('\\\\-', 'http://asd')
URL {
  href: 'http://-/',
  origin: 'http://-',
  protocol: 'http:',
  username: '',
  password: '',
  host: '-',
  hostname: '-',
  port: '',
  pathname: '/',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

I guess we can close this now?

@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

Ah yes, #36613 was backported to v14 with v14.17.0.

I guess we can close this now?

Yes, feel free to. Thank you for the issue! I'm glad I could help out ;)

@jonnydgreen
Copy link
Author

Will do and thanks for the help and clarifications - I learned something new today about Windows UNC paths! :)

@bl-ue
Copy link
Contributor

bl-ue commented Jun 8, 2021

Yeah.

Now let me tell you something: you might be inclined to think that in https://google.com, https:// is the protocol. Actually, it's not. https: is. The actual URL is //google.com, which is simply a UNIX UNC path, because, you know, UNIX uses / for slashes and Windows uses \\ for slashes, so //google.com == \\google.com :)

@jonnydgreen
Copy link
Author

I did not know that either, that's really interesting and things make a lot more sense now! 🚀 - thank you very much!

@bl-ue
Copy link
Contributor

bl-ue commented Jun 8, 2021

For future reference: this is a duplicate of #36559.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants