Bug 1455705 fix how browserSettings.proxyConfig sets network prefs, r?rpl draft
authorShane Caraveo <scaraveo@mozilla.com>
Fri, 20 Apr 2018 19:43:36 -0500
changeset 786065 ffdf0d83a201444062240b1b4f7b067b904895e4
parent 785586 cc0d7de218cb0c260c8ba0cf6637845ad2222f49
push id107389
push usermixedpuppy@gmail.com
push dateSat, 21 Apr 2018 00:44:31 +0000
reviewersrpl
bugs1455705
milestone61.0a1
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]