Bug 1517025 - Do not allow percent symbol in URL hostnames r=kershaw
☠☠ backed out by 22471aa9be27 ☠ ☠
authorValentin Gosu <valentin.gosu@gmail.com>
Mon, 21 Jan 2019 08:39:34 +0000
changeset 454672 419cb778d5316033fc772898d2160e9998c6ee14
parent 454671 af184e29a54633ac5b8dc0a7726dd556676c243e
child 454673 48c7d643d2fa9caf7ebe07831cb5d15fa7f82a76
push id76453
push uservalentin.gosu@gmail.com
push dateMon, 21 Jan 2019 11:44:22 +0000
treeherderautoland@1e173178e49f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskershaw
bugs1517025
milestone66.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1517025 - Do not allow percent symbol in URL hostnames r=kershaw Differential Revision: https://phabricator.services.mozilla.com/D16694
netwerk/base/nsStandardURL.cpp
netwerk/test/unit/test_bug412457.js
netwerk/test/unit/test_bug464591.js
netwerk/test/unit/test_standardurl.js
--- a/netwerk/base/nsStandardURL.cpp
+++ b/netwerk/base/nsStandardURL.cpp
@@ -55,30 +55,26 @@ nsIIDNService *nsStandardURL::gIDN = nul
 // may race when reading this values, but that's OK because in the worst
 // case we will just dispatch a noop runnable to the main thread.
 bool nsStandardURL::gInitialized = false;
 
 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.
 //
 constexpr bool TestForInvalidHostCharacters(char c) {
   // Testing for these:
   // CONTROL_CHARACTERS " #/:?@[\\]*<>|\"";
   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);
 
 //----------------------------------------------------------------------------
 // nsStandardURL::nsSegmentEncoder
 //----------------------------------------------------------------------------
 
--- a/netwerk/test/unit/test_bug412457.js
+++ b/netwerk/test/unit/test_bug412457.js
@@ -24,22 +24,22 @@ function run_test() {
   Assert.equal(newURI.asciiHost, "example.com");
 
   newURI = newURI.mutate().setSpec("http://example.com:80").finalize();
   Assert.equal(newURI.asciiHost, "example.com");
 
   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");
 }
--- a/netwerk/test/unit/test_bug464591.js
+++ b/netwerk/test/unit/test_bug464591.js
@@ -1,20 +1,22 @@
  // 1.percent-encoded IDN that contains blacklisted character should be converted
  //   to punycode, not UTF-8 string
  // 2.only hostname-valid percent encoded ASCII characters should be decoded
  // 3.IDN convertion must not bypassed by %00
 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 =
   [
     {
       name: "network.enableIDN",
       newVal: true
     },
     {
       name: "network.IDN_show_punycode",
@@ -70,9 +72,14 @@ function run_test() {
   for (let i = 0; i < reference.length; ++i) {
     try {
       let result = stringToURL("http://" + reference[i][0]).host;
       equal(result, reference[i][1]);
     } catch (e) {
       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");
+  }
 }
--- a/netwerk/test/unit/test_standardurl.js
+++ b/netwerk/test/unit/test_standardurl.js
@@ -1,10 +1,11 @@
 "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)
 {
   /* Use if/else instead of |do_check_eq(expect, a.spec == b.spec)| so
      that we get the specs output on the console if the check fails.
    */
   if (expect) {
@@ -307,19 +308,19 @@ add_test(function test_accentEncoding()
   run_next_test();
 });
 
 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();
 });
 
 add_test(function test_hugeStringThrows()
 {
   let prefs = Cc["@mozilla.org/preferences-service;1"]
                 .getService(Ci.nsIPrefService);
   let maxLen = prefs.getIntPref("network.standard-url.max-length");
@@ -691,8 +692,22 @@ add_test(function test_idna_host() {
   equal(url.asciiSpec, "http://user:password@xn--lt-uia.example.org:8080/path?query");
 
   url = stringToURL("http://user:password@www.ält.com:8080/path?query#etc");
   url = url.mutate().setRef("").finalize();
   equal(url.spec, "http://user:password@www.xn--lt-uia.com:8080/path?query");
 
   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();
+});