Improve DNS-over-HTTPS configuration logic

This patch improves the DNS-over-HTTPS configuration and sets defaults
for the general DNS logic. The following changes are included:

- Insecure Chromium DNS client is always OFF (OFF is the Chromium
  default as well)
- Add DnsMode::SystemOnly, which configures Chromium to only use the
  system DNS
- The default DNS configuration is DnsMode::SystemOnly
- Rename DnsMode::Secure to DnsMode::SecureOnly and
  DnsMode::WithFallback to DnsMode::SecureWithFallback to be clearer
  what each enum value does
- Add error handling for invalid URI templates
- Added test cases to handle the new logic
- Some minor refactoring for cleanup purposes with the new defaults and
  logic taken into consideration
- Some minor bug fixes

Task-number: QTBUG-98284
Pick-to: 6.6
Change-Id: Ie332166f8b5b83c8939af35e4eb8b69b417abdcf
Reviewed-by: Leena Miettinen <riitta-leena.miettinen@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Michael Brüning <michael.bruning@qt.io>

5 files changed, 120 insertions, 83 deletions

diff --git a/src/core/api/qwebengineglobalsettings.cpp b/src/core/api/qwebengineglobalsettings.cpp
index b4b09e013..0f2472c47 100644
--- a/src/core/api/qwebengineglobalsettings.cpp
+++ b/src/core/api/qwebengineglobalsettings.cpp
@@ -3,16 +3,23 @@
 
 #include "qwebengineglobalsettings.h"
 #include "qwebengineglobalsettings_p.h"
+#include <QDebug>
 
 #ifdef signals
 #undef signals
 #endif
 
+#include "content/browser/network_service_instance_impl.h"
 #include "content/public/browser/network_service_instance.h"
 #include "services/network/network_service.h"
 
 QT_BEGIN_NAMESPACE
 
+ASSERT_ENUMS_MATCH(net::SecureDnsMode::kSecure, QWebEngineGlobalSettings::DnsMode::SecureOnly)
+ASSERT_ENUMS_MATCH(net::SecureDnsMode::kAutomatic,
+                   QWebEngineGlobalSettings::DnsMode::SecureWithFallback)
+ASSERT_ENUMS_MATCH(net::SecureDnsMode::kOff, QWebEngineGlobalSettings::DnsMode::SystemOnly)
+
 /*!
     \class QWebEngineGlobalSettings
     \brief The QWebEngineGlobalSettings class configures global properties of the web engine.
@@ -22,9 +29,9 @@ QT_BEGIN_NAMESPACE
     The QWebEngineGlobalSettings class is a singleton that configures global properties
     of the web engine.
 
-    Invoke configureDnsOverHttps() to configure DNS-over-HTTPS capabilities.
+    Invoke setDnsMode() and setDnsServerTemplates() to configure DNS-over-HTTPS.
 
-    \sa QWebEngineGlobalSettings::configureDnsOverHttps()
+    \sa QWebEngineGlobalSettings::setDnsMode(), QWebEngineGlobalSettings::setDnsServerTemplates()
 */
 
 QWebEngineGlobalSettings::QWebEngineGlobalSettings(QObject *p)
@@ -50,46 +57,73 @@ QWebEngineGlobalSettings *QWebEngineGlobalSettings::instance()
 
     This enum sets the DNS-over-HTTPS mode:
 
