1608894 - use the new certificate viewer in thunderbird (about:certificate?cert=)

Status: RESOLVED FIXED

(Thunderbird :: Security, enhancement, P1)

Description

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

The new certificate viewer (see bug 1553524) shipped in Firefox 71. Thunderbird should switch over to using it so the old implementation can be removed.

From what I can tell, the old implementation is referenced in two places:

https://searchfox.org/comm-central/rev/87456630f1a92870f0c03a404def84207ef50c95/mailnews/extensions/smime/content/msgReadSecurityInfo.js#219
https://searchfox.org/comm-central/rev/87456630f1a92870f0c03a404def84207ef50c95/mailnews/extensions/smime/content/msgCompSecurityInfo.js#284

It should be possible to change those along the lines of:

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/browser/actors/NetErrorParent.jsm#294-304

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: WIP-Bug-1608894_new-certificate-viewer-in-thunderbird.patch

I need some feedback. I have a few questions:

1. If you open the certificate, you have two options: PEM(Cert) and PEM(Chain) to download the certificate. It's <html:a> component with href="data;<data>" and when we click, nothing is happening. What do we need to keep in mind while working on the <html:a> in the Thunderbird?
2. critcal.svg icon is not loading up. I have tried to override the icon with the messenger one. But it's not loading up. Do we need to take care of anything while overriding?

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9148075 [details] [diff] [review]
WIP-Bug-1608894_new-certificate-viewer-in-thunderbird.patch

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

data uris should be able to be loaded into a tab

For the override, I guess it wont work because it's in browser and not something already "known"? I guess if the certificate viewer is using a browser uri that's basically wrong.

::: mailnews/extensions/smime/content/msgReadSecurityInfo.js
@@ +228,5 @@
> +      "_blank",
> +      "centerscreen,chrome,titlebar",
> +      cert
> +    );
> +  }

you should remove the old way, as once we've removed our usage they will remove that

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: Bug-1608894_new-certificate-viewer-in-thunderbird-0.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9149441 [details] [diff] [review]
Bug-1608894_new-certificate-viewer-in-thunderbird-0.patch

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

You should set security.aboutcertificate.enabled true

::: mail/base/content/specialTabs.js
@@ +25,5 @@
> +  picker.init(mail3PaneWindow, fileName, Ci.nsIFilePicker.modeSave);
> +  picker.defaultString = fileName;
> +  picker.defaultExtension = "pem";
> +  picker.appendFilters(Ci.nsIFilePicker.filterAll);
> +  let filePickerResult = await new Promise(resolve => {

This should be right, but I think it would be easier to read without a promise. Like https://searchfox.org/comm-central/rev/41733433edd6a3a2d0369ce03eb4fab62bf38c3b/mailnews/extensions/newsblog/content/feed-subscriptions.js#2518-2526

@@ +412,5 @@
>      },
>  
>      // Other about:* pages.
>      function(aDocument, aTab) {
> +      if (aDocument.URL.startsWith("about:certificate")) {

you should instead map it above, in inContentWhitelist. 
(if you put it in index 3, then the implementaton should be at index 3 here)

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1608894_new-certificate-viewer-in-thunderbird-1.patch

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9149649 [details] [diff] [review]
Bug-1608894_new-certificate-viewer-in-thunderbird-1.patch

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

If I open the certificate manager and try to click open a certificate, all I get is
JavaScript error: chrome://pippki/content/pippki.js, line 46: TypeError: can't access property "switchToTabHavingURI", win is nul

::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
@@ +266,5 @@
>    );
>  }
>  
>  function viewCertHelper(parent, cert) {
> +  if (Services.prefs.getBoolPref("security.aboutcertificate.enabled")) {

You can remove these checks. I only meant to add the actual pref since it seems to affect things in toolkit.

::: mailnews/extensions/smime/content/msgReadSecurityInfo.js
@@ +214,3 @@
>  
>  function viewCertHelper(parent, cert) {
> +  if (Services.prefs.getBoolPref("security.aboutcertificate.enabled")) {

remove

Comment 7

[Khushil Mistry [:khushil324]]

This part is coming from the Mozilla Central pippki.js: https://searchfox.org/mozilla-central/source/security/manager/pki/resources/content/pippki.js#33-50
How can we override this part?

Comment 8

[Magnus Melin [:mkmelin]]

I've got a patch to fix that. Will upload once I got it fully tested.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: Bug 1608894 - use getMostRecentWindow to find mainwindow, and hook that up for Thunderbird too. r=smaug

Comment 10

[Magnus Melin [:mkmelin]]

With the mozilla-central patch above we get the right window.
The Thunderbird patch needs to also add handling to open a tab for this case at https://searchfox.org/comm-central/rev/cfa832291ae06d6b814f8a9ebe1991a08f25da92/mail/base/content/contentAreaClick.js#256

Something like

     document.getElementById("tabmail").openTab("contentTab", {
      contentPage: url,
      clickHandler: "specialTabs.aboutClickHandler(event);",
    });
``

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: Bug-1608894_new-certificate-viewer-in-thunderbird-2.patch

Comment 12

[Khushil Mistry [:khushil324]]

We have a problem with icon critcal.svg that we need to keep in my mind and ask the M-C developers to change it so that we can override it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ca84f6daedd
use getMostRecentWindow to find mainwindow, and hook that up for Thunderbird too. r=smaug

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6ca84f6daedd

Comment 15

[Magnus Melin [:mkmelin]]

Reopening until we land the Thunderbird part.

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 9150362 [details] [diff] [review]
Bug-1608894_new-certificate-viewer-in-thunderbird-2.patch

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

This seems to be working now. r=mkmelin

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3d86054fed85
use the new certificate viewer in thunderbird (about:certificate?cert=). r=mkmelin