url.resolve auth behavior if host does *not* change

[msakrejda]

#1480 was noted as a breaking change in node 6, but it looks like this also strips out auth information for cases where the host does not change:

maciek@mothra:~$ n 5.12
maciek@mothra:~$ <<<"console.log(require('url').resolve('https://user:password@example.com', 'https://example.com/foo'))" node
https://user:password@example.com/foo
maciek@mothra:~$ n 6.3.1
maciek@mothra:~$ <<<"console.log(require('url').resolve('https://user:password@example.com', 'https://example.com/foo'))" node
https://example.com/foo

As far as I can tell, these cases are not covered by the tests, so I'm not sure if this change is intentional, and it seems heavy-handed if so. This seems to be responsible for the issue I ran into in follow-redirects and I think it means that follow-redirects in its current state is broken on node 6. I'd like to figure out if the proper place to fix this is here or in follow-redirects.

[jasnell]

I'll take a look shortly!

[imyller]

This is due to issue #1435 fixed in PR #1480

The fix however never takes in to account the case when host does not actually change - thus leading to this bug.

at https://github.com/nodejs/node/blob/master/lib/url.js#L782

  if (relative.host || relative.host === '') {
      result.host = relative.host;
      result.auth = null;
    }
    if (relative.hostname || relative.hostname === '') {
      result.hostname = relative.hostname;
      result.auth = null;
    }

Should be:

 if (relative.host || relative.host === '') {
      if ( result.host !== relative.host ) result.auth = null;
      result.host = relative.host;
    }
    if (relative.hostname || relative.hostname === '') {
      if ( result.hostname !== relative.hostname ) result.auth = null;
      result.hostname = relative.hostname;
    }

I've implemented a fix and a test case for this.

@jasnell I'd be happy to provide a PR if you feel the solution is acceptable.

[msakrejda]

Ok, I found this comment which suggests that url.resolve(base, relative) should mostly follow new URL(base, relative).href. Running my example above in Chromium (Version 51.0.2704.79 Ubuntu 16.04 (64-bit)) and Firefox (48.0 on the same machine), both keep the auth.

However, they're even more lenient than your patch @imyller:

> new URL('https://user:password@example.com:81', 'https://example.com/foo').href
"https://user:password@example.com:81/"
> new URL('https://user:password@example.com:81', 'https://example.com:10/foo').href
"https://user:password@example.com:81/"
> new URL('https://user:password@example.com', 'https://example.com:10/foo').href
"https://user:password@example.com/"

(both Chromium and Firefox)

That is, it looks like in the browser, the hostname must match to keep auth, but the host itself (i.e., including the port) can be different. Thoughts?

[imyller]

@uhoh-itsmaciek Good point. Different port, different service, different auth - possibly.

But I think the parameters are reversed in your example: It's new URL(relative, base).href as documented in https://developer.mozilla.org/en-US/docs/Web/API/URL/URL . Even that does not match the output of url.resolve because every time the relative URL in new URL(..) has protocol defined it overrides the entire base URL.

If it is more correct, I can modify the PR to preserve auth if hostname and/or host matches, but remove it if port does not. That should be on the safe side.

I'd like to hear @jasnell 's feedback and throughts on this first.

[msakrejda]

Oh great catch. Sorry, I'm relatively new to JS. Just for the record, this is the output with the correct invocation (again, both Chromium and FF):

new URL('https://example.com/foo', 'https://user:password@example.com:81').href
"https://example.com/foo"
new URL('https://example.com:10/foo', 'https://user:password@example.com:81').href
"https://example.com:10/foo"
new URL('https://example.com:10/foo', 'https://user:password@example.com').href
"https://example.com:10/foo"

Which is, I believe, how your patch handles this. It might make sense to add test cases to cover at least some of this to codify the expected behavior, but yeah, I defer to @jasnell.

[imyller]

I won't open a PR until I'm sure we are on the right path here, but you can preview the changes at imyller@5cbab64

Test case for this is included in test/parallel/test-url.js as you can see.

[msakrejda]

Right, I saw the basic test case you added; I meant we should test that same-hostname-different-port does strip out auth--that does not seem to be covered right now.

[imyller]

Actually I've already found a separate bug with port handling that I'm preparing to open issue+PR with. It's affecting only url.resolveObject and it will include patch and test case for the scenario where the port number actually changes.

Currently adding that port changing test will fail for unrelated reasons to this auth issue. This might confuse and mislead someone trying to review the PR.

I'd assume fixing two things in single PR is not encouraged.

[imyller]

Just opened PR with a fix.

This is a regression introduced in #1480, which correctly does what it promises, but does not take into account the case when host/hostname is not modified.

@uhoh-itsmaciek Adding meaningful test for same-hostname-different-port scenario requires PR #8214 to land first.