-    \value WithFallback Enable DNS-over-HTTPS with fallbacks. If a host
-    can't be resolved, try the insecure DNS client of Chromium. If that fails as
-    well, try the system DNS host resolution, which can be secure or insecure.
-    \value Secure Enable DNS-over-HTTPS and only allow the secure Chromium
-    DNS client to resolve hosts.
+    \value SystemOnly This is the default. Use the system DNS host resolution.
+    \value SecureWithFallback Enable DNS-over-HTTPS (DoH). DoH servers have to be
+    provided through QWebEngineGlobalSettings::setDnsServerTemplates(). If a host can't be resolved
+    via the provided servers, the system DNS host resolution is used.
+    \value SecureOnly Enable DNS-over-HTTPS and only allow hosts to be resolved this way.
+    DoH servers have to be provided through QWebEngineGlobalSettings::setDnsServerTemplates().
+    If the DNS-over-HTTPS resolution fails, there is no fallback and DNS host resolution
+    fails completely.
 */
 
 /*!
-    \fn QWebEngineGlobalSettings::configureDnsOverHttps(DnsMode dnsMode,
-                                                        const QString &dnsOverHttpsTemplates)
+    \fn void QWebEngineGlobalSettings::setDnsMode(DnsMode dnsMode, const QStringList
+    &dnsServerTemplates)
+
+    Set \a dnsMode to DnsMode::SystemOnly to use the system DNS resolution.
+
+    Set \a dnsMode to DnsMode::SecureOnly to only allow DNS-over-HTTPS host resolution using servers
+    from \a dnsServerTemplates.
 
-    Configures the Chromium stub host resolver, thus allowing DNS-over-HTTPS functionality.
+    Set \a dnsMode to DnsMode::SecureWithFallback to enable DNS-over-HTTPS host resolution using
+    servers from \a dnsServerTemplates,with a fallback to the system DNS.
 
-    Set \a dnsMode to QWebEngineGlobalSettings::DnsMode::WithFallback to enable secure DNS
-    host resolution with a fallback to insecure DNS host resolution and a final fallback to
-    the system DNS resolution, which can be secure or insecure. Set it to
-    QWebEngineGlobalSettings::DnsMode::Secure to only allow secure DNS host resolution via
-    the Chromium DNS client.
+    A list \a dnsServerTemplates is a list of \l{https://datatracker.ietf.org/d7oc/html/rfc6570}{URI
+    templates}. One example URI template is https://dns.google/dns-query{?dns}.
 
-    Independently of \a {dnsMode}, \a dnsOverHttpsTemplates has to be set to one or multiple
-    valid \l{https://datatracker.ietf.org/doc/html/rfc6570}{URI templates} separated by
-    whitespace characters. One example URI template is https://dns.google/dns-query{?dns}.
+    This function returns \c false if the \a dnsServerTemplates list is empty or contains URI
+    templates that cannot be parsed for DnsMode::SecureOnly or DnsMode::SecureWithFallback.
+    Otherwise, it returns \c true meaning the DNS mode change is triggered.
 */
