url: runtime-deprecate url.parse() with invalid ports

Trott · Trott · commit 3bed5f11e039 · 2022-11-30T13:41:23.000-08:00
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>
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -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
diff --git a/lib/url.js b/lib/url.js
@@ -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) {
@@ -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 &&
@@ -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}`;
     }
diff --git a/test/parallel/test-url-parse-invalid-input.js b/test/parallel/test-url-parse-invalid-input.js
@@ -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
@@ -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);
+  });
+}
