Bug 1455705 fix how browserSettings.proxyConfig sets network prefs, r=rpl
authorShane Caraveo <scaraveo@mozilla.com>
Fri, 20 Apr 2018 19:43:36 -0500
changeset 468616 1bd0b3c256ab364125ee5c113a18f5449cd00ec7
parent 468615 f12de32d3468fd00d541c2a8557cb0a33ebf398d
child 468617 ee291d63749722cfc51215f24476fc1d6783f8ea
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrpl
bugs1455705
milestone61.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 1455705 fix how browserSettings.proxyConfig sets network prefs, r=rpl proxyConfig set urls onto the pref rather than host names. This adds a round-trip test with a test that uses the proxy after setting the config. Also fixes setting prefs when httpProxyAll is true. MozReview-Commit-ID: FpXKjnOXEkl
toolkit/components/extensions/parent/ext-browserSettings.js
toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
toolkit/components/extensions/test/xpcshell/test_ext_proxy_settings.js
toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
--- a/toolkit/components/extensions/parent/ext-browserSettings.js
+++ b/toolkit/components/extensions/parent/ext-browserSettings.js
@@ -206,22 +206,18 @@ ExtensionPreferencesManager.addSetting("
       "network.proxy.autoconfig_url": value.autoConfigUrl,
       "network.proxy.share_proxy_settings": value.httpProxyAll,
       "network.proxy.socks_version": value.socksVersion,
       "network.proxy.no_proxies_on": value.passthrough,
     };
 
     for (let prop of ["http", "ftp", "ssl", "socks"]) {
       if (value[prop]) {
-        let url = new URL(prop === "socks" ?
-                          `http://${value[prop]}` :
-                          value[prop]);
-        prefs[`network.proxy.${prop}`] = prop === "socks" ?
-          url.hostname :
-          `${url.protocol}//${url.hostname}`;
+        let url = new URL(`http://${value[prop]}`);
+        prefs[`network.proxy.${prop}`] = url.hostname;
         let port = parseInt(url.port, 10);
         prefs[`network.proxy.${prop}_port`] = isNaN(port) ? 0 : port;
       } else {
         prefs[`network.proxy.${prop}`] = undefined;
         prefs[`network.proxy.${prop}_port`] = undefined;
       }
     }
 
