Closed Bug 256510 Opened 20 years ago Closed 17 years ago

Return receipts don't use Multiple identities

Categories

(MailNews Core :: Backend, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.9alpha7

People

(Reporter: bugzilla, Assigned: lorenzo)

Details

(Keywords: privacy, verified1.8.1.8)

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040803
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040803

When you receive an email which was sent to any identity which isn't the main
one for that account, the sender gets a return receipt from the main identity
instead of an email from the account which he sent the email to.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Product: Browser → Seamonkey
Thunderbird 1.0 has the same problem...
Summary: Return receipts don't use Multiple identities → Return receipts don't use Multiple identities
Can somebody have a look at this and confirm it? It's an importante privacy
issue and a major loss of what Mozilla offers. If you can't use Return receipts
for your identities, what do you want Return receipts? :)
Still exists in Thunderbird 1.0.2.

Since the logic to pick the right identity to reply to emails already exists,
fixing that bug shouldn't be _that_ hard...
Still there in 20050803...
Component: MailNews: Return Receipts → General
Keywords: mail6, privacy
Product: Mozilla Application Suite → Thunderbird
Maybe the same logic proppossed for bug# 303310 could be used here...
Is this bug what the extension "Correct Identity" tries to fix? Can you confirm
that?

https://addons.mozilla.org/extensions/moreinfo.php?application=thunderbird&id=1203

If so, there are certainly too less people with confirmation rights here, when
they even cannot confirm bugs with extension-fixes around...
(In reply to comment #6)
> Is this bug what the extension "Correct Identity" tries to fix? Can you confirm
> that?
> 
> https://addons.mozilla.org/extensions/moreinfo.php?application=thunderbird&id=1203
> 
> If so, there are certainly too less people with confirmation rights here, when
> they even cannot confirm bugs with extension-fixes around...

I don't think so, the problem has nothing to do with the reply identity but the
identity that Mozilla uses when returns the 'Return receipts' for the mail...
Flags: blocking1.8.0.1?
Not a security/stability release blocker. Need a tested, safe fix before we can consider it for the branches
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-
Flags: blocking-thunderbird2?
We should NOT send MDNs from a specific identity. In some cases ALL identities are the false ones. This occurs, when I'm using my webhosters service to collect messages from different mail accounts. This service sends the collected messages to a free eligible account/identity.

Having a look into the message header shows the original correct "To: My Name <foo@sender.de>" line. This address would be the only correct one, to use for the MDN.

The problem is, we have to use a working identity to send by SMTP. So we are not free to use foo@sender.de as "From: ....". A compromise would be to have the correct original senders address foo@sender.de in the messages body.
(In reply to comment #9)
> We should NOT send MDNs from a specific identity. In some cases ALL identities
> are the false ones. This occurs, when I'm using my webhosters service to
> collect messages from different mail accounts. This service sends the collected
> messages to a free eligible account/identity.
> 
> Having a look into the message header shows the original correct "To: My Name
> <foo@sender.de>" line. This address would be the only correct one, to use for
> the MDN.
> 
> The problem is, we have to use a working identity to send by SMTP. So we are
> not free to use foo@sender.de as "From: ....". A compromise would be to have
> the correct original senders address foo@sender.de in the messages body.
> 

Yes, but from my point of view, that's is a particular case. I mean, if the problem is that your SMTP server isn't going to accept messages sent from any of your identities, then the problem is not only with the MDN, but also with normal mails you try to send from those identities and Thunderbird can't do anything to solve that.

Also, according to RFC 2298: "The From field of the message header of the MDN MUST contain the address of the person for whom the message disposition notification is being issued", so if the MDN was issued to name@domain.com, the MDN from field must be name@domain.com
Not going to stop ship for this, but this seems like a good bug to fix for thunderbird 2 so I'm putting it into the Thunderbird 2 milestone bucket. 

Flags: blocking-thunderbird2? → blocking-thunderbird2-
Target Milestone: --- → Thunderbird2.0
(In reply to comment #11)
> Not going to stop ship for this, but this seems like a good bug to fix for
> thunderbird 2 so I'm putting it into the Thunderbird 2 milestone bucket. 
> 

It has a minus sign for blocking Tbird 2.  Does that mean it won't be fixed in 2.0?



QA Contact: grylchan → general
(In reply to comment #12)
> (In reply to comment #11)
> > Not going to stop ship for this, but this seems like a good bug to fix for
> > thunderbird 2 so I'm putting it into the Thunderbird 2 milestone bucket. 
> > 
> 
> It has a minus sign for blocking Tbird 2.  Does that mean it won't be fixed in
> 2.0?
> 
The bug is still present in Thunderbird 2.0.0.4 (20070604)!
This is an important issue, that should be fixed.
OS: Windows XP → All
Target Milestone: Thunderbird2.0 → ---
Ok...i'll try to fix it!
Status: NEW → ASSIGNED
Assignee: bienvenu → lorenzo
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch Patch for nsMsgMdnGenerator.cpp (obsolete) — Splinter Review
Ok this fix should be ok, i hope.
Please double check the file since is my very first patch.
Attachment #272749 - Flags: review?(bienvenu)
Attachment #272749 - Attachment description: Path for nsMsgMdnGenerator.cpp → Pacth for nsMsgMdnGenerator.cpp
Attachment #272749 - Attachment description: Pacth for nsMsgMdnGenerator.cpp → Patch for nsMsgMdnGenerator.cpp
Comment on attachment 272749 [details] [diff] [review]
Patch for nsMsgMdnGenerator.cpp

Thx for the patch!

I think you'll want to fall back to the first identity in case we didn't find a match.

 Other than that, it looks OK. There are a couple of space formatting things:

m_identity=ident

should be

m_identity = ident;

and
+            if(servIdentities)

should be 
if (servIdentities)

And I think this comment:

+            // fix for bug 256510. Now search for the correct identity looking
+            // the field TO on the message header


should read something like:

Find the correct identity based on the "To:" header.

Is it possible to get into this code where's we're not on the To: field? If so, that would make falling back to some identity even more important.
Attachment #272749 - Flags: review?(bienvenu) → review-
I don't know if it's a good idea to fall back to the first identity if we haven't a match. This way the user who sended the message requesting the receipt would receive a message from an address he probably doesn't know (the first identity).
Anyway maybe is better than sending nothing at all...
Yeah, otherwise it could be a privacy issue. We still have it now, but IMO this should be fixed. If I don't want to publicize my first identity of that account I also don't want to send a recipe from that address. That's why we shouldn't send a recipe for that case.
Updated. As suggested by Henrik in case there is no match, i don't set m_identity so we don't send the message at all. If this is not ok, just tell me.
Attachment #272749 - Attachment is obsolete: true
Attachment #272927 - Flags: review?(bienvenu)
I believe you're allowed to request an mdn for someone on the cc list or even bcc - I don't see anything here that disallows that http://www.faqs.org/rfcs/rfc2298.html and I see some things that imply a mail sender could, e.g., the part about sending out multiple e-mails to handle multiple return receipts. And we have code and ui to handle this (in particular, if the return receipt requestee isn't in the to or cc list). So you can't simply not send a return receipt in that case. So I think you have to fall back to the first identity. The privacy issue is dealt with by prompting the user if the e-mail address of the identity used isn't in the to or cc list.

Ok, this version use the first identity if there is no match. Anyway i've not understand this: "privacy issue is dealt with by prompting
the user if the e-mail address of the identity used isn't in the to or cc list". In fact if you get a mail with EVERY ADDRESS in the To header (assume nothing in CC) the prompt to the user is the same as an email with one identity in the To header. That is why i would apply the first patch, anyway i'm not a thunderbird guru or expert.
Attachment #272927 - Attachment description: Patch for nsMsgMdnGenerator.cpp → Patch for nsMsgMdnGenerator.cpp (no match -> no send)
I mean we allow you to configure our mdn response prompting differently if you aren't on the to: or cc: list - look in account, settings, return receipts.

When the prompt is displayed, you can look at the message header pane and see if you want to reply to it based on what the to: address is - if you don't want to your first identity to get used, just say no to sending the mdn response.

Is there a reason your patch doesn't also look for the the identity based on the cc: header if you don't find one based on the to: header? I.e., is this intentional, or just didn't seem worth the bother?
Attached patch Patch for nsMsgMdnGenerator.cpp (obsolete) — Splinter Review
Yes, you are right on all points. Don't send anything makes the settings in the options totally useless! I've also forgot looking in the Cc header, it wasn't intentional. Finally, there was another error looking for the email address: it's not possibile to use MailAddrMatch because for example the To can have a lot of addresses so you won't find anything.
Attachment #272927 - Attachment is obsolete: true
Attachment #272989 - Attachment is obsolete: true
Attachment #272927 - Flags: review?(bienvenu)
Attachment #273114 - Flags: review?(bienvenu)
Comment on attachment 273114 [details] [diff] [review]
Patch for nsMsgMdnGenerator.cpp

thx for the patch.

The braces here aren't needed:

+            if (!m_identity)
+            {
+              rv = accountManager->GetFirstIdentityForServer(m_server, getter_AddRefs(m_identity));
+            }

and this brace should be on its own line:

+              if (!m_identity) {
+                for (PRUint32 i = 0; i < count; i++)


But what's more worrisome is this check:

 PL_strcasestr(mailTo.get(), identEmail.get()))

Maybe this is what we do in our other identity detection code (I'll check) but it will take something like a@b.uk and have a positive match with a@b.uk.com, which isn't right. Maybe we don't care, but I'd want to make sure.

In any case, I think you'd want to use something like

mailTo.Find(identEmail, PR_TRUE) != kNotFound)
Comment on attachment 273114 [details] [diff] [review]
Patch for nsMsgMdnGenerator.cpp

I looked through our other identity managing code, and it does a simple Find as well, so if you just redo that code to use .Find, and fix the braces, we'll be done. Thx again
Attachment #273114 - Flags: review?(bienvenu) → review-
The functionality suggested by Lorenzo Simionato and David Bienvenu will be a great improvement with respect to the code we have today, and I hope that this patch will soon enter the official versions of Thunderbird. This would - for the first time - make sending return receipts a usable feature for me.

But on the long term, I have a few more suggestions:

* When Thunderbird asks whether it should send a return receipt, it should display the criterion which led to this question, e.g. "You are not in the To or Cc of the message" or "The sender is outside of your domain". This would be a help for quick decisions on whether I would like to send this return receipt. (Yes, I can look this up in the e-mail itself, but especially for very long To or Cc lists, this is awful.)

* When Thunderbird cannot determine one of the e-mail addresses of the account from the To and Cc fields, it should ask about which identity to use. Would it be so difficult to include in the pop-up window which asks about sending the return receipt a drop-down menu with the list of all identities for the given account? When the user has chosen "Always send" for the case "If I'm not in the To or Cc of the message", the question about the identity to use can be omitted and the first identity chosen. But if the user has configured "Ask me", he/she should not only be asked yes or no, but also which identity to use.

Thanks for your contributions! I'm really happy to see that something goes on with this bug.
Ok...we should be closer
Attachment #273114 - Attachment is obsolete: true
Attachment #273809 - Flags: review?(bienvenu)
Comment on attachment 273809 [details] [diff] [review]
[checked in] Patch for nsMsgMdnGenerator.cpp 

looks good, thx, Lorenzo. Do you need me to check this in for you?
Attachment #273809 - Flags: review?(bienvenu) → review+
Yes, please. I don't have CVS access. I would also put a ? on 'approval1.8.1.6' to get this fix also in the next 2.0 release. Correct? Thx for your time
I've noted that the file in the last released thunderbird is a little different from trunk...so to get this patch also in the next release i've to test and eventually modify a little the patch and send another one just for this branch?
If you could do that, that would be very helpful. Otherwise, I'd have to do it myself...
Ok, that the version for the 1.8 branch, so it should be shipped with maybe thunderbird 2.0.0.6.
If you could please check in this to 1.8 and the other to trunk, i think we are done.
Attachment #274138 - Flags: review?(bienvenu)
(In reply to comment #32)

> If you could please check in this to 1.8 and the other to trunk, i think we are
> done.

I have successfully compiled Lorenzo's new patch with the 1.8 branch, and it works in principal. But there is still one problem:

(In reply to comment #24)
> Maybe this is what we do in our other identity detection code (I'll check) but
> it will take something like a@b.uk and have a positive match with a@b.uk.com,
> which isn't right. Maybe we don't care, but I'd want to make sure.

This problem is still there, even with the .Find method for string comparisons. I have sent an e-mail requesting a return receipt with something like
"To: my@e-mail.com.net <another@e-mail.com>".
The only part in this To: header corresponding to an e-mail of one of my Thunderbird identities is "my@e-mail.com". Lorenzo's patch found this identity "my@e-mail.com" although 1.) it is part of the real name, not of the address in the To: field, and although 2.) "my@e-mail.com.net" from the To: header is not the same address as "my@e-mail.com".

In fact, I don't wonder, because I guess that the .Find method does a simple string comparison, comparing the addresses from the identities to the whole string of the To: header.

Anyway, I remarked that the same happens when I reply to this message: Also my identity "my@e-mail.com" gets selected although it was not really in the To: or CC: fields.

So I suggest leaving the patch as it is for the moment and include it into Thunderbird 2.0.0.6. But on the long term, a stricter comparison would be desirable, both for replies and for return receipts. One could e.g. use a function which splits a To: or CC: header string into its parts (separated by comma), extracting the e-mail address from each part (omitting the real name if present) and yielding a list of e-mail addresses in some suitable format as result. Then this list could be compared entry by entry with the available identities. [By the way, everything necessary for this functionality should already be present in Thunderbird, because exactly this happens when a mail message is displayed.]
It sure makes sense to me to get this checked in ASAP as is and file any new bugs thereafter. Thanks for staying with this.
Component: General → MailNews: Backend
Keywords: checkin-needed
Product: Thunderbird → Core
QA Contact: general → backend
Hardware: PC → All
Comment on attachment 273809 [details] [diff] [review]
[checked in] Patch for nsMsgMdnGenerator.cpp 

Landed on trunk as per comment #28.
Attachment #273809 - Attachment description: Patch for nsMsgMdnGenerator.cpp → Patch for nsMsgMdnGenerator.cpp [checked in]
Keywords: checkin-needed
Will there be more work on this bug or can we close it as resolved?
I think we should mark this fixed and open new bugs for remaining issues. If I understand correctly, the problem Bernd points out is real, but probably not common.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Yes, for the additional issue we could open new bugs.
Anyway i think we could also check in "Patch for nsMsgMdnGenerator.cpp 1.8 branch" to get the fix also in the next 2.0 release. Correct?
Lorenzo, can you run a test with a current trunk nightly build and check if it is working properly? So after David gives his r+ we could ask for approval1.8.1.7.
Yes, before sending the patch i've compiled fine with the 1.8 branch and tested it, all seems ok. This patch is almost the same of the checked in, just a few type hange.
(In reply to comment #20)
> can't simply not send a return receipt in that case. So I think you have to
> fall back to the first identity. The privacy issue is dealt with by prompting
> the user if the e-mail address of the identity used isn't in the to or cc list.

David, this is only half-implemented currently. If the recipients address is not listed in To or Cc you get ask, right. But there is no warning that another identity is used therefor. Should this be covered in a follow-up bug? 

Otherwise I can verify the behavior. Getting mail for an additional identity Thunderbird uses this one instead of the default identity.

=> VERIFIED.

David, could you please review the 1.8 branch patch? If we could check this in 1.8 we avoid sending of other private mail addresses, which will mainly fix the privacy issue for that bug.
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9 M7
Comment on attachment 274138 [details] [diff] [review]
[checked in] Patch for nsMsgMdnGenerator.cpp 1.8 branch

looks fine, thx.
Attachment #274138 - Flags: review?(bienvenu)
Attachment #274138 - Flags: review+
Attachment #274138 - Flags: approval1.8.1.7?
Attachment #273809 - Attachment description: Patch for nsMsgMdnGenerator.cpp [checked in] → [checked in] Patch for nsMsgMdnGenerator.cpp
>But there is no warning that another
>identity is used therefor. Should this be covered in a follow-up bug? 

Personally, I always base my decision whether or not to send an MDN response on who's asking for it, not who the mail was sent to. But if folks think such a warning is worthwhile, then someone can file a new bug.
Attachment #274138 - Flags: approval1.8.1.7? → approval1.8.1.7+
Comment on attachment 274138 [details] [diff] [review]
[checked in] Patch for nsMsgMdnGenerator.cpp 1.8 branch

Landed on MOZILLA_1_8_BRANCH.
Attachment #274138 - Attachment description: Patch for nsMsgMdnGenerator.cpp 1.8 branch → [checked in] Patch for nsMsgMdnGenerator.cpp 1.8 branch
verified fixed on the 1.8 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.8pre) Gecko/20071019 Thunderbird/2.0.0.7pre ID:2007101904. I received the RR from the identity I sent it to, not the default identity. Adding keyword.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: