137071 - [UE] Replies to or forwarding S/MIME encrypted mail should default to encrypt

Status: VERIFIED FIXED

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

Description

[Dave Roberts]

When a message is received that was encrypted using S/MIME, any reply to that
message should also default to "Always encrypt" or "Encrypt if possible". 
Currently this has to be set manually.

This was the normal action for Communicator 4, and is also true of other mail
clients.  I consider this fairly important as it could lead a user to respond in
plaintext.

Comment 1

[John Unruh]

Looks like a dupe of bug 29753

Comment 2

[Dave Roberts]

I don't believe it is.  29753 is to do with replying to multiple recipients
where a certificate for one of them is not held.

This bug is to do with the default setting of the UI being "No Encryption" when
you hit reply.

Comment 3

[Charles Rosendahl]

Grouping related bugs.  These bugs, whether it be compose, reply, forward,
single/multiple recipients, need a whenever possible option for encryption.

Additionally, 4.7x and OE allow the user the option to send the message
unencrypted if one of the intended recipients does not have a cert available to
the sender at the time of composition.

The interaction should be:
under mail&newsgroup Account settings:
- never, whenever possible, always  (currently only never, always)

within composition:
Upon sending the message, a check, then dialog (for whenever possible/always
options)
-list of people missing certs (instead of the current one at a time dialogs)
-option to send unencrypted

Comment 4

[Charles Rosendahl]

Additionally, there are some workflow/logistics that need to be defined in the
algorithms as pointed out in the defect.  In addition to set preferences, if a
message comes in signed and encrypted, then replies should be encrypted as well.

Forwarding follows a different, yet similar path, depending on the nature of the
message being forwarded.

Comment 5

[lchiang]

*** Bug 140163 has been marked as a duplicate of this bug. ***

Comment 6

[Charles Rosendahl]

regression from 4.7x functionality. - added regression keyword.  Raising
severity (not priority) as this will be a common complaint of users using smime
capabilities.

Comment 7

[Stephane Saux]

kai

Comment 8

[Kai Engert (:KaiE:)]

I believe it is not a regression.

With 4.7x, if your default is to send non-encrypted, when replying to an
encrypted message, the new compose window will not have the encryption option
enabled. I just tested.

Comment 9

[Kai Engert (:KaiE:)]

There is an exception to this bug.
            =========

Imagine you have received an encrypted message, but you are unable to decrypt.

Maybe it's an archived message, and you don't have the private key anymore.
Maybe somebody else is still using an old certificate to encrypt, for which you
lost the matching private key.
Maybe you are reading the message on a machine, where you don't have your
private key installed.

In all those situations, you will see a broken encryption icon.
You will not see the message contents.

The user might want to reply, maybe telling the other person, reading the
message was not possible.

What should happen? Some time back, Stephane suggested, sending encrypted is not
required in that situation.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Suggested Implementation

Comment 11

[David :Bienvenu]

adding myself and mscott to cc list. I'd like mscott to review, if he can, since
he's much more familiar with this code. The patch is large, but it's not as
scary as it looks. I think (blasphemy coming up) that using pldhash is a bit of
an overkill, because we're hardly ever going to have more than one entry to keep
track of so a simpler data structure might be the way to go.

Comment 12

[Jean-Francois Ducarroz]

Comment on attachment 88040 [details] [diff] [review]
Suggested Implementation

in msgCompSMIMEOverlay.js:
gEncryptedURIService is not checked for null before beeing used! that could
result is the abort of the JS and potentially miss imortant stuff.

Apart that, looks good. R=ducarroz

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Patch v2

This patch adds the null check, as requested. Thanks for catching this!

Comment 14

[Kai Engert (:KaiE:)]

Comment on attachment 88155 [details] [diff] [review]
Patch v2

carrying forward r=ducarroz

Comment 15

[Scott MacGregor]

Comment on attachment 88155 [details] [diff] [review]
Patch v2

In msgHdrViewSMIMEOverlay.js,
the variable gMyLastEncryptedURI seems superfluous, we only use it in one
place...

you could probably just do:
gEncryptedURIService.rememberEncrypted(GetLoadedMessage());
then get rid of gMyLastEncryptedURI.

David mentioned that the hash table seems like overkill in
nsEncryptedsMIMEURIsService, I tend to agree. Most users only have a single
3-pane open which means there would be only a single entry in this table. Even
if a user has multiple 3-panes and multiple stand alone windows open, we are
still talking about 3 or 4 entries IMHO. 

I bet it would simplify your patch quite a bit from an ADT point of view if you
just used a void array of msg URIs.

Other than that, everything looks good.

Comment 16

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Ok, you convinced me to use a simpler datastructure.

I assume you recommended nsCStringArray (not nsVoidArray), because I have to
find the entries by string.

I'm attaching a new patch. The only change is the implementation of the
service, which is now much simpler.

While the code is now shorter, it is a little bit slower, if I understand
correclty, because every time we insert, remove or lookup, we create an
additional copy of the input string. That is necessary because the type used by
nsCStringArray is incompatible with the type used when defining the interface
to use AUTF8String - this type was suggested to be used for URIs by dmose.

The new patch is 125 lines shorter.

Comment 17

[Jean-Francois Ducarroz]

Comment on attachment 88185 [details] [diff] [review]
Patch v3

r=ducarroz

Comment 18

[Scott MacGregor]

Comment on attachment 88185 [details] [diff] [review]
Patch v3

sr=mscott

Comment 19

[Kai Engert (:KaiE:)]

Checked in to trunk, marking fixed.

I did not check in the patch exactly as reviewed. Before I checked in, I
realized that one filename was too long for Mac.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: Patch v4, slightly corrected version for Mac compatibility

Comment 21

[Kai Engert (:KaiE:)]

Comment on attachment 88302 [details] [diff] [review]
Patch v4, slightly corrected version for Mac compatibility

carrying forward r=ducarroz and sr=mscott

Comment 22

[Charles Rosendahl]

Looking at the patch, this bug only applies to replies to encrypted messages,
not forwarding, correct?

Comment 23

[Kai Engert (:KaiE:)]

No, it should also work with forwarding. Both when replying and forwarding, the
URI of the original message gets passed on to the new compose window.

Comment 24

[Kai Engert (:KaiE:)]

Reopening bug. Unfortunately, the patch does not yet cover the case "forward
inline".

Comment 25

[Charles Rosendahl]

Case #0:
Forwarding encrypted messages is not defaulting to encrypt when the forward
message is inline.

Case #1:
Hitting reply all to the encrypted message defaults to encrypt.  Sending
succeeds.  However, none of the recipients are able to read the message (broken
key icon).

I created a message and sent it to John, Jimmy, and myself encrypted
I deleted their certs after sending the orignial message and restarted the
browser bfore replying.
I replied all to the message.  The default was set to encrypt. 
The message seemed to go out successfully encrypted.
They both replied back that they were not able to decrypt the message.

Case #2:
I did not have a recipient cert for Kaie in my Cert Manager.
I created an encrypted mail message to myself.
I replied all to the message.
I added Kaie to the list of recipients (in this order: carosendahl,kaie)
the message sent successfully encrypted (supposedly).

However, If I hit reply all to the message, made kaie (invalid recipient) the
first user in the address list, the failure is detected and the message is not
sent. (in this order: kaie, carosendahl)

Case #1 and #2 probably need a separate bug filed against it so that we can fix
it should it exist in the branch now as well, since I'm not sure if this is the
patch that caused this problem.

Comment 26

[Kai Engert (:KaiE:)]

Attachment: Additional patch

This patch is not from me, it is from ducarroz, who was kind enough to write it
for me.

The problem is: When forwarding inline, the "original message uri" param does
not get set. But the other patch in 137071 requires it to be set.

This patch sets the param for inline forwarded messages, too.

Comment 27

[Kai Engert (:KaiE:)]

Comment on attachment 88571 [details] [diff] [review]
Additional patch

