From 9f4cd702019705baa6abe8e37590992f8031f862 Mon Sep 17 00:00:00 2001 From: Johann Hofmann Date: Mon, 15 Oct 2018 23:00:08 +0000 Subject: [PATCH] Bug 1484255 - Add Telemetry Events for the certificate error pages. r=nhnt11,keeler Differential Revision: https://phabricator.services.mozilla.com/D8281 --HG-- extra : moz-landing-system : lando --- browser/actors/NetErrorChild.jsm | 43 ++++++++ browser/app/profile/firefox.js | 2 + browser/base/content/aboutNetError-new.xhtml | 16 +-- browser/base/content/aboutNetError.xhtml | 14 +-- browser/base/content/browser.js | 12 --- browser/base/content/test/about/browser.ini | 4 +- .../test/about/browser_aboutCertError.js | 36 ------- .../about/browser_aboutCertError_telemetry.js | 102 ++++++++++++++++++ browser/base/content/test/about/head.js | 37 +++++++ browser/components/nsBrowserGlue.js | 7 ++ docshell/base/nsDocShell.cpp | 13 --- .../manager/ssl/nsISecurityUITelemetry.idl | 10 +- toolkit/components/telemetry/Events.yaml | 47 ++++++++ 13 files changed, 260 insertions(+), 83 deletions(-) create mode 100644 browser/base/content/test/about/browser_aboutCertError_telemetry.js diff --git a/browser/actors/NetErrorChild.jsm b/browser/actors/NetErrorChild.jsm index ecacca199ab6..66419a268ef4 100644 --- a/browser/actors/NetErrorChild.jsm +++ b/browser/actors/NetErrorChild.jsm @@ -14,6 +14,8 @@ ChromeUtils.defineModuleGetter(this, "BrowserUtils", ChromeUtils.defineModuleGetter(this, "WebNavigationFrames", "resource://gre/modules/WebNavigationFrames.jsm"); +XPCOMUtils.defineLazyGlobalGetters(this, ["URL"]); + XPCOMUtils.defineLazyGetter(this, "gPipNSSBundle", function() { return Services.strings.createBundle("chrome://pipnss/locale/pipnss.properties"); }); @@ -322,6 +324,7 @@ class NetErrorChild extends ActorChild { detailLink.append(input.data.codeString); detailLink.title = input.data.codeString; detailLink.id = "errorCode"; + detailLink.dataset.telemetryId = "error_code_link"; let fragment = BrowserUtils.getLocalizedFragment(doc, linkPrefix, detailLink); technicalInfo.appendChild(fragment); var errorCode = doc.getElementById("errorCode"); @@ -353,6 +356,12 @@ class NetErrorChild extends ActorChild { let errWhatToDoTitle = doc.getElementById("edd_nssBadCert"); let est = doc.getElementById("errorWhatToDoTitleText"); + doc.body.setAttribute("code", msg.data.codeString); + + // Need to do this here (which is not exactly at load but a few ticks later), + // because this is the first time we have access to the error code. + this.recordLoadEvent(doc); + switch (msg.data.code) { case SSL_ERROR_BAD_CERT_DOMAIN: case SEC_ERROR_OCSP_INVALID_SIGNING_CERT: @@ -530,11 +539,13 @@ class NetErrorChild extends ActorChild { case "click": if (aEvent.button == 0) { if (this.isAboutCertError(doc)) { + this.recordClick(aEvent.originalTarget); this.onCertError(aEvent.originalTarget, doc.defaultView); } else { this.onClick(aEvent); } } + break; } } @@ -696,6 +707,38 @@ class NetErrorChild extends ActorChild { }); } + getCSSClass(doc) { + let searchParams = new URL(doc.documentURI).searchParams; + return searchParams.get("s"); + } + + recordLoadEvent(doc) { + let cssClass = this.getCSSClass(doc); + // Telemetry values for events are max. 80 bytes. + let errorCode = doc.body.getAttribute("code").substring(0, 40); + Services.telemetry.recordEvent("security.ui.certerror", "load", "aboutcerterror", errorCode, { + "has_sts": (cssClass == "badStsCert").toString(), + "is_frame": (doc.ownerGlobal.parent != doc.ownerGlobal).toString(), + }); + } + + recordClick(element) { + let telemetryId = element.dataset.telemetryId; + if (!telemetryId) { + return; + } + let doc = element.ownerDocument; + let cssClass = this.getCSSClass(doc); + // Telemetry values for events are max. 80 bytes. + let errorCode = doc.body.getAttribute("code").substring(0, 40); + let panel = doc.getElementById("badCertAdvancedPanel"); + Services.telemetry.recordEvent("security.ui.certerror", "click", telemetryId, errorCode, { + "panel_open": (panel.style.display == "none").toString(), + "has_sts": (cssClass == "badStsCert").toString(), + "is_frame": (doc.ownerGlobal.parent != doc.ownerGlobal).toString(), + }); + } + onClick(event) { let {documentURI} = event.target.ownerDocument; diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index 42a3da0f4c82..0c64489ad6b0 100644 --- a/browser/app/profile/firefox.js +++ b/browser/app/profile/firefox.js @@ -957,6 +957,8 @@ pref("browser.security.newcerterrorpage.enabled", true); pref("browser.security.newcerterrorpage.enabled", false); #endif +pref("security.certerrors.recordEventTelemetry", true); + // Whether to start the private browsing mode at application startup pref("browser.privatebrowsing.autostart", false); diff --git a/browser/base/content/aboutNetError-new.xhtml b/browser/base/content/aboutNetError-new.xhtml index 0295acbd3d3c..a0c63bf633d0 100644 --- a/browser/base/content/aboutNetError-new.xhtml +++ b/browser/base/content/aboutNetError-new.xhtml @@ -164,7 +164,7 @@
@@ -176,10 +176,10 @@
- + - +
@@ -192,25 +192,25 @@

