url: fix hostname pattern to not accept empty string #5157

Closed
wants to merge 2 commits into
from

Projects

None yet

3 participants

@msavva
msavva commented Mar 27, 2013

Disallow matching of empty string as a valid hostname. Could lead to failure in URL remapping tables for http-proxy.

@bnoordhuis
Member

Can you add some regression tests? I'm trying to come up with a plausible scenario where it matters. url.parse('http:///') still works the same with your patch, if I'm not mistaken.

@msavva
msavva commented Mar 28, 2013

Not sure how to have an "empty hostname invalid" unit test in test-url.js given that there doesn't seem to be a code path for negative test cases.

Before the last commit which changed the hostname pattern (de0303d), empty strings were not valid matches. After the change, regular expression remapping in the router table for http-proxy is broken for me. For example port-forwarding using exports.router = { '.*/a/' : 'http://localhost:8080/a/'} fails because the regexp on the left is clobbered by url.parse() into '/*/a/'.

The change in this pull request fixes this and also restores empty hostnames to invalid, as was the case before de0303d.

@bnoordhuis
Member

Not sure how to have an "empty hostname invalid" unit test in test-url.js given that there doesn't seem to be a code path for negative test cases.

Just add a few (function() { ... })(); blocks at the bottom that check for failure.

@msavva
msavva commented Mar 31, 2013

The added test in test-url.js should now show the difference the fix in this pull request makes. Previously http://.*/ would be parsed as a '.' host and hostname, breaking regex hostname matching in http-proxy.

@chrisdickinson

Closing this -- we're going to start moving url to match the whatwg url spec, which parses the hostname as .*. Thanks for your patch -- sorry it took so long to get back to you on this.

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