From 0969c40fb8e5e6c21c279561cdf7624237e902db Mon Sep 17 00:00:00 2001 From: Valentin Gosu Date: Tue, 6 Apr 2021 13:18:31 +0000 Subject: [PATCH] Bug 1699691 - Add more confirmation tests r=necko-reviewers,dragana Also makes sure not to trigger two confirmations when the URI pref changes. Differential Revision: https://phabricator.services.mozilla.com/D109620 --- netwerk/dns/TRRService.cpp | 16 +- netwerk/test/unit/head_trr.js | 3 + netwerk/test/unit/test_trr_confirmation.js | 276 +++++++++++++++------ 3 files changed, 222 insertions(+), 73 deletions(-) diff --git a/netwerk/dns/TRRService.cpp b/netwerk/dns/TRRService.cpp index 180f08498b65..94df4ab0cc74 100644 --- a/netwerk/dns/TRRService.cpp +++ b/netwerk/dns/TRRService.cpp @@ -224,8 +224,12 @@ bool TRRService::GetParentalControlEnabledInternal() { } void TRRService::SetDetectedTrrURI(const nsACString& aURI) { + LOG(("SetDetectedTrrURI(%s", nsPromiseFlatCString(aURI).get())); // If the user has set a custom URI then we don't want to override that. + // If the URI is set via doh-rollout.uri, mURIPrefHasUserValue will be false + // (see TRRServiceBase::OnTRRURIChange) if (mURIPrefHasUserValue) { + LOG(("Already has user value. Not setting URI")); return; } @@ -286,8 +290,9 @@ void TRRService::GetPrefBranch(nsIPrefBranch** result) { bool TRRService::MaybeSetPrivateURI(const nsACString& aURI) { bool clearCache = false; nsAutoCString newURI(aURI); - ProcessURITemplate(newURI); + LOG(("MaybeSetPrivateURI(%s)", newURI.get())); + ProcessURITemplate(newURI); { MutexAutoLock lock(mLock); if (mPrivateURI.Equals(newURI)) { @@ -586,9 +591,16 @@ TRRService::Observe(nsISupports* aSubject, const char* aTopic, MOZ_ASSERT(NS_IsMainThread(), "wrong thread"); LOG(("TRR::Observe() topic=%s\n", aTopic)); if (!strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) { + TRR* prevConf = mConfirmation.mTask; + ReadPrefs(NS_ConvertUTF16toUTF8(aData).get()); mConfirmation.RecordEvent("pref-change"); - HandleConfirmationEvent(ConfirmationEvent::PrefChange); + + // We should only trigger a new confirmation if reading the prefs didn't + // already trigger one. + if (prevConf == mConfirmation.mTask) { + HandleConfirmationEvent(ConfirmationEvent::PrefChange); + } } else if (!strcmp(aTopic, kOpenCaptivePortalLoginEvent)) { // We are in a captive portal LOG(("TRRservice in captive portal\n")); diff --git a/netwerk/test/unit/head_trr.js b/netwerk/test/unit/head_trr.js index a6c1f79ef572..f11db4c483c3 100644 --- a/netwerk/test/unit/head_trr.js +++ b/netwerk/test/unit/head_trr.js @@ -333,6 +333,9 @@ function trrQueryHandler(req, resp, url) { global.dns_query_counts[domain][type] + 1 || 1; let flags = global.dnsPacket.RECURSION_DESIRED; + if (!response.answers && !response.flags) { + flags |= 2; // SERVFAIL + } flags |= response.flags || 0; let buf = global.dnsPacket.encode({ type: "response", diff --git a/netwerk/test/unit/test_trr_confirmation.js b/netwerk/test/unit/test_trr_confirmation.js index 3aaf878ec3f7..b6a4a5cb6e92 100644 --- a/netwerk/test/unit/test_trr_confirmation.js +++ b/netwerk/test/unit/test_trr_confirmation.js @@ -41,8 +41,61 @@ add_task(async function start_trr_server() { }); await trrServer.start(); dump(`port = ${trrServer.port}\n`); + + await trrServer.registerDoHAnswers(`faily.com`, "NS", { + answers: [ + { + name: "faily.com", + ttl: 55, + type: "NS", + flush: false, + data: "ns.faily.com", + }, + ], + }); + + for (let i = 0; i < 15; i++) { + await trrServer.registerDoHAnswers(`failing-domain${i}.faily.com`, "A", { + error: 600, + }); + await trrServer.registerDoHAnswers(`failing-domain${i}.faily.com`, "AAAA", { + error: 600, + }); + } }); +function trigger15Failures() { + // We need to clear the cache in case a previous call to this method + // put the results in the DNS cache. + dns.clearCache(true); + + let dnsRequests = []; + for (let i = 0; i < 15; i++) { + dnsRequests.push( + new TRRDNSListener(`failing-domain${i}.faily.com`, { + expectedAnswer: "127.0.0.1", + }) + ); + } + + return Promise.all(dnsRequests); +} + +async function registerNS(delay) { + return trrServer.registerDoHAnswers("confirm.example.com", "NS", { + answers: [ + { + name: "confirm.example.com", + ttl: 55, + type: "NS", + flush: false, + data: "test.com", + }, + ], + delay, + }); +} + add_task(async function confirm_off() { Services.prefs.setCharPref( "network.trr.confirmationNS", @@ -75,17 +128,7 @@ add_task(async function confirm_ok() { "network.trr.confirmationNS", "confirm.example.com" ); - await trrServer.registerDoHAnswers("confirm.example.com", "NS", { - answers: [ - { - name: "confirm.example.com", - ttl: 55, - type: "NS", - flush: false, - data: "test.com", - }, - ], - }); + await registerNS(0); await trrServer.registerDoHAnswers("example.com", "A", { answers: [ { @@ -99,7 +142,7 @@ add_task(async function confirm_ok() { }); Services.prefs.setCharPref( "network.trr.uri", - `https://foo.example.com:${trrServer.port}/dns-query` // No server on this port + `https://foo.example.com:${trrServer.port}/dns-query` ); Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST); equal( @@ -111,18 +154,7 @@ add_task(async function confirm_ok() { equal(await trrServer.requestCount("example.com", "A"), 1); await waitForConfirmationState(CONFIRM_OK, 1000); - await trrServer.registerDoHAnswers("confirm.example.com", "NS", { - answers: [ - { - name: "confirm.example.com", - ttl: 55, - type: "NS", - flush: false, - data: "test.com", - }, - ], - delay: 500, - }); + await registerNS(500); Services.prefs.setIntPref( "network.trr.mode", Ci.nsIDNSService.MODE_NATIVEONLY @@ -148,18 +180,7 @@ add_task(async function confirm_timeout() { Ci.nsIDNSService.MODE_NATIVEONLY ); equal(dns.currentTrrConfirmationState, CONFIRM_OFF); - await trrServer.registerDoHAnswers("confirm.example.com", "NS", { - answers: [ - { - name: "confirm.example.com", - ttl: 55, - type: "NS", - flush: false, - data: "test.com", - }, - ], - delay: 7000, - }); + await registerNS(7000); Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST); equal( dns.currentTrrConfirmationState, @@ -196,18 +217,8 @@ add_task(async function multiple_failures() { Ci.nsIDNSService.MODE_NATIVEONLY ); equal(dns.currentTrrConfirmationState, CONFIRM_OFF); - await trrServer.registerDoHAnswers("confirm.example.com", "NS", { - answers: [ - { - name: "confirm.example.com", - ttl: 55, - type: "NS", - flush: false, - data: "test.com", - }, - ], - delay: 100, - }); + + await registerNS(100); Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST); equal( dns.currentTrrConfirmationState, @@ -215,27 +226,150 @@ add_task(async function multiple_failures() { "Should be CONFIRM_TRYING_OK" ); await waitForConfirmationState(CONFIRM_OK, 1000); - - for (let i = 0; i < 15; i++) { - await trrServer.registerDoHAnswers(`domain${i}.example.com`, "A", { - error: 600, - }); - await trrServer.registerDoHAnswers(`domain${i}.example.com`, "AAAA", { - error: 600, - }); - } - - let p = waitForConfirmationState(CONFIRM_TRYING_OK, 3000); - let dnsRequests = []; - for (let i = 0; i < 15; i++) { - dnsRequests.push( - new TRRDNSListener(`domain${i}.example.com`, { - expectedAnswer: "127.0.0.1", - }) - ); - } - - await p; - await Promise.all(dnsRequests); - await waitForConfirmationState(CONFIRM_OK, 1000); + await registerNS(4000); + let failures = trigger15Failures(); + await waitForConfirmationState(CONFIRM_TRYING_OK, 3000); + await failures; + // Check that failures during confirmation are ignored. + await trigger15Failures(); + equal( + dns.currentTrrConfirmationState, + CONFIRM_TRYING_OK, + "Should be CONFIRM_TRYING_OK" + ); + await waitForConfirmationState(CONFIRM_OK, 4500); +}); + +add_task(async function test_connectivity_change() { + await registerNS(100); + Services.prefs.setIntPref( + "network.trr.mode", + Ci.nsIDNSService.MODE_NATIVEONLY + ); + let confirmationCount = await trrServer.requestCount( + "confirm.example.com", + "NS" + ); + Services.prefs.setIntPref("network.trr.mode", Ci.nsIDNSService.MODE_TRRFIRST); + equal( + dns.currentTrrConfirmationState, + CONFIRM_TRYING_OK, + "Should be CONFIRM_TRYING_OK" + ); + await waitForConfirmationState(CONFIRM_OK, 1000); + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + 1 + ); + Services.obs.notifyObservers( + null, + "network:captive-portal-connectivity", + "clear" + ); + + // The state was already OK. Should still be OK after the event + await waitForConfirmationState(CONFIRM_OK, 1000); + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + 1 + ); + + await registerNS(500); + let failures = trigger15Failures(); + // The failure should cause us to go into CONFIRM_TRYING_OK and do an NS req + await waitForConfirmationState(CONFIRM_TRYING_OK, 3000); + await failures; + Services.obs.notifyObservers( + null, + "network:captive-portal-connectivity", + "clear" + ); + // The notification should cause us to send a new confirmation request + equal( + dns.currentTrrConfirmationState, + CONFIRM_TRYING_OK, + "Should be CONFIRM_TRYING_OK" + ); + await waitForConfirmationState(CONFIRM_OK, 1000); + // two extra confirmation events should have been received by the server + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + 3 + ); +}); + +add_task(async function test_network_change() { + let confirmationCount = await trrServer.requestCount( + "confirm.example.com", + "NS" + ); + equal(dns.currentTrrConfirmationState, CONFIRM_OK); + + Services.obs.notifyObservers(null, "network:link-status-changed", "up"); + equal(dns.currentTrrConfirmationState, CONFIRM_OK); + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + ); + + let failures = trigger15Failures(); + // The failure should cause us to go into CONFIRM_TRYING_OK and do an NS req + await waitForConfirmationState(CONFIRM_TRYING_OK, 3000); + await failures; + // The network up event should reset the confirmation to TRYING_OK and do + // another NS req + Services.obs.notifyObservers(null, "network:link-status-changed", "up"); + equal(dns.currentTrrConfirmationState, CONFIRM_TRYING_OK); + await waitForConfirmationState(CONFIRM_OK, 1000); + // two extra confirmation events should have been received by the server + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + 2 + ); +}); + +add_task(async function test_uri_pref_change() { + let confirmationCount = await trrServer.requestCount( + "confirm.example.com", + "NS" + ); + equal(dns.currentTrrConfirmationState, CONFIRM_OK); + Services.prefs.setCharPref( + "network.trr.uri", + `https://foo.example.com:${trrServer.port}/dns-query?changed` + ); + equal(dns.currentTrrConfirmationState, CONFIRM_TRYING_OK); + await waitForConfirmationState(CONFIRM_OK, 1000); + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + 1 + ); +}); + +add_task(async function test_autodetected_uri() { + const defaultPrefBranch = Services.prefs.getDefaultBranch(""); + let defaultURI = defaultPrefBranch.getCharPref("network.trr.uri"); + defaultPrefBranch.setCharPref( + "network.trr.uri", + `https://foo.example.com:${trrServer.port}/dns-query?changed` + ); + // For setDetectedTrrURI to work we must pretend we are using the default. + Services.prefs.clearUserPref("network.trr.uri"); + await waitForConfirmationState(CONFIRM_OK, 1000); + let confirmationCount = await trrServer.requestCount( + "confirm.example.com", + "NS" + ); + dns.setDetectedTrrURI( + `https://foo.example.com:${trrServer.port}/dns-query?changed2` + ); + equal(dns.currentTrrConfirmationState, CONFIRM_TRYING_OK); + await waitForConfirmationState(CONFIRM_OK, 1000); + equal( + await trrServer.requestCount("confirm.example.com", "NS"), + confirmationCount + 1 + ); + + // reset the default URI + defaultPrefBranch.setCharPref("network.trr.uri", defaultURI); });