Closed Bug 1207130 Opened 4 years ago Closed 4 years ago

Add a checkbox for reporting cert errors to aboutCertError.xhtml

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 46
Iteration:
46.3 - Jan 25
Tracking Status
firefox44 --- affected
firefox46 --- verified

People

(Reporter: past, Assigned: ttaubert)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 file, 2 obsolete files)

Based on Bram's UI spec:

http://brampitoyo.github.io/fx-untrusted-connection/

We need to add a checkbox with the label “Report errors like this to Mozilla” that ensures the error is reported back to Mozilla when checked. I assume this is going to use FHR, but if we only want to know the total count of sessions where we are hitting this, we could use telemetry, too.
Flags: qe-verify+
I suspect that either Tim or Javaun would have more context about this checkbox.
Flags: needinfo?(ttaubert)
Flags: needinfo?(jmoradi)
Priority: P1 → P3
Priority: P3 → P2
I'm talking w/ Bram, he just showed me we already have a reporting checkbox as here: https://rc4-md5.badssl.com/

Where does this one go?
Flags: needinfo?(jmoradi)
Ah, interesting. This is from aboutNetError.xhtml which, as Gijs put it, is mostly handling network errors, but also some special cases like this. Looking at the code I see that it sends a special report to https://data.mozilla.com/submit/sslreports, which AFAIK is neither FHR nor telemetry:

https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2844

One question that comes up is how is the report supposed to be triggered in the new UI? In the old one, clicking on the "Report this error" link would do it, unless automatic submission was already selected. In the event of a previously unchecked checkbox, do we interpret the user checking it as an indication to send the report right away (and also report automatically in the future)?

Another question I now have is whether we want to move the "special" errors that are currently handled by aboutNetError to aboutCertError, or is this set of bugs just an attempt to modernize the latter? If we just want to get rid of Larry and the old UI, the page referenced in comment 2 will still provide the report submission behavior for this case. Perhaps we want to report other kinds of cert errors as well (the ones that are currently being handled by aboutCertError)?
Flags: needinfo?(ttaubert)
Flags: needinfo?(jmoradi)
Flags: needinfo?(bram)
The behaviour is as follows:
* When current state is unchecked, error should be sent when clicking the checkbox (like what Panos wrote)
* When current state is checked, error should be sent when error page loads (like what Panos wrote)
* If user wants to uncheck, he has to open the error page first. But we already send a report on page load. And obviously, we cannot “unsend” what was just sent. This behaviour is fine. Unchecking means saying “The report for this page was already sent just now, but on all future reports, don’t send any”.
Flags: needinfo?(bram)
Flags: needinfo?(jmoradi)
Trying to prioritize this work against other bugs.  Richard, can you explain the importance of this bug and it's impact.  What would we do with these reports if we started receiving them in the FF 45/46 timeline.
Flags: needinfo?(rlb)
The behavior specified in comment 4 has already been implemented for about:neterror in bug 1218971. It shouldn't be too hard to reuse it for about:certerror here.
Priority: P2 → P3
Certificate error reporting is our only way of figuring out *which* secure web sites are causing problems for our users.  Without this, we just see mysterious increases in failures without any way to take action on them.

For example, we currently know that ~11.5% of TLS transactions fail, but we have no idea why (!)

Bin 3 of http://mzl.la/22thYLu
Bin 5 of http://mzl.la/1MBe2fZ

Marco: I would really like this to get done ASAP.  As Panos notes, it should be pretty much a copy/paste from aboutNetError.xhtml.
Flags: needinfo?(rlb) → needinfo?(mmucci)
Hi Richard, the team reviewed bug 1207130 this morning at our daily update.  We are currently at maximum capacity for Release 46 working on other priorities.  The earliest the bug could be prioritized, given our current progress, is Release 47.
Flags: needinfo?(mmucci)
emk, any chance you have a chance to look at this?
Flags: needinfo?(VYV03354)
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(VYV03354)
Gijs, are you a good person to review this? Let me know if you want someone else to do this :)

