765575 - Yousendit fails on some non-ASCII attachments

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[Manuel Bärenz]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120608021316

Steps to reproduce:

I was trying to send some attachments to a friend via filelink/yousendit.


Actual results:

It failed on several of them exactly when they contained non-ASCII letters in the filenames, such as german umlauts: The files were displayed as being uploaded and the links were included in the file, but when one tries to download them, it said that there were no files attached to this link.

Comment 1

[Makoto Kato [:m_kato]]

Ikezoe-san knows root cause.  He says both server and client have a bug.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: Possigble fix for Thunderbird

Unfortunately YouSendIt does not support RFC2231 yet, I suppose this patch fixes the issue in Thunderbird if YouSentIt spports RFC2231.

Comment 3

[David :Bienvenu]

Maxim, any thoughts on this patch or any other possible fix for this?

Comment 4

[David :Bienvenu]

Maxim, see comment 3

Comment 5

[David :Bienvenu]

Can someone give an exact file name that causes the issue so the YSI folks can reproduce the bug? thx!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

A sample is:
∀.eml

I guess any non-ASCII characters cause the issue.

Comment 8

[Maxim Baz]

Attachment: proposed fix

Hi, I'm attaching fix of this bug.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 641156 [details] [diff] [review]
proposed fix

Review of attachment 641156 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/cloudfile/nsYouSendIt.js
@@ +1019,5 @@
>      let fileContents = "--" + boundary +
>        "\r\nContent-Disposition: form-data; name=\"bid\"\r\n\r\n" +
>         this._urlInfo.fileId;
>  
> +    let fileName = /^[\040-\176]+$/.test(this.file.leafName) 

Why do we do this check, instead of just encoding the leafName with encodeURIComponent every time?

Comment 10

[Maxim Baz]

Dennis comments:
If file name contains only US-ASCII with space character, then the encoded file name will have %20 in the file name, e.g. (my%20file.txt) which doesn't provide a good user experience.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to Maxim Baz from comment #10)
> Dennis comments:
> If file name contains only US-ASCII with space character, then the encoded
> file name will have %20 in the file name, e.g. (my%20file.txt) which doesn't
> provide a good user experience.

Alright, that sounds reasonable.

The fix looks OK, but I'm unable to test it currently (the YouSendIt servers seem to be timing out on profile retrieval requests again).

Could you please add a test for this in test-cloudfile-backend-yousendit.js?

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to Maxim Baz from comment #10)
> Dennis comments:
> If file name contains only US-ASCII with space character, then the encoded
> file name will have %20 in the file name, e.g. (my%20file.txt) which doesn't
> provide a good user experience.

I dunno where that my%20file.txt name would be showing but obviously that place should be using decodeURIComponent or it wouldn't display non-ascii files reasonable. (If it's somewhere server-side, the server side is buggy.)

Comment 13

[Maxim Baz]

Attachment: proposed fix with tests

Added test.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 641554 [details] [diff] [review]
proposed fix with tests

Code looks good, tests pass, and I've confirmed that this fixes the issue.

Thanks for the test case - highly appreciated.

Comment 15

[Mark Banner (:standard8)]

Unfortunately this is too late to take in 14. We'll get it landed for 15 though.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/42fdd61be0ba
https://hg.mozilla.org/releases/comm-aurora/rev/f0adfc883ef2

Thanks for the patch, Maxim! One request, for future patches, please follow the directions at the link below. It makes life easier for those checking in on your behalf. Thanks again!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 17

[Ryan VanderMeulen [:RyanVM]]

And backed out due to failures of the new test on OSX.
https://hg.mozilla.org/comm-central/rev/85c33a9e9f46
https://hg.mozilla.org/releases/comm-aurora/rev/6d7cc83a5dfc

Comment 18

[Magnus Melin [:mkmelin]]

Can you please also answer comment 12? Are non-ascii chars showing there ok?

Comment 19

[Mike Conley (:mconley) (:⚙️)]

(In reply to Magnus Melin from comment #18)
> Can you please also answer comment 12? Are non-ascii chars showing there ok?

When clicking on the link, it seems that the user is sent to a "download" page, which shows the filename of the file about to be downloaded.

Unfortunately, it doesn't appear to URL decode the filename on the server side, so the name appears like: my%20file.txt.

Maxim - you might want to file a bug internally at YouSendIt about that.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

So the tests for this patch are failing on OSX on our Mozmill testing machines.

This is one of those frustrating situations where I cannot seem to reproduce the failure locally, so I've been doing try builds.

I've been able to determine that the failure is occurring because the ∀.eml apparently does not exist at runtime for the tests.

I'm going to assume that this is a packaging problem, and will consult Mark Banner about this. In the meantime, I'll split the test out and we can land the fix and the test later.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: Fix patch [checked-in to comm-central]

This is the fix, extracted.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Tests

And here are the tests.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 642559 [details] [diff] [review]
Fix patch [checked-in to comm-central]

Attachment 642559 [details] [diff] checked in to comm-central: https://hg.mozilla.org/comm-central/rev/ff0d44ec5e82

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Attachment: Mozmill test

Whoops, forgot to include the ∀.eml file.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-aurora, assuming Standard8's approval (since the applied patch is a subset of the one that already had approval, and Standard8 is super busy).

https://hg.mozilla.org/releases/comm-aurora/rev/4bc10d6ac415

Comment 26

[Ludovic Hirlimann [:Usul]]

Why is this still open ?

Comment 28

[Magnus Melin [:mkmelin]]

Indeed, it landed on comm-central too - http://hg.mozilla.org/comm-central/rev/ff0d44ec5e82

No need to keep it open to fix the server side bug (comment 19) - if that's still a problem.