380354 - TB2 adds ".eml" extension to attached messages, causes virus scanner to complain; TB1.5 was OK

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Rich Wales]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20061201 Firefox/2.0.0.3 (Ubuntu-feisty)
Build Identifier: version 2.0.0.0 (20070326)

When I attach a message in TB 2 (either by dragging it into the attachment window, or by forwarding if I've set my preferences to forward as attachments), the attached message is given a file name ending in ".eml".  This is causing the virus scanner at my workplace to flag the e-mail as containing a "POSSIBLE VIRUS" (via an obnoxious notation in the "Subject:" header line).

This started happening right after I switched to TB 2.  It doesn't appear to have been happening in TB 1.5; file names of attached e-mail messages in TB 1.5 don't seem to have an automatic ".eml" extension.

I need to have TB 2 stop adding this ".eml" extension (either take out the code or supply a configuration option to allow disabling ".eml").  Otherwise, I'll have to go back to 1.5.

I did ask the sysadmins at work to stop flagging e-mail messages as containing viruses solely on the basis of ".eml" attachments, but they're unwilling to change this policy because of concerns over the Nimda worm.  Thankfully, all they're doing right now is flagging the affected messages as suspicious, but I imagine they could very easily decide to block such messages in the future.

Reproducible: Always

Steps to Reproduce:
1. Start composing a message.
2. Pick another message and drag it into the composition window to attach it.
3. Alternatively, set preferences to forward messages as attachments, then forward a message.

Comment 1

[Rich Wales]

I think this bug is a duplicate of bug 220646.  But I'm hesitant to mark this bug as a duplicate of that bug, since 220646 has been marked "VERIFIED FIXED", and I don't want people to assume the problem has gone away.

Comment 2

[Magnus Melin [:mkmelin]]

You can always forward inline instead... I think the other solution is bug 190298.

Not having the .eml extension cause(d) problems e.g. when subjects happen to end in .com. Like "news from example.com" + it's confusing for users when saving and opening the file from outside tb.

Comment 3

[Rich Wales]

Forwarding inline might be OK, but sometimes I need to attach more than one message in a single new e-mail, and I'm really not up to doing a copy/paste on each individual message.

It sounds like some kinds of problems will result either way -- if ".eml" is used, or if it is =not= used.  The problems you (Magnus) describe -- which, BTW, I've never seen -- sound as serious as the problem I'm describing (which I guess you've never seen).  This sounds like a very good argument in favour of having the ".eml" file name extension be a configurable option -- so that people (like me) who cannot live with it can turn it off.

If people simply cannot tolerate the idea of =any= TB user =ever= being allowed to send out an e-mail attachment without an extension of some sort on the file name, then PLEASE provide a configuration option to allow the use of some other extension (other than ".eml") which isn't currently being targeted by virus scanners.

Comment 4

[voracity]

This is causing more severe problems in my case. My hosting provider rejects any email with a .eml extension, so I've been unable to receive any forwarded emails from Thunderbird 2 users.

This may or may not be an issue, depending on how common this kind of filtering is.

Comment 5

[rsx11m]

(In reply to comment #2)
> You can always forward inline instead... I think the other solution is bug
> 190298.

Forwarding in-line requires an explicit action of the user because the current default is to forward as an attachment. Also, he or she may not be aware of this setting and its consequences. Thus, combined with the suffix issue addressed here, the current default for forwarding e-mails provides the least probability that the message is actually received (I have therefore added a request to REOPEN bug 230448 to make in-line forwards the default setting). The ability to change the name of an attachment proposed in bug 190298 could present a workaround, but would have to be done individually for each message, thus I think that this is not a practical solution.

> Not having the .eml extension cause(d) problems e.g. when subjects happen to
> end in .com. Like "news from example.com" + it's confusing for users when
> saving and opening the file from outside tb.

The ".com" suffix issue with ".eml" omitted can be avoided as proposed in bug 271211 when changing all "." to "_" by default. A ".eml" suffix could be added when saving a message/rfc822 attachment if not present. 

I am wondering if RCF2046 (one of the successors of the RFC152x) or any other MIME-related standards require a filename argument to be specified at all, I couldn't find anything. A fairly simple checkbox next to the forward inline/attachment menu could be used to control the presence of a filename in message/rfc822 attachments. Obviously, an ISP's filter may be set to treat such a message as virus-suspicious as well and discard it. Also, if a recipient doesn't view attachments inline, the attachment would show up as Part 1.x and may confuse the recipient.

The combination of an "omit filename" checkbox with the option of specifying an arbitrary suffix for e-mails forwarded as attachment should provide the greatest flexibility to work around provider filtering. Again, filtering and removal of attachments simply based on the suffix is a questionable approach, but it is necessary to find a pragmatic solution for this issue.

Comment 6

[Milan Kupcevic]

When forwarding or attaching a message we should set the Content-Disposition filename suggestion without adding any extension, or even remove any extension if it is already part of the filename.

As described in part 2.1, 2.2, and 2.3 of the RFC 2183 “the sender may want to suggest a filename” AND when “the receiving MUA writes the entity to a file, the suggested filename should be used as a basis for the actual filename.”

Please note that filename extension serves as a file-system specific way to identify actual type of a file which is stored in the same file-system. Email system is using MIME Content-Type for that purpose and the file extension is irrelevant and even unwanted because the file in question may be saved to a file-system that does not use filename extensions at all. MUA should add the extension, or set the creator code, or set the Type code, as appropriate for the target file-system at the time of writing to the specific file-system. 

The MIME content-type concept is invented to solve the problem of transferring files between different systems. Translation between MIME content-type and file-system specific file type is OS specific.

Any MUA that is relying on the filename extension from the Content-Disposition filename field to recognize content-type is non-conforming non-interoperable and should not be used. We should not try to fix the problems of deficient substandard mailers. 

So, when forwarding or attaching a message we should set the Content-Disposition filename suggestion without adding any extension, or even remove any extension if it is already part of the filename.

Comment 7

[Magnus Melin [:mkmelin]]

Milan, please refrain from spamming multiple bugs with the same comment.

On newer linuxes, the extension might not matter much, but older systems still rely on the extension at least partly (or so I'm told). But there too, there is nothing prohibiting one from using file extensions.

Not that that really matters. What matters is that regular (windows) users saving the received mails are not likely to be able to open them if their file name doesn't end with .eml.

Comment 8

[Milan Kupcevic]

(In reply to comment #7)
> On newer linuxes, the extension might not matter much, but older systems still
> rely on the extension at least partly (or so I'm told). But there too, there is
> nothing prohibiting one from using file extensions.

The new habit (introduced in TB 2) of adding .eml filename extension when forwarding a message as an attachment has devastating effects on TB user base. This is very high price we are going to pay to please a bunch of MSOE users. People are switching back to TB 1.5 and thinking about alternative MUA's because of huge amount of rejected and silently filtered/eaten-up mail with .eml attachments. 

> 
> Not that that really matters. What matters is that regular (windows) users
> saving the received mails are not likely to be able to open them if their file
> name doesn't end with .eml.
> 

MUA running on windows should add the extension when saving the message attachment. Adding the extension at the time of attaching forwarded message is inappropriate. The .eml extension is hated by email server administrators and by antivirus/security guys so it is very likely that the message in question will not get to the recipient. 

The troubled MSOE users that are not able to see and/or to save the message attachment should switch to Thunderbird immediately. Of course, Thunderbird running on windows should add the .eml extension to the filename when saving the attachment to a file.

Comment 9

[Rich Gray (:rbgray)]

Confirming and taking a stab at setting appropriate flags.  This is core bug affecting both Thunderbird 2.0 & SeaMonkey 1.1 and thereafter, including trunk (SM and I presume TB).

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.8) Gecko/20071008 SeaMonkey/1.1.5
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a9pre) Gecko/2007100601 SeaMonkey/2.0a1pre

My take:
This is not really about standards.   Whether TB & SM send a suggested filename for an e-mail attachment or not, they are standards compliant.  This is about working around (stupid!) practices.  I pretty much agree with Milan Kupcevic's comment in https://bugzilla.mozilla.org/show_bug.cgi?id=220646#c84

>You can not change the behaviour of MSOE developers, MTA administrators, 
>and antivirus/security guys, but you can just reverse this patch and 
>bring back forwarding without .eml extension as it was in TB 1.5 
>(Bug 380354); sanitize the suggested file name created from the message 
>subject (Bug 271211) ; and let TB users to be able to rename the 
>suggested file name of any attachment (Bug 190298);

Comment 10

[WADA:World Anti-bad-Duping Agency]

(1) ".eml" battle
(1-A) If ".eml" is removed, many many O.E users complain to Tb users
(1-B) If ".eml" is added, some ISP's complain to Tb users or interfere Tb users

Workaround of (1-B) :
Fortunately, Bug 220646 changed back only "Forward As Attachment" to "add .eml", didn't change Drag & Drop (doesn't add ".eml"). So you can use name/filename without ".eml" if you attach mail via Drag&Drop using Tb 2.0, although you loose name/filename based on subject when Drag&Drop. But it's far better than remove of  mail attachment by ISP.

Please wait for fix of Bug 190298.

FYI.
This bug is similar to Bug 65794.
(2) Content-Disposition: inline / attachment battle
(2-A) If "attachment" for text/plain, many many O.E users complain to Tb users,
      "text attachment can not be viewed as inline".
      (Tb bypassed this problem, and is still bypassing the problem) 
(2-B) If "inline" for text/plain, users of some mailers complain to Tb user,
      "attachment is not displayed in attachment pane, then unable to save"
      (then Bug 65794 was opened)
Tb itself solved this battle by toggle of View/Display Attachments Inline, so some peoples complaint "Tb ignores Content-Disposition: inline / attachment"...

Comment 11

[rsx11m]

WADA, could you explain in some more detail why bug 190298 blocks this one? It is certainly related, and being able to rename an already attached file would be good, but this seems to be a different issue. As stated before, I'd consider manually renaming an attached message a workaround, but not a general solution to the ".eml" problem. Just assume you want to forward multiple e-mails as attachments and have to rename them individually. The Drag & Drop workaround is more attractive, but introduces additional steps when composing a message, and only a single message can be attached at a time.

I see your point of different expectations concerning the ".eml" suffix by the receiving client and the ISP filtering, even more a reason to have this configurable. Based on the discussions here, I'd distinguish four cases:

(a) Inline = "safe mode" understood by all clients and ok for all ISP's.
(b) As Attachment with file name specified and ".eml" suffix (covers 1-A).
(c) Same as (b) without ".eml" and "."-to-"_" conversion (covers 1-B).
(d) As Attachment without any file name specified (covers 1-B).

With currently only (a) and (b) existing, cases (c) and (d) could be added by additional enumerations for the mail.forward_message_mode preference and the corresponding dialog. An additional preference may specify the suffix to be used for (b), defaulting to ".eml". Since the file name is generated before being presented to the user in the compose window, my impression is that bug 190298 becomes applicable only after that.

Comment 12

[Rich Gray (:rbgray)]

(In reply to comment #11)
> WADA, could you explain in some more detail why bug 190298 blocks this one? 
I'll take a shot.   Because bug 190298 (rename attachments) is a potential fix for this one.  The block establishes a relationship between the bugs.

You consider bug 190298 to be a workaround, after a pref, but I don't see much point in the pref.  The problem is that the variation is a function of the recipients.  Some recipients will want .eml (apparently to save them the agony of a Save As??)  For other recipients, the .eml will cause the mail to be rejected or dropped because of idiot system "security" measures.   If not sanitized, some subject lines will cause trouble at the receiving end.  

The simple fact is that there is no one size fits all solution that will make it painless for the sender (short of an address book per-recipient pref.)  No matter what, there will be situations where the attachment name needs to be fixed up.  Hence 190298 needs to happen first to give the sender the ability to correct for the varying receiving end stupidities.  Any further steps merely reduce the probability that the sender will have to tweak the attachment name, b ut the need to change the name can't be eliminated, due to the two side of .eml. 

I'm wondering if bug 190298 can somehow be elevated from "enhancement" to fix ("normal", "major"?) status somehow, so it will get a little more attention.  

Comment 13

[rsx11m]

(In reply to comment #12)
> You consider bug 190298 to be a workaround, after a pref, but I don't see much
> point in the pref.  The problem is that the variation is a function of the
> recipients.  Some recipients will want .eml (apparently to save them the agony
> of a Save As??)  For other recipients, the .eml will cause the mail to be
> rejected or dropped because of idiot system "security" measures.   If not
> sanitized, some subject lines will cause trouble at the receiving end.  

Point taken. Thus, there would be no way around the capability of renaming attachments, but I nevertheless would argue that a preference setting for the "most likely" type of recipient is useful in order to minimize the number of messages where the file name needs to be edited. For example, I could decide that the (1-A) "Save As" issue is outweighed by other users (1-B) not receiving the attachment at all, and set my default respectively. As you point out, a true fix may only be to associate the filename pattern with each user in the address book, or being able to define preferences different from the default by domains, in a way similar to HTML/plain-text send options.

> I'm wondering if bug 190298 can somehow be elevated from "enhancement" to fix
> ("normal", "major"?) status somehow, so it will get a little more attention.  

I would second this. When that report started out, ISP filtering by suffix probably wasn't much of an issue (yet), and the initial motivation there was some malformed file name when attaching messages before switching focus.

Comment 14

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #11)
I think that:
1) sanitizing could be a good thing in all cases, and it should probably always happen;
2) a (not necessarily) hidden pref could be used to change the suffix that is added when attaching a message we forward. That suffix could be empty aswell. Grouped with 1), that effectively merges cases (b) and (c) in rsx11m's comment.
3) case (d) should be tested and evaluated, and could just as well be the best case. According to Magnus, it's a little problematic and requires some further fixing however.

Comment 15

[rsx11m]

I have distinguished cases (b) and (c) to allow easy switching between adding and omitting any suffix from the filename when forwarding as attachment. Otherwise, you'd always have to (re)enter the suffix when switching to (b). I agree that "sanitizing" the file name is preferable for both cases. Item (d) could be deferred, it shouldn't delay covering the other cases. As argued already in my comment #5 above, (a) should be the default setting as the mode offering the best chance that a forwarded message is received and readable at the other end.

Comment 16

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #15)
IMHO, omitting the suffix == adding an empty one :)
I agree that (a) should be the default, but some default (possibly empty) suffix should be provided for the case (b)/(c) anyway.

Comment 17

[Joshua Cranmer [:jcranmer]]

Attachment: Short patch to change extension

This is a simple patch intended to start some more discussion.

What this does is add a hidden pref "mail.forward_extension" that, if present, allows one to specify the extension. If not, it defaults to ".eml"; note that the pref must include the period.

Thoughts/questions/comments/concerns?

Comment 18

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #17)
> Created an attachment (id=286101) [details]
> Short patch to change extension
> 
> This is a simple patch intended to start some more discussion.
> 
> What this does is add a hidden pref "mail.forward_extension" that, if present,
> allows one to specify the extension. If not, it defaults to ".eml"; note that
> the pref must include the period.
> 
> Thoughts/questions/comments/concerns?

To satisfy even more users, I guess it should be possible to also define an empty extension.

Comment 19

[rsx11m]

(In reply to comment #17 and comment #18)
This looks good. Thus, let me reiterate the cases in my comment #11:

> (a) Inline = "safe mode" understood by all clients and ok for all ISP's.
> (b) As Attachment with file name specified and ".eml" suffix (covers 1-A).
> (c) Same as (b) without ".eml" and "."-to-"_" conversion (covers 1-B).
> (d) As Attachment without any file name specified (covers 1-B).

To implement this in a global setting, the enumeration in mailnews.js could be extended as follows:

pref("mail.forward_message_mode", 2);
  // 0=as attachment, filename sanitized with suffix,
  // 2=forward as inline with attachments (default),
  // 3=as attachment, filename sanitized without suffix,
  // 4=no filename specified.

This incorporates bug 230448 as well as comment #15 and comment #16

The patch in attachment 286101 [details] [diff] [review] would have replaced the line
    attachment->SetName(subject + extension);
by the following case statement based on mail.forward_message_mode:

    switch (mode) {
    case 0: attachment->SetName(subject_san + extension);
            break;
    case 3: attachment->SetName(subject_san);
            break;
    case 4: attachment->SetName("");  // sort this out later
            break;
    default: // should not happen, throw an exception
    }

along with a for loop to clean up the subject:
    subject_san[i] = (isalnum(subject[i]) ? subject[i] : '_');

Then, only the UI elements would need to be added to expand the droplist menu (Thunderbird) or radio buttons (SeaMonkey) by the respective new options, and adding a string field for the extension if this is supposed to be a more accessible preference.

That could be a good preliminary solution for the issue, serving as basis for a possible domain-based setting, and while waiting for bug 190298 to be solved.

Comment 20

[rsx11m]

(addendum to comment #19)
> ... as well as comment #15 and comment #16

I meant to refer to comment #14 and #15. The proposed sanitizing is done in all cases, and is even a bit more aggressive than just changing the dots, to make sure that the generated file name is good for all (most) operating systems. Case (d)=4 is represented by an empty string as an indicator to omit the file name fields later, where again, this special case can be deferred for a more comprehensive solution (I added it for the sake of completeness).

Comment 21

[Rimas Kudelis [use needinfo to ping]]

Just a note: I think Magnus has stated that in current situation, an attachment cannot be nameless (Tb wouldn't work correctly with a nameless attachment), so I suspect it's a bit more complicated than you probably think.

Comment 22

[rsx11m]

Rimas, thanks for your remark. Yes, I figured from your prior post that case (d) might be difficult to implement, therefore the "sort this out later" comment. I have no problem focusing on (a)-(c) for now only. Remember though that we may run into a similar problem if the Subject header of the forwarded message is empty, thus resulting in an empty "" or ".eml" file name. This could be caught at the beginning of the patch by replacing an empty string with some constant one, similar to the drag-and-drop case.

Comment 23

[Rimas Kudelis [use needinfo to ping]]

I also can't get rid of the idea that case 0 and case 3 are actually the same case.

Comment 24

[rsx11m]

Well, you can view mode 3 as a special case of mode 0, that's correct, but I distinguished those for easier switching. Consider an example where you have one recipient [e-mail] who's ISP filters by suffix and another [e-mail] who's doesn't, but y needs the suffix. You would set mail.forward_extension=".eml" once and send an e-mail to y with a mode=0. To send an e-mail to x, you'd set mode=3. Sending to x again, just revert to mode=0 (menu or radio button, maybe just a checkbox). If you only have the suffix but no mode 3, you would have to delete the suffix when mailing to x and have to reenter it when mailing to y. In the long run, if those modes could be associated with domains or individual users, IMHO it would be easier for the user to classify those simply as "suffix" vs. "non-suffix" domains/users rather than specifying a suffix for each domain/user separately.

Comment 25

[rsx11m]

Attachment: Draft for UI-related patches to implement comment #19

This is a draft for adding the "no suffix" and "no name" cases 3 and 4 to the user interface. I apologize for the wrong format, it's my first proposed patch, yet have to figure out how to do it right... :-)

In compose.xul, the "value=4" line should be out-commented if this case is not implemented per comment #21 - also, corresponding changes would have to be performed for the radio buttons in pref-composing_messages.xul

Comment 26

[rsx11m]

Attachment: Screen shot of updated menu options in TB 2.0

...and this is how it looks when applied to Thunderbird (here: 2.0.0.6 release).

Comment 27

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #24)
I don't think users are supposed to switch this option. It's there to be set once and forgotten IMO.

Comment 28

[rsx11m]

Attachment: Patch for UI/preference-related changes adding file-extension elements

(In reply to comment #27)
> It's there to be set once and forgotten IMO.

Are you sure that one can make this assumption in general?
Going back to comment #10 -
> (1) ".eml" battle
> (1-A) If ".eml" is removed, many many O.E users complain to Tb users
> (1-B) If ".eml" is added, some ISP's complain to Tb users or interfere Tb users
it appears that (1-A) and (1-B) are mutually exclusive, and there is no universal solution (other than forwarding inline) to cover both cases. Thus, a user who forwards mail to both class (1-A) and (1-B) recipients *will* have to switch the mode. Unless bug 190298 is solved, the user would have to go through the configuration editor each time. I agree though that the suffix itself probably will be set just once and then can be toggled as necessary. My aim is to have some sort of easy interface to the preferences for which the implementation is reasonably straight-forward.

I have attached a second patch that implements an alternative to making this accessible through the user interface. It keeps the enumerations for mail.forward_message_mode as they are, instead allows to specify the suffix and toggle its applicability by additional UI elements. This may be more attractive than the comment #25 version, since it doesn't involve changes in mode handling for mailWindowOverlay.js. It would replace the "case" statement in comment #19 by a simpler "if" statement on mail.forward_add_extension. It also solves issues with forwarding by a rule - bug 312025 - which would bypass any solution by bug 190298 due to the lack of a compose window. On the downside, it contains more obvious additions to the user interface than the first alternative which just extended the drop list.

Comment 29

[rsx11m]

Attachment: Preference dialog with checkbox and text field for extension

Assuming that the checkbox is implemented, I would consider the text field for the extension optional; mail.forward_extension still should be listed with a default in mailnews.js anyway then for easier identification.

Comment 30

[Magnus Melin [:mkmelin]]

I fail to see the need for anyone to specify that extension, if it is set it should be .eml - otherwise people just set it to something it isn't. Ok, some people evidently want to exclude it - but that's another thing.

Comment 31

[Rimas Kudelis [use needinfo to ping]]

I remember someone proposing .txt. :)
But actually, isn't that just as effective as no extension at all?

Actually, after all this discussion that went, I still don't want to believe that Mozilla people are the ones having to work around stupid bugs or practices. What we actually do is just delay their effect, instead of removing the cause. Now, I'll probably provoke some useless flame again, sorry about it.

So, let's say MSOE correctly handles attachments without .eml, we send them without an extension, we're cool. But hey! Suddently the viruses start acting the same. I wonder what the mail admins would do in such case - would they just block ALL attachments, or what? :)

The problem we're solving here is not actually our problem. No matter if Thunderbird names an attachment, or not, no matter if the name has an extension, or not, Thundebird acts according to the standards and RFC's.

The actual problem is how mail servers differentiate between ordinary and abusing messages, and that problem is actually not really solvable by just changing a name of an attachment or anything else. That's why, in my strong not-humble opinion, only checks for known viruses (and spam) should be performed on e-mails.  

Again, if a certain admin thinks that a certain level of false positives satisfies him (or the entity he works for), and you fall into that percentage, then... I guess you're probably just out of luck or something. If that admin doesn't like executable attachments, you shouldn't be surprised if the email with such attachment hasn't reached its recipient on that administrators server. Same applies to rfc822 attachments – if a certain administrator (falsly, IMO) believes, that rfc822 attachments are only useful to propagate viruses, and applies that belief in his practice of eliminating viruses, then you basically have three long-term choices: either convincing the admin he's wrong, or replacing the admin, or adapting to his stupid rules (forwarding inline, in our case). I think removing filename extension is only beneficial until the first few viruses that also exploit this possibility show up, and that's about it.

Perhaps, if rfc822 attachments are really that dangerous, one day we'll just learn to zip them first, just like we do with exe files now? :)

Sorry for this bugspam... I just couldn't hold it.

Comment 32

[Joshua Cranmer [:jcranmer]]

We will take the following as granted:

1. The proper and best thing to do for forwarding is forward by attachment.
2. We will also, for various reasons, support forward inline (preferable in some circumstances, discouraged in others)
3. Some MUAs will not accept .eml messages in the belief that they are viruses.
4. Some MTAs will not accept .eml messages in the belief that they are viruses.
5. Many MUAs and MTAs naively trust the extension over the Content-Type.
6. Other MUAs and MTAs trust the Content-Type header over the extension.

Nos. 5 and 6 are well beyond are control (even more so than 3 and 4) and are included mostly for the sake of completeness. If, in any given circumstance, we have a MTA that refuses message/rfc822 Content-Type headers because it `is' a virus, there is nothing we can do except ask them to get a more effective virus scanner.

In a perfect world, the subject name shouldn't matter if the Content-Type header is message/rfc822. It is in that case where merely changing the extension type to a random, but obviously-for-email value (like .rfc822). As others have stated, no-name attachments are currently problematic and should be commented with a comment saying along the lines of 'XXX: unusable until empty attachment names are supported'.

But, as others have pointed out, this is not a perfect world. We don't care about given #3 if it puts us in a damned-if-you-do-damned-if-you-don't loop. It's #4 that is the problem: MTAs are harder to avoid than MUAs, so we have to try to work around them if possible.

In the end, it's not our problem, but we're still getting the flak for other people's problems (and I'm getting used to it: it's happened to me countless times in the past month). So we have a choice: a) complain to the problematic MTAs (where will probably get a response similar to `buzz off') or b) implement the suggestions of rsx11m.pub, which purists are going to complain about. My personal preference is to just go with option b. After all, if the RFCs and the real-world conflict, it's better to go with the real world.

To [e-mail]:
Thank you for the preference work to fix this bug. I can put together the backend support for the preferences sometime later this week...

Comment 33

[Joshua Cranmer [:jcranmer]]

Attachment: Updated patch

(Obsoleting my old patch and the base for this patch; would you like me to obsolete the other draft patch and screenshot ?)

This one is [e-mail]'s latest patch with the back-end code worked in. Empty attachment values should work, although I disclaim that I have not tested this whatsoever.

I feel, however, that the extension should be a hidden pref (i.e., handled only by about:config but still listed in the prefs.js) because it shouldn't be changed unless someone really knows what he or she is doing.

Hmm, I seem to be generating spam today, but then again, my replies to other bugs reach a lot more people... :-/

Comment 34

[rsx11m]

Attachment: Patch for UI/preference settings in SM 2.0

Joshua, thanks for updating the C-part. I'm creating quite a bit of spam as well, but that's hopefully ok as long as it is productive. :-)

We can probably leave the first drop-down patch draft until it is certain that the checkbox version is the one to go with.

I have also updated the UI part for SeaMonkey 2.0, the specific patch is attached. Based on comment #30 I didn't include a text field for the extension suffix; it looks better this way in the radio-button version, and it seems that not having a UI element for this is favored right now. In the Thunderbird version, commenting lines 65 and 106 out or removing them from compose.xul should remove this field as well. I agree that mail.forward_extension should be retained at least as a hidden preference, to allow changing the suffix if ISP's "adapt" to whatever new challenge they may see.

BTW: The text "Attachment with Suffix" combined with "attachment ending in" seemed a bit long to me, thus I made it "As Attachment" again in my second patch.

Comment 35

[rsx11m]

Attachment: Composition Preference tab in SM 2.0

I've noticed that you reverted my change for mail.forward_message_mode from 0 to 2 as the default (mailnews.js) in your patch. I had included this based on the discussion in comment #15 and comment #16 on what the default among the four options should be, given that there is no clear-cut solution for forwarding as attachment. I fully agree with your list in comment #32 but would see it more from the user's side. The default should reflect the option of "least resistance" or the best chance of the message being accurately transmitted to and displayed by the other user's client. While forwarding as attachment clearly is the best solution to retain the original structure of the message, I see from my activities at the MZ forums that a lot of users are having problems with it (either because of the suffix issue, or because some receiving clients can't handle it, or just because people would like to edit the forwarded message before it goes out). Obviously, I'm not a "purist" and see things from the pragmatic side, i.e., it has to work for the majority of users. Thus, I definitely think that forwarding inline should be the default setting, and would hope that this can be addressed within the scope of this bug...

