Last Comment Bug 764312 - Filelink UbuntuOne: cannot upload file with filenames containing '(' or ')'
: Filelink UbuntuOne: cannot upload file with filenames containing '(' or ')'
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: FileLink (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 16.0
Assigned To: James Henstridge
:
Mentors:
Depends on: 793749
Blocks: 744037
  Show dependency treegraph
 
Reported: 2012-06-13 03:05 PDT by Jb Piacentino
Modified: 2012-09-25 03:18 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix construction of OAuth signature base string (1.11 KB, patch)
2012-06-14 05:18 PDT, James Henstridge
no flags Details | Diff | Splinter Review
Fix only the encoding of the URL in the OAuth signature base string (854 bytes, patch)
2012-06-14 22:35 PDT, James Henstridge
mozilla: review-
florian: review-
Details | Diff | Splinter Review
Use PLAINTEXT auth (builds on top of attachment 614241 from bug 744035) (1.36 KB, patch)
2012-06-22 04:38 PDT, James Henstridge
mozilla: review+
Details | Diff | Splinter Review

Description Jb Piacentino 2012-06-13 03:05:59 PDT
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 2012-06-14 01:55:30 PDT
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 2012-06-14 05:18:30 PDT
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.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2012-06-14 08:14:33 PDT
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 2012-06-14 08:15:52 PDT
(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 2012-06-14 22:35:39 PDT
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.
Comment 6 Florian Quèze [:florian] [:flo] 2012-06-15 02:23:57 PDT
(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 2012-06-15 02:35:36 PDT
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] [:flo] 2012-06-15 02:40:17 PDT
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] [:flo] 2012-06-15 02:52:30 PDT
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 2012-06-15 03:29:16 PDT
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] [:flo] 2012-06-15 04:18:22 PDT
(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 2012-06-18 11:42:46 PDT
With this patch, I tried uploading a file with & in the name - it failed. Have you tried that, James?
Comment 13 David :Bienvenu 2012-06-18 11:44:37 PDT
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 2012-06-19 01:09:52 PDT
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] [:flo] 2012-06-19 02:54:44 PDT
(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 2012-06-22 04:38:16 PDT
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.
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-06-28 19:06:33 PDT
https://hg.mozilla.org/comm-central/rev/b6b7503a7ed3

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