I tested this patch. It works and fixes the forward inline problem.

I reviewed this patch. It looks good to me.

r=kaie

Comment 28

[Scott MacGregor]

Comment on attachment 88571 [details] [diff] [review]
Additional patch

sr=mscott

Comment 29

[Kai Engert (:KaiE:)]

Additional patch has been checked in to trunk.
Marking fixed again.

Comment 30

[Kai Engert (:KaiE:)]

Charles, in reply to your comment 25:

case #0: Now fixed by the additional patch, can you please verify? (builds
starting 2002-06-21-08-trunk should have the fix)

case #1: Thanks for finding that nasty bug, it is a separate problem. I filed
153243 and it's meanwhile fixed.

case #2: Is the same as case #1

Comment 31

[Stephane Saux]

adding adt1.0.1 keyword. Charles is verifying right now.

Comment 32

[Charles Rosendahl]

Verified on 0621 Trunk builds.

Forwarded inline, as attachment, reply, reply all.
Original and additional recipients used in replies.
Original, subset of original, and new recipients used in forwarding of mail
messages.

Comment 33

[Jaime Rodriguez, Jr.]

adt1.0.1 (on ADT's behalf) approval for checkin to the 1.0 branch, pending
drivers' approval. pls check this in asap, then add the "fixed1.0.1" keyword.

Comment 34

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 35

[Kai Engert (:KaiE:)]

Comment on attachment 88571 [details] [diff] [review]
Additional patch

Marking a=valeski
He gave me approval online in a chat, and we were talking about both patches.

Comment 36

[Kai Engert (:KaiE:)]

Both patches checked in to 1_0 branch.

Comment 37

[Charles Rosendahl]

Same tests as comment #32 on the 06/25 branch builds.

Comment 38

[lchiang]

I believe the fix to this bug has regressed 
http://bugzilla.mozilla.org/show_bug.cgi?id=154734.

Comment 39

[Kai Engert (:KaiE:)]

Heads Up: I think I found two more problems related to this bug. I will file
bugs soon, after I know more details.

Issue 1: I have one profile with one message, only with that particular message
I see a problem. When I forward that message, in the opening window encryption
is not enabled. I need to analyze why.

Issue 2: Copy an encrypted message to one of your "Local Folders". Open that
message from there. Do "Forward Inline". No composition window opens, but an
error message is shown instead.

Comment 40

[Charles Rosendahl]

Issue 2 is probably related to the space in "Local Folders".  happens for all
messages, not just signed/encrypted messages.

Comment 41

[Jean-Francois Ducarroz]

this fix cause bug 155476

Comment 42

[Jean-Francois Ducarroz]

other regression caused by this fix: bug 155253

Comment 43

[Kai Engert (:KaiE:)]

CC'ing ducarroz FYI.

In response to my own comment 39, and to Charles follow up comment 40, only
talking about issue #2, the error message:
- I'm not sure yet whether it is indeed caused by the space in the folder name,
because I had the space fix from bug 154734 applied when testing.
- I confirm that it happens for any message, not only encrypted messages.
- It only happens when forwarding inline.
- I believe what I see is bug 155476, because I see the same error message as
reported in that bug, and JF told me, forwarding inline works by using the same
logic as opening a draft message.
- trying to understand what causes the problem, I tried to back out the patch
from this bug named "Additional Patch" (attachment 88571 [details] [diff] [review]), and after backing it
out, I no longer see my problem #2.

I will have a closer look at what is happening, and will comment in bug 155476.
One hint I have already is, the debug version shows the following warning in
addition to the UI error message:

ARNING: malformed hostname, file
/home/kaie/100/mozilla/netwerk/base/src/nsURLParsers.cpp, line 574
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/kaie/100/mozilla/mailnews/local/src/nsMailboxService.cpp, line 568
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/kaie/100/mozilla/mailnews/compose/src/nsMsgCompose.cpp, line 1454
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv)) failed, file
/home/kaie/100/mozilla/mailnews/compose/src/nsMsgComposeService.cpp, line 645

