1509586 - ReferenceError: reference to undefined property "securityInfo" msgCompSecurityInfo.js:48:5

Status: RESOLVED FIXED

(MailNews Core :: Composition, enhancement)

Description

[:aceman]

Just opening a compose window and using View->"message security info" produces
ReferenceError: reference to undefined property "securityInfo" msgCompSecurityInfo.js:48:5

The code is:
  try {
    if (params.compFields.securityInfo.requireEncryptMessage) {
      allow_ldap_cert_fetching = true;
    }
  }
  catch (e)
  {
  }

If the attribute is optional in params.compFields, we could check for it properly.

Comment 1

[Magnus Melin [:mkmelin]]

Yes looks like just bad code

Comment 2

[:aceman]

Attachment: 1509586.patch

Looks like a typo or not migrated code, see
https://searchfox.org/comm-central/search?q=requireEncryptMessage&case=true&regexp=false&path=
and
https://searchfox.org/comm-central/source/suite/extensions/smime/content/msgCompSMIMEOverlay.js#223

Further in the msgCompSecurityInfo.js we check for params.smFields.requireEncryptMessage.

Comment 3

[:aceman]

I have tested that params.smFields.requireEncryptMessage does properly return 'false' when not composing encrypted message (I have no SMIME cert).

Comment 4

[:aceman]

Right, when the code was landed at https://bug119394.bmoattachments.org/attachment.cgi?id=87208 it had .compFields.securityInfo.requireEncryptMessage at at least 2 places.

These days, in msgCompSMIMEOverlay.js params.smFields and params.compFields.composeSecure seems to be the same, due to
https://searchfox.org/comm-central/source/mail/extensions/smime/content/msgCompSMIMEOverlay.js#38 and then https://searchfox.org/comm-central/source/mail/extensions/smime/content/msgCompSMIMEOverlay.js#229 .

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9027410 [details] [diff] [review]
1509586.patch

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

Sure, though I'd prefer if you don't declare it a all. It's only used once. There's the strange while loop which only is ever exercised once. You could remove the loop and have and do the rest in 
if (params.smFields.requireEncryptMessage)

Comment 6

[:aceman]

I have now even imported a SMIME certificate and can toggle to encrypted message and then params.smFields.requireEncryptMessage is 'true'.

I have noticed the strange loop that you say and there is more potential cleanup to do.
Can I do it in a new bug?

The fix here actually allows the following code in the loop to run when params.smFields.requireEncryptMessage is true (encrypted message) and that code tries fetch SMIME certificates for the recipients from LDAP server.
That code was dead due to the error for a long time it seems so that feature was broken.
Fortunately there is another fetch of missing certs when the message is sent so that may be why this went unnoticed.
But this is and enterprise feature which I would propose for ESR60 so the patch should be minimal.
Can we agree on that?

Comment 7

[Magnus Melin [:mkmelin]]

Sure, go ahead.

Comment 8

[:aceman]

Thanks.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fab70cf9e354
Look for requireEncryptMessage in params.smFields in msgCompSecurityInfo.js. r=mkmelin

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9027410 [details] [diff] [review]
1509586.patch

Uplift, right?

Comment 11

[Jorg K (CEST = GMT+2)]

TB 60.3.2 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/cc3a5040f78d7db669a48c77dec43e1790843684

Comment 12

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/54e10abf7960a7c7008c0f929e4ef4f1f730259a

Comment 13

[Jorg K (CEST = GMT+2)]

Beta (TB 64 beta 4):
https://hg.mozilla.org/releases/comm-beta/rev/54e10abf7960a7c7008c0f929e4ef4f1f730259a

Comment 14

[Lukas.Wringer]

Hi,

has this fix been applied in 68.* or 78.* ? As I commented in Bug #1275180 fetching S/MIME certificates from an ldap won't work...