Bug 1517025 - Do not allow percent symbol in URL hostnames r=kershaw

Differential Revision: https://phabricator.services.mozilla.com/D16694

--HG--
extra : moz-landing-system : lando

netwerk/base/nsStandardURL.cpp

@@ -60,10 +60,6 @@ const char nsStandardURL::gHostLimitDigits[] = {'/', '\\', '?', '#', 0};
 bool nsStandardURL::gPunycodeHost = true;
 
 // Invalid host characters
-// We still allow % because it is in the ID of addons.
-// Any percent encoded ASCII characters that are not allowed in the
-// hostname are not percent decoded, and will be parsed just fine.
-//
 // Note that the array below will be initialized at compile time,
 // so we do not need to "optimize" TestForInvalidHostCharacters.
 //
@@ -73,7 +69,7 @@ constexpr bool TestForInvalidHostCharacters(char c) {
   return (c > 0 && c < 32) ||  // The control characters are [1, 31]
          c == ' ' || c == '#' || c == '/' || c == ':' || c == '?' || c == '@' ||
          c == '[' || c == '\\' || c == ']' || c == '*' || c == '<' ||
-         c == '>' || c == '|' || c == '"';
+         c == '>' || c == '|' || c == '"' || c == '%';
 }
 constexpr ASCIIMaskArray sInvalidHostChars =
     CreateASCIIMask(TestForInvalidHostCharacters);

netwerk/test/unit/test_bug412457.js

@@ -29,17 +29,17 @@ function run_test() {
   newURI = newURI.mutate().setSpec("http://example.com/foo").finalize();
   Assert.equal(newURI.asciiHost, "example.com");
 
-  // Characters that are invalid in the host, shouldn't be decoded.
-  newURI = newURI.mutate().setSpec("http://example.com%3ffoo").finalize();
-  Assert.equal(newURI.asciiHost, "example.com%3ffoo");
-  newURI = newURI.mutate().setSpec("http://example.com%23foo").finalize();
-  Assert.equal(newURI.asciiHost, "example.com%23foo");
-  newURI = newURI.mutate().setSpec("http://example.com%3bfoo").finalize();
-  Assert.equal(newURI.asciiHost, "example.com%3bfoo");
-  newURI = newURI.mutate().setSpec("http://example.com%3a80").finalize();
-  Assert.equal(newURI.asciiHost, "example.com%3a80");
-  newURI = newURI.mutate().setSpec("http://example.com%2ffoo").finalize();
-  Assert.equal(newURI.asciiHost, "example.com%2ffoo");
-  newURI = newURI.mutate().setSpec("http://example.com%00").finalize();
-  Assert.equal(newURI.asciiHost, "example.com%00");
+  // Characters that are invalid in the host
+  Assert.throws(() => { newURI = newURI.mutate().setSpec("http://example.com%3ffoo").finalize(); },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
+  Assert.throws(() => { newURI = newURI.mutate().setSpec("http://example.com%23foo").finalize(); },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
+  Assert.throws(() => { newURI = newURI.mutate().setSpec("http://example.com%3bfoo").finalize(); },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
+  Assert.throws(() => { newURI = newURI.mutate().setSpec("http://example.com%3a80").finalize(); },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
+  Assert.throws(() => { newURI = newURI.mutate().setSpec("http://example.com%2ffoo").finalize(); },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
+  Assert.throws(() => { newURI = newURI.mutate().setSpec("http://example.com%00").finalize(); },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
 }

netwerk/test/unit/test_bug464591.js

@@ -5,11 +5,13 @@
 let reference = [
    ["www.example.com%e2%88%95www.mozill%d0%b0.com%e2%81%84www.mozilla.org",
     "www.example.xn--comwww-re3c.xn--mozill-8nf.xn--comwww-rq0c.mozilla.org"],
-   ["www.mozill%61%2f.org", "www.mozilla%2f.org"], // a slash is not valid in the hostname
-   ["www.e%00xample.com%e2%88%95www.mozill%d0%b0.com%e2%81%84www.mozill%61.org",
-    "www.e%00xample.xn--comwww-re3c.xn--mozill-8nf.xn--comwww-rq0c.mozilla.org"],
 ];
 
+let badURIs = [
+   ["www.mozill%61%2f.org"], // a slash is not valid in the hostname
+   ["www.e%00xample.com%e2%88%95www.mozill%d0%b0.com%e2%81%84www.mozill%61.org"],
+]
+
 let prefData =
   [
     {
@@ -75,4 +77,9 @@ function run_test() {
       ok(false, "Error testing "+reference[i][0]);
     }
   }
+
+  for (let i = 0; i < badURIs.length; ++i) {
+    Assert.throws(() => { let result = stringToURL("http://" + badURIs[i][0]).host; },
+                /NS_ERROR_MALFORMED_URI/, "bad escaped character");
+  }
 }

netwerk/test/unit/test_standardurl.js

@@ -1,5 +1,6 @@
 "use strict";
 
+ChromeUtils.import('resource://gre/modules/Services.jsm');
 const gPrefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
 
 function symmetricEquality(expect, a, b)
@@ -312,9 +313,9 @@ add_test(function test_percentDecoding()
   var url = stringToURL("http://%70%61%73%74%65%62%69%6E.com");
   Assert.equal(url.spec, "http://pastebin.com/");
 
-  // We shouldn't unescape characters that are not allowed in the hostname.
-  url = stringToURL("http://example.com%0a%23.google.com/");
-  Assert.equal(url.spec, "http://example.com%0a%23.google.com/");
+  // Disallowed hostname characters are rejected even when percent encoded
+  Assert.throws(() => { url = stringToURL("http://example.com%0a%23.google.com/"); },
+                /NS_ERROR_MALFORMED_URI/, "invalid characters are not allowed");
   run_next_test();
 });
 
@@ -696,3 +697,17 @@ add_test(function test_idna_host() {
 
   run_next_test();
 });
+
+add_test(function test_bug1517025() {
+  Assert.throws(() => { let other = stringToURL("https://b%9a/"); },
+                /NS_ERROR_UNEXPECTED/, "bad URI");
+
+  Assert.throws(() => { let other = stringToURL("https://b%9ª/"); },
+                /NS_ERROR_MALFORMED_URI/, "bad URI");
+
+  let base = stringToURL("https://bug1517025.bmoattachments.org/attachment.cgi?id=9033787");
+  Assert.throws(() => { let uri = Services.io.newURI("/\\b%9ª", "windows-1252", base); },
+                /NS_ERROR_MALFORMED_URI/, "bad URI");
+
+  run_next_test();
+});