- +
- +

- +

- +
- +
diff --git a/browser/base/content/aboutNetError.xhtml b/browser/base/content/aboutNetError.xhtml index 4833f06f367a..67d2ecfdc46f 100644 --- a/browser/base/content/aboutNetError.xhtml +++ b/browser/base/content/aboutNetError.xhtml @@ -133,7 +133,7 @@
@@ -141,7 +141,7 @@ init for other error types .-->

- +

@@ -152,9 +152,9 @@
- + - +
@@ -170,14 +170,14 @@
- +
- +
- +
diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js index ef2d506b6604..47ae29bba53f 100644 --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -3041,15 +3041,10 @@ var BrowserOnClick = { }, onCertError(browser, elementId, isTopFrame, location, securityInfoAsString, frameId) { - let secHistogram = Services.telemetry.getHistogramById("SECURITY_UI"); let securityInfo; switch (elementId) { case "exceptionDialogButton": - if (isTopFrame) { - secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_CLICK_ADD_EXCEPTION); - } - securityInfo = getSecurityInfo(securityInfoAsString); let params = { exceptionAdded: false, securityInfo }; @@ -3098,9 +3093,6 @@ var BrowserOnClick = { break; case "returnButton": - if (isTopFrame) { - secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE); - } goBackFromErrorPage(); break; @@ -3110,10 +3102,6 @@ var BrowserOnClick = { case "advancedButton": case "moreInformationButton": - if (isTopFrame) { - secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_UNDERSTAND_RISKS); - } - securityInfo = getSecurityInfo(securityInfoAsString); let errorInfo = getDetailedCertErrorInfo(location, securityInfo); diff --git a/browser/base/content/test/about/browser.ini b/browser/base/content/test/about/browser.ini index 732e18748cde..1398872fabc4 100644 --- a/browser/base/content/test/about/browser.ini +++ b/browser/base/content/test/about/browser.ini @@ -5,10 +5,10 @@ support-files = searchSuggestionEngine.sjs searchSuggestionEngine.xml POSTSearchEngine.xml + dummy_page.html [browser_aboutCertError.js] -support-files = - dummy_page.html +[browser_aboutCertError_telemetry.js] [browser_aboutHome_search_POST.js] [browser_aboutHome_search_composing.js] [browser_aboutHome_search_searchbar.js] diff --git a/browser/base/content/test/about/browser_aboutCertError.js b/browser/base/content/test/about/browser_aboutCertError.js index ee725dc8baaa..14ede38e8dd0 100644 --- a/browser/base/content/test/about/browser_aboutCertError.js +++ b/browser/base/content/test/about/browser_aboutCertError.js @@ -7,47 +7,11 @@ const GOOD_PAGE = "https://example.com/"; const GOOD_PAGE_2 = "https://example.org/"; -const DUMMY_PAGE = getRootDirectory(gTestPath).replace("chrome://mochitests/content", GOOD_PAGE) + "dummy_page.html"; const BAD_CERT = "https://expired.example.com/"; const UNKNOWN_ISSUER = "https://self-signed.example.com "; const BAD_STS_CERT = "https://badchain.include-subdomains.pinning.example.com:443"; const {TabStateFlusher} = ChromeUtils.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {}); -function injectErrorPageFrame(tab, src) { - return ContentTask.spawn(tab.linkedBrowser, {frameSrc: src}, async function({frameSrc}) { - let loaded = ContentTaskUtils.waitForEvent(content.wrappedJSObject, "DOMFrameContentLoaded"); - let iframe = content.document.createElement("iframe"); - iframe.src = frameSrc; - content.document.body.appendChild(iframe); - await loaded; - // We will have race conditions when accessing the frame content after setting a src, - // so we can't wait for AboutNetErrorLoad. Let's wait for the certerror class to - // appear instead (which should happen at the same time as AboutNetErrorLoad). - await ContentTaskUtils.waitForCondition(() => - iframe.contentDocument.body.classList.contains("certerror")); - }); -} - -async function openErrorPage(src, useFrame) { - let tab; - if (useFrame) { - info("Loading cert error page in an iframe"); - tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, DUMMY_PAGE); - await injectErrorPageFrame(tab, src); - } else { - let certErrorLoaded; - tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, () => { - gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, src); - let browser = gBrowser.selectedBrowser; - certErrorLoaded = BrowserTestUtils.waitForErrorPage(browser); - }, false); - info("Loading and waiting for the cert error"); - await certErrorLoaded; - } - - return tab; -} - add_task(async function checkReturnToAboutHome() { info("Loading a bad cert page directly and making sure 'return to previous page' goes to about:home"); for (let useFrame of [false, true]) { diff --git a/browser/base/content/test/about/browser_aboutCertError_telemetry.js b/browser/base/content/test/about/browser_aboutCertError_telemetry.js new file mode 100644 index 000000000000..712be780e164 --- /dev/null +++ b/browser/base/content/test/about/browser_aboutCertError_telemetry.js @@ -0,0 +1,102 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +requestLongerTimeout(2); + +const BAD_CERT = "https://expired.example.com/"; +const BAD_STS_CERT = "https://badchain.include-subdomains.pinning.example.com:443"; + +add_task(async function checkTelemetryClickEvents() { + info("Loading a bad cert page and verifying telemetry click events arrive."); + + let oldCanRecord = Services.telemetry.canRecordExtended; + Services.telemetry.canRecordExtended = true; + + registerCleanupFunction(() => { + Services.telemetry.canRecordExtended = oldCanRecord; + }); + + // For obvious reasons event telemetry seems to sync with the content + // processes asynchronously, so we need to wait for the main process + // to catch up through the entire test. + Services.telemetry.clearEvents(); + await TestUtils.waitForCondition(() => { + let events = Services.telemetry.snapshotEvents( + Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true).content; + return !events || !events.length; + }); + + for (let useFrame of [false, true]) { + let recordedObjects = [ + "advanced_button", + "learn_more_link", + "auto_report_cb", + "error_code_link", + "clipboard_button_top", + "clipboard_button_bot", + "return_button_top", + "return_button_adv", + ]; + + if (!useFrame) { + recordedObjects.push("exception_button"); + } + + for (let object of recordedObjects) { + let tab = await openErrorPage(BAD_CERT, useFrame); + let browser = tab.linkedBrowser; + + let loadEvents = await TestUtils.waitForCondition(() => { + let events = Services.telemetry.snapshotEvents( + Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true).content; + if (events && events.length) { + events = events.filter(e => e[1] == "security.ui.certerror" && e[2] == "load"); + if (events.length == 1 && events[0][5].is_frame == useFrame.toString()) { + return events; + } + } + return null; + }); + + is(loadEvents.length, 1, `recorded telemetry for the load testing ${object}, useFrame: ${useFrame}`); + + await ContentTask.spawn(browser, {frame: useFrame, objectId: object}, async function({frame, objectId}) { + let doc = frame ? content.document.querySelector("iframe").contentDocument : content.document; + + await ContentTaskUtils.waitForCondition(() => doc.body.classList.contains("certerror"), "Wait for certerror to be loaded"); + + let domElement = doc.querySelector(`[data-telemetry-id='${objectId}']`); + domElement.click(); + }); + + let clickEvents = await TestUtils.waitForCondition(() => { + let events = Services.telemetry.snapshotEvents( + Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, true).content; + if (events && events.length) { + events = events.filter(e => e[1] == "security.ui.certerror" && e[2] == "click" && e[3] == object); + if (events.length == 1 && events[0][5].is_frame == useFrame.toString()) { + return events; + } + } + return null; + }, "Has captured telemetry events."); + + is(clickEvents.length, 1, `recorded telemetry for the click on ${object}, useFrame: ${useFrame}`); + + // We opened an extra tab for the SUMO page, need to close it. + if (object == "learn_more_link") { + BrowserTestUtils.removeTab(gBrowser.selectedTab); + } + + if (object == "exception_button") { + let certOverrideService = Cc["@mozilla.org/security/certoverride;1"] + .getService(Ci.nsICertOverrideService); + certOverrideService.clearValidityOverride("expired.example.com", -1); + } + + BrowserTestUtils.removeTab(gBrowser.selectedTab); + } + } +}); diff --git a/browser/base/content/test/about/head.js b/browser/base/content/test/about/head.js index 95db02e8529d..e0cdad7a2919 100644 --- a/browser/base/content/test/about/head.js +++ b/browser/base/content/test/about/head.js @@ -1,5 +1,42 @@ /* eslint-env mozilla/frame-script */ +function injectErrorPageFrame(tab, src) { + return ContentTask.spawn(tab.linkedBrowser, {frameSrc: src}, async function({frameSrc}) { + let loaded = ContentTaskUtils.waitForEvent(content.wrappedJSObject, "DOMFrameContentLoaded"); + let iframe = content.document.createElement("iframe"); + iframe.src = frameSrc; + content.document.body.appendChild(iframe); + await loaded; + // We will have race conditions when accessing the frame content after setting a src, + // so we can't wait for AboutNetErrorLoad. Let's wait for the certerror class to + // appear instead (which should happen at the same time as AboutNetErrorLoad). + await ContentTaskUtils.waitForCondition(() => + iframe.contentDocument.body.classList.contains("certerror")); + }); +} + +async function openErrorPage(src, useFrame) { + let dummyPage = getRootDirectory(gTestPath).replace("chrome://mochitests/content", "https://example.com") + "dummy_page.html"; + + let tab; + if (useFrame) { + info("Loading cert error page in an iframe"); + tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, dummyPage); + await injectErrorPageFrame(tab, src); + } else { + let certErrorLoaded; + tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, () => { + gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser, src); + let browser = gBrowser.selectedBrowser; + certErrorLoaded = BrowserTestUtils.waitForErrorPage(browser); + }, false); + info("Loading and waiting for the cert error"); + await certErrorLoaded; + } + + return tab; +} + function waitForCondition(condition, nextTest, errorMsg, retryTimes) { retryTimes = typeof retryTimes !== "undefined" ? retryTimes : 30; var tries = 0; diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js index 163abbc17422..f7e538ff3a1f 100644 --- a/browser/components/nsBrowserGlue.js +++ b/browser/components/nsBrowserGlue.js @@ -1553,6 +1553,13 @@ BrowserGlue.prototype = { ContextualIdentityService.load(); }); + Services.tm.idleDispatchToMainThread(() => { + let enableCertErrorUITelemetry = + Services.prefs.getBoolPref("security.certerrors.recordEventTelemetry", false); + Services.telemetry.setEventRecordingEnabled("security.ui.certerror", + enableCertErrorUITelemetry); + }); + // Load the Login Manager data from disk off the main thread, some time // after startup. If the data is required before this runs, for example // because a restored page contains a password field, it will be loaded on diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 7981d3dbcb3d..6263d022b9dd 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -4545,15 +4545,6 @@ nsDocShell::DisplayLoadError(nsresult aError, nsIURI* aURI, cssClass.AssignLiteral("badStsCert"); } - uint32_t bucketId; - if (isStsHost) { - // measuring STS separately allows us to measure click through - // rates easily - bucketId = nsISecurityUITelemetry::WARNING_BAD_CERT_TOP_STS; - } else { - bucketId = nsISecurityUITelemetry::WARNING_BAD_CERT_TOP; - } - // See if an alternate cert error page is registered nsAutoCString alternateErrorPage; nsresult rv = @@ -4562,10 +4553,6 @@ nsDocShell::DisplayLoadError(nsresult aError, nsIURI* aURI, if (NS_SUCCEEDED(rv)) { errorPage.Assign(alternateErrorPage); } - - if (!IsFrame() && errorPage.EqualsIgnoreCase("certerror")) { - Telemetry::Accumulate(mozilla::Telemetry::SECURITY_UI, bucketId); - } } else { error = "nssFailure2"; } diff --git a/security/manager/ssl/nsISecurityUITelemetry.idl b/security/manager/ssl/nsISecurityUITelemetry.idl index bd076a609e9f..693277fb8767 100644 --- a/security/manager/ssl/nsISecurityUITelemetry.idl +++ b/security/manager/ssl/nsISecurityUITelemetry.idl @@ -116,13 +116,13 @@ const uint32_t WARNING_GEOLOCATION_REQUEST_NEVER_SHARE = 49; // const uint32_t WARNING_PHISHING_PAGE_FRAME_GET_ME_OUT_OF_HERE = 66; // const uint32_t WARNING_PHISHING_PAGE_FRAME_IGNORE_WARNING = 67; -const uint32_t WARNING_BAD_CERT_TOP = 68; -const uint32_t WARNING_BAD_CERT_TOP_STS = 69; -const uint32_t WARNING_BAD_CERT_TOP_CLICK_ADD_EXCEPTION = 70; +//const uint32_t WARNING_BAD_CERT_TOP = 68; +//const uint32_t WARNING_BAD_CERT_TOP_STS = 69; +//const uint32_t WARNING_BAD_CERT_TOP_CLICK_ADD_EXCEPTION = 70; const uint32_t WARNING_BAD_CERT_TOP_CLICK_VIEW_CERT = 71; const uint32_t WARNING_BAD_CERT_TOP_DONT_REMEMBER_EXCEPTION = 72; -const uint32_t WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE = 73; -const uint32_t WARNING_BAD_CERT_TOP_UNDERSTAND_RISKS = 74; +//const uint32_t WARNING_BAD_CERT_TOP_GET_ME_OUT_OF_HERE = 73; +//const uint32_t WARNING_BAD_CERT_TOP_UNDERSTAND_RISKS = 74; // removed WARNING_BAD_CERT_TOP_TECHNICAL_DETAILS = 75; const uint32_t WARNING_BAD_CERT_TOP_ADD_EXCEPTION_BASE = 76; diff --git a/toolkit/components/telemetry/Events.yaml b/toolkit/components/telemetry/Events.yaml index 5ea9bb371810..82829fc63355 100644 --- a/toolkit/components/telemetry/Events.yaml +++ b/toolkit/components/telemetry/Events.yaml @@ -552,6 +552,53 @@ devtools.main: os: The OS name and version e.g. "Linux 4.4.0-1014-aws", "Darwin 14.5.0", "Windows_NT 6.1.7601" or "Windows_NT 10.0.15063." This can be used to make sense of data when a feature is only available from a particular operating system build number. session_id: The start time of the session in milliseconds since epoch (Unix Timestamp) e.g. 1396381378123. +security.ui.certerror: + load: + objects: ["aboutcerterror"] + bug_numbers: + - 1484255 + description: > + The about:certerror page is loaded, keyed by error code, see https://searchfox.org/mozilla-central/source/security/nss/lib/mozpkix/include/pkix/Result.h + expiry_version: "70" + notification_emails: + - jhofmann@mozilla.com + - seceng-telemetry@mozilla.com + release_channel_collection: opt-in + record_in_processes: ["content"] + products: + - firefox + extra_keys: + is_frame: If the error page is loaded in an iframe. + has_sts: If the error page is for a site with HSTS headers or with a pinned key. + click: + objects: [ + "advanced_button", + "exception_button", + "return_button_top", + "return_button_adv", + "learn_more_link", + "auto_report_cb", + "error_code_link", + "clipboard_button_top", + "clipboard_button_bot", + ] + bug_numbers: + - 1484255 + description: > + User interaction by click events on the cert error page. Keyed by error code, see https://searchfox.org/mozilla-central/source/security/nss/lib/mozpkix/include/pkix/Result.h + expiry_version: "70" + notification_emails: + - jhofmann@mozilla.com + - seceng-telemetry@mozilla.com + release_channel_collection: opt-in + record_in_processes: ["content"] + products: + - firefox + extra_keys: + is_frame: If the error page is loaded in an iframe. + has_sts: If the error page is for a site with HSTS headers or with a pinned key. + panel_open: If the advanced panel was open at the time of the interaction. + security.ui.identitypopup: open: objects: ["identity_popup"]