Last Comment Bug 765575 - Yousendit fails on some non-ASCII attachments
: Yousendit fails on some non-ASCII attachments
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: 13 Branch
: x86_64 All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Maxim Baz
:
Mentors:
: 771073 771131 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-17 05:42 PDT by Manuel Bärenz
Modified: 2013-10-13 04:42 PDT (History)
12 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
-
+
fixed


Attachments
Possigble fix for Thunderbird (1.98 KB, patch)
2012-06-18 02:18 PDT, Hiroyuki Ikezoe (:hiro)
no flags Details | Diff | Review
proposed fix (1.23 KB, patch)
2012-07-11 12:21 PDT, Maxim Baz
no flags Details | Diff | Review
proposed fix with tests (3.77 KB, patch)
2012-07-12 11:55 PDT, Maxim Baz
mconley: review+
standard8: approval‑comm‑aurora+
Details | Diff | Review
Fix patch [checked-in to comm-central] (1.23 KB, patch)
2012-07-16 06:27 PDT, Mike Conley (:mconley) - (Needinfo me!)
mconley: review+
standard8: approval‑comm‑aurora+
Details | Diff | Review
Tests (1.66 KB, patch)
2012-07-16 06:27 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Review
Mozmill test (2.54 KB, patch)
2012-07-16 08:12 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Review

Description Manuel Bärenz 2012-06-17 05:42:00 PDT
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] 2012-06-17 18:21:50 PDT
Ikezoe-san knows root cause.  He says both server and client have a bug.
Comment 2 Hiroyuki Ikezoe (:hiro) 2012-06-18 02:18:56 PDT
Created attachment 633972 [details] [diff] [review]
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 2012-06-19 09:13:34 PDT
Maxim, any thoughts on this patch or any other possible fix for this?
Comment 4 David :Bienvenu 2012-06-19 09:13:53 PDT
Maxim, see comment 3
Comment 5 David :Bienvenu 2012-06-27 15:04:03 PDT
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) 2012-06-27 15:18:54 PDT
A sample is:
∀.eml

I guess any non-ASCII characters cause the issue.
Comment 7 Makoto Kato [:m_kato] 2012-07-05 21:50:13 PDT
*** Bug 771073 has been marked as a duplicate of this bug. ***
Comment 8 Maxim Baz 2012-07-11 12:21:23 PDT
Created attachment 641156 [details] [diff] [review]
proposed fix

Hi, I'm attaching fix of this bug.
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-07-12 08:03:14 PDT
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 2012-07-12 09:04:28 PDT
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) - (Needinfo me!) 2012-07-12 09:08:46 PDT
(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 2012-07-12 10:36:57 PDT
(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 2012-07-12 11:55:31 PDT
Created attachment 641554 [details] [diff] [review]
proposed fix with tests

Added test.
Comment 14 Mike Conley (:mconley) - (Needinfo me!) 2012-07-13 07:18:16 PDT
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) 2012-07-13 10:06:41 PDT
Unfortunately this is too late to take in 14. We'll get it landed for 15 though.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-07-13 17:58:02 PDT
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] 2012-07-13 19:58:13 PDT
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 2012-07-14 13:06:57 PDT
Can you please also answer comment 12? Are non-ascii chars showing there ok?
Comment 19 Mike Conley (:mconley) - (Needinfo me!) 2012-07-16 06:11:41 PDT
(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) - (Needinfo me!) 2012-07-16 06:13:42 PDT
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) - (Needinfo me!) 2012-07-16 06:27:29 PDT
Created attachment 642559 [details] [diff] [review]
Fix patch [checked-in to comm-central]

This is the fix, extracted.
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-07-16 06:27:51 PDT
Created attachment 642560 [details] [diff] [review]
Tests

And here are the tests.
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-07-16 07:23:58 PDT
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) - (Needinfo me!) 2012-07-16 08:12:38 PDT
Created attachment 642594 [details] [diff] [review]
Mozmill test

Whoops, forgot to include the ∀.eml file.
Comment 25 Mike Conley (:mconley) - (Needinfo me!) 2012-07-16 08:42:59 PDT
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] 2013-03-28 07:29:34 PDT
Why is this still open ?
Comment 27 Sebastian H. [:aryx][:archaeopteryx] 2013-03-28 10:49:19 PDT
*** Bug 771131 has been marked as a duplicate of this bug. ***
Comment 28 Magnus Melin 2013-10-13 04:42:51 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.