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.
  • Loading branch information
Trott committed Nov 21, 2022
1 parent 5664822 commit daf9370
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 3 deletions.
18 changes: 17 additions & 1 deletion doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -3284,7 +3284,7 @@ Node-API callbacks.
<!-- YAML
changes:
- version:
- v19.0.0
- v19.0.0
pr-url: https://github.com/nodejs/node/pull/44919
description: Documentation-only deprecation.
-->
Expand All @@ -3295,6 +3295,22 @@ Type: Documentation-only
have security implications. Use the [WHATWG URL API][] instead. CVEs are not
issued for `url.parse()` vulnerabilities.

### DEP0170: Invalid port when using url.parse()

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

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
an error in future versions of Node.js, as the [WHATWG URL API][] does already.

[NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
[RFC 6066]: https://tools.ietf.org/html/rfc6066#section-3
[RFC 8247 Section 2.4]: https://www.rfc-editor.org/rfc/rfc8247#section-2.4
Expand Down
11 changes: 9 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,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 @@ -516,6 +516,13 @@ 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 (code === CHAR_COLON) {
const detail = `The URL ${url} is invalid. Future versions of Node.js will throw an error.`;
process.emitWarning(detail, 'DeprecationWarning', 'DEP0170');
}
self.hostname = hostname.slice(0, i);
return `/${hostname.slice(i)}${rest}`;
}
Expand Down
19 changes: 19 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,22 @@ 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',
];
const expectedWarnings = badURLs.map(
(url) => [
`The URL ${url} is invalid. Future versions of Node.js will throw an error.`,
'DEP0170',
]
);
common.expectWarning({
DeprecationWarning: expectedWarnings,
});
badURLs.forEach((badURL) => {
url.parse(badURL);
});
}

0 comments on commit daf9370

Please sign in to comment.