Skip to content

Commit

Permalink
url: runtime-deprecate url.parse() with invalid ports
Browse files Browse the repository at this point in the history
These URLs throw with WHATWG URL. They are permitted with url.parse()
but that allows potential host spoofing by sending a domain name as the
port.

PR-URL: #45526
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
Trott committed Nov 30, 2022
1 parent 2589fb1 commit 3bed5f1
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 3 deletions.
6 changes: 5 additions & 1 deletion doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -3302,13 +3302,17 @@ issued for `url.parse()` vulnerabilities.

<!-- YAML
changes:
- version:
- REPLACEME
pr-url: https://github.com/nodejs/node/pull/45526
description: Runtime deprecation.
- version:
- v19.2.0
pr-url: https://github.com/nodejs/node/pull/45576
description: Documentation-only deprecation.
-->

Type: Documentation-only
Type: Runtime

[`url.parse()`][] accepts URLs with ports that are not numbers. This behavior
might result in host name spoofing with unexpected input. These URLs will throw
Expand Down
13 changes: 11 additions & 2 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,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 @@ -506,7 +506,8 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
return this;
};

function getHostname(self, rest, hostname) {
let warnInvalidPort = true;
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 @@ -516,6 +517,14 @@ function getHostname(self, rest, hostname) {
code !== CHAR_COLON);

if (!isValid) {
// If leftover starts with :, then it represents an invalid port.
// But url.parse() is lenient about it for now.
// Issue a warning and continue.
if (warnInvalidPort && code === CHAR_COLON) {
const detail = `The URL ${url} is invalid. Future versions of Node.js will throw an error.`;
process.emitWarning(detail, 'DeprecationWarning', 'DEP0170');
warnInvalidPort = false;
}
self.hostname = hostname.slice(0, i);
return `/${hostname.slice(i)}${rest}`;
}
Expand Down
29 changes: 29 additions & 0 deletions test/parallel/test-url-parse-invalid-input.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const childProcess = require('child_process');
const url = require('url');

// https://github.com/joyent/node/issues/568
Expand Down Expand Up @@ -74,3 +75,31 @@ 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) => {
childProcess.exec(`${process.execPath} -e "url.parse('${badURL}')"`,
common.mustCall((err, stdout, stderr) => {
assert.strictEqual(err, null);
assert.strictEqual(stdout, '');
assert.match(stderr, /\[DEP0170\] DeprecationWarning:/);
})
);
});

// Warning should only happen once per process.
const expectedWarning = [
`The URL ${badURLs[0]} is invalid. Future versions of Node.js will throw an error.`,
'DEP0170',
];
common.expectWarning({
DeprecationWarning: expectedWarning,
});
badURLs.forEach((badURL) => {
url.parse(badURL);
});
}

0 comments on commit 3bed5f1

Please sign in to comment.