Bug 1732792 - Fall back to direct request upon fetch failure in RemoteSettings r=leplatrem,mixedpuppy

Depends on D127170

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

services/settings/Utils.jsm

@@ -112,12 +112,25 @@ var Utils = {
   async fetch(input, init = {}) {
     return new Promise(function(resolve, reject) {
       const request = new ServiceRequest();
+      function fallbackOrReject(err) {
+        if (
+          // At most one recursive Utils.fetch call (bypassProxy=false to true).
+          bypassProxy ||
+          !request.isProxied ||
+          !request.bypassProxyEnabled
+        ) {
+          reject(err);
+          return;
+        }
+        resolve(Utils.fetch(input, { ...init, bypassProxy: true }));
+      }
 
       request.onerror = () =>
-        reject(new TypeError("NetworkError: Network request failed"));
+        fallbackOrReject(new TypeError("NetworkError: Network request failed"));
       request.ontimeout = () =>
-        reject(new TypeError("Timeout: Network request failed"));
-      request.onabort = () => reject(new DOMException("Aborted", "AbortError"));
+        fallbackOrReject(new TypeError("Timeout: Network request failed"));
+      request.onabort = () =>
+        fallbackOrReject(new DOMException("Aborted", "AbortError"));
       request.onload = () => {
         // Parse raw response headers into `Headers` object.
         const headers = new Headers();
@@ -141,9 +154,9 @@ var Utils = {
         resolve(new Response(request.response, responseAttributes));
       };
 
-      const { method = "GET", headers = {} } = init;
+      const { method = "GET", headers = {}, bypassProxy = false } = init;
 
-      request.open(method, input, true);
+      request.open(method, input, { bypassProxy });
       // By default, XMLHttpRequest converts the response based on the
       // Content-Type header, or UTF-8 otherwise. This may mangle binary
       // responses. Avoid that by requesting the raw bytes.

services/settings/test/unit/test_remote_settings_utils.js

@@ -3,6 +3,7 @@
 const { TestUtils } = ChromeUtils.import(
   "resource://testing-common/TestUtils.jsm"
 );
+const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { Utils } = ChromeUtils.import("resource://services-settings/Utils.jsm");
 
 const BinaryOutputStream = Components.Constructor(
@@ -16,6 +17,12 @@ server.start(-1);
 registerCleanupFunction(() => server.stop(() => {}));
 const SERVER_BASE_URL = `http://localhost:${server.identity.primaryPort}`;
 
+const proxyServer = new HttpServer();
+proxyServer.identity.add("http", "localhost", server.identity.primaryPort);
+proxyServer.start(-1);
+registerCleanupFunction(() => proxyServer.stop(() => {}));
+const PROXY_PORT = proxyServer.identity.primaryPort;
+
 // A sequence of bytes that would become garbage if it were to be read as UTF-8:
 // - 0xEF 0xBB 0xBF is a byte order mark.
 // - 0xC0 on its own is invalid (it's the first byte of a 2-byte encoding).
@@ -28,6 +35,17 @@ server.registerPathHandler("/binary.dat", (request, response) => {
   binaryOut.writeByteArray([0xef, 0xbb, 0xbf, 0xc0]);
 });
 
+// HTTPS requests are proxied with CONNECT, but our test server is HTTP,
+// which means that the proxy will receive GET http://localhost:port.
+var proxiedCount = 0;
+proxyServer.registerPrefixHandler("/", (request, response) => {
+  ++proxiedCount;
+  Assert.equal(request.path, "/binary.dat", `Proxy request ${proxiedCount}`);
+  // Close connection without sending any response.
+  response.seizePower();
+  response.finish();
+});
+
 add_task(async function test_utils_fetch_binary() {
   let res = await Utils.fetch(`${SERVER_BASE_URL}/binary.dat`);
 
@@ -75,3 +93,73 @@ add_task(async function test_utils_fetch_has_conservative() {
   let internalChannel = channel.QueryInterface(Ci.nsIHttpChannelInternal);
   Assert.ok(internalChannel.beConservative, "beConservative flag is set");
 });
+
+add_task(async function test_utils_fetch_has_conservative() {
+  let channelPromise = TestUtils.topicObserved("http-on-modify-request");
+  await Utils.fetch(`${SERVER_BASE_URL}/binary.dat`);
+
+  let channel = (await channelPromise)[0].QueryInterface(Ci.nsIHttpChannel);
+
+  Assert.equal(channel.URI.spec, `${SERVER_BASE_URL}/binary.dat`, "URL OK");
+
+  let internalChannel = channel.QueryInterface(Ci.nsIHttpChannelInternal);
+  Assert.ok(internalChannel.beConservative, "beConservative flag is set");
+});
+
+add_task(async function test_utils_fetch_with_bad_proxy() {
+  Services.prefs.setIntPref("network.proxy.type", 1);
+  Services.prefs.setStringPref("network.proxy.http", "127.0.0.1");
+  Services.prefs.setIntPref("network.proxy.http_port", PROXY_PORT);
+  Services.prefs.setBoolPref("network.proxy.allow_hijacking_localhost", true);
+
+  // The URL that we're going to request.
+  const DESTINATION_URL = `${SERVER_BASE_URL}/binary.dat`;
+
+  Assert.equal(proxiedCount, 0, "Proxy not used yet");
+  {
+    info("Bad proxy, default prefs");
+    let res = await Utils.fetch(DESTINATION_URL);
+    Assert.equal(res.status, 201, "Bypassed bad proxy");
+    // 10 instead of 1 because of reconnect attempts after a dropped request.
+    Assert.equal(proxiedCount, 10, "Proxy was used by HttpChannel");
+  }
+
+  // Disables the failover logic from HttpChannel.
+  Services.prefs.setBoolPref("network.proxy.failover_direct", false);
+  proxiedCount = 0;
+  {
+    info("Bad proxy, disabled network.proxy.failover_direct");
+    let res = await Utils.fetch(DESTINATION_URL);
+    Assert.equal(res.status, 201, "Bypassed bad proxy");
+    // 10 instead of 1 because of reconnect attempts after a dropped request.
+    Assert.equal(proxiedCount, 10, "Proxy was used by ServiceRequest");
+  }
+
+  proxiedCount = 0;
+  {
+    info("Using internal option of Utils.fetch: bypassProxy=true");
+    let res = await Utils.fetch(DESTINATION_URL, { bypassProxy: true });
+    Assert.equal(res.status, 201, "Bypassed bad proxy");
+    Assert.equal(proxiedCount, 0, "Not using proxy when bypassProxy=true");
+  }
+
+  // Disables the failover logic from ServiceRequest/Utils.fetch
+  Services.prefs.setBoolPref("network.proxy.allow_bypass", false);
+  proxiedCount = 0;
+
+  info("Bad proxy, disabled network.proxy.allow_bypass");
+  await Assert.rejects(
+    Utils.fetch(DESTINATION_URL),
+    /NetworkError/,
+    "Bad proxy request should fail without failover"
+  );
+  // 10 instead of 1 because of reconnect attempts after a dropped request.
+  Assert.equal(proxiedCount, 10, "Attempted to use proxy again");
+
+  Services.prefs.clearUserPref("network.proxy.type");
+  Services.prefs.clearUserPref("network.proxy.http");
+  Services.prefs.clearUserPref("network.proxy.http_port");
+  Services.prefs.clearUserPref("network.proxy.allow_hijacking_localhost");
+  Services.prefs.clearUserPref("network.proxy.failover_direct");
+  Services.prefs.clearUserPref("network.proxy.allow_bypass");
+});

toolkit/modules/ServiceRequest.jsm

@@ -9,6 +9,7 @@
  * can be set, Telemetry collected, etc. in a central place.
  */
 
+const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { Log } = ChromeUtils.import("resource://gre/modules/Log.jsm");
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
@@ -47,7 +48,23 @@ class ServiceRequest extends XMLHttpRequest {
       // Disable cutting edge features, like TLS 1.3, where middleboxes might brick us
       internal.beConservative = true;
       // Disable use of proxy for this request if necessary.
-      internal.bypassProxy = options?.bypassProxy;
+      if (options?.bypassProxy && this.bypassProxyEnabled) {
+        internal.bypassProxy = true;
+      }
     }
   }
+
+  get bypassProxy() {
+    let { channel } = this;
+    return channel.QueryInterface(Ci.nsIHttpChannelInternal).bypassProxy;
+  }
+
+  get isProxied() {
+    let { channel } = this;
+    return !!(channel instanceof Ci.nsIProxiedChannel && channel.proxyInfo);
+  }
+
+  get bypassProxyEnabled() {
+    return Services.prefs.getBoolPref("network.proxy.allow_bypass", true);
+  }
 }

toolkit/modules/tests/xpcshell/test_servicerequest_xhr.js

@@ -28,3 +28,35 @@ add_task(async function test_tls_conservative() {
     "TLS setting in request channel is not set to conservative for XHR"
   );
 });
+
+add_task(async function test_bypassProxy_default() {
+  const request = new ServiceRequest();
+  request.open("GET", "http://example.com", true);
+  const sr_channel = request.channel.QueryInterface(Ci.nsIHttpChannelInternal);
+
+  ok(!sr_channel.bypassProxy, "bypassProxy is false on SR channel");
+  ok(!request.bypassProxy, "bypassProxy is false for SR");
+});
+
+add_task(async function test_bypassProxy_true() {
+  const request = new ServiceRequest();
+  request.open("GET", "http://example.com", { bypassProxy: true });
+  const sr_channel = request.channel.QueryInterface(Ci.nsIHttpChannelInternal);
+
+  ok(sr_channel.bypassProxy, "bypassProxy is true on SR channel");
+  ok(request.bypassProxy, "bypassProxy is true for SR");
+});
+
+add_task(async function test_bypassProxy_disabled_by_pref() {
+  const request = new ServiceRequest();
+
+  ok(request.bypassProxyEnabled, "bypassProxyEnabled is true");
+  Services.prefs.setBoolPref("network.proxy.allow_bypass", false);
+  ok(!request.bypassProxyEnabled, "bypassProxyEnabled is false");
+
+  request.open("GET", "http://example.com", { bypassProxy: true });
+  const sr_channel = request.channel.QueryInterface(Ci.nsIHttpChannelInternal);
+
+  ok(!sr_channel.bypassProxy, "bypassProxy is false on SR channel");
+  ok(!request.bypassProxy, "bypassProxy is false for SR");
+});