1747977 - Cannot open attachment when its filename contains an illegal characters e.g. ":", slash

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Robert]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:94.0) Gecko/20100101 Firefox/94.0

Steps to reproduce:

I use: TB 91.4.1 x64 (pl-PL), Win 8.1 Pro x64 / Win 10 x64 (21H1) - bug is on many different machines - at the same time (after automatically upgrade existing installation to 91.4.1)
My accounts use: POP3

When i receive an new mail with attached PDF files witch contains an illegal (in filenames) character - i have ":", i cannot open this by clicking on its filename (the down left corner of the window) when its assigned to external PDF Reader (i use Acrobat Reader DC), when its assigned to open internally to Thunderbird, then it open fine.

Actual results:

When i click to open attachment with illegal chars nothing is happens (when PDF file is attached to external program), when attached PDF is assigned and opened internally in Thunderbird is opened fine.

This is observed in TB 91.4.1, the TB 68.12.1 and 78.14.0 i think is not affected - the same messages (*.eml file) are working fine until now, when TB is upgraded to 91.x - no user reports about this until yesterday (massive TB upgrade to 91.4.1 - resolved bug with S/MIME and forward PDF attachment)

TB 68.12.1 / 78.14.0 and 91.3.2 - removes illegal chars before open directly attached files, but 91.4.1 is not

example of failure filename: " Numer polisy: 1234567878901 _ Numer EH: 032123456789 _ Nazwa Klienta: COMPANY NAME LLC _ Kod decyzji: XY _ Numer IDS: 12345678901 .pdf" - SPACE character is on the first place - this is not a copy/paste mistake :)

In the error console we can see 2 bugs:

[Exception... "Component returned failure code: 0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH) [nsIXPCComponents_Utils.readUTF8URI]" nsresult: "0x80520001 (NS_ERROR_FILE_UNRECOGNIZED_PATH)" location: "JS frame :: resource://gre/modules/L10nRegistry.jsm :: L10nRegistry.loadSync :: line 692" data: no] L10nRegistry.jsm:692:19
loadSync resource://gre/modules/L10nRegistry.jsm:692
fetchFile resource://gre/modules/L10nRegistry.jsm:607
generateResourceSetSync resource://gre/modules/L10nRegistry.jsm:512
map self-hosted:221
generateResourceSetSync resource://gre/modules/L10nRegistry.jsm:507
generateResourceSetsForLocaleSync resource://gre/modules/L10nRegistry.jsm:449
next self-hosted:1430
generateBundlesSync resource://gre/modules/L10nRegistry.jsm:186
next self-hosted:1430
touchNext resource://gre/modules/Localization.jsm:167
generateBundles resource://gre/modules/Localization.jsm:473
<anonimowa> resource:///modules/OTRUI.jsm:16
get resource://gre/modules/XPCOMUtils.jsm:62
_str resource:///modules/OTRUI.jsm:20
initStrings resource:///modules/OTRUI.jsm:38
init resource:///modules/OTRUI.jsm:259
InterpretGeneratorResume self-hosted:1482
AsyncFunctionNext self-hosted:692

---

Uncaught (in promise)
Exception { name: "", message: "Component returned failure code: 0x8071007b [nsIFile.createUnique]", result: 2154889339, filename: "chrome://messenger/content/msgHdrView.js", lineNumber: 2017, columnNumber: 0, data: null, stack: "saveAndOpen@chrome://messenger/content/msgHdrView.js:2017:18\nopen@chrome://messenger/content/msgHdrView.js:2052:19\n", location: XPCWrappedNative_NoHelper }
​
columnNumber: 0
​
data: null
​
filename: "chrome://messenger/content/msgHdrView.js"
​
lineNumber: 2017
​
location: XPCWrappedNative_NoHelper { QueryInterface: QueryInterface(), filename: Getter, name: Getter, … }
​
message: "Component returned failure code: 0x8071007b [nsIFile.createUnique]"
​
name: ""
​
result: 2154889339
​
stack: "saveAndOpen@chrome://messenger/content/msgHdrView.js:2017:18\nopen@chrome://messenger/content/msgHdrView.js:2052:19\n"
​
<prototype>: ExceptionPrototype { toString: toString(), name: Getter, message: Getter, … }

Expected results:

TB should open attached PDF in external programs as usually in previous version of TB like 68.x and 78.x and older 91.x versions

Comment 1

[Robert]

The same thing is happens w.g. when i open a message from Sent folder.

Comment 2

[Magnus Melin [:mkmelin]]

Must be from bug 1737711.

Comment 3

[Robert]

For now, works only right click on filename, select Save Attachment As (illegal chars are removed properly ), then we must manually open file form saved location in external program.

Comment 4

[Anje]

Also report in Support Forum regarding illegal char using a forward slash:
https://support.mozilla.org/en-US/questions/1362678

Comment 5

[Rachel Martin]

Looks like GenerateValidFilename() needs to be called in this new code from bug 1737711/bug 1741820:
https://searchfox.org/comm-central/rev/b54051609fd9b2633dba4045ce9d189ce63660b4/mail/base/content/msgHdrView.js#2120-2240

Comment 6

[Rachel Martin]

https://github.com/Betterbird/thunderbird-patches/blob/main/91/bugs/1747977-sanitise-filename.patch
Also cleans up code that determines the extension twice with two different tricks, one of which doesn't work for edge cases.

Comment 7