Comment 44

[Kai Engert (:KaiE:)]

Follow up to my comment 39, issue 1 - the problem that the original implemented
functionality does not work for one particular message in a POP3 folder.

I think what I see is caused by the fix introduced with bug 154734. In the
forward inline case, the URI that S/Mime logic remembers has a different
escaping from what will be seen later during message compose init. Therefore, we
do not find the URI in our lookup table.

Note that bug 154734 has caused regression bug 155476. Anyway, if we continue to
stay with the patch from bug 154734, we need an additional patch for bug 137071.
In this small additional patch, we need to achieve escaping consistency in what
we store and lookup in our internal S/Mime URI table.

Comment 45

[Kai Engert (:KaiE:)]

Another issue.
Because of bug 155476 (caused by 154734), you currently can not see the
following problem. You need a build which has the patch from 154734 backed out,
or which is from a date before 154734 was checked in.

I found the new bug 155513.
Because of bug 155513, the intended behaviour of this bug 137071 does not yet
work when forwarding inline a message stored in "Local Folders".

In order to fix 137071 completely, we need to either fix bug 137071, or must use
an additional patch for 137071, in order to ignore the upper/lowercase of mail
URIs we remember in our table.

Comment 46

[Kai Engert (:KaiE:)]

> In order to fix 137071 completely, we need to either fix bug 137071, or must use
> an additional patch for 137071, in order to ignore the upper/lowercase of mail
> URIs we remember in our table.

This should have said:

In order to fix 137071 completely, we need to either fix bug 155513, or must use
an additional patch for 137071, in order to ignore the upper/lowercase of mail
URIs we remember in our table.

Comment 47

[Jean-Francois Ducarroz]

>we need to achieve escaping consistency in what we store and lookup in our
>internal S/Mime URI table.

Kaie, can you give me example of escaping inconsistency?

Comment 48

[Stephane Saux]

JF: isn't bug 155513 what Kai is talking about?

Comment 49

[Jean-Francois Ducarroz]

bug 155513 is about capitalization, not escaping problem!

Comment 50

[Jean-Francois Ducarroz]

bug 155638 has been file to partially backup this fix on the branch until we get
the correct fix.

Comment 51

[Kai Engert (:KaiE:)]

> Kaie, can you give me example of escaping inconsistency?

Let me explain what consistency would be:

We only have "escaping consistency" if all S/Mime code dealing with message URIs
can expect that the URI is
- either always escaped
- or never escaped

Only if the S/Mime code knows in advance, that the "URI is escaped" property is
always true or always false, it can store the URIs directly in its lookup table,
without having to "normalize" it first.

Right now, the 137071 S/Mime code does not try to normalize the escaping, and if
an URI is sometimes escaped, but sometimes it is not, the lookup fails, because
the string comparison in the lookup table fails.

So, we need to know the answer to this questions:

1) Is the URI stored in originalMsgURI
- always escaped
- always not escaped
- sometimes escaped, sometimes not escaped

2) Is the URI returned by GetSelectedMessage() always in the same escaping
format as the URI as it is seen in gMsgCompose.originalMsgURI?

Comment 52

[Kai Engert (:KaiE:)]

We opened bug 155671 to track the work required to re-implement the logic to fix
the forward inline case.

Comment 53

[Charles Rosendahl]

kai,

did 137071 really cause regressions or did it expose fundamental design problems
and defects at another level?  From everything that I looked at and have tested,
I still think that your fix was correct with the side effect of exposing the
underlying escaping/unescaping inconsistencies.

No mind - I take it 155671 will find a solution all the way around post RTM or
pre RTM?

Comment 54

[Stephane Saux]

Charles,
 I don't think, nor have I heard anybody say, that there are "fundamental design
problems". Just defects here and there in the implementation.

 We may have a partial 137071 functionality for RTM. If Kai and JF can come up
with a fix that resolves the issues, and that patch passes the smell test, adt
will look at it. Otherwise, we'll have a great solution for post-RTM.