350825 - When detaching, comma in target file name or path will break link to local file because X-Mozilla-External-Attachment-URL is truncated at first comma

Status: RESOLVED FIXED

(MailNews Core :: Attachments, defect)

Description

[Jörg Thönnes]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

If the attachment file name contains a comma ",", the file can be detached
successfully, the reference to the local file system is incomplete. The URL
in the mail contains the complete path up to, but not including the first comma.


Reproducible: Always

Steps to Reproduce:
1. Create a mail with an attachment, eg. "Attach, attach.doc"
2. Save mail as draft, possibly move it to some temp. TEST folder
3. Open saved mail and attachment -- OK
4. Detach attachment
5. Open saved mail with detached file
6. Open the attachment

Actual Results:  
File Not Found

The file .../Attach cannot be found. Please check the location and try again.

Expected Results:  
The detached file ".../Attach, attach.doc" can be located on the filesystem and opened successfully.

Comment 1

[David :Bienvenu]

I'll look into this...

Comment 2

[Andrew Williamson [:eviljeff]]

This applies to WinXP, not just Linux, and is still the case now.
Fixing it might just be a matter of encoding the ',' character before the uri is passed in as a parameter in startProcessing in nsMessenger (http://mxr.mozilla.org/mailnews/source/mailnews/base/src/nsMessenger.cpp#2979).  

Comment 3

[Andrew Williamson [:eviljeff]]

In my add-on I've rewritten the detaching part of messenger in JavaScript and simply doing 'replace(/,/g,"%2C")' to the uri string before its used seems to fix it.  I'm not sure what this would be in C++ or where the best place to put it would be though or I'd make the patch file.

Comment 6

[Keith Dear]

--- Comment #1 from WADA <[e-mail]>  2009-01-14 23:56:38 PST ---
(in response to my duplicate (oops) bug report 387639)
(Q1) Following header was generated after detach, wasn't it?
> > Content-Disposition: ...; filename="Dear, Keith - 13.04.09.doc"
> > X-Mozilla-External-Attachment-URL: file:///C:/.../Dear
> > ("," and string after "," is cut-off)
(Q2) Before detach, the attachment was opened successufully, wasn't it?

Q1: No, nothing was added to the headers after detach, even in View|Headers|All.
Q2: yes that is correct.

Comment 7

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #6)
> even in View|Headers|All.
Check mail source(View/Message Source) and check mail headers for the attachment part.

Comment 8

[Keith Dear]

ok then, yes, after detaching the file the source includes this:

Content-Type: application/msword;
 name="Dear, Keith - 13.04.09.doc"
Content-Disposition: attachment; filename="Dear, Keith - 13.04.09.doc"
X-Mozilla-External-Attachment-URL: file:///C:/temp/Dear
X-Mozilla-Altered: AttachmentDetached; date="Fri Jan 16 08:26:30 2009"

The original MIME headers for this attachment are:
Content-Type: application/msword;
 name="Dear, Keith - 13.04.09.doc"
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="Dear, Keith - 13.04.09.doc"

Comment 9

[Nicolas Thierry-Mieg]

this bug still occurs, using seamonkey 2.13 on linux x86_64.
Also, I believe bug 580253 is a dupe of this bug.

Comment 11

[Thomas D. (:thomas8)]

Jim, if comment 2 and comment 3 cited below are true, this looks like a one-line fix?

(In reply to Andrew Williamson [:eviljeff] from comment #2)
> Fixing it might just be a matter of encoding the ',' character before the
> uri is passed in as a parameter in startProcessing in nsMessenger
> (http://mxr.mozilla.org/mailnews/source/mailnews/base/src/nsMessenger.
> cpp#2979).

(In reply to Andrew Williamson [:eviljeff] from comment #3)
> In my add-on I've rewritten the detaching part of messenger in JavaScript
> and simply doing 'replace(/,/g,"%2C")' to the uri string before its used
> seems to fix it.

Comment 12

[Johannes]

This bug is still open and it's really annoying! Why is it so difficult to fix it? Can someone fix this, please? 
The behaviour looks a little similiar to bug 900117.

BTW: I wonder what people without the possibility to edit the message source do in such cases. I use Header Tools Lite -> Edit Full Source to correct the broken path.
https://addons.mozilla.org/en-US/thunderbird/addon/header-tools-lite/

Comment 13

[Don-Young Choe]

This bug is still alive and 12 years old. Fix this, please!
I have to use notepad to edit the message file with the help of URL-encoding tool. (My "Message Store Type" is "File per message (maildir)".)

Comment 15

[c.steinke]

This bug still exists. (Thunderbird 60.9.1, mbox file format).

I am using the attachment detach feature a lot. With this bug, I cannot trust it anymore.
Please please please fix this bug!
Thanks

Comment 16

[Chris]

Thunderbird 68.4.2 again truncates the link to a detached file at the first comma in the filename

See https://support.mozilla.org/en-US/questions/1278071#answer-1285865

Comment 17

[HansPL]

Still exists in 68.6.0 (64-Bit) Linux.

Comment 18

[HansPL]

Attachment: detaching_commas.eml.pdf

Example of defective .eml with the errors highlighted.

Comment 19

[Chris]

Attachment: Thunderbird 68.12.1 Troubleshooting Information

Problem recurs under 68.12.1

Comment 20

[HansPL]

Still defective in 78.3.1…  what a shame.

Comment 21

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/42a71ab4cc114ddc6bb78af20937e559b7b8e59c/mailnews/mime/src/mimeobj.cpp#172
and https://searchfox.org/comm-central/rev/42a71ab4cc114ddc6bb78af20937e559b7b8e59c/mailnews/base/src/nsMessenger.cpp#2517-2528

Hmm, aren't the file names encoded?

Comment 22

[Val Kulkov]

Fifteen years later, Thunderbird 78.11.0. The bug is still there. It looks like all that needs to be done is add the comma in mDetachedFileUris() to the list of characters that need to be URLencoded, just like it is done for the space characters.

Comment 23

[Rachel Martin]

Val, can you please clarify this suggestion. mDetachedFileUris is a list of URIs defined here:
https://searchfox.org/comm-central/rev/fa532a6d47183bed8d0a5016ff27e092f791c9e1/mailnews/base/src/nsMessenger.cpp#2319
So which "list of characters" are you referring to? You don't need to submit a patch, you could just copy the modified code into a comment here.

Comment 24

[Rachel Martin]

This is where the URI of the saved file is constructed:
https://searchfox.org/comm-central/rev/20577b15cbba0144791385497892348220fa3037/mailnews/base/src/nsMessenger.cpp#704-709
Apparently a comma is valid in a URI and not percent-encoded.

Later on, the damage happens here:
https://searchfox.org/comm-central/rev/20577b15cbba0144791385497892348220fa3037/mailnews/base/src/nsMessenger.cpp#2625
where URLs are strung together with a comma, and then later parsed here:
https://searchfox.org/comm-central/rev/20577b15cbba0144791385497892348220fa3037/mailnews/mime/src/mimeobj.cpp#172
The design issue lies in stringing file URLs together with a comma and later splitting at a comma, when a comma can be part of the file URLs.

A hacky one-line fix could be to percent-encode the comma before nsMessenger.cpp#2625. That's a bit like the suggestion in comment #22 which in itself is not a solution.

Comment 25

[Rachel Martin]

https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/350825-commas-detached-attachment.patch

Comment 26

[Val Kulkov]

Great investigative work, Rachel! I think you pinned it. Your patch in comment #25 makes total sense to me. I looked for what I meant by saying the "list of characters" in comment #22 and I could not find it :-(

Comment 27

[BB]

Attachment: 350825-commas-detached-attachment.patch

Comment 28

[Magnus Melin [:mkmelin]]

Comment on attachment 9331104 [details] [diff] [review]
350825-commas-detached-attachment.patch

Looks good, r=mkmelin

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ebd51aa99865
Percent-encode commas in URI of detached attachment. r=mkmelin