assigning a hostname with port 80 to URL.host will not override the existing port

[giltayar]

• Version: 8.8.1, but also occurs in 10.0.0
• Platform: MacOS
• Subsystem: url

Overriding the host property with a host that has port 80, will not override the port.
Example:

const {URL} = require('url')

const u = new URL('http://localhost:3000/foo')
u.host = 'some-domain:80'
console.log(u.href) // expected http://some-domain:80/foo, actual: http://some-domain:3000/foo 

// note that any other port besides 80 works correctly:

const u2 = new URL('http://localhost:3000/foo')
u2.host = 'some-domain:801'
console.log(u2.href) // http://some-domain:801/foo

The same bug applies to https with port 443.

[targos]

/cc @nodejs/url

[targos]

I can see where the problem is in node_url.cc but I don't understand enough how the state machine is implemented to fix it.
The port is ignored because scheme is http and port is 80, but it doesn't check what the current port is.

Refs:

node/src/node_url.cc

Line 1751 in 9c8b479

url->port = NormalizePort(url->scheme, static_cast<int>(port));

node/src/node_url.cc

Lines 682 to 687 in 9c8b479

	inline int NormalizePort(const std::string& scheme, int p) {
	#define XX(name, port) if (scheme == name && p == port) return -1;
	SPECIALS(XX);
	#undef XX
	return p;
	}

[AyushG3112]

Should the port be overridden unconditionally?

[targos]

Not exactly. In this case, port should be deleted (because the default port for http is 80)

[AyushG3112]

I've been going through the code and will give fixing this a shot in a couple hours if nobody else picks it up first. I believe a test for this would be needed too, right?

[domenic]

This is per spec I believe, so I don't know why this is tagged confirmed-bug. https://url.spec.whatwg.org/#port-state step 2.1.3.

[targos]

@domenic What happens here is that the port remains at its original value (3000) instead of being set to null.

[domenic]

Got it! Yeah, adding a test to https://github.com/w3c/web-platform-tests/blob/master/url/setters_tests.json#L608 would be great to catch this kind of thing in all implementations.

[eduardbme]

checked on master

$ ~/Documents/opensource/node/node -v
v11.0.0-pre
$ ~/Documents/opensource/node/node test.js
http://some-domain:3000/foo
http://some-domain:3000/foo

[vsemozhetbyt]

@eduardbcom There is a typo in OP, the last line should be console.log(u2.href).

[eduardbme]

my bad, overlooked

[AyushG3112]

The fix is simple but causing a regression. Trying to figure it out.

[giltayar]

@vsemozhetbyt - the typo is in my bug description, and I fixed it (the typo, not the bug...).

[AyushG3112]

Removing this line fixes the issue:

https://github.com/nodejs/node/blob/master/lib/internal/url.js#L279

However, this then introduces a regression in https://github.com/nodejs/node/blob/ef86f2c8524fd65aeda60cad45ea8e3001941cd0/test/parallel/test-whatwg-url-setters.js

[eduardbme]

I think in that way we spread URL parse logic between two different parts (js and c++), and that's not good. Much better approach (to my mind) is to make all parse stuff within c++ part, where main logic is placed.

So I propose to change

node/src/node_url.cc

Line 2031 in 9c8b479

if (url.port > -1)

to

if (url.port > -1 && !IsSpecial(url.scheme.c_str(), url.port))

And add second isSpecial version:

inline bool IsSpecial(const std::string& scheme, const int& p) {
  #define XX(name, port) if (scheme == name && p == port) return true;

  SPECIALS(XX);

  #undef XX

  return false;
}

No need to change current tests, [03:37|% 100|+ 2240|- 0]: Done.
But of course new tests to cover that are required.

[AyushG3112]

@eduardbcom The problem with that(I think) will be that the port will be overwritten and not deleted. I tried to do that earlier.

[eduardbme]

@AyushG3112 Please, provide problem example. Thx.