So what I did was mostly "copy" the basic approach taken by about:neterror, we should really spend some time soon and merge those, they're almost the same.

From the Browser:SendSSLErrorReport message and its handlers I removed the |elementId| parameter. Didn't know what to take here and saw that it's unused anyway.

I removed the |reportBtn| from about:neterror, that was nowadays permanently hidden and it seems like we don't support manual submission anymore. Either the user has automatic submission enabled and then we submit it on page load, or the user ticks off the checkbox and then we submit too.

I rewrote the whole SSL error report test to be able to handle both about:neterror and about:certerror, didn't I say they're almost the same? :)
Attachment #8707518 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8707518 [details] [diff] [review]
0001-Bug-1207130-Add-a-checkbox-for-automatic-error-repor.patch

Review of attachment 8707518 [details] [diff] [review]:
-----------------------------------------------------------------

Apologies for the delay here.

(In reply to Tim Taubert [:ttaubert] from comment #10)
> So what I did was mostly "copy" the basic approach taken by about:neterror,
> we should really spend some time soon and merge those, they're almost the
> same.

Can you file a followup bug about this?

> I rewrote the whole SSL error report test to be able to handle both
> about:neterror and about:certerror, didn't I say they're almost the same? :)

Thanks for making that test much easier to read!


It seems like there's a separate patch here about how the advanced panel behaves, while the new markup isn't part of that panel. Could that be split off into a separate change?

Generally there's enough comments that I'd like to see this again before stamping - and it might be worth asking Panos to have a look? He did bug 1207146.

