Skip to content

Commit

Permalink
Revert "url: improve port validation"
Browse files Browse the repository at this point in the history
This reverts commit 5f7730e.

This change broke too many edge cases in the ecosystem. Reverting it
re-introduces some host-spoofing possibilities, so we won't want to
revert forever, but the issue is long-lived enough and not sufficiently
critical that we can't wait for a major release to introduce it as a
breaking change. After this lands, I plan to re-introduce this as a
change that throws a warning rather than an error, after which we can
land a semver-major that re-introduces the error and try to get the word
out to maintainers of likely-affected packages.

Closes: nodejs#45514
Refs: nodejs#45012
PR-URL: nodejs#45517
Fixes: nodejs#45514
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
  • Loading branch information
Trott authored and marco-ippolito committed Nov 23, 2022
1 parent 9ae2a5f commit bd960bf
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 18 deletions.
8 changes: 2 additions & 6 deletions lib/url.js
Expand Up @@ -387,7 +387,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {

// validate a little.
if (!ipv6Hostname) {
rest = getHostname(this, rest, hostname, url);
rest = getHostname(this, rest, hostname);
}

if (this.hostname.length > hostnameMaxLen) {
Expand Down Expand Up @@ -506,7 +506,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
return this;
};

function getHostname(self, rest, hostname, url) {
function getHostname(self, rest, hostname) {
for (let i = 0; i < hostname.length; ++i) {
const code = hostname.charCodeAt(i);
const isValid = (code !== CHAR_FORWARD_SLASH &&
Expand All @@ -516,10 +516,6 @@ function getHostname(self, rest, hostname, url) {
code !== CHAR_COLON);

if (!isValid) {
// If leftover starts with :, then it represents an invalid port.
if (hostname.charCodeAt(i) === 58) {
throw new ERR_INVALID_URL(url);
}
self.hostname = hostname.slice(0, i);
return `/${hostname.slice(i)}${rest}`;
}
Expand Down
16 changes: 16 additions & 0 deletions test/parallel/test-url-parse-format.js
Expand Up @@ -865,6 +865,22 @@ const parseTests = {
href: 'http://a%22%20%3C\'b:b@cd/e?f'
},

// Git urls used by npm
'git+ssh://git@github.com:npm/npm': {
protocol: 'git+ssh:',
slashes: true,
auth: 'git',
host: 'github.com',
port: null,
hostname: 'github.com',
hash: null,
search: null,
query: null,
pathname: '/:npm/npm',
path: '/:npm/npm',
href: 'git+ssh://git@github.com/:npm/npm'
},

'https://*': {
protocol: 'https:',
slashes: true,
Expand Down
12 changes: 0 additions & 12 deletions test/parallel/test-url-parse-invalid-input.js
Expand Up @@ -74,15 +74,3 @@ if (common.hasIntl) {
(e) => e.code === 'ERR_INVALID_URL',
'parsing http://\u00AD/bad.com/');
}

{
const badURLs = [
'https://evil.com:.example.com',
'git+ssh://git@github.com:npm/npm',
];
badURLs.forEach((badURL) => {
assert.throws(() => { url.parse(badURL); },
(e) => e.code === 'ERR_INVALID_URL',
`parsing ${badURL}`);
});
}

0 comments on commit bd960bf

Please sign in to comment.