url: stop running url.parse() hostnames through the WHATWG host parser

src/js/node/url.ts

@@ -26,7 +26,7 @@
 "use strict";
 
 const { URL, URLSearchParams } = globalThis;
-const [domainToASCII, domainToUnicode] = $cpp("NodeURL.cpp", "Bun::createNodeURLBinding");
+const [domainToASCII, domainToUnicode, toASCII] = $cpp("NodeURL.cpp", "Bun::createNodeURLBinding");
 const { urlToHttpOptions } = require("internal/url");
 const { validateString } = require("internal/validators");
 const ObjectSetPrototypeOf = Object.setPrototypeOf;
@@ -75,6 +75,14 @@ var protocolPattern = /^([a-z0-9.+-]+:)/i,
   nonHostChars = ["%", "/", "?", ";", "#"].concat(autoEscape),
   hostEndingChars = ["/", "?", "#"],
   hostnameMaxLen = 255,
+  /*
+   * Prevents spoofing bugs caused by IDNA toASCII mapping a character into one
+   * that changes how the host is interpreted. ':' spoofs the protocol, '@' the
+   * auth, and '[' / ']' make a non-IPv6 host look like IPv6.
+   */
+  forbiddenHostChars = /[\0\t\n\r #%/:<>?@[\\\]^|]/,
+  // For IPv6, permit '[', ']', and ':'.
+  forbiddenHostCharsIpv6 = /[\0\t\n\r #%/<>?@\\^|]/,
   // protocols that can allow "unsafe" and "unwise" chars.
   unsafeProtocol = {
     __proto__: null,
@@ -110,12 +118,7 @@ function urlParse(
   if ($isObject(url) && url instanceof Url) return url;
 
   var u = new Url();
-  try {
-    u.parse(url, parseQueryString, slashesDenoteHost);
-  } catch (e) {
-    $putByIdDirect(e, "input", url);
-    throw e;
-  }
+  u.parse(url, parseQueryString, slashesDenoteHost);
   return u;
 }
 
@@ -340,14 +343,28 @@ Url.prototype.parse = function parse(url: string, parseQueryString?: boolean, sl
       this.hostname = this.hostname.toLowerCase();
     }
 
-    /*
-     * IDNA Support: Returns a punycoded representation of "domain".
-     * It only converts parts of the domain name that
-     * have non-ASCII characters, i.e. it doesn't matter if
-     * you call it with a domain that already is ASCII-only.
-     */
-    if (this.hostname) {
-      this.hostname = new URL("http://" + this.hostname).hostname;
+    if (this.hostname !== "") {
+      if (ipv6Hostname) {
+        if (forbiddenHostCharsIpv6.test(this.hostname)) {
+          throw $ERR_INVALID_URL(url);
+        }
+      } else {
+        /*
+         * IDNA Support: Returns a punycoded representation of "domain".
+         * It only converts parts of the domain name that
+         * have non-ASCII characters, i.e. it doesn't matter if
+         * you call it with a domain that already is ASCII-only.
+         */
+        this.hostname = toASCII(this.hostname);
+
+        /*
+         * An empty hostname or a forbidden character can only have been
+         * introduced by toASCII, since getHostname filters them out otherwise.
+         */
+        if (this.hostname === "" || forbiddenHostChars.test(this.hostname)) {
+          throw $ERR_INVALID_URL(url);
+        }
+      }
     }
 
     var p = this.port ? ":" + this.port : "";
@@ -437,7 +454,6 @@ function isIpv6Hostname(hostname: string) {
   );
 }
 
-let warnInvalidPort = true;
 function getHostname(self, rest, hostname: string, url) {
   for (let i = 0; i < hostname.length; ++i) {
     const code = hostname.$charCodeAt(i);
@@ -450,12 +466,10 @@ function getHostname(self, rest, hostname: string, url) {
 
     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;
+      if (code === Char.COLON) {
+        // node passes the reason where ERR_INVALID_ARG_VALUE expects the value,
+        // which reads oddly. Kept so the message matches node's exactly.
+        throw $ERR_INVALID_ARG_VALUE("url", "Invalid port in url", url);
       }
       self.hostname = hostname.slice(0, i);
       return `/${hostname.slice(i)}${rest}`;

src/jsc/bindings/NodeURL.cpp

@@ -4,6 +4,48 @@
 
 namespace Bun {
 
+// The errors UTS #46 reports that the WHATWG URL Standard ignores, by turning
+// off CheckHyphens and VerifyDnsLength. ICU has no option for either.
+static constexpr int allowedIDNAErrors = UIDNA_ERROR_EMPTY_LABEL | UIDNA_ERROR_LABEL_TOO_LONG | UIDNA_ERROR_DOMAIN_NAME_TOO_LONG | UIDNA_ERROR_LEADING_HYPHEN | UIDNA_ERROR_TRAILING_HYPHEN | UIDNA_ERROR_HYPHEN_3_4;
+static constexpr size_t hostnameBufferLength = 2048;
+
+// UTS #46 ToASCII. Returns a null string when the domain is not valid.
+static WTF::String nameToASCII(const WTF::String& input)
+{
+    if (input.isEmpty())
+        return {};
+
+    WTF::String domain = input;
+    domain.convertTo16Bit();
+
+    auto* encoder = &WTF::URLParser::internationalDomainNameTranscoder();
+    char16_t hostnameBuffer[hostnameBufferLength];
+    UErrorCode error = U_ZERO_ERROR;
+    UIDNAInfo processingDetails = UIDNA_INFO_INITIALIZER;
+    const auto span = domain.span16();
+    int32_t numCharactersConverted = uidna_nameToASCII(encoder, span.data(), span.size(), hostnameBuffer, hostnameBufferLength, &processingDetails, &error);
+
+    if (U_SUCCESS(error) && !(processingDetails.errors & ~allowedIDNAErrors) && numCharactersConverted)
+        return WTF::String(std::span { hostnameBuffer, static_cast<unsigned int>(numCharactersConverted) });
+    return {};
+}
+
+// The IDNA mapping `url.parse()` applies to a hostname. Unlike `domainToASCII`
+// this performs no host parsing, so IPv4 and IPv6 hosts are left untouched.
+JSC_DEFINE_HOST_FUNCTION(jsToASCII, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame))
+{
+    auto& vm = JSC::getVM(globalObject);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    auto domain = callFrame->argument(0).toWTFString(globalObject);
+    RETURN_IF_EXCEPTION(scope, {});
+
+    auto ascii = nameToASCII(domain);
+    if (ascii.isNull())
+        return JSC::JSValue::encode(jsEmptyString(vm));
+    return JSC::JSValue::encode(JSC::jsString(vm, WTF::move(ascii)));
+}
+
 JSC_DEFINE_HOST_FUNCTION(jsDomainToASCII, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame))
 {
     auto& vm = JSC::getVM(globalObject);
@@ -52,23 +94,11 @@ JSC_DEFINE_HOST_FUNCTION(jsDomainToASCII, (JSC::JSGlobalObject * globalObject, J
 
     if (domain.containsOnlyASCII())
         return JSC::JSValue::encode(arg0);
-    if (domain.is8Bit())
-        domain.convertTo16Bit();
 
-    constexpr static int allowedNameToASCIIErrors = UIDNA_ERROR_EMPTY_LABEL | UIDNA_ERROR_LABEL_TOO_LONG | UIDNA_ERROR_DOMAIN_NAME_TOO_LONG | UIDNA_ERROR_LEADING_HYPHEN | UIDNA_ERROR_TRAILING_HYPHEN | UIDNA_ERROR_HYPHEN_3_4;
-    constexpr static size_t hostnameBufferLength = 2048;
-
-    auto encoder = &WTF::URLParser::internationalDomainNameTranscoder();
-    char16_t hostnameBuffer[hostnameBufferLength];
-    UErrorCode error = U_ZERO_ERROR;
-    UIDNAInfo processingDetails = UIDNA_INFO_INITIALIZER;
-    const auto span = domain.span16();
-    int32_t numCharactersConverted = uidna_nameToASCII(encoder, span.data(), span.size(), hostnameBuffer, hostnameBufferLength, &processingDetails, &error);
-
-    if (U_SUCCESS(error) && !(processingDetails.errors & ~allowedNameToASCIIErrors) && numCharactersConverted) {
-        return JSC::JSValue::encode(JSC::jsString(vm, WTF::String(std::span { hostnameBuffer, static_cast<unsigned int>(numCharactersConverted) })));
-    }
-    return JSC::JSValue::encode(jsEmptyString(vm));
+    auto ascii = nameToASCII(domain);
+    if (ascii.isNull())
+        return JSC::JSValue::encode(jsEmptyString(vm));
+    return JSC::JSValue::encode(JSC::jsString(vm, WTF::move(ascii)));
 }
 
 JSC_DEFINE_HOST_FUNCTION(jsDomainToUnicode, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame))
@@ -123,9 +153,6 @@ JSC_DEFINE_HOST_FUNCTION(jsDomainToUnicode, (JSC::JSGlobalObject * globalObject,
 
     domain.convertTo16Bit();
 
-    constexpr static int allowedNameToUnicodeErrors = UIDNA_ERROR_EMPTY_LABEL | UIDNA_ERROR_LABEL_TOO_LONG | UIDNA_ERROR_DOMAIN_NAME_TOO_LONG | UIDNA_ERROR_LEADING_HYPHEN | UIDNA_ERROR_TRAILING_HYPHEN | UIDNA_ERROR_HYPHEN_3_4;
-    constexpr static int hostnameBufferLength = 2048;
-
     auto encoder = &WTF::URLParser::internationalDomainNameTranscoder();
     char16_t hostnameBuffer[hostnameBufferLength];
     UErrorCode error = U_ZERO_ERROR;
@@ -135,7 +162,7 @@ JSC_DEFINE_HOST_FUNCTION(jsDomainToUnicode, (JSC::JSGlobalObject * globalObject,
 
     int32_t numCharactersConverted = uidna_nameToUnicode(encoder, span.data(), span.size(), hostnameBuffer, hostnameBufferLength, &processingDetails, &error);
 
-    if (U_SUCCESS(error) && !(processingDetails.errors & ~allowedNameToUnicodeErrors) && numCharactersConverted) {
+    if (U_SUCCESS(error) && !(processingDetails.errors & ~allowedIDNAErrors) && numCharactersConverted) {
         return JSC::JSValue::encode(JSC::jsString(vm, WTF::String(std::span { hostnameBuffer, static_cast<unsigned int>(numCharactersConverted) })));
     }
     return JSC::JSValue::encode(jsEmptyString(vm));
@@ -145,13 +172,15 @@ JSC::JSValue createNodeURLBinding(Zig::GlobalObject* globalObject)
 {
     VM& vm = globalObject->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
-    auto binding = constructEmptyArray(globalObject, nullptr, 2);
+    auto binding = constructEmptyArray(globalObject, nullptr, 3);
     RETURN_IF_EXCEPTION(scope, {});
     ASSERT(binding);
     auto domainToAsciiFunction = JSC::JSFunction::create(vm, globalObject, 1, "domainToAscii"_s, jsDomainToASCII, ImplementationVisibility::Public);
     ASSERT(domainToAsciiFunction);
     auto domainToUnicodeFunction = JSC::JSFunction::create(vm, globalObject, 1, "domainToUnicode"_s, jsDomainToUnicode, ImplementationVisibility::Public);
     ASSERT(domainToUnicodeFunction);
+    auto toAsciiFunction = JSC::JSFunction::create(vm, globalObject, 1, "toASCII"_s, jsToASCII, ImplementationVisibility::Public);
+    ASSERT(toAsciiFunction);
     binding->putByIndexInline(
         globalObject,
         (unsigned)0,
@@ -162,6 +191,11 @@ JSC::JSValue createNodeURLBinding(Zig::GlobalObject* globalObject)
         (unsigned)1,
         domainToUnicodeFunction,
         false);
+    binding->putByIndexInline(
+        globalObject,
+        (unsigned)2,
+        toAsciiFunction,
+        false);
     return binding;
 }
 

test/js/node/test/parallel/test-url-parse-format.js

@@ -865,22 +865,6 @@ 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,

test/js/node/test/parallel/test-url-parse-invalid-input.js

@@ -83,25 +83,13 @@ if (common.hasIntl) {
   badURLs.forEach((badURL) => {
     common.spawnPromisified(process.execPath, ['-e', `url.parse(${JSON.stringify(badURL)})`])
       .then(common.mustCall(({ code, stdout, stderr }) => {
-        assert.strictEqual(code, 0);
-        assert.strictEqual(stdout, '');
-        // NOTE: bun formats errors slightly differently from node, but we're
-        // printing the same deprecation message.
-        // assert.match(stderr, /\[DEP0170\] DeprecationWarning:/);
-        assert.match(stderr, /\DEP0170/);
-        assert.match(stderr, /\DeprecationWarning/);
+        assert.strictEqual(code, 1);
       }));
   });
 
-  // Warning should only happen once per process.
-  common.expectWarning({
-    DeprecationWarning: {
-      // NOTE: this warning is noisy and annoying. We've disabled it intentionally.
-      // DEP0169: '`url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.',
-      DEP0170: `The URL ${badURLs[0]} is invalid. Future versions of Node.js will throw an error.`,
-    },
-  });
   badURLs.forEach((badURL) => {
-    url.parse(badURL);
+    assert.throws(() => url.parse(badURL), {
+      code: 'ERR_INVALID_ARG_VALUE',
+    });
   });
 }

test/js/node/url/url-parse-format.test.js

@@ -861,21 +861,6 @@ describe("url.parse then url.format", () => {
     //   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",
-    },
     // TODO: Support parsing these.
     //
     // "https://*": {