::: browser/base/content/aboutcerterror/aboutCertError.xhtml
@@ +53,5 @@
>  
>        function toggleVisibility(id)
>        {
>          var node = document.getElementById(id);
> +        node.style.display = node.style.display == "none" ? "block" : "none";

This is a scary-ish change. Why was it necessary? Also, "block" will only work with some elements. Can we use "unset" or "initial" to achieve what we want, if just "" didn't cut it?


... also, you don't seem to use it yourself, you just changed how the advancedPanel works with it. What was the reason to do this? Could that be a separate, orthogonal patch?

::: browser/base/content/content.js
@@ +220,5 @@
> +    return content.document.documentURI.startsWith("about:certerror");
> +  },
> +
> +  handleEvent(event) {
> +    if (!this.isAboutCertError) {

How safe is this? Should we be checking event.isTrusted (is that true in this case?) ? Can other pages fire an event on, say, an iframe that's loaded about:certerror for a URI that's same-origin with them?

@@ +261,5 @@
> +    let automatic = Services.prefs.getBoolPref("security.ssl.errorReporting.automatic");
> +    content.dispatchEvent(new content.CustomEvent("AboutCertErrorOptions", {
> +      detail: JSON.stringify({
> +        enabled: Services.prefs.getBoolPref("security.ssl.errorReporting.enabled"),
> +        automatic: automatic

nit: automatic,

(don't repeat, and leave a trailing comma, please)

@@ +269,5 @@
> +    if (automatic) {
> +      this.onSendReport();
> +    }
> +
> +    // hide parts of the UI we don't need yet

Shouldn't these be hidden by default and shown when we need them?

@@ +299,5 @@
> +    let doc = content.document;
> +    let location = doc.location.href;
> +
> +    let serhelper = Cc["@mozilla.org/network/serialization-helper;1"]
> +                     .getService(Ci.nsISerializationHelper);

nit: indenting - either align with [ or indent 2 or 4 spaces from the start of lines in the same block.

@@ +301,5 @@
> +
> +    let serhelper = Cc["@mozilla.org/network/serialization-helper;1"]
> +                     .getService(Ci.nsISerializationHelper);
> +
> +    let serializable =  docShell.failedChannel.securityInfo

nit: wonky spacing

@@ +308,5 @@
> +
> +    let serializedSecurityInfo = serhelper.serializeToString(serializable);
> +
> +    sendAsyncMessage("Browser:SendSSLErrorReport", {
> +        documentURI: doc.documentURI,

nit: 2-space indent is used further up.

@@ +311,5 @@
> +    sendAsyncMessage("Browser:SendSSLErrorReport", {
> +        documentURI: doc.documentURI,
> +        location: {hostname: doc.location.hostname, port: doc.location.port},
> +        securityInfo: serializedSecurityInfo
> +      });

nit: align } with start of line in block.

::: browser/base/content/test/general/browser.ini
@@ -15,5 @@
>    browser_fxa_oauth.html
>    browser_fxa_oauth_with_keys.html
>    browser_fxa_web_channel.html
>    browser_registerProtocolHandler_notification.html
> -  browser_ssl_error_reports_content.js

Umm, so we're no longer using it? But it's still being changed in the diff... is that just git/hg/splinter being confused? I don't see it being removed entirely in the diff. Maybe that should be in there?

::: browser/base/content/test/general/browser_ssl_error_reports.js
@@ +3,5 @@
> +const URL_REPORTS = "https://example.com/browser/browser/base/content/test/general/ssl_error_reports.sjs?";
> +const URL_BAD_CHAIN = "https://badchain.include-subdomains.pinning.example.com";
> +const URL_NO_CERT = "https://fail-handshake.example.com";
> +const URL_BAD_CERT = "https://expired.example.com/";
> +const URL_BAD_STS_CERT = "https://badchain.include-subdomains.pinning.example.com:443";

uber-nit: some of these have trailing slashes and some not. Please make them the same.

@@ +9,5 @@
>  
>  SimpleTest.requestCompleteLog();
>  
> +Services.prefs.setBoolPref("security.ssl.errorReporting.enabled", true);
> +Services.prefs.setIntPref("security.cert_pinning.enforcement_level", 2);

*indistinct mutter about how we have no uniformity in our pref naming - camelcase right next to foo_bar naming, sigh. *

(not your fault, I know)

@@ +14,2 @@
>  
> +registerCleanupFunction(() => {

For my sanity, can we make this run cleanup() too, and pull that function up to the top?

@@ +41,3 @@
>    gBrowser.selectedTab = tab;
> +  let browser = tab.linkedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);

why not:

let tab = yield BrowserTestUtils.openNewForegroundTab("about:blank");
let browser = tab.linkedBrowser;

? :-)

@@ +73,3 @@
>    });
>  
> +  yield Promise.all([promiseReport, promiseReport]);

one of these should be promiseRetry, presumably?

@@ +82,3 @@
>  
> +function* testSetAutomatic(testURL, suffix, errorURISuffix) {
> +  Services.prefs.setBoolPref("security.ssl.errorReporting.automatic", false);

nit: stick the full pref name here in a variable, you reuse it another 3 or 4 times. :-)

@@ +89,3 @@
>    gBrowser.selectedTab = tab;
> +  let browser = tab.linkedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);

can use withNewForegroundTab here, too.

@@ +109,4 @@
>  
> +  // Check that the pref was flipped.
> +  let automatic = Services.prefs.getBoolPref("security.ssl.errorReporting.automatic");
> +  ok(automatic, "automatic SSL report submission enabled");

nit: ... whereas then in the checks, I would personally just stick the pref in the test statement, or if the check being split across lines is annoying, do:

let isAutomaticReportingEnabled = () => Services.prefs.getBoolPref(automaticReportingPref);

and then call it in the test.

@@ +129,2 @@
>    Services.prefs.setBoolPref("security.ssl.errorReporting.enabled", false);
> +  Services.prefs.setBoolPref("security.ssl.errorReporting.automatic", false);

Can we add a test that having this be true (but enabled still false) also doesn't submit anything?

@@ +135,3 @@
>    gBrowser.selectedTab = tab;
> +  let browser = tab.linkedBrowser;
> +  yield BrowserTestUtils.browserLoaded(browser);

more utility function cleanup here

::: browser/base/content/test/general/browser_ssl_error_reports_content.js
@@ -1,3 @@
> -addMessageListener("Browser:SSLErrorReportStatus", function(message) {
> -  sendSyncMessage("ssler-test:SSLErrorReportStatus", {reportStatus:message.data.reportStatus});
> -});

If we're not deleting this file outright, can you please rename the file so it doesn't have the browser_ prefix anymore? Right now it's confusing that this isn't an actual test, just a file used by a test. I realize manifests mean we don't need the conventions anymore, but IME the conventions were kind of useful... :-)

::: browser/themes/shared/aboutCertError.css
@@ +96,5 @@
>  
>  /* Advanced section is hidden via inline styles until the link is clicked */
>  #advancedPanel {
> +  position: absolute;
> +  width: calc(100% - 24px);

So I'm guessing this is because of the padding of 2 * 12px. But even then, the 24px is kind of magical... what exactly is going on here? Can we avoid the calc with box-sizing? If the 24px is because of other elements, can we use a CSS variable to make sure this doesn't get out of sync?

Also, it seems your new content isn't actually part of this container, so why change it?
Attachment #8707518 - Flags: review?(gijskruitbosch+bugs) → feedback+
(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Tim Taubert [:ttaubert] from comment #10)
> > So what I did was mostly "copy" the basic approach taken by about:neterror,
> > we should really spend some time soon and merge those, they're almost the
> > same.
> 
> Can you file a followup bug about this?

Filed bug 1240594.
(In reply to :Gijs Kruitbosch from comment #11)
> ::: browser/base/content/aboutcerterror/aboutCertError.xhtml
> @@ +53,5 @@
> >        function toggleVisibility(id)
> >        {
> >          var node = document.getElementById(id);
> > +        node.style.display = node.style.display == "none" ? "block" : "none";
> 
> This is a scary-ish change. Why was it necessary? Also, "block" will only
> work with some elements. Can we use "unset" or "initial" to achieve what we
> want, if just "" didn't cut it?

Reverted those changes. Turns out they were only necessary unless the advanced panel isn't position:absolute. (please see the end of this comment)

> ::: browser/base/content/content.js
> @@ +220,5 @@
> > +    return content.document.documentURI.startsWith("about:certerror");
> > +  },
> > +
> > +  handleEvent(event) {
> > +    if (!this.isAboutCertError) {
> 
> How safe is this? Should we be checking event.isTrusted (is that true in
> this case?) ? Can other pages fire an event on, say, an iframe that's loaded
> about:certerror for a URI that's same-origin with them?

I basically copied the approach from the AboutNetErrorListener, hoping/assuming that this would be the right way to do it. I don't think the even would be trusted here.

> @@ +269,5 @@
> > +    if (automatic) {
> > +      this.onSendReport();
> > +    }
> > +
> > +    // hide parts of the UI we don't need yet
> 
> Shouldn't these be hidden by default and shown when we need them?

Yeah... that was recklessly copied from about:neterror. Changed to hidden by CSS and unhidden by code.

> ::: browser/base/content/test/general/browser.ini
> @@ -15,5 @@
> >    browser_fxa_oauth.html
> >    browser_fxa_oauth_with_keys.html
> >    browser_fxa_web_channel.html
> >    browser_registerProtocolHandler_notification.html
> > -  browser_ssl_error_reports_content.js
> 
> Umm, so we're no longer using it? But it's still being changed in the
> diff... is that just git/hg/splinter being confused? I don't see it being
> removed entirely in the diff. Maybe that should be in there?

Yeah, I forgot to "git rm" that file, sorry.

> ::: browser/base/content/test/general/browser_ssl_error_reports.js
> @@ +73,3 @@
> >    });
> >  
> > +  yield Promise.all([promiseReport, promiseReport]);
> 
> one of these should be promiseRetry, presumably?

Oops, yeah. Thanks :)

> @@ +129,2 @@
> >    Services.prefs.setBoolPref("security.ssl.errorReporting.enabled", false);
> > +  Services.prefs.setBoolPref("security.ssl.errorReporting.automatic", false);
> 
> Can we add a test that having this be true (but enabled still false) also
> doesn't submit anything?

Added.

> ::: browser/themes/shared/aboutCertError.css
> @@ +96,5 @@
> >  
> >  /* Advanced section is hidden via inline styles until the link is clicked */
> >  #advancedPanel {
> > +  position: absolute;
> > +  width: calc(100% - 24px);
> 
> So I'm guessing this is because of the padding of 2 * 12px. But even then,
> the 24px is kind of magical... what exactly is going on here? Can we avoid
> the calc with box-sizing? If the 24px is because of other elements, can we
> use a CSS variable to make sure this doesn't get out of sync?

I'm not sure why I thought this was needed, looks fine without specifying any |width|, removed that.

> Also, it seems your new content isn't actually part of this container, so
> why change it?

Added the |position:absolute| so that the page content doesn't jump to the top when expanded, that's how about:neterror behaves/ is styled.
Attachment #8707518 - Attachment is obsolete: true
Attachment #8709167 - Flags: review?(past)
Attachment #8709167 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8709167 [details] [diff] [review]
0001-Bug-1207130-Add-a-checkbox-for-automatic-error-repor.patch, v2

Review of attachment 8709167 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming you also get r=past, and with the below addressed. :-)

::: browser/base/content/test/general/browser_ssl_error_reports.js
@@ +59,4 @@
>  
> +function* testSendReportAutomatically(testURL, suffix, errorURISuffix) {
> +  Services.prefs.setBoolPref(PREF_REPORT_AUTOMATIC, true);
> +  Services.prefs.setCharPref(PREF_REPORT_URL, URL_REPORTS + suffix);

The end of this method calls cleanup(), which also resets the _ENABLED pref, and this method gets called repeatedly, which means after the first run that pref could be false (it's only set in the global scope at the top of the file) if that were the default. I think we should probably remove the setter in the global scope and set it at the top of this function (and others that need it). As a sanity check, please check if this test still passes if you set the default value of the pref to false. :-)

@@ +78,1 @@
>    gBrowser.removeTab(tab);

I don't really understand when this needs to be yield BrowserTestUtils.removeTab(tab), and when just this will suffice. Can you explain, as you seem to have implemented BTU's implementation? :-)
Attachment #8709167 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #15)
> @@ +78,1 @@
> >    gBrowser.removeTab(tab);
> 
> I don't really understand when this needs to be yield
> BrowserTestUtils.removeTab(tab), and when just this will suffice. Can you
> explain, as you seem to have implemented BTU's implementation? :-)

Oh, that was part of making closing a tab async. So what's async is actually the pending sessionstore messages containing the data from just before it was closed. So gBrowser.removeTab() works synchronously but the <browser> will be alive just a little longer until all outstanding sessionstore messages have been delivered to the parent. You'd want to use that BTU function only if you check for any SessionStore functionality right after the tab is gone, e.g. if it shows up in the list of tabs to "Undo Close".
(In reply to :Gijs Kruitbosch from comment #15)
> ::: browser/base/content/test/general/browser_ssl_error_reports.js
> @@ +59,4 @@
> >  
> > +function* testSendReportAutomatically(testURL, suffix, errorURISuffix) {
> > +  Services.prefs.setBoolPref(PREF_REPORT_AUTOMATIC, true);
> > +  Services.prefs.setCharPref(PREF_REPORT_URL, URL_REPORTS + suffix);
> 
> The end of this method calls cleanup(), which also resets the _ENABLED pref,
> and this method gets called repeatedly, which means after the first run that
> pref could be false (it's only set in the global scope at the top of the
> file) if that were the default. I think we should probably remove the setter
> in the global scope and set it at the top of this function (and others that
> need it). As a sanity check, please check if this test still passes if you
> set the default value of the pref to false. :-)

Good call done. Passes with default value false.
r=Gijs
Attachment #8709167 - Attachment is obsolete: true
Attachment #8709167 - Flags: review?(past)
Attachment #8709348 - Flags: review?(past)
Comment on attachment 8709348 [details] [diff] [review]
0001-Bug-1207130-Add-a-checkbox-for-automatic-error-repor.patch, v3

Review of attachment 8709348 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks!
Attachment #8709348 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/65b288d5b3bb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Iteration: --- → 46.3 - Jan 25
Priority: P3 → P1
QA Contact: paul.silaghi
Is there a test page for this ?
Flags: needinfo?(past)
Not that I know of, no. I confess that I set the qe-verify+ flag without knowing where these reports can be viewed. Perhaps Tim knows?
Flags: needinfo?(past) → needinfo?(ttaubert)
Unless of course you meant "a test page to get Firefox to send such a report", in which case there are plenty in https://badssl.com (e.g. wrong.host.badssl.com).
Yeah, https://wrong.host.badssl.com/ is indeed a good test page.
Flags: needinfo?(ttaubert)
This caused a misalignment of the advanced section - http://i.imgur.com/mtJyj8E.png (similar behavior in bug 1218971)
Do we want to fix this?
Flags: needinfo?(past)
Yes, please. If you could file a followup that would be great!
Flags: needinfo?(past)
Depends on: 1243310
Couple of more observations:
1. Comparing https://wrong.host.badssl.com/ vs https://rc4.badssl.com/, on https://wrong.host.badssl.com/ I see a "report sent" notification after checking the "report errors like this" option. So, probably, a bit of consistency would be nice here.
2. The checkbox is not keyboard accessible.
Marking as verified fixed on 46.0a2 (2016-01-25) Win 7, Ubuntu 14.04, OS X 10.10.5.
Let me know if you want me to file anything from comment 28.
Status: RESOLVED → VERIFIED
(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> Couple of more observations:
> 1. Comparing https://wrong.host.badssl.com/ vs https://rc4.badssl.com/, on
> https://wrong.host.badssl.com/ I see a "report sent" notification after
> checking the "report errors like this" option. So, probably, a bit of
> consistency would be nice here.
> 2. The checkbox is not keyboard accessible.

Can you file 2 bugs for these 2 issues? Thanks!
Flags: needinfo?(paul.silaghi)
(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> 1. Comparing https://wrong.host.badssl.com/ vs https://rc4.badssl.com/, on
> https://wrong.host.badssl.com/ I see a "report sent" notification after
> checking the "report errors like this" option. So, probably, a bit of
> consistency would be nice here.

I was talking to Mark this and last week about trying to rip this UI out. It's never been implemented properly and doesn't serve a real purpose for our users. Mark, did you have a bug for that already?
Flags: needinfo?(mgoodwin)
(In reply to Tim Taubert [:ttaubert] from comment #31)
> (In reply to Paul Silaghi, QA [:pauly] from comment #28)
> Mark, did you have a bug for that already?

I do, this is happening as part of bug 1241455 - which I'm just tidying tests up for now.
Flags: needinfo?(mgoodwin)
I think that bit is only there to enforce that reporting is taking place for users that check the unchecked option. Otherwise one can wonder whether a report has been sent in this particular instance. But I can see the argument that the vast majority of users won't care about this case.
(In reply to Panos Astithas [:past] from comment #33)
> Otherwise one can wonder whether a report has been sent in this particular instance.
Agreed. IMHO, the case with the "report sent" notification is more suggestive than the case without it.
Flags: needinfo?(paul.silaghi)
Depends on: 1243353
Depends on: 1241455
(In reply to Paul Silaghi, QA [:pauly] from comment #28)
> Couple of more observations:
> 1. Comparing https://wrong.host.badssl.com/ vs https://rc4.badssl.com/, on
> https://wrong.host.badssl.com/ I see a "report sent" notification after
> checking the "report errors like this" option. So, probably, a bit of
> consistency would be nice here.

(In reply to Mark Goodwin [:mgoodwin] from comment #32)
> I do, this is happening as part of bug 1241455 - which I'm just tidying
> tests up for now.
I haven't filed anything for the first issue, I guess it's ok to consider that bug 1241455 covers this
You need to log in before you can comment on or make changes to this bug.