Closed Bug 1474820 Opened 6 years ago Closed 6 years ago

Add the 'Accept the Risk and Add Exception' Button to the new certificate error pages

Categories

(Firefox :: Security, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: trisha, Assigned: trisha, Mentored)

References

Details

Attachments

(1 file, 5 obsolete files)

The new certificate error pages should have an "Accept the Risk and Add Exception" Button instead of "Add Exception..." With this, when the user clicks the button, we get rid of exception dialogue pop-up and instead surface the certificate information below the fold, on the same page.
Assignee: nobody → guptatrisha97
Status: NEW → ASSIGNED
Blocks: 1463693
Flags: behind-pref+
Flags: behind-pref+
I'm looking at the designs in bug 1463693:

"Go back (recommended)" + "Accept the Risk and Add Exception"

The first label is currently translated as "Wróć do poprzedniej strony (zalecane)" in Polish. Now imagine the second button added to that.

Those two buttons are going to become so large in some localization that they'll take up the entire window's width. Please make sure to test the layout with very long labels.
Comment on attachment 8995822 [details]
Bug 1474820 - Add the 'Accept the Risk and Add Exception' Button to the new certificate error pages

https://reviewboard.mozilla.org/r/260158/#review267748

This doesn't actually add any exception when the user clicks the button.

::: browser/base/content/browser.js:3125
(Diff revision 1)
> -        // If the user added the exception cert, attempt to reload the page
> +          // If the user added the exception cert, attempt to reload the page
> -        if (params.exceptionAdded) {
> +          if (params.exceptionAdded) {
> -          browser.reload();
> +            browser.reload();
> -        }
> +          }
> +        } else {
> +          browser.reload();

Calling browser.reload() here isn't enough, this should add an exception in the same way that the exceptionDialog is doing: https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/security/manager/pki/resources/content/exceptionDialog.js#336
Attachment #8995822 - Flags: review?(jhofmann) → review-
Attached patch bug1474820.patch (obsolete) — Splinter Review
Attachment #8995822 - Attachment is obsolete: true
Attachment #8998163 - Flags: review?(jhofmann)
Comment on attachment 8998163 [details] [diff] [review]
bug1474820.patch

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

Can you please add a test for this (opening a bad cert page and adding an exception via clicking on the button) to https://searchfox.org/mozilla-central/source/browser/base/content/test/about/browser_aboutCertError.js?

Let me know if you need help with that!

Thanks!

::: browser/base/content/browser.js
@@ +2975,5 @@
>          sslStatus = securityInfo.SSLStatus;
>          let params = { exceptionAdded: false,
>                         sslStatus };
> +        if (Services.prefs.getBoolPref("browser.security.newcerterrorpage.enabled", false)) {
> +          var overrideService = Cc["@mozilla.org/security/certoverride;1"]

nit: please use let here and below

@@ +2987,5 @@
> +          }
> +          if (sslStatus.isNotValidAtThisTime) {
> +            flags |= overrideService.ERROR_TIME;
> +          }
> +          var uri = gBrowser.currentURI;

So, thinking about it, it might be sensible to stick with the existing behavior and use Services.uriFixup.createFixupURI like we do in exceptionDialog.js https://searchfox.org/mozilla-central/rev/f0c15db995198a1013e1c5f5b5bea54ef83f1049/security/manager/pki/resources/content/exceptionDialog.js#127, based on the "location" variable.
Attachment #8998163 - Flags: review?(jhofmann) → review-
Attached patch bug1474820.patch (obsolete) — Splinter Review
Thank you for the review, I've made the changes. This successfully ran the mochitests, though I hope the button ID looks fine?
Attachment #8998163 - Attachment is obsolete: true
Attachment #8999455 - Flags: review?(jhofmann)
Comment on attachment 8999455 [details] [diff] [review]
bug1474820.patch

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

This looks great, but I'd really like a new test for it!

Did this successfully pass try on top of the latest central (with the new error pages enabled by default)?

::: browser/base/content/browser.js
@@ +2988,5 @@
> +          if (sslStatus.isNotValidAtThisTime) {
> +            flags |= overrideService.ERROR_TIME;
> +          }
> +          let uri = Services.uriFixup.createFixupURI(location, 0);
> +          let gCert = sslStatus.serverCert;

nit: please just call this "cert" instead of "gCert"
Attachment #8999455 - Flags: review?(jhofmann)
Attached patch bug1474820.patch (obsolete) — Splinter Review
Attachment #8999455 - Attachment is obsolete: true
Attachment #9000229 - Flags: review?(jhofmann)
Comment on attachment 9000229 [details] [diff] [review]
bug1474820.patch

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

Thanks!

::: browser/base/content/test/about/browser_aboutCertError.js
@@ +85,5 @@
>  });
>  
> +add_task(async function checkExceptionDialogButton() {
> +  info("Loading a bad cert page and making sure the exceptionDialogButton directly adds an exception");
> +  for (let useFrame of [false]) {

So, this loop can be removed since adding an exception technically shouldn't be possible in frames, although I see that some weird circumstances actually made it possible to do so in the new cert error pages. I will file a bug for fixing this.

Anyway, please remove this line :)
Attachment #9000229 - Flags: review?(jhofmann) → review+
Attached patch bug1474820.patch (obsolete) — Splinter Review
Carrying r+ over from the previous patch
Attachment #9000229 - Attachment is obsolete: true
Attachment #9001868 - Flags: review+
Attached patch bug1474820.patchSplinter Review
Attachment #9001868 - Attachment is obsolete: true
Attachment #9001905 - Flags: review+
Keywords: checkin-needed
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb194e17f26
Add the 'Accept the Risk and Add Exception' Button to the new certificate error pages r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4bb194e17f26
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1484440
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: