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

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[Jb Piacentino]

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

Comment 1

[James Henstridge]

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.

Comment 2

[James Henstridge]

Attachment: 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.

Comment 3

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

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

[David :Bienvenu]

(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.

Comment 5

[James Henstridge]

Attachment: 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.

Comment 6

[Florian Quèze [:florian]]

(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.

Comment 7

[James Henstridge]

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 8

[Florian Quèze [:florian]]

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?

Comment 9

[Florian Quèze [:florian]]

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 :).

Comment 10

[James Henstridge]

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.

Comment 11

[Florian Quèze [:florian]]

(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 :).

Comment 12

[David :Bienvenu]

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

Comment 13

[David :Bienvenu]

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

minusing based on & issue.

Comment 14

[James Henstridge]

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.

Comment 15

[Florian Quèze [:florian]]

(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.

Comment 16

[James Henstridge]

Attachment: Use PLAINTEXT auth (builds on top of attachment 614241 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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/b6b7503a7ed3