Filelink UbuntuOne: cannot upload file with filenames containing '(' or ')'

RESOLVED FIXED in Thunderbird 16.0

Status

Thunderbird
FileLink
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jb Piacentino, Assigned: James Henstridge)

Tracking

Trunk
Thunderbird 16.0
x86_64
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Tried to filelink 'Agreement (clean).doc' to UbuntuOne and got an 'Upload error' dlg, saying "Unable to upload Agreement (clean).doc to Ubuntu One."
(Assignee)

Comment 1

5 years ago
This appears to be a problem with the OAuth signature on the request.  I was able to successfully attach a file with parentheses in its name after hacking oauth.jsm to write out a PLAINTEXT OAuth signature instead of HMAC-SHA1.

I'm still investigating to see which side the problem lies.
(Assignee)

Comment 2

5 years ago
Created attachment 633105 [details] [diff] [review]
Fix construction of OAuth signature base string

I've investigated the problem, and the following patch fixes the problem for me.

The code in oauth.jsm did not seem to be generating the signature base string correctly.  This patch reflects my understanding of the OAuth 1.0 spec (http://oauth.net/core/1.0a/#anchor13), and agrees with the behaviour of the Python OAuth library and another JavaScript OAuth library I've used (http://oauth.googlecode.com/svn/code/javascript/oauth.js).

I realise that this code is also shared by the DropBox backend, so some testing there is in order (I haven't yet done this).

I've only tested this against trunk, but it should also apply to the beta code base too.
Assignee: nobody → james
Status: NEW → ASSIGNED
Attachment #633105 - Flags: review?(dbienvenu)
This patch appears to break Dropbox account set up - I'm getting this back from Dropbox:

{"error": "OAuthError in API v1+. Request mis-signed: Invalid or Missing Signature"}

Comment 4

5 years ago
(In reply to Mike Conley (:mconley) from comment #3)
> This patch appears to break Dropbox account set up - I'm getting this back
> from Dropbox:
> 
> {"error": "OAuthError in API v1+. Request mis-signed: Invalid or Missing
> Signature"}

which makes it possible it breaks twitter auth, I think.
(Assignee)

Comment 5

5 years ago
Created attachment 633405 [details] [diff] [review]
Fix only the encoding of the URL in the OAuth signature base string

Okay, here is a simpler version that doesn't touch the encoding of the parameters in the signature base string.

The spec clearly says that the URL should be encoded according to http://oauth.net/core/1.0a/#encoding_parameters, so switching to percentEncode() there shouldn't be a compatibility issue.

Looking at the U1 backend code, I don't think there are any cases where the double vs. single encoding of data would cause problems, so this simpler patch seems to solve the problem highlighted by this bug.
Attachment #633105 - Attachment is obsolete: true
Attachment #633105 - Flags: review?(dbienvenu)
Attachment #633405 - Flags: review?(dbienvenu)
(In reply to James Henstridge from comment #5)

> The spec clearly says that the URL should be encoded according to
> http://oauth.net/core/1.0a/#encoding_parameters, so switching to
> percentEncode() there shouldn't be a compatibility issue.

The parameters are encoded at: http://hg.mozilla.org/comm-central/annotate/6be9b667699d/mail/base/modules/oauth.jsm#l113

The signature base is encoded with the .map(percentEncode) and the usage of already escaped & and = characters at http://hg.mozilla.org/comm-central/annotate/6be9b667699d/mail/base/modules/oauth.jsm#l122

Your previous patch was triple-encoding the parameters.

Also note that even if the parameters weren't already encoded at line 113, your previous patch wouldn't match the spec that says (http://oauth.net/core/1.0a/#anchor13) "All the request parameters MUST be encoded as described in Parameter Encoding prior to constructing the Signature Base String." To respect this, the parameters have to be encoded before they are sorted to build the signature base string.
(Assignee)

Comment 7

5 years ago
Yep, looks like I was mistaken.  I'll blame that on staring at long signature strings for too long last night.  In that case, the current version of the patch should be all that is needed to fix the signature calculation.
Comment on attachment 633405 [details] [diff] [review]
Fix only the encoding of the URL in the OAuth signature base string

>     let signatureBase =
>-      aMethod + "&" + encodeURIComponent(urlSpec) + "&" +
>+      percentEncode(aMethod) + "&" + percentEncode(urlSpec) + "&" +

I don't see any reason to call percentEncode on the method.
The spec says at http://oauth.net/core/1.0a/#rfc.section.9.1.3 "The HTTP request method used to send the request. Value MUST be uppercase, for example: HEAD, GET , POST, etc. "
Only uppercase letters are acceptable in the method, so if you really want to do something to "fix" the parameter, you can do aMethod.toUpperCase()
But there's no reason for the method to not be uppercase already, so I think it's reasonable to keep the aMethod value as is. However, it would be nice to fix the comment at http://hg.mozilla.org/comm-central/annotate/6be9b667699d/mail/base/modules/oauth.jsm#l75 to only contain uppercase examples of values ;).


For the urlSpec value, I think the change from encodeURIComponent to percentEncode won't hurt, although it shouldn't really be needed. urlSpec is the base URL that is usually constant. It doesn't contain parameters. How come you end up with parameters directly in the URL instead of having them in the query string as GET parameters?
Attachment #633405 - Flags: review-
I took the opportunity of reviewing this patch to also review the oauth.jsm file in general and I found 2 issues:

- The parameters provided in aOAuthParams http://hg.mozilla.org/comm-central/annotate/6be9b667699d/mail/base/modules/oauth.jsm#l80 are never percentEncoded.
The original use case for aOAuthParams encodes the value itself: http://hg.mozilla.org/comm-central/annotate/9c0bd4891167/chat/protocols/twitter/twitter.js#l741
The comment at line 180 should mention that the parameters provided in aOAuthParams are expected to already be encoded.

- The POST parameters aren't handled at all. The original code handled them like this: http://hg.mozilla.org/comm-central/annotate/9c0bd4891167/chat/protocols/twitter/twitter.js#l422
This coded didn't work for PUT requests so I guess it has been ditched at some point during the development of the Filelink feature, but not having it makes oauth.jsm completely unsuitable for Twitter.
Instead of removing it, I think the correct fix would have been to add a test like http://hg.mozilla.org/comm-central/annotate/9c0bd4891167/mail/base/modules/http.jsm#l60 that we added in bug 735219.

Neither of these issues seem to matter for UbuntuOne though :).
(Assignee)

Comment 10

5 years ago
The API used by the Ubuntu One back end is described here:

https://one.ubuntu.com/developer/files/store_files/cloud

Uploading files is performed via a PUT request, with the desired location and file name in the path.  So uploading a file with parentheses in the file name results in a URL containing parentheses.
(In reply to James Henstridge from comment #10)
> uploading a file with parentheses in the file
> name results in a URL containing parentheses.

Ok. I don't think that's the best API choice, but I guess changing it now would be super painful for not much benefit. As I said, I don't think the change from encodeURIComponent to percentEncode for the urlSpec would be a problem for other use cases, so go ahead with it :).
Assignee: james → nobody
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink

Comment 12

5 years ago
With this patch, I tried uploading a file with & in the name - it failed. Have you tried that, James?

Comment 13

5 years ago
Comment on attachment 633405 [details] [diff] [review]
Fix only the encoding of the URL in the OAuth signature base string

minusing based on & issue.
Attachment #633405 - Flags: review?(dbienvenu) → review-
(Assignee)

Comment 14

5 years ago
I've investigated the ampersand issue, and it looks like an issue on our side: the OAuth signature is correct with my patch, but the URL path is being unescaped on the server somewhere before signature verification.

I've filed a bug to look into this, but should be able to work around it in the U1 backend.

If there are too many more issues like this, it might make sense to extend oauth.jsm to also support PLAINTEXT mode and use that: we do all our requests over https, so it should still be safe from MITM attacks.
(In reply to James Henstridge from comment #14)

> If there are too many more issues like this, it might make sense to extend
> oauth.jsm to also support PLAINTEXT mode and use that: we do all our
> requests over https, so it should still be safe from MITM attacks.

There's already a patch adding support for the PLAINTEXT method (and other oauth.jsm extensions), see attachment 614241 [details] [diff] [review] in bug 744035.
(Assignee)

Comment 16

5 years ago
Created attachment 635692 [details] [diff] [review]
Use PLAINTEXT auth (builds on top of attachment 614241 [details] [diff] [review] from bug 744035)

Here is an alternative fix for the bug built on top of the OAuth PLAINTEXT patch from bug 744035 that hasn't been applied yet.  I've left a few of my own review comments about that patch, but it works well enough with the U1 back end as is.
Attachment #633405 - Attachment is obsolete: true
Attachment #635692 - Flags: review?(dbienvenu)

Updated

5 years ago
Depends on: 744035

Updated

5 years ago
Attachment #635692 - Flags: review?(dbienvenu) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b6b7503a7ed3
Assignee: nobody → james
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Depends on: 793749
(Assignee)

Updated

5 years ago
No longer depends on: 744035
You need to log in before you can comment on or make changes to this bug.