[Rachel Martin]

Patch depends on our fix for bug 1744709.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1747977 - Sanitize attachment file name before saving to disk. r=mkmelin

Calls DownloadPaths.sanitize on the file name, which replaces invalid characters. Also adds a way for a test to "open" attachments, allowing this action to be tested.

Comment 9

[Rachel Martin]

How about fixing the other issues with this code as well? Getting the extension twice, once with this.name.split(".").pop(), which doesn't work if there is no extension, and another time with let match = this.name.match(/\.([^.]+)$/); let extension = match ? match[1] : null; should be fixed. Is the proposed new behaviour consistent with
https://searchfox.org/comm-central/rev/d55e12934777fded86d5ad1e8bd6a6d3a8ac32b0/mail/base/content/mailCommands.js#476-498 ?

Comment 10

[Geoff Lankow (:darktrojan)]

(In reply to Rachel Martin from comment #9)

this.name.split(".").pop(), which doesn't work if there is no extension

Where this runs there is always a content type (which has priority) or an extension. In the toolkit version of this code the arguments are hard-wired in, which we should do, but not in this bug.

let match = this.name.match(/\.([^.]+)$/); let extension = match ? match[1] : null; should be fixed.

I don't see a problem with this. Explain?

Is the proposed new behaviour consistent with
https://searchfox.org/comm-central/rev/d55e12934777fded86d5ad1e8bd6a6d3a8ac32b0/mail/base/content/mailCommands.js#476-498 ?

It's similar but it does catch some more problem cases. AFAICT the old behaviour isn't consistent with it either.

Comment 11

[Rachel Martin]

The observation is that the extension is determined twice with different results:
console.log("file".split(".").pop()) gives file, let match = "file".match(/\.([^.]+)$/); let extension = match ? match[1] : null; console.log(extension) gives null. Given that getFromTypeAndExtension() apparently prefers the type and the extension can be empty,
https://searchfox.org/mozilla-central/rev/83e67336083df9f9a3d1e0c33f2ba19703d57161/netwerk/mime/nsIMIMEService.idl#42
this code will likely work:
https://searchfox.org/comm-central/rev/4e02ce9fa250616e3a8091b8fea0990a339c0e63/mail/base/content/msgHdrView.js#2066
Either the type is application/pdf and will be preferred and the wrong result ignored or there is a ".pdf" present and the split/pop will yield the correct result.

For the sake of clarity we'd prefer to determine the extension once and not twice with a method that always gives the correct result instead of relying on other conditions where a wrong result doesn't cause an issue. Whether using a regular expression isn't too big a gun to determine the the characters after the last dot lies in the eyes of the beholder. We'd suggest:
let dotPos = name.lastIndexOf("."); let extension = dotPos >= 0 ? name.substr(dotPos + 1) : "";

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cf349129e14e
Sanitize attachment file name before saving to disk. r=mkmelin

Comment 13

[Rachel Martin]

Attachment: 1747977-follow-up-extension-handling.patch

Comment 14

[Rachel Martin]

Attachment: 1747977-follow-up-extension-handling.patch

Linted. Also note the API nsIMIMEInfo getFromTypeAndExtension(in ACString aMIMEType, in AUTF8String aFileExt);, so it makes more sense to pass an empty string instead of null.

Comment 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 9258268 [details]
Bug 1747977 - Sanitize attachment file name before saving to disk. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): bug 1737711
User impact if declined: attachments with weird filenames won't open
Testing completed (on c-c, etc.): landed last week
Risk to taking this patch (and alternatives if risky): low but not zero

Comment 17

[Wayne Mery (:wsmwk)]

Comment on attachment 9258268 [details]
Bug 1747977 - Sanitize attachment file name before saving to disk. r=mkmelin

[Triage Comment]
Approved for beta

Comment 18

[Rachel Martin]

Can't set "checkin-needed" on this bug.

Comment 19

[Magnus Melin [:mkmelin]]

Landing it soon.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1b9aae48e071
Follow-up: Tidy extension handling. r=darktrojan

Comment 21

[Rob Lemley [:rjl]]

Thunderbird 97.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/11f6cee78510
https://hg.mozilla.org/releases/comm-beta/rev/b497507e8e2e

Comment 22

[Robert]

May i ask when this patch will be applied to 91esr version of TB ? :)

Comment 23

[Magnus Melin [:mkmelin]]

After beta testing is successful (it just landed on beta).

Comment 26

[Wayne Mery (:wsmwk)]

Comment on attachment 9258268 [details]
Bug 1747977 - Sanitize attachment file name before saving to disk. r=mkmelin

[Triage Comment]
Approved for esr91

Comment 27

[Rob Lemley [:rjl]]

browser_openAttachment.js in this patch is tangled up in the async changes from Bug 1676114. Geoff, can you rebase it for esr91?

Comment 28

[Geoff Lankow (:darktrojan)]

Attachment: 1747977-attachment-sanitize-esr91.diff

I added Rachel's changes, since they might as well go in too.

Comment 29

[Rob Lemley [:rjl]]

Comment on attachment 9262210 [details] [diff] [review]
1747977-attachment-sanitize-esr91.diff

[Triage Comment]
Moving esr91 approval to correct patch.

Comment 30

[Rob Lemley [:rjl]]

Thunderbird 91.6.0:
https://hg.mozilla.org/releases/comm-esr91/rev/0a36181bb9dc