Bug 1918928 - Only allow to wait for HTTPS RR when TRR is confirmed, a=RyanVM

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

netwerk/protocol/http/HttpBaseChannel.h

@@ -365,6 +365,15 @@ class HttpBaseChannel : public nsHashPropertyBag,
   NS_IMETHOD SetIsUserAgentHeaderModified(bool value) override;
   NS_IMETHOD GetIsUserAgentHeaderModified(bool* value) override;
 
+  NS_IMETHOD GetCaps(uint32_t* aCaps) override {
+    if (!aCaps) {
+      return NS_ERROR_INVALID_ARG;
+    }
+
+    *aCaps = mCaps;
+    return NS_OK;
+  }
+
   NS_IMETHOD SetClassicScriptHintCharset(
       const nsAString& aClassicScriptHintCharset) override;
   NS_IMETHOD GetClassicScriptHintCharset(

netwerk/protocol/http/nsHttpChannel.cpp

@@ -642,20 +642,34 @@ nsresult nsHttpChannel::OnBeforeConnect() {
 // that HTTPS records can be resolved on this network - false otherwise.
 // When TRR is enabled, we always return true, as resolving HTTPS
 // records don't depend on the network.
-static bool canUseHTTPSRRonNetwork(bool* aTRREnabled = nullptr) {
+static bool canUseHTTPSRRonNetwork(bool& aTRREnabled) {
+  // Respect the pref.
+  if (StaticPrefs::network_dns_force_use_https_rr()) {
+    aTRREnabled = true;
+    return true;
+  }
+
+  aTRREnabled = false;
+
   if (nsCOMPtr<nsIDNSService> dns = mozilla::components::DNS::Service()) {
     nsIDNSService::ResolverMode mode;
     // If the browser is currently using TRR/DoH, then it can
     // definitely resolve HTTPS records.
-    if (NS_SUCCEEDED(dns->GetCurrentTrrMode(&mode)) &&
-        (mode == nsIDNSService::MODE_TRRFIRST ||
-         mode == nsIDNSService::MODE_TRRONLY)) {
-      if (aTRREnabled) {
-        *aTRREnabled = true;
+    if (NS_SUCCEEDED(dns->GetCurrentTrrMode(&mode))) {
+      if (mode == nsIDNSService::MODE_TRRFIRST) {
+        RefPtr<TRRService> trr = TRRService::Get();
+        if (trr && trr->IsConfirmed()) {
+          aTRREnabled = true;
+        }
+      } else if (mode == nsIDNSService::MODE_TRRONLY) {
+        aTRREnabled = true;
+      }
+      if (aTRREnabled) {
+        return true;
       }
-      return true;
     }
   }
+
   if (RefPtr<NetworkConnectivityService> ncs =
           NetworkConnectivityService::GetSingleton()) {
     nsINetworkConnectivityService::ConnectivityState state;
@@ -691,7 +705,7 @@ nsresult nsHttpChannel::MaybeUseHTTPSRRForUpgrade(bool aShouldUpgrade,
     // blocking HTTPS requests, then we should skip them so we don't
     // needlessly wait for a timeout.
     bool trrEnabled = false;
-    if (!canUseHTTPSRRonNetwork(&trrEnabled)) {
+    if (!canUseHTTPSRRonNetwork(trrEnabled)) {
       return true;
     }
 
@@ -6665,7 +6679,7 @@ nsresult nsHttpChannel::BeginConnect() {
       !(mLoadInfo->TriggeringPrincipal()->IsSystemPrincipal() &&
         mLoadInfo->GetExternalContentPolicyType() !=
             ExtContentPolicy::TYPE_DOCUMENT) &&
-      !mConnectionInfo->UsingConnect() && canUseHTTPSRRonNetwork(&trrEnabled) &&
+      !mConnectionInfo->UsingConnect() && canUseHTTPSRRonNetwork(trrEnabled) &&
       StaticPrefs::network_dns_use_https_rr_as_altsvc();
   if (!httpsRRAllowed) {
     DisallowHTTPSRR(mCaps);
@@ -6858,8 +6872,10 @@ nsresult nsHttpChannel::MaybeStartDNSPrefetch() {
       mDNSBlockingThenable = mDNSBlockingPromise.Ensure(__func__);
     }
 
+    bool unused;
     if (StaticPrefs::network_dns_use_https_rr_as_altsvc() && !mHTTPSSVCRecord &&
-        !(mCaps & NS_HTTP_DISALLOW_HTTPS_RR) && canUseHTTPSRRonNetwork()) {
+        !(mCaps & NS_HTTP_DISALLOW_HTTPS_RR) &&
+        canUseHTTPSRRonNetwork(unused)) {
       MOZ_ASSERT(!mHTTPSSVCRecord);
 
       OriginAttributes originAttributes;

netwerk/protocol/http/nsIHttpChannelInternal.idl

@@ -518,4 +518,9 @@ interface nsIHttpChannelInternal : nsISupports
      * overridden header value.
      */
     [must_use] attribute boolean isUserAgentHeaderModified;
+
+    /**
+     * For testing purposes only.
+     */
+    readonly attribute unsigned long caps;
 };

netwerk/test/unit/test_bug_1918928.js

@@ -0,0 +1,157 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+"use strict";
+
+const { TestUtils } = ChromeUtils.importESModule(
+  "resource://testing-common/TestUtils.sys.mjs"
+);
+
+const CONFIRM_OK = 2;
+const CONFIRM_DISABLED = 5;
+
+function WaitHTTPSRR(input) {
+  const NS_HTTP_FORCE_WAIT_HTTP_RR = 1 << 22;
+  return (input & NS_HTTP_FORCE_WAIT_HTTP_RR) !== 0;
+}
+
+async function waitForConfirmationState(state, msToWait = 0) {
+  await TestUtils.waitForCondition(
+    () => Services.dns.currentTrrConfirmationState == state,
+    `Timed out waiting for ${state}. Currently ${Services.dns.currentTrrConfirmationState}`,
+    1,
+    msToWait
+  );
+  equal(
+    Services.dns.currentTrrConfirmationState,
+    state,
+    "expected confirmation state"
+  );
+}
+
+let h2Port;
+
+function makeChan(url) {
+  let chan = NetUtil.newChannel({
+    uri: url,
+    loadUsingSystemPrincipal: true,
+    contentPolicyType: Ci.nsIContentPolicy.TYPE_DOCUMENT,
+  }).QueryInterface(Ci.nsIHttpChannel);
+  return chan;
+}
+
+function channelOpenPromise(chan, flags) {
+  return new Promise(resolve => {
+    function finish(req, buffer) {
+      resolve([req, buffer]);
+    }
+    chan.asyncOpen(new ChannelListener(finish, null, flags));
+  });
+}
+
+add_setup(async function setup() {
+  h2Port = Services.env.get("MOZHTTP2_PORT");
+  Assert.notEqual(h2Port, null);
+  Assert.notEqual(h2Port, "");
+
+  trr_test_setup();
+  registerCleanupFunction(async () => {
+    trr_clear_prefs();
+  });
+});
+
+function ActivityObserver() {}
+
+ActivityObserver.prototype = {
+  caps: 0,
+  observeActivity(aChannel, aType, aSubtype) {
+    try {
+      aChannel.QueryInterface(Ci.nsIChannel);
+      if (
+        aChannel.URI.spec ===
+          `https://foo.example.com:${h2Port}/server-timing` &&
+        aType === Ci.nsIHttpActivityObserver.ACTIVITY_TYPE_HTTP_TRANSACTION &&
+        aSubtype === Ci.nsIHttpActivityObserver.ACTIVITY_SUBTYPE_REQUEST_HEADER
+      ) {
+        this.caps = aChannel.QueryInterface(Ci.nsIHttpChannelInternal).caps;
+      }
+    } catch (_) {}
+  },
+};
+
+// Test in TRRFIRST mode, channel only wait for HTTPS RR when TRR is confirmed.
+add_task(async function test_caps_in_trr_first() {
+  Services.prefs.setCharPref("network.trr.confirmationNS", "skip");
+  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST);
+  equal(Services.dns.currentTrrConfirmationState, CONFIRM_DISABLED);
+
+  let observerService = Cc[
+    "@mozilla.org/network/http-activity-distributor;1"
+  ].getService(Ci.nsIHttpActivityDistributor);
+  let observer = new ActivityObserver();
+  observerService.addObserver(observer);
+
+  let chan = makeChan(`https://foo.example.com:${h2Port}/server-timing`);
+  await channelOpenPromise(chan);
+
+  Assert.ok(!WaitHTTPSRR(observer.caps));
+
+  let trrServer = new TRRServer();
+  registerCleanupFunction(async () => {
+    await trrServer.stop();
+  });
+  await trrServer.start();
+  await trrServer.registerDoHAnswers("confirm.example.com", "NS", {
+    answers: [
+      {
+        name: "confirm.example.com",
+        ttl: 55,
+        type: "NS",
+        flush: false,
+        data: "test.com",
+      },
+    ],
+  });
+
+  Services.dns.clearCache(true);
+  Services.prefs.setCharPref(
+    "network.trr.confirmationNS",
+    "confirm.example.com"
+  );
+
+  Services.prefs.setCharPref(
+    "network.trr.uri",
+    `https://foo.example.com:${trrServer.port()}/dns-query`
+  );
+  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST);
+  await waitForConfirmationState(CONFIRM_OK, 1000);
+
+  observer.caps = 0;
+  chan = makeChan(`https://foo.example.com:${h2Port}/server-timing`);
+  await channelOpenPromise(chan);
+
+  Assert.ok(WaitHTTPSRR(observer.caps));
+  await trrServer.stop();
+});
+
+// Test in TRRONLY mode, channel always wait for HTTPS RR.
+add_task(async function test_caps_in_trr_only() {
+  Services.prefs.setCharPref(
+    "network.trr.confirmationNS",
+    "confirm.example.com"
+  );
+  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRONLY);
+  equal(Services.dns.currentTrrConfirmationState, CONFIRM_DISABLED);
+
+  let observerService = Cc[
+    "@mozilla.org/network/http-activity-distributor;1"
+  ].getService(Ci.nsIHttpActivityDistributor);
+  let observer = new ActivityObserver();
+  observerService.addObserver(observer);
+
+  let chan = makeChan(`https://foo.example.com:${h2Port}/server-timing`);
+  await channelOpenPromise(chan);
+
+  Assert.ok(WaitHTTPSRR(observer.caps));
+});

netwerk/test/unit/test_httpssvc_https_upgrade.js

@@ -58,7 +58,7 @@ add_setup(async function setup() {
     await TestUtils.waitForCondition(() => Services.io.socketProcessLaunched);
   }
 
-  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST);
+  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRONLY);
 });
 
 function makeChan(url) {

netwerk/test/unit/test_use_httpssvc.js

@@ -34,7 +34,7 @@ add_setup(async function setup() {
     await TestUtils.waitForCondition(() => Services.io.socketProcessLaunched);
   }
 
-  Services.prefs.setIntPref("network.trr.mode", 2); // TRR first
+  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRONLY);
 });
 
 function makeChan(url) {
@@ -177,7 +177,7 @@ add_task(async function testFallback() {
   });
   await trrServer.start();
 
-  Services.prefs.setIntPref("network.trr.mode", 3);
+  Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRONLY);
   Services.prefs.setCharPref(
     "network.trr.uri",
     `https://foo.example.com:${trrServer.port()}/dns-query`

netwerk/test/unit/xpcshell.toml

@@ -310,6 +310,12 @@ skip-if = [
 ["test_bug1725766.js"]
 skip-if = ["os == 'android'"] # skip because of bug 1589327
 
+["test_bug_1918928.js"]
+skip-if = [
+  "socketprocess_networking", # confirmation state isn't passed cross-process
+  "appname == 'thunderbird'", # bug 1760097
+]
+
 ["test_cache-control_request.js"]
 
 ["test_cache-entry-id.js"]