Yousendit fails on some non-ASCII attachments

RESOLVED FIXED in Thunderbird 16.0

Status

Thunderbird
FileLink
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Manuel Bärenz, Assigned: Maxim Baz)

Tracking

13 Branch
Thunderbird 16.0
x86_64
All
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird14-, thunderbird15+ fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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.
Ikezoe-san knows root cause.  He says both server and client have a bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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

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

Comment 4

5 years ago
Maxim, see comment 3
tracking-thunderbird14: --- → +

Comment 5

5 years ago
Can someone give an exact file name that causes the issue so the YSI folks can reproduce the bug? thx!
A sample is:
∀.eml

I guess any non-ASCII characters cause the issue.

Updated

5 years ago
Duplicate of this bug: 771073
(Assignee)

Comment 8

5 years ago
Created attachment 641156 [details] [diff] [review]
proposed fix

Hi, I'm attaching fix of this bug.

Updated

5 years ago
Attachment #641156 - Flags: review?(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?
(Assignee)

Comment 10

5 years ago
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.
(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

5 years ago
(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.)
(Assignee)

Comment 13

5 years ago
Created attachment 641554 [details] [diff] [review]
proposed fix with tests

Added test.
Attachment #641156 - Attachment is obsolete: true
Attachment #641156 - Flags: review?(mconley)

Updated

5 years ago
Attachment #641554 - Flags: review?(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.
Attachment #641554 - Flags: review?(mconley) → review+

Updated

5 years ago
Assignee: nobody → mbaz

Updated

5 years ago
Attachment #641554 - Flags: approval-comm-aurora?

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Attachment #633972 - Attachment is obsolete: true

Updated

5 years ago
OS: Linux → All
Unfortunately this is too late to take in 14. We'll get it landed for 15 though.
tracking-thunderbird14: + → -
tracking-thunderbird15: --- → +
Attachment #641554 - Flags: approval-comm-aurora? → approval-comm-aurora+
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-thunderbird15: --- → fixed
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
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
Status: RESOLVED → REOPENED
status-thunderbird15: fixed → ---
Resolution: FIXED → ---

Comment 18

5 years ago
Can you please also answer comment 12? Are non-ascii chars showing there ok?
(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.
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.
Created attachment 642559 [details] [diff] [review]
Fix patch [checked-in to comm-central]

This is the fix, extracted.
Attachment #641554 - Attachment is obsolete: true
Attachment #642559 - Flags: review+
Attachment #642559 - Flags: approval-comm-aurora?
Created attachment 642560 [details] [diff] [review]
Tests

And here are the tests.
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
Attachment #642559 - Attachment description: Fix patch → Fix patch [checked-in to comm-central]
Created attachment 642594 [details] [diff] [review]
Mozmill test

Whoops, forgot to include the ∀.eml file.
Attachment #642560 - Attachment is obsolete: true
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
status-thunderbird15: --- → fixed
Attachment #642559 - Flags: approval-comm-aurora? → approval-comm-aurora+
Why is this still open ?
Duplicate of this bug: 771131

Comment 28

4 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.