@@ -363,19 +359,19 @@ this.browserSettings = class extends Ext
                 autoLogin: Services.prefs.getBoolPref("signon.autologin.proxy"),
                 proxyDNS: Services.prefs.getBoolPref("network.proxy.socks_remote_dns"),
                 httpProxyAll: Services.prefs.getBoolPref("network.proxy.share_proxy_settings"),
                 socksVersion: Services.prefs.getIntPref("network.proxy.socks_version"),
                 passthrough: Services.prefs.getCharPref("network.proxy.no_proxies_on"),
               };
 
               for (let prop of ["http", "ftp", "ssl", "socks"]) {
-                let url = Services.prefs.getCharPref(`network.proxy.${prop}`);
+                let host = Services.prefs.getCharPref(`network.proxy.${prop}`);
                 let port = Services.prefs.getIntPref(`network.proxy.${prop}_port`);
-                proxyConfig[prop] = port ? `${url}:${port}` : url;
+                proxyConfig[prop] = port ? `${host}:${port}` : host;
               }
 
               return proxyConfig;
             },
             // proxyConfig is unsupported on android.
             undefined, false, ["android"]
           ),
           {
@@ -392,24 +388,36 @@ this.browserSettings = class extends Ext
 
               let value = details.value;
 
               if (!PROXY_TYPES_MAP.has(value.proxyType)) {
                 throw new ExtensionError(
                   `${value.proxyType} is not a valid value for proxyType.`);
               }
 
+              if (value.httpProxyAll) {
+                // Match what about:preferences does with proxy settings
+                // since the proxy service does not check the value
+                // of share_proxy_settings.
+                for (let prop of ["ftp", "ssl", "socks"]) {
+                  value[prop] = value.http;
+                }
+              }
+
               for (let prop of ["http", "ftp", "ssl", "socks"]) {
-                let url = value[prop];
-                if (url) {
-                  if (prop === "socks") {
-                    url = `http://${url}`;
-                  }
+                let host = value[prop];
+                if (host) {
                   try {
-                    new URL(url);
+                    // Fixup in case a full url is passed.
+                    if (host.includes("://")) {
+                      value[prop] = new URL(host).host;
+                    } else {
+                      // Validate the host value.
+                      new URL(`http://${host}`);
+                    }
                   } catch (e) {
                     throw new ExtensionError(
                       `${value[prop]} is not a valid value for ${prop}.`);
                   }
                 }
               }
 
               if (value.proxyType === "autoConfig" || value.autoConfigUrl) {
--- a/toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
@@ -230,37 +230,37 @@ add_task(async function test_browser_set
 
   await testSetting(
     "useDocumentFonts", false,
     {"browser.display.use_document_fonts": 0});
   await testSetting(
     "useDocumentFonts", true,
     {"browser.display.use_document_fonts": 1});
 
-  async function testProxy(config, expectedPrefs) {
+  async function testProxy(config, expectedPrefs, expectedConfig = config) {
     // proxyConfig is not supported on Android.
     if (AppConstants.platform === "android") {
       return Promise.resolve();
     }
 
     let proxyConfig = {
-      proxyType: "system",
+      proxyType: "none",
       autoConfigUrl: "",
       autoLogin: false,
       proxyDNS: false,
       httpProxyAll: false,
       socksVersion: 5,
       passthrough: "localhost, 127.0.0.1",
       http: "",
       ftp: "",
       ssl: "",
       socks: "",
     };
     return testSetting(
-      "proxyConfig", config, expectedPrefs, Object.assign(proxyConfig, config)
+      "proxyConfig", config, expectedPrefs, Object.assign(proxyConfig, expectedConfig)
     );
   }
 
   await testProxy(
     {proxyType: "none"},
     {"network.proxy.type": proxySvc.PROXYCONFIG_DIRECT},
   );
 
@@ -304,55 +304,77 @@ add_task(async function test_browser_set
   await testProxy(
     {
       proxyType: "manual",
       http: "http://www.mozilla.org",
       autoConfigUrl: "",
     },
     {
       "network.proxy.type": proxySvc.PROXYCONFIG_MANUAL,
-      "network.proxy.http": "http://www.mozilla.org",
+      "network.proxy.http": "www.mozilla.org",
       "network.proxy.http_port": 0,
       "network.proxy.autoconfig_url": "",
+    },
+    {
+      proxyType: "manual",
+      http: "www.mozilla.org",
+      autoConfigUrl: "",
+    }
+  );
+
+  // When using proxyAll, we expect all proxies to be set to
+  // be the same as http.
+  await testProxy(
+    {
+      proxyType: "manual",
+      http: "http://www.mozilla.org:8080",
+      ftp: "http://www.mozilla.org:1234",
+      httpProxyAll: true,
+    },
+    {
+      "network.proxy.type": proxySvc.PROXYCONFIG_MANUAL,
+      "network.proxy.http": "www.mozilla.org",
+      "network.proxy.http_port": 8080,
+      "network.proxy.ftp": "www.mozilla.org",
+      "network.proxy.ftp_port": 8080,
+      "network.proxy.ssl": "www.mozilla.org",
+      "network.proxy.ssl_port": 8080,
+      "network.proxy.socks": "www.mozilla.org",
+      "network.proxy.socks_port": 8080,
+      "network.proxy.share_proxy_settings": true,
+    },
+    {
+      proxyType: "manual",
+      http: "www.mozilla.org:8080",
+      ftp: "www.mozilla.org:8080",
+      ssl: "www.mozilla.org:8080",
+      socks: "www.mozilla.org:8080",
+      httpProxyAll: true,
     }
   );
 
   await testProxy(
     {
       proxyType: "manual",
-      http: "http://www.mozilla.org:8080",
-      httpProxyAll: true,
-    },
-    {
-      "network.proxy.type": proxySvc.PROXYCONFIG_MANUAL,
-      "network.proxy.http": "http://www.mozilla.org",
-      "network.proxy.http_port": 8080,
-      "network.proxy.share_proxy_settings": true,
-    }
-  );
-
-  await testProxy(
-    {
-      proxyType: "manual",
-      http: "http://www.mozilla.org:8080",
+      http: "www.mozilla.org:8080",
       httpProxyAll: false,
-      ftp: "http://www.mozilla.org:8081",
-      ssl: "http://www.mozilla.org:8082",
+      ftp: "www.mozilla.org:8081",
+      ssl: "www.mozilla.org:8082",
       socks: "mozilla.org:8083",
       socksVersion: 4,
       passthrough: ".mozilla.org",
     },
     {
       "network.proxy.type": proxySvc.PROXYCONFIG_MANUAL,
-      "network.proxy.http": "http://www.mozilla.org",
+      "network.proxy.http": "www.mozilla.org",
       "network.proxy.http_port": 8080,
       "network.proxy.share_proxy_settings": false,
-      "network.proxy.ftp": "http://www.mozilla.org",
+      "network.proxy.ftp": "www.mozilla.org",
       "network.proxy.ftp_port": 8081,
-      "network.proxy.ssl": "http://www.mozilla.org",
+      "network.proxy.ssl": "www.mozilla.org",
       "network.proxy.ssl_port": 8082,
       "network.proxy.socks": "mozilla.org",
       "network.proxy.socks_port": 8083,
       "network.proxy.socks_version": 4,
       "network.proxy.no_proxies_on": ".mozilla.org",
     }
   );
 
@@ -479,25 +501,16 @@ add_task(async function test_bad_value_p
     async () => {
       await browser.test.assertRejects(
         browser.browserSettings.proxyConfig.set({value: {
           proxyType: "abc",
         }}),
         /abc is not a valid value for proxyType/,
         "proxyConfig.set rejects with an invalid proxyType value.");
 
-      for (let protocol of ["http", "ftp", "ssl"]) {
-        let value = {proxyType: "manual"};
-        value[protocol] = "abc";
-        await browser.test.assertRejects(
-          browser.browserSettings.proxyConfig.set({value}),
-          `abc is not a valid value for ${protocol}.`,
-          `proxyConfig.set rejects with an invalid ${protocol} value.`);
-      }
-
       await browser.test.assertRejects(
         browser.browserSettings.proxyConfig.set({value: {
           proxyType: "autoConfig",
         }}),
         /undefined is not a valid value for autoConfigUrl/,
         "proxyConfig.set for type autoConfig rejects with an empty autoConfigUrl value.");
 
       await browser.test.assertRejects(
new file mode 100644
--- /dev/null
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_proxy_settings.js
@@ -0,0 +1,79 @@
+"use strict";
+
+ChromeUtils.import("resource://gre/modules/ExtensionUtils.jsm");
+ChromeUtils.defineModuleGetter(this, "HttpServer",
+                               "resource://testing-common/httpd.js");
+
+const {
+  createAppInfo,
+  promiseShutdownManager,
+  promiseStartupManager,
+} = AddonTestUtils;
+
+AddonTestUtils.init(this);
+
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
+
+// We cannot use createHttpServer because it also messes with proxies.  We want
+// httpChannel to pick up the prefs we set and use those to proxy to our server.
+// If this were to fail, we would get an error about making a request out to
+// the network.
+const proxy = new HttpServer();
+proxy.start(-1);
+proxy.registerPathHandler("/fubar", (request, response) => {
+  response.setStatusLine(request.httpVersion, 200, "OK");
+  response.write("ok");
+});
+registerCleanupFunction(() => {
+  return new Promise(resolve => {
+    proxy.stop(resolve);
+  });
+});
+
+add_task(async function test_proxy_settings() {
+  async function background(host, port) {
+    browser.webRequest.onBeforeRequest.addListener(details => {
+      browser.test.assertEq(host, details.proxyInfo.host, "proxy host matched");
+      browser.test.assertEq(port, details.proxyInfo.port, "proxy port matched");
+    }, {urls: ["http://example.com/*"]});
+    browser.webRequest.onCompleted.addListener(details => {
+      browser.test.notifyPass("proxytest");
+    }, {urls: ["http://example.com/*"]});
+    browser.webRequest.onErrorOccurred.addListener(details => {
+      browser.test.notifyFail("proxytest");
+    }, {urls: ["http://example.com/*"]});
+
+    // Wait for the settings before testing a request.
+    await browser.browserSettings.proxyConfig.set({value: {
+      proxyType: "manual",
+      http: `${host}:${port}`,
+    }});
+    browser.test.sendMessage("ready");
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      permissions: [
+        "proxy",
+        "webRequest",
+        "browserSettings",
+        "<all_urls>",
+      ],
+    },
+    useAddonManager: "temporary",
+    background: `(${background})("${proxy.identity.primaryHost}", ${proxy.identity.primaryPort})`,
+  });
+
+  await promiseStartupManager();
+  await extension.startup();
+  await extension.awaitMessage("ready");
+  equal(Services.prefs.getStringPref("network.proxy.http"), proxy.identity.primaryHost, "proxy address is set");
+  equal(Services.prefs.getIntPref("network.proxy.http_port"), proxy.identity.primaryPort, "proxy port is set");
+  let ok = extension.awaitFinish("proxytest");
+  let contentPage = await ExtensionTestUtils.loadContentPage("http://example.com/fubar");
+  await ok;
+
+  await contentPage.close();
+  await extension.unload();
+  await promiseShutdownManager();
+});
--- a/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell-common.ini
@@ -62,16 +62,18 @@ skip-if = (os == "win" && !debug) #Bug 1
 skip-if = true # This test no longer tests what it is meant to test.
 [test_ext_permission_xhr.js]
 [test_ext_persistent_events.js]
 [test_ext_privacy.js]
 [test_ext_privacy_disable.js]
 [test_ext_privacy_update.js]
 [test_ext_proxy_auth.js]
 [test_ext_proxy_onauthrequired.js]
+[test_ext_proxy_settings.js]
+skip-if = os == "android" # proxy settings are not supported on android
 [test_ext_proxy_socks.js]
 [test_ext_proxy_speculative.js]
 [test_ext_redirects.js]
 [test_ext_runtime_connect_no_receiver.js]
 [test_ext_runtime_getBrowserInfo.js]
 [test_ext_runtime_getPlatformInfo.js]
 [test_ext_runtime_id.js]
 [test_ext_runtime_onInstalled_and_onStartup.js]