-void QWebEngineGlobalSettings::configureDnsOverHttps(DnsMode dnsMode,
-                                                     const QString &dnsOverHttpsTemplates)
+bool QWebEngineGlobalSettings::setDnsMode(DnsMode dnsMode, const QStringList &dnsServerTemplates)
 {
     Q_D(QWebEngineGlobalSettings);
-
+    if (dnsMode != DnsMode::SystemOnly) {
+        const QString servers = dnsServerTemplates.join(QChar::Space);
+        const std::string templates = servers.toStdString();
+        absl::optional<net::DnsOverHttpsConfig> dnsOverHttpsConfig =
+                net::DnsOverHttpsConfig::FromString(templates);
+        if (!dnsOverHttpsConfig.has_value())
+            return false;
+        d->dnsOverHttpsTemplates = templates;
+    }
     d->dnsMode = dnsMode;
-    d->dnsOverHttpsTemplates = dnsOverHttpsTemplates.toStdString();
-    d->isDnsOverHttpsUserConfigured = true;
-
-    // Make sure that DoH settings are in effect immediately if the network service already exists,
-    // thus allowing to change DoH configuration at any point
-    network::mojom::NetworkService *networkService = content::GetNetworkService();
-    if (networkService) {
-        networkService->ConfigureStubHostResolver(
-                d->insecureDnsClientEnabled, net::SecureDnsMode(d->dnsMode),
-                *net::DnsOverHttpsConfig::FromString(d->dnsOverHttpsTemplates),
-                d->additionalInsecureDnsTypesEnabled);
+    d->configureStubHostResolver();
+    return true;
+}
+
+/*!
+    \internal
+*/
+void QWebEngineGlobalSettingsPrivate::configureStubHostResolver()
+{
+    if (content::GetNetworkServiceAvailability()
+        != content::NetworkServiceAvailability::NOT_CREATED) {
+        network::mojom::NetworkService *networkService = content::GetNetworkService();
+        if (networkService) {
+            qDebug() << "doh set to" << dnsOverHttpsTemplates << " -- "
+                     << (dnsMode == QWebEngineGlobalSettings::DnsMode::SecureOnly ? "SecureOnly"
+                                 : dnsMode == QWebEngineGlobalSettings::DnsMode::SystemOnly
+                                 ? "SystemOnly"
+                                 : "SecureWithFallback");
+            absl::optional<net::DnsOverHttpsConfig> dohConfig = dnsOverHttpsTemplates.empty()
+                    ? net::DnsOverHttpsConfig()
+                    : net::DnsOverHttpsConfig::FromString(dnsOverHttpsTemplates);
+            networkService->ConfigureStubHostResolver(insecureDnsClientEnabled,
+                                                      net::SecureDnsMode(dnsMode), *dohConfig,
+                                                      additionalInsecureDnsTypesEnabled);
+        }
     }
 }
 
diff --git a/src/core/api/qwebengineglobalsettings.h b/src/core/api/qwebengineglobalsettings.h
index cec2c136b..f89aeeadb 100644
--- a/src/core/api/qwebengineglobalsettings.h
+++ b/src/core/api/qwebengineglobalsettings.h
@@ -23,9 +23,8 @@ public:
     static QWebEngineGlobalSettings *instance();
 
     // Mapping net::SecureDnsMode
-    enum class DnsMode { WithFallback = 1, Secure = 2 };
-
-    void configureDnsOverHttps(DnsMode dnsMode, const QString &dnsOverHttpsTemplates);
+    enum class DnsMode { SystemOnly = 0, SecureWithFallback = 1, SecureOnly = 2 };
+    bool setDnsMode(DnsMode dnsMode, const QStringList &dnsServerTemplates);
 
 private:
     QWebEngineGlobalSettings(QObject *p = nullptr);
diff --git a/src/core/api/qwebengineglobalsettings_p.h b/src/core/api/qwebengineglobalsettings_p.h
index 4c15f62b0..412697c0f 100644
--- a/src/core/api/qwebengineglobalsettings_p.h
+++ b/src/core/api/qwebengineglobalsettings_p.h
@@ -25,17 +25,17 @@ class Q_WEBENGINECORE_PRIVATE_EXPORT QWebEngineGlobalSettingsPrivate
 {
 public:
     QWebEngineGlobalSettingsPrivate()
-        : dnsMode(QWebEngineGlobalSettings::DnsMode::WithFallback)
+        : dnsMode(QWebEngineGlobalSettings::DnsMode::SystemOnly)
         , dnsOverHttpsTemplates("")
-        , insecureDnsClientEnabled(true)
-        , additionalInsecureDnsTypesEnabled(true)
-        , isDnsOverHttpsUserConfigured(false){};
+        , insecureDnsClientEnabled(false)
+        , additionalInsecureDnsTypesEnabled(false){};
 
     QWebEngineGlobalSettings::DnsMode dnsMode;
     std::string dnsOverHttpsTemplates;
-    bool insecureDnsClientEnabled;
-    bool additionalInsecureDnsTypesEnabled;
-    bool isDnsOverHttpsUserConfigured;
+    const bool insecureDnsClientEnabled;
+    const bool additionalInsecureDnsTypesEnabled;
+
+    void configureStubHostResolver();
 };
 
 QT_END_NAMESPACE
diff --git a/src/core/net/system_network_context_manager.cpp b/src/core/net/system_network_context_manager.cpp
index e96af8200..ed676fcbc 100644
--- a/src/core/net/system_network_context_manager.cpp
+++ b/src/core/net/system_network_context_manager.cpp
@@ -265,18 +265,7 @@ void SystemNetworkContextManager::OnNetworkServiceCreated(network::mojom::Networ
     // resolver of the network service here, each time it is instantiated, with our global
     // DNS-Over-HTTPS settings. This ensures that the global settings don't get lost
     // on reinstantiation and are in effect upon initial instantiation.
-    QWebEngineGlobalSettings *const globalSettings = QWebEngineGlobalSettings::instance();
-    if (globalSettings->d_ptr->isDnsOverHttpsUserConfigured) {
-        const bool insecureDnsClientEnabled = globalSettings->d_ptr->insecureDnsClientEnabled;
-        const bool additionalInsecureDnsTypesEnabled =
-                globalSettings->d_ptr->additionalInsecureDnsTypesEnabled;
-        const net::SecureDnsMode dnsMode = net::SecureDnsMode(globalSettings->d_ptr->dnsMode);
-        const absl::optional<net::DnsOverHttpsConfig> dnsOverHttpsTemplates =
-                net::DnsOverHttpsConfig::FromString(globalSettings->d_ptr->dnsOverHttpsTemplates);
-        content::GetNetworkService()->ConfigureStubHostResolver(insecureDnsClientEnabled, dnsMode,
-                                                                *dnsOverHttpsTemplates,
-                                                                additionalInsecureDnsTypesEnabled);
-    }
+    QWebEngineGlobalSettings::instance()->d_ptr->configureStubHostResolver();
 }
 
 void SystemNetworkContextManager::AddSSLConfigToNetworkContextParams(network::mojom::NetworkContextParams *network_context_params)
diff --git a/tests/auto/core/qwebengineglobalsettings/tst_qwebengineglobalsettings.cpp b/tests/auto/core/qwebengineglobalsettings/tst_qwebengineglobalsettings.cpp
index 71788ded5..e3986674f 100644
--- a/tests/auto/core/qwebengineglobalsettings/tst_qwebengineglobalsettings.cpp
+++ b/tests/auto/core/qwebengineglobalsettings/tst_qwebengineglobalsettings.cpp
@@ -36,15 +36,21 @@ void tst_QWebEngineGlobalSettings::dnsOverHttps_data()
 {
     QTest::addColumn<QWebEngineGlobalSettings::DnsMode>("dnsMode");
     QTest::addColumn<QString>("uriTemplate");
-    QTest::addColumn<bool>("isWithCustomDnsServer");
+    QTest::addColumn<bool>("isMockDnsServerCalledExpected");
     QTest::addColumn<bool>("isDnsResolutionSuccessExpected");
-    QTest::newRow("DnsMode::Secure (mock DNS)")
-            << QWebEngineGlobalSettings::DnsMode::Secure
-            << QStringLiteral("https://127.0.0.1:3000/dns-query{?dns}") << true << false;
-    QTest::newRow("DnsMode::Secure (real DNS)")
-            << QWebEngineGlobalSettings::DnsMode::Secure
-            << QStringLiteral("https://dns.google/dns-query{?dns}") << false << true;
-
+    QTest::addColumn<bool>("isConfigurationSuccessExpected");
+    QTest::newRow("DnsMode::SystemOnly (no DoH server)")
+            << QWebEngineGlobalSettings::DnsMode::SystemOnly << QStringLiteral("") << false << true
+            << true;
+    QTest::newRow("DnsMode::SecureOnly (mock DoH server)")
+            << QWebEngineGlobalSettings::DnsMode::SecureOnly
+            << QStringLiteral("https://127.0.0.1:3000/dns-query{?dns}") << true << false << true;
+    QTest::newRow("DnsMode::SecureOnly (real DoH server)")
+            << QWebEngineGlobalSettings::DnsMode::SecureOnly
+            << QStringLiteral("https://dns.google/dns-query{?dns}") << false << true << true;
+    QTest::newRow("DnsMode::SecureOnly (Empty URI Templates)")
+            << QWebEngineGlobalSettings::DnsMode::SecureOnly << QStringLiteral("") << false << false
+            << false;
     // Note: In the following test, we can't verify that the DoH server is called first and
     // afterwards insecure DNS is tried, because for the DoH server to ever be used when the DNS
     // mode is set to DnsMode::WithFallback, Chromium starts an asynchronous DoH server DnsProbe and
@@ -52,33 +58,45 @@ void tst_QWebEngineGlobalSettings::dnsOverHttps_data()
     // DNS response, which in turn requires that certificate errors aren't ignored and
     // non-self-signed certificates are used for correct encryption. Instead of implementing
     // all of that, this test verifies that Chromium tries probing the configured DoH server only.
-    QTest::newRow("DnsMode::WithFallback (mock DNS)")
-            << QWebEngineGlobalSettings::DnsMode::WithFallback
-            << QStringLiteral("https://127.0.0.1:3000/dns-query{?dns}") << true << true;
+    QTest::newRow("DnsMode::SecureWithFallback (mock DoH server)")
+            << QWebEngineGlobalSettings::DnsMode::SecureWithFallback
+            << QStringLiteral("https://127.0.0.1:3000/dns-query{?dns}") << true << true << true;
+    QTest::newRow("DnsMode::SecureWithFallback (Empty URI Templates)")
+            << QWebEngineGlobalSettings::DnsMode::SecureWithFallback << QStringLiteral("") << false
+            << false << false;
 }
 
 void tst_QWebEngineGlobalSettings::dnsOverHttps()
 {
     QFETCH(QWebEngineGlobalSettings::DnsMode, dnsMode);
     QFETCH(QString, uriTemplate);
-    QFETCH(bool, isWithCustomDnsServer);
+    QFETCH(bool, isMockDnsServerCalledExpected);
     QFETCH(bool, isDnsResolutionSuccessExpected);
-    bool isDnsServerCalled = false;
+    QFETCH(bool, isConfigurationSuccessExpected);
+    bool isMockDnsServerCalled = false;
     bool isLoadSuccessful = false;
 
+    QWebEngineGlobalSettings *globalSettings = QWebEngineGlobalSettings::instance();
+    bool configurationSuccess = globalSettings->setDnsMode(dnsMode, QStringList() << uriTemplate);
+    QCOMPARE(configurationSuccess, isConfigurationSuccessExpected);
+
+    if (!configurationSuccess) {
+        // In this case, DNS has invalid configuration, so the DNS change transaction is not
+        // triggered and the result of the DNS resolution depends on the current DNS mode, which is
+        // set by the previous run of this function.
+        return;
+    }
     HttpsServer httpsServer(":/cert/localhost.crt", ":/cert/localhost.key", ":/cert/RootCA.pem",
                             3000, this);
-    if (isWithCustomDnsServer) {
-        QObject::connect(
-                &httpsServer, &HttpsServer::newRequest, this, [&isDnsServerCalled](HttpReqRep *rr) {
-                    QVERIFY(rr->requestPath().contains(QByteArrayLiteral("/dns-query?dns=")));
-                    isDnsServerCalled = true;
-                    rr->close();
-                });
-        QVERIFY(httpsServer.start());
-        httpsServer.setExpectError(true);
-        httpsServer.setVerifyMode(QSslSocket::PeerVerifyMode::VerifyNone);
-    }
+    QObject::connect(&httpsServer, &HttpsServer::newRequest, this,
+                     [&isMockDnsServerCalled](HttpReqRep *rr) {
+                         QVERIFY(rr->requestPath().contains(QByteArrayLiteral("/dns-query?dns=")));
+                         isMockDnsServerCalled = true;
+                         rr->close();
+                     });
+    QVERIFY(httpsServer.start());
+    httpsServer.setExpectError(isMockDnsServerCalledExpected);
+    httpsServer.setVerifyMode(QSslSocket::PeerVerifyMode::VerifyNone);
 
     QWebEngineProfile profile;
     QWebEnginePage page(&profile);
@@ -87,15 +105,12 @@ void tst_QWebEngineGlobalSettings::dnsOverHttps()
     connect(&page, &QWebEnginePage::loadFinished, this,
             [&isLoadSuccessful](bool ok) { isLoadSuccessful = ok; });
 
-    QWebEngineGlobalSettings *globalSettings = QWebEngineGlobalSettings::instance();
-    globalSettings->configureDnsOverHttps(dnsMode, uriTemplate);
-
     page.load(QUrl("https://google.com/"));
-    if (!loadSpy.wait(10000)) {
+    if (!loadSpy.wait(20000)) {
         QSKIP("Couldn't load page from network, skipping test.");
     }
 
-    QTRY_COMPARE(isDnsServerCalled, isWithCustomDnsServer);
+    QTRY_COMPARE(isMockDnsServerCalled, isMockDnsServerCalledExpected);
     QCOMPARE(isLoadSuccessful, isDnsResolutionSuccessExpected);
     QVERIFY(httpsServer.stop());
 }