Comment 36

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #35)
> Created an attachment (id=286525) [details]

Perhaps the second option should be moved to a separate line? Like this:

* Inline
* As attachment [] with extension

I'm also not sure about the localizability of the second option.

> see things from the pragmatic side, i.e., it has to work for the majority of
> users. Thus, I definitely think that forwarding inline should be the default
> setting, and would hope that this can be addressed within the scope of this
> bug...

Agreed. I think most people don't actually care about the initial headers of the message that was forwarded to them.

Comment 37

[Rimas Kudelis [use needinfo to ping]]

Also, I think, that, given we provide the "with extension" checkbox, the suggested layout of this option in Seamonkey makes a little more sense than that in Thunderbird, so perhaps this radio version should be copied to Thunderbird too?

Comment 38

[rsx11m]

Attachment: Alternative patch for UI/preferences in SM 2.0

(In reply to comment #36 and comment #37)
> Perhaps the second option should be moved to a separate line? Like this:
> * Inline
> * As attachment [] with extension

That's certainly possible, the alignments are a bit more complex though. I've added an alternative patch for this, looks a bit strange though on the screen. It's a trade-off between horizontal and vertical space. I have also noticed with the recent latest-trunk nightly that somebody appears to be rearranging the preference dialogs for the suite, thus I'm not sure how much space to either side will be available.

> I'm also not sure about the localizability of the second option.

Are you thinking with respect to layout (that's defined in the XUL files and the same for all locales) or the language (defined in the locale-specific DTD file)?

> Also, I think, that, given we provide the "with extension" checkbox, the
> suggested layout of this option in Seamonkey makes a little more sense than
> that in Thunderbird, so perhaps this radio version should be copied to
> Thunderbird too?

If I remember correctly, earlier versions of Thunderbird had them in the radio-button form. If yes, it must have been redesigned some time. I've tried to retain the current structure as much as possible, to avoid surprises for the users and to limit implementation efforts.

>> Thus, I definitely think that forwarding inline should be the default setting
> Agreed. I think most people don't actually care about the initial headers of
> the message that was forwarded to them.

Thanks for your support on this, I think this is plausible. Joshua?

Comment 39

[rsx11m]

Attachment: Vertical radio-button arrangement in SM 2.0

(I have obsoleted now the first patch with the four-line drop-down menu)

Comment 40

[Rich Gray (:rbgray)]

Again, a pref strikes me as limited value, but after looking at https://bugzilla.mozilla.org/attachment.cgi?id=286401, I have to think why not make those options available for the Forward As menu item?  Now you have the ability of making the appropriate choice as compose time.

Comment 41

[rsx11m]

This would probably be a good idea, the question is how to implement it, and how much effort is involved. I assume you intend this to be an option in addition to the preference.

Right now, the function nsMsgCompose::CreateMessage() is called and a case statement branches into nsIMsgCompType::ForwardAsAttachment into the code attachment 286517 [details] [diff] [review] patches for mailnews/compose/src/nsMsgCompose.cpp.
Whether or not a suffix is added is determined within this function from the mail.forward_add_extension preference.

The current code in msgWindowOverlay.js provides functions MsgForwardMessage (for the "Forward" button and the Forward selection in the menus), which is branching based on mail.forward_message_mode, and two functions MsgForwardAsAttachment and MsgForwardAsInline for the "Forward As" Menu choices. While MsgForwardAsAttachment could be split into MsgForwardAsAttachmentExt and MsgForwardAsAttachmentNoExt, the question remains how to pass that information to nsMsgCompose::CreateMessage() without changing the preference. One option would be to extent the nsIMsgCompType enumeration by two additional values, causing to use either the preference to be used or the value corresponding to the "Forward As" choice. Anyway, this probably involves a couple of changes in various files.

Comment 42

[rsx11m]

(In reply to comment #19)
> 
> along with a for loop to clean up the subject:
>     subject_san[i] = (isalnum(subject[i]) ? subject[i] : '_');

Did anybody get a chance to look into this? Sanitizing the subject for naming the attachment and deciding on the defaults seem to be the last issues left for a preliminary patch to ease the problem in - hopefully - one of the next releases.

As said, restricting the file name suggestion to alphanumeric characters only may be overdoing it, but I'd consider the following special characters potential candidates in addition to '.' for replacement with '_' in the file name:

   . , : ; ! $ % ^ ~ & * ? ' " / \ | < > [ ] { } ( )
   (how about white-space and non-ASCII characters?)

Those may have a special meaning when passed as parameters in certain operating systems. On the other hand, one may argue that it is the client's responsibility to ensure that the file name is converted as necessary to be in compliance with any operating system restrictions.

Comment 43

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #42)
I guess that's fine. Too restrictive, yes, but you know, it's hard to draw the line which characters are safe and which are not.

My vote for nameless attachments – you'd solve two problems in one shot if these were supported. ;)

Comment 44

[rsx11m]

(In reply to comment #43)
> Too restrictive, yes, but you know, it's hard to draw the
> line which characters are safe and which are not.

It may be a potential localization issue if the subject is "oversanitized" to alphanumeric ASCII code only. The subject appears to come in UTF-16 encoding. Certain languages may not use the ASCII subset at all, in which case the entire subject would be reduced to all underscores. Thus, on a second thought, maybe it's better to start off with converting the dots only, following the bug 271211 suggestion, and possibly extend it if needed.

The following code is drafted from the XPCOM string guide examples, as a suggestion to extend Joshua's patch. It should also take care of the empty subject issue.

  nsString subject_san;
  nsString::iterator substr_start, substr_end;

  // copy subject string to subject_san, use default if empty
  if (subject.isEmpty())
     subject_san.Assign(NS_LITERAL_STRING("Attached Message"));
  else
     subject_san.Assign(subject);

  subject_san.BeginWriting(substr_start);
  subject_san.EndWriting(substr_end);

  // change all . to _
  while (substr_start != substr_end) {
     if (*substr_start == '.') *substr_start = '_';
     ++substr_start;
  }

This goes before attachment->SetName(subject[_san] + extension);

Comment 45

[Rimas Kudelis [use needinfo to ping]]

(In reply to comment #44)
hm... I'd go probably with "change spaces and punctuation" then.

Comment 46

[rsx11m]

This could be accomplished by changing the loop to

  // change all white-space and punctuation to _
  while (substr_start != substr_end) {
     PRUnichar aChar = *substr_start;
     if (IsAscii(aChar) && !IsAsciiAlpha(aChar) && !IsAsciiDigit(aChar))
        *substr_start = '_';
     ++substr_start;
  }

This would correspond to (isascii(c) && !ispunct(c)) using ctype functions and wouldn't touch the non-ASCII UTF-16 encodings. I'm sure though that people may complain then if their '+' and '-' are changed to '_' as well. Also, some escaping is done later, e.g., an original

Subject: A "test" name

line will be encoded as name="A \"test\" name.eml" argument. As a compromise, the other option would be to replace the "if" statement by a "case" switch, covering the special characters listed in comment #42 along with white-space characters, and leave all others untouched.

(correction to comment #44)
>   // copy subject string to subject_san, use default if empty
>   if (subject.isEmpty())

This was supposed to be subject.IsEmpty(), sorry for the typo.

Comment 47

[rsx11m]

Attachment: Testing scenarios, suffix and attachment name

I did some testing with the code discussed in the last few comments. Both the dot-only code in comment #44 as well as the code to sanitize all punctuation in comment #46 [you'll have to use NS_IsAscii*() to make it go through the compiler] did their job as intended. I've chosen long ASCII and ISO-8859-1 subject messages to ensure that they fit, no issues with creating the attachment name and passing it through a suffix-filtering MTA with the ".eml" extension switched off. Also, non-ASCII characters were passed well in all cases. The "Save" dialog displayed file names correctly, including special and non-ASCII characters. Windows showed problems saving file names containing a tabulator.

I tend to agree with Rimas that changing all ASCII punctuation and white-space characters is the safest way to name the attachment. The version with a restricted number of characters as listed in comment #42 would have resulted in almost the same file name suggestion as the comment #46 version. If the subject is short, the impact of this sanitizing should be minimal; if it is long, the message file likely will be renamed anyway when saved to disk. On the other hand, a more thorough sanitizing may also be performed for all attachment names within a bug 190298 fix later.

Additional comments are welcome, this patch for a first-stage solution looks close to being ready for review then. I'll go ahead and compile the individual patches to update attachment 286517 [details] [diff] [review]

Comment 48

[Rimas Kudelis [use needinfo to ping]]

How about non-ascii spacing? I see a potential flaw here (e.g., an ASCII space wouldn't pass, but a CJK space would?). Perhaps there is a function that would determine if any Unicode character is a spacing character or not?

Comment 49

[rsx11m]

I was thinking about that too, but it seems unlikely that any non-ASCII character has a special meaning in an operating system, e.g., similar to ASCII white spaces (argument delimiters), quotes (enclosing arguments), or directory delimiters. Thus, my guess is that it's reasonably safe not to convert those.

Furthermore, looking at http://www.unicode.org/charts/symbols.html there is a variety of possible candidates for exclusion - which ones to take?

Browsing through the source, I couldn't identify any function that would correspond to the NS_IsAscii*() versions for the full Unicode range.

Comment 50

[Rimas Kudelis [use needinfo to ping]]

Another idea which is probably not implementable either – perhaps it's possible to differentiate the set of safe symbols depending on locale – ie., if I, say, live in Japan, then safe and understandable set of symbols for me is quite different from what is safe and understandable for a russian. It would be something similar to what IE7 (and Firefox too?) does with punycode URL's – it displays the exact symbol only when that symbol is in the ANSI charset for the users locale, and displays its hex value in other cases.

Or perhaps I'm just thinking too much... 

Comment 51

[Magnus Melin [:mkmelin]]

I think you might be over thinking it here. The issue you wanted deal with was hindering attachments from potentially being named with something ending in .com (and possibly some more extensions), no? Why not just check if the 4th last char is '.' All the other sanitation seem redundant. 

Re the UI for choosing whether to add extension or not, I don't think there should be one. Remember windows even hides extensions by default...

Comment 52

[rsx11m]

(In reply to comment #50)
Restricting the permissible character range to those found in the language-specific set of the sender IMHO won't help much. It contradicts somehow the spirit of Unicode, and the recipient may use a different locale than the sender. Also, if the receiving MUA or operating system doesn't support any non-ASCII characters, nothing we can do about it anyway.

I think that the main purpose of sanitizing here is to avoid any problems with special characters or spaces that may cause problems with virus or spam filters, or that have a specific meaning to an operating system and are therefore ambiguous. The current version seems to ensure that well. Anything further should probably go beyond the scope of ".eml" attachments, e.g., handled within the bug 190298 discussion.

(In reply to comment #51)
> Re the UI for choosing whether to add extension or not, I don't think there
> should be one. Remember windows even hides extensions by default...

Magnus, please go back to comment #24 and comment #28 - Windows may be hiding the extension by default, but this doesn't change the MTA's or MUA's behavior. Thus, I think the user has to be able to switch it easily on a case-by-case basis (at least while this cannot be configured by domain or recipient or can change the file name of an attachment in the compose window in a second stage of fixing this bug, but even after that it makes sense).

Comment 53

[Magnus Melin [:mkmelin]]

What I meant was, it's nothing a normal user should have to care about (+ real risk they aren't even familiar with extensions). Just makes a the UI confusing IMO.

Comment 54

[rsx11m]

I see your point. Unfortunately, a user is forced to care about it when an e-mail bounces back or arrives only with the "unsafe attachment has been removed" message. If they don't figure it out on their own, it is easier to point them (e.g., in the MZ support forums) to a checkbox than to a configuration editor setting. I assume that a user will usually not touch a default setting of an option which he or she doesn't understand.

Comment 55

[rsx11m]

Attachment: Proposed fix (first stage)

Let me update the current patch as promised some comments ago, since it contains a couple of changes not posted yet. This consolidated patch obsoletes attachment 286517 [details] [diff] [review]

compose.xul and compose.dtd: (TB)
Version of attachment 286491 [details] [diff] [review] without text field for extension (in response to comment #30), also updating the label for the checkbox. The text of the drop-down menu is reverted to "As Attachment" (original wording).

pref-composing_messages.xul and pref-composing_messages.dtd: (SM)
Horizontal implementation of attachment 286524 [details] [diff] [review] (fits in both Linux and Windows builds for en-US locale).

mail_help.xhtml: (SM)
Updated help text for extended composition preferences dialog, adding explanation for checkbox (addressing the concerns in comment #53).

mailnews.js:
Sets the default preferences to inline as initially proposed (based on 2 out of 3 opinions, no other arguments were stated), the default extension for forwarding as attachment is ".eml".

nsMsgCompose.cpp:
Extension handling of attachment 286517 [details] [diff] [review] combined with converting all ASCII punctuation and white spaces to '_' in suggested file name (according to comment #46, first version - this can be easily replaced by the comment #44 dot-only version). I have also verified that the empty subject case and a modified extension work with this code.

Comment 56

[rsx11m]

Attachment: Proposed fix (alternative, dot-only version)

This patch differs in the C-part only from attachment 287577 [details] [diff] [review]

The default subject in the case of an empty "Subject:" heading is now derived from  messageAttachmentSafeName in composeMsgs.properties, thus corresponding to the name used for drag-and-drop attaching of a message. This makes it localizable as well, and would also apply to the more restrictive attachment name sanitation proposed in the comment #55 patch.

Secondly, this patch implements bug 271211 strictly and converts dots only. Compared with comment #44, this is substantially simplified by using the subject_san.ReplaceChar('.', '_'); function rather than an iterator and a loop.

That's the last patch from my side for now, IMHO both versions work well. Waiting for any further comments then, and please feel free to break it.

Comment 57

[rsx11m]

Are there any objections flagging attachment 287729 [details] [diff] [review] for review?
It has been about a week without further comments on this...

I performed some more testing to verify that leaving the special operating-system characters in the file name works. The "Save As" dialog of both TB and SM replaces those with '-' instead, and the webmail service I've tested it against does a similar cleanup. Thus, the dot-only version seems to be fine, and also should not have any impact on a more comprehensive sanitation in bug 190298 (or beyond).

Comment 58

[Magnus Melin [:mkmelin]]

I'm still with comment 51. We already have the inline option UI. It's easy to say to users to just use that if such cases. 

To satisfy the users who absolutely do not want the extension, I guess I'm on board with having a hidden pref, but we don't need a pref about what that extension should be. It's eml or nothing. 

Also, I don't think we want to change the default forwarding format.

Comment 59

[rsx11m]

I think there is overall agreement about the C-backend for creating the attachment name and its sanitation. This alone would provide some relief for the affected users, no doubt. The discussion is obviously on which additional steps should be taken along with it, to make this more convenient and accessible (i.e., useful) for the end user.

> (In reply to comment #58) We already have the inline option UI.
> It's easy to say to users to just use that if such cases.

It is undisputed that forwarding inline is a universal method to avoid any conflict with MTA/MUA behavior (which is why IMHO it should be the default). However, not in all cases it is appropriate (comment #3).

> To satisfy the users who absolutely do not want the extension, ...

This is not a matter of choice but dictated by the MTA/MUA's through which the individual e-mail is going (comment #28), I hope everybody can agree on that. Making this parameter visibly selectable by minimally extending the prefs UI with a single check box should be reasonable, and it fits well into current space of the dialog window. An additional option is comment #40 (by context menu), but this involves further implementation work.

> ... hidden pref, but we don't need a pref about what that
> extension should be. It's eml or nothing. 

I partially agree with that. While the boolean pref should be visible, the mail.forward_extension preference can be removed from mailnews.js to avoid overcrowding of the config editor window. It shouldn't hurt though to leave it as a hidden pref in the C-code (and Joshua's code for this wouldn't have to be changed), for those who know about it and want to switch the extension anyway.

> Also, I don't think we want to change the default forwarding format.

Why not? This default was apparently set in 1999 by bug 22791, and a review if a setting is still adequate seems to be reasonable while working on a related issue. There are plenty of reasons given here in this report alone to suggest that forwarding as attachment is not (or no longer) a well justified default.

Comment 60

[Joshua Cranmer [:jcranmer]]

Interjecting after a long bout of silent reading.

(In reply to comment #59)
> > To satisfy the users who absolutely do not want the extension, ...
> 
> This is not a matter of choice but dictated by the MTA/MUA's through which the
> individual e-mail is going (comment #28), I hope everybody can agree on that.
> Making this parameter visibly selectable by minimally extending the prefs UI
> with a single check box should be reasonable, and it fits well into current
> space of the dialog window. An additional option is comment #40 (by context
> menu), but this involves further implementation work.

This is not a case where inclusion loses functionality--having it in if you don't want it is not going to hurt you, and I doubt that most users would be going to 

> > ... hidden pref, but we don't need a pref about what that
> > extension should be. It's eml or nothing. 
> 
> I partially agree with that. While the boolean pref should be visible, the
> mail.forward_extension preference can be removed from mailnews.js to avoid
> overcrowding of the config editor window. It shouldn't hurt though to leave it
> as a hidden pref in the C-code (and Joshua's code for this wouldn't have to be
> changed), for those who know about it and want to switch the extension anyway.

Hidden pref at least; fully putting it in the options UI is probably too much. mail.forward_extension definitely has some use-cases, so I vote for having it.

> > Also, I don't think we want to change the default forwarding format.
> 
> Why not? This default was apparently set in 1999 by bug 22791, and a review if
> a setting is still adequate seems to be reasonable while working on a related
> issue. There are plenty of reasons given here in this report alone to suggest
> that forwarding as attachment is not (or no longer) a well justified default.

It would be worthwhile to reopen the discussion of whether or not to change the default.

rsx11m.pub, would you like me to mark you as the assignee for this bug?

Comment 61

[rsx11m]

(In reply to comment #60)
> Interjecting after a long bout of silent reading.

Welcome back!

> Hidden pref at least; fully putting it in the options UI is probably too much.
> mail.forward_extension definitely has some use-cases, so I vote for having it.

Both mail.forward_add_extension and mail.forward_extension are defined as visible preferences in mailnews.js in the currently proposed patch. However, I removed the text element for mail.forward_extension that was present in the initial Thunderbird UI patch. Thus, the TB screen shot is obsolete, the SM version had the checkbox only from the beginning.

It would be easy to remove (or just out-comment) mail.forward_extension from mailnews.js to make it a hidden preference, if this should be the conclusion. It is equally easy to keep it there with the UI showing the checkbox only.

BTW: I think attachment 286517 [details] [diff] [review] can be marked obsolete now as well.

> would you like me to mark you as the assignee for this bug?

Thanks for the offer - yes, I would appreciate that.

Comment 62

[Magnus Melin [:mkmelin]]

> mail.forward_extension definitely has some use-cases, so I vote for having it.

Mind giving those use-cases?

If you would like to change the forward format, open a new rfe about that and list the reasons for it.

Comment 63

[rsx11m]

Magnus, thanks for setting the assignee.

> (In reply to comment #62) Mind giving those use-cases?

First of all, I think it's in general not a good idea to "hardwire" strings in a code beyond the defaults for a hidden preference. Lots of other bugs deal with making strings configurable, which - admitted - are more highly visible though (e.g., "Re:" and "[Fwd:]"). To name the other extreme, adding an entry for message/rfc822=".eml" into mimeTypes.rdf would be overdoing it as well. I don't know what specifically Joshua had in mind, but I could think of cases where localized country settings or the use of specific e-mail clients or conventions in a company or organization would prefer to name the extension in a different way. Also, who knows what "ideas" ISP's may come up with in the future to filter messages. Again, I don't see any harm in retaining this as a hidden preference; those who don't know about it won't care and just get the default extension. This sounds like a good compromise.

> If you would like to change the forward format, open a new rfe about that and
> list the reasons for it.

Such a new RFE would likely be resolved as a duplicate of bug 230448, which was filed a long time ago with rather weak arguments and has expired since.
I had added a request to reopen it about two months ago. During the earlier discussions here in this bug, the topic came up when talking about the different options to forward e-mails, and in which cases which problems occur with which option. Thus, I felt it was legitimate to include the question on the default here, in a similar way as we included the file-name sanitation issue from bug 271211 here as well = all deal with "forwarding as attachment". If it is the common opinion that the forwarding default question should be separated from the attachment issue, then please reopen bug 230448 as Core/Trunk RFE so that this part the discussion can be continued there.

Comment 64

[Magnus Melin [:mkmelin]]

Adding code complexity just for the sake of it (and for highly hypothetical reasons) make no sense at all.

That the RFE about format change didn't get any attention for many years should give you a clue on how desired it really is. It simply hasn't been much of a problem for anyone. 

Comment 65

[rsx11m]

Magnus, thanks for reopening bug 230448. I'll split off the default forward setting from the current patch then and put it up for discussion in that RFE.
Given the feedback in the MZ forums, it is indeed a problem for many end users confused by that default setting. Not all users end up commenting in bugzilla...

Comment 66

[rsx11m]

I have briefly summarized the discussions here on the forwarding defaults in
bug 230448 comment #7 for those not following this bug. Please comment there if I misrepresented or misinterpreted any of your statements, or to add further to
the discussion.

(In reply to comment #64)
> Adding code complexity just for the sake of it (and for highly hypothetical
> reasons) make no sense at all.

That's more than hypothetical. I recall that there have been other suffixes around in the past (e.g., ".msg", ".mail") for message files. Right now it is ".eml", that's correct, but there is no guarantee that it will stay that way in the future. As for complexity, it is one function call versus one string assignment, thus I assume this wouldn't change anything significantly.

Comment 67

[Magnus Melin [:mkmelin]]

That's actually a reason not to add the choice. Unfortunately .msg also comes in a m$ version which is not rfc822...

There are also other places where extension is added, so the complexity is more than that. Especially as it's all for nothing.

Comment 68

[rsx11m]

> There are also other places where extension is added

Ok, that part of your argument I would accept. Searching for other hard-wired occurrences of ".eml" lists quite a few places where it has been hard-wired as well, even if empty-extension cases are accepted. Offering a choice for the attachment just for forwarding but not for anything else would not be harmful but certainly inconsistent. Adding a hidden pref to all of those occurrences is obviously well beyond the scope of this bug.

Joshua, this affects your part of the patch; any more arguments to keep it configurable specifically for forwarding mail? Otherwise I have to agree with Magnus that considering alternative extensions would ask for a more general (and separate) solution.

Comment 69

[rsx11m]

Attachment: Proposed fix (v2)

Summary - when forwarding as attachment:
* adds checkbox to composition preferences (toggles extension)
* adds hidden preference to change extension (default ".eml")
* sanitation of subject for attachment name (bug 271211)
* forwarding defaults unchanged (split off to bug 230448)

Comment 70

[rsx11m]

No new comments came in, thus I have now posted an updated patch and flagged it for review. This patch retains the mail.forward_extension hidden preference for now; if the reviewer(s) conclude that it should go, I'll replace this part with the hard-wired version.

While I agree with Magnus that a configuration option for the extension ideally should be provided consistently throughout the code, other special cases for file extensions are already introduced (e.g., bug 217149 and bug 350819 on opening message files with alternative extensions). A user has to be aware that hidden preferences are used at his or her own risk, and there are other hidden preferences which may cause problems if used incorrectly, thus this concept should be well understood. Since the mail.forward_extension pref doesn't appear to prevent any larger solution, I don't see a reason not to provide this hook for anybody who might find a use for it.

Note that I changed the access-key definitions in the TB preference dialog, as there was a conflict with autoSaveEnd in the branch version (removed for trunk per bug 379698). As stated in comment #38, the SM preferences are about to be migrated (this is bug 404263 for the mail/news pane), in case this has to be considered at this point.

Comment 71

[Karsten Düsterloh]

Comment on attachment 290896 [details] [diff] [review]
Proposed fix (v2)

FWIW, this bug is about an edge case, where (stupid) use of virus/spam filters may harm actually legitimate mail. OTOH, not adding the extension may be harmful as well, so there's no perfect solution...

Cluttering the UI with menuitems almost noone will ever need, though, is not the way.
So while I can't speak for TB's UI (and thus don't comment on it in general in the following), please remove the UI portions for SM entirely!


>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.xul
>Index: suite/locales/en-US/chrome/mailnews/pref/pref-composing_messages.dtd
>Index: suite/locales/en-US/chrome/common/help/mail_help.xhtml

Drop changes to these three files.

>Index: mailnews/compose/src/nsMsgCompose.cpp
>===================================================================
>+                PRBool addExtension = PR_FALSE;

Even if I would think that having two prefs is useful, this default should be PR_TRUE. ;-)
But I don't think so - we should just set mail.forward_extension to ".eml" by default and everyone who doesn't like this can change it to empty or ".myrandomfileextension" or whatever.
Having two prefs here is just excessive.

>+              // copy subject string to subject_san, use default if empty
>+              if (subject.IsEmpty())
>+              {
>+                nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID);
>+                NS_ENSURE_TRUE(bundleService, 0);

You should use the rv version of do_GetService and check that.

>+                nsCOMPtr<nsIStringBundle> composeBundle;
>+                bundleService->CreateBundle("chrome://messenger/locale/messengercompose/composeMsgs.properties",
>+                                             getter_AddRefs(composeBundle));
>+                NS_ENSURE_TRUE(composeBundle, 0);

You should use the rv returned by CreateBundle and check that.

>+              // change all '.' to '_'  see bug #271211
>+              subject_san.ReplaceChar('.', '_');
>+              attachment->SetName(subject_san + extension);

If we're adding a non-empty extension (with first char = '.'), there's no need to replace the dots. Otherwise, if extension is empty, there's no need to add it (although that's probably cheap enough to not require special fix-up)...

Comment 72

[rsx11m]

(In reply to comment #71)
> FWIW, this bug is about an edge case, where (stupid) use of virus/spam filters
> may harm actually legitimate mail. OTOH, not adding the extension may be
> harmful as well, so there's no perfect solution...

I definitely agree, as shows the general discussion here and elsewhere.
 
> Cluttering the UI with menuitems almost noone will ever need, though, is not
> the way.
> So while I can't speak for TB's UI (and thus don't comment on it in general in
> the following), please remove the UI portions for SM entirely!

I realize that there have to be priorities in what goes into the UI and what not. As stated throughout the comments in this bug, there are good reasons to provide the checkbox, and the people affected would benefit from it. I'll drop the related SM-specific portions then, but hope that the checkbox will stay for TB, as it is more difficult to access the configuration editor in TB than in SM.
 
> Even if I would think that having two prefs is useful, this default should be
> PR_TRUE. ;-)

Oops, thanks for catching that!

> But I don't think so - we should just set mail.forward_extension to ".eml" by
> default and everyone who doesn't like this can change it to empty or
> ".myrandomfileextension" or whatever.
> Having two prefs here is just excessive.

Ok, if your point is that the number of related preferences should be one but not two, I'd rather keep the boolean variable and drop the extension name (per discussion around comment #60).

> You should use the rv version of do_GetService and check that.
> You should use the rv returned by CreateBundle and check that.

I'll look those up.

> If we're adding a non-empty extension (with first char = '.'), there's no need
> to replace the dots. Otherwise, if extension is empty, there's no need to add
> it (although that's probably cheap enough to not require special fix-up)...

The issue discussed in bug 271211 was also that attachments named like "something.com.eml" were rejected by some MTA. Thus, I'd prefer to keep it that way to ensure that the extension is clearly defined when included in the name.

Comment 73

[rsx11m]

Attachment: Proposed fix (v3)

Updated patch:
* checkbox only in TB composition preferences, no longer for SM
* boolean preference to add or omit ".eml" extension (see note below)
* string preference for extension dropped (comment #60 and following)
* no changes to subject sanitation (regardless of extension setting)
* rv's checked for bundle services, other polishing

Note: Dropping instead the boolean mail.forward_add_extension in favor of the mail.forward_extension string to reduce the number of preferences to one would effectively also eliminate the checkbox still envisioned for the TB preference pane, since providing a UI element for the string preference was considered inadequate (comment #33).

Comment 74

[Karsten Düsterloh]

Comment on attachment 293600 [details] [diff] [review]
Proposed fix (v3)

Basically fine, just some second thought nits:

>Index: mailnews/compose/src/nsMsgCompose.cpp
>===================================================================
>-              attachment->SetName(subject + NS_LITERAL_STRING(".eml"));
>+              nsString extension;
>+              nsString subject_san;
>+
>+              if (prefs)
>+              {
>+                PRBool addExtension = PR_TRUE;
>+                prefs->GetBoolPref("mail.forward_add_extension", &addExtension);
>+                extension = addExtension ? NS_LITERAL_STRING(".eml") : EmptyString();
>+              }
>+              else
>+                extension = NS_LITERAL_STRING(".eml");

If you take /addExtension/ outside of the if block and just change its value inside, you'll get away with setting /extension/ just once (conditionally afterwards) and get rid of using /EmptyString/ entirely. 
But ...

>+              // change all '.' to '_'  see bug #271211
>+              subject_san.ReplaceChar('.', '_');
>+              attachment->SetName(subject_san + extension);

... if you put the subject fixup before the /extension/ stuff, you can even drop /extension/ and add the extension directly to /subject_san/ when needed.

Sorry for this extra hop.

Comment 75

[rsx11m]

Attachment: Proposed fix (v4)

> Sorry for this extra hop.

No problem, it certainly makes sense to remove the now redundant extension variable. This patch is the most compact version I could come up with, also has just a single occurence of the hard-wired ".eml" extension.

Comment 76

[David :Bienvenu]

Comment on attachment 294485 [details] [diff] [review]
Proposed fix (v4)

thx for the patch.

Comment 77

[Magnus Melin [:mkmelin]]

I think that checkbox (which I still think is not necessary) should get disabled when Inline is selected. Or on a new line if you think about forwarding multiple attachments. 

Comment 78

[rsx11m]

David, thanks for your quick approval of the patch. I assume that this is ready to be checked in then.

Magnus: I was thinking about disabling the checkbox for inline forwarding myself in the initial layout. However, the option of forwarding as attachment is still given from the menu, and forwarding by a filter rule is currently always done as attachment. Thus, I think that the box has to stay enabled even if forwarding inline is selected as the default. The space to the right of the drop-down box appears to be sufficient for the text box, and it is related to that, so I don't quite see the need or benefit to put it on a separate line.

Comment 79

[Magnus Melin [:mkmelin]]

Ok, maybe you are correct. 

Checking in mail/components/preferences/compose.xul;
/cvsroot/mozilla/mail/components/preferences/compose.xul,v  <--  compose.xul
new revision: 1.14; previous revision: 1.13
done
Checking in mail/locales/en-US/chrome/messenger/preferences/compose.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/preferences/compose.dtd,v  <--  compose.dtd
new revision: 1.8; previous revision: 1.7
done
Checking in mailnews/mailnews.js;
/cvsroot/mozilla/mailnews/mailnews.js,v  <--  mailnews.js
new revision: 3.309; previous revision: 3.308
done
Checking in mailnews/compose/src/nsMsgCompose.cpp;
/cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v  <--  nsMsgCompose.cpp
new revision: 1.536; previous revision: 1.535
done

->FIXED

Comment 80

[Magnus Melin [:mkmelin]]

Attachment: proposed fix (as checked in)

Made a few minor changes before checking in. (Trailing space and var naming.)

Comment 81

[rsx11m]

Thanks and a happy new year to all then!

Comment 82

[Biju]

Just curious... 
Is there any chance ThunderBird send local file:// URL embedded in email message when sending attachment, forwarding mail with/without attachment etc.
I once experienced such an issue, ie my temp dir name was embedded in mail I send.
but could not reproduce it again.

I am asking it now because I see statement 

 attachment->SetUrl(uri);

in mailnews/compose/src/nsMsgCompose.cpp at the end of attachment 294661 [details] [diff] [review]

Comment 83

[rsx11m]

> (In reply to comment #82) send local file:// URL embedded in email
> message when sending attachment, forwarding mail with/without attachment etc.

That's probably bug 190298 comment #19 (attachment without a name). Since forwarding as attachment always generates a name, and the internal structure of the message itself isn't touched, I wouldn't expect any such issue here.