Skip to content

Commit

Permalink
url: improve port validation
Browse files Browse the repository at this point in the history
If a port is not a number, throw rather than treating the `:` that
delineates the port as part of the path. This is consistent with WHATWG
URL and also mitigates hostname-spoofing.

Concerns about hostname-spoofing were raised and presented in excellent
detail by pyozzi-toss (pyozzi@toss.im/Security-Tech Team in Toss).

PR-URL: #45012
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Trott authored Oct 17, 2022
1 parent 80c7bae commit 5f7730e
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 18 deletions.
8 changes: 6 additions & 2 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {

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

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

function getHostname(self, rest, hostname) {
function getHostname(self, rest, hostname, url) {
for (let i = 0; i < hostname.length; ++i) {
const code = hostname.charCodeAt(i);
const isValid = (code !== CHAR_FORWARD_SLASH &&
Expand All @@ -512,6 +512,10 @@ function getHostname(self, rest, hostname) {
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: 0 additions & 16 deletions test/parallel/test-url-parse-format.js
Original file line number Diff line number Diff line change
Expand Up @@ -865,22 +865,6 @@ const parseTests = {
href: 'http://a%0D%22%20%09%0A%3C\'b:b@c/%0D%0Ad/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: 12 additions & 0 deletions test/parallel/test-url-parse-invalid-input.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,3 +74,15 @@ 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 5f7730e

Please sign in to comment.