Closed Bug 1633665 Opened 9 months ago Closed 9 months ago

Broken list of recipient S/MIME certificates in composer "view / message security info"

Categories

(MailNews Core :: Security: S/MIME, defect)

defect

Tracking

(thunderbird_esr68 unaffected)

RESOLVED FIXED
Thunderbird 77.0
Tracking Status
thunderbird_esr68 --- unaffected

People

(Reporter: KaiE, Assigned: mkmelin)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

In a composer window, with S/MIME configured, and recipients added, click menu view / message security info (or click the security button).

The listbox should show the list of all recipients and their certificate status.

This worked in TB 68, but it's broken in nightly. I think it was regressed by bug 1594896.

This fixes it, and cleans up a bunch of code.
On a side note, checking those certificates is S L O W! Like 1 sec per address. But it works now.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #9143978 - Flags: review?(benc)
Comment on attachment 9143978 [details] [diff] [review]
bug1633665_smime_sec_info.patch

Thanks Magnus, I've tested it locally and it looks good.
Attachment #9143978 - Flags: feedback+
Comment on attachment 9143978 [details] [diff] [review]
bug1633665_smime_sec_info.patch

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

All looks good to me, barring the issue in nsSMimeJSHelper.cpp.
(All my other comments are just sanity checks and nits)

::: mail/components/compose/content/MsgComposeCommands.js
@@ -4023,2 @@
>        .createInstance(Ci.nsISMimeJSHelper)
> -      .getNoCertAddresses(gMsgCompose.compFields, emailAddresses);

I don't understand how this ever worked. I'm assuming there's some magic going on in the xpidl JS bindings to return a value using the extra parameter. In any case your change makes it way clearer.

@@ +4025,5 @@
>    } catch (e) {
>      return;
>    }
>  
> +  if (emailAddresses.length === 0) {

Seeing === here makes me think I'm missing something. Would == do?

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +1888,3 @@
>          .createInstance(Ci.nsISMimeJSHelper)
> +        .getNoCertAddresses(gMsgCompose.compFields);
> +      return addresses.length === 0;

Again, === instead of == makes me think I'm missing some JS subtlety here...

::: mailnews/extensions/smime/content/msgCompSecurityInfo.js
@@ +34,4 @@
>  
>    while (true) {
>      try {
> +      // Out parameters - must be objects.

I had to look up the XPIDL docs to confirm this. Ugh, that is cumbersome.
Never thought I'd see the C++ side more ergonomic than JS :-)
Good reason to avoid out and inout params in idl files wherever possible.

@@ +102,5 @@
> +  let signedElement = document.getElementById("signed");
> +  let encryptedElement = document.getElementById("encrypted");
> +  if (params.smFields.requireEncryptMessage) {
> +    if (params.isEncryptionCertAvailable && canEncrypt) {
> +      encryptedElement.value = gBundle.getString("StatusYes");

I assume we're no longer worried about gBundle being unset?

::: mailnews/extensions/smime/src/nsSMimeJSHelper.cpp
@@ +46,5 @@
>    certs.SetCapacity(mailbox_count);
>  
>    nsCOMPtr<nsIX509CertDB> certdb = do_GetService(NS_X509CERTDB_CONTRACTID);
>  
> +  *canEncrypt = emailAddresses.IsEmpty();

SetCapacity doesn't change the size, so emailAddresses is always empty at this point (and isn't it backwards? don't you want the result to be false if there aren't any addresses?).

*canEncrypt = (mailbox_count > 0) ? true : false;

maybe?
Attachment #9143978 - Flags: review?(benc) → review+

(In reply to Ben Campbell from comment #3)

  • if (emailAddresses.length === 0) {

Seeing === here makes me think I'm missing something. Would == do?

== would work too.

I assume we're no longer worried about gBundle being unset?

Yes, it would always be set.

*canEncrypt = (mailbox_count > 0) ? true : false;

Ah, good catch! I'll fix it.

Or well, that should be initialized to canEncrypt = true. We "can encrypt" to 0 recipients.

Target Milestone: --- → Thunderbird 77.0

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/e1fe33ee1a8b
fix (and code cleanup) for broken list of recipient S/MIME certificates in composer "view / message security info". r=benc DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.