OAuthSignatureCalculator mustn't re-encodes query params, close AsyncHttpClient#921

Author: slandelle

api/src/main/java/org/asynchttpclient/oauth/OAuthSignatureCalculator.java

@@ -128,34 +128,33 @@ private String encodedParams(long oauthTimestamp, String nonce, List<Param> form
         OAuthParameterSet allParameters = new OAuthParameterSet(allParametersSize);
 
         // start with standard OAuth parameters we need
-        allParameters.add(KEY_OAUTH_CONSUMER_KEY, consumerAuth.getKey());
+        allParameters.add(KEY_OAUTH_CONSUMER_KEY, Utf8UrlEncoder.encodeQueryElement(consumerAuth.getKey()));
         allParameters.add(KEY_OAUTH_NONCE, nonce);
         allParameters.add(KEY_OAUTH_SIGNATURE_METHOD, OAUTH_SIGNATURE_METHOD);
         allParameters.add(KEY_OAUTH_TIMESTAMP, String.valueOf(oauthTimestamp));
         if (userAuth.getKey() != null) {
-            allParameters.add(KEY_OAUTH_TOKEN, userAuth.getKey());
+            allParameters.add(KEY_OAUTH_TOKEN, Utf8UrlEncoder.encodeQueryElement(userAuth.getKey()));
         }
         allParameters.add(KEY_OAUTH_VERSION, OAUTH_VERSION_1_0);
 
         if (formParams != null) {
             for (Param param : formParams) {
-                allParameters.add(param.getName(), param.getValue());
+                // formParams are not already encoded
+                allParameters.add(Utf8UrlEncoder.encodeQueryElement(param.getName()), Utf8UrlEncoder.encodeQueryElement(param.getValue()));
             }
         }
         if (queryParams != null) {
             for (Param param : queryParams) {
+             // queryParams are already encoded
                 allParameters.add(param.getName(), param.getValue());
             }
         }
         return allParameters.sortAndConcat();
     }
 
-    /**
-     * Method for calculating OAuth signature using HMAC/SHA-1 method.
-     */
-    public String calculateSignature(String method, Uri uri, long oauthTimestamp, String nonce,
+    StringBuilder signatureBaseString(String method, Uri uri, long oauthTimestamp, String nonce,
                                      List<Param> formParams, List<Param> queryParams) {
-
+        
         // beware: must generate first as we're using pooled StringBuilder
         String baseUrl = baseUrl(uri);
         String encodedParams = encodedParams(oauthTimestamp, nonce, formParams, queryParams);
@@ -169,6 +168,16 @@ public String calculateSignature(String method, Uri uri, long oauthTimestamp, St
         // and all that needs to be URL encoded (... again!)
         sb.append('&');
         Utf8UrlEncoder.encodeAndAppendQueryElement(sb, encodedParams);
+        return sb;
+    }
+    
+    /**
+     * Method for calculating OAuth signature using HMAC/SHA-1 method.
+     */
+    public String calculateSignature(String method, Uri uri, long oauthTimestamp, String nonce,
+                                     List<Param> formParams, List<Param> queryParams) {
+
+        StringBuilder sb = signatureBaseString(method, uri, oauthTimestamp, nonce, formParams, queryParams);
 
         ByteBuffer rawBase = StringUtils.charSequence2ByteBuffer(sb, UTF_8);
         byte[] rawSignature = mac.digest(rawBase);
@@ -202,18 +211,18 @@ private String constructAuthHeader(String signature, String nonce, long oauthTim
         return sb.toString();
     }
 
-    protected synchronized String generateNonce() {
+    protected long generateTimestamp() {
+        return System.currentTimeMillis() / 1000L;
+    }
+
+    protected String generateNonce() {
         byte[] nonceBuffer = NONCE_BUFFER.get();
         ThreadLocalRandom.current().nextBytes(nonceBuffer);
         // let's use base64 encoding over hex, slightly more compact than hex or decimals
         return Base64.encode(nonceBuffer);
 //      return String.valueOf(Math.abs(random.nextLong()));
     }
 
-    protected long generateTimestamp() {
-        return System.currentTimeMillis() / 1000L;
-    }
-
     /**
      * Container for parameters used for calculating OAuth signature.
      * About the only confusing aspect is that of whether entries are to be sorted
@@ -230,8 +239,7 @@ public OAuthParameterSet(int size) {
         }
 
         public OAuthParameterSet add(String key, String value) {
-            Parameter p = new Parameter(Utf8UrlEncoder.encodeQueryElement(key), Utf8UrlEncoder.encodeQueryElement(value));
-            allParameters.add(p);
+            allParameters.add(new Parameter(key, value));
             return this;
         }
 

api/src/test/java/org/asynchttpclient/oauth/OAuthSignatureCalculatorTest.java

@@ -34,8 +34,9 @@
 /**
  * Tests the OAuth signature behavior.
  *
- * See <a href="https://oauth.googlecode.com/svn/code/javascript/example/signature.html">Signature Tester</a> for an
- * online oauth signature checker.
+ * See <a href=
+ * "https://oauth.googlecode.com/svn/code/javascript/example/signature.html"
+ * >Signature Tester</a> for an online oauth signature checker.
  *
  */
 public class OAuthSignatureCalculatorTest {
@@ -52,16 +53,16 @@ public class OAuthSignatureCalculatorTest {
     final static long TIMESTAMP = 1191242096;
 
     private static class StaticOAuthSignatureCalculator extends OAuthSignatureCalculator {
-        
+
         private final long timestamp;
         private final String nonce;
-        
+
         public StaticOAuthSignatureCalculator(ConsumerKey consumerAuth, RequestToken userAuth, long timestamp, String nonce) {
             super(consumerAuth, userAuth);
-            this.timestamp = timestamp;   
+            this.timestamp = timestamp;
             this.nonce = nonce;
         }
-        
+
         @Override
         protected long generateTimestamp() {
             return timestamp;
@@ -72,7 +73,64 @@ protected String generateNonce() {
             return nonce;
         }
     }
-    
+
+    // sample from RFC https://tools.ietf.org/html/rfc5849#section-3.4.1
+    private void testSignatureBaseString(Request request) {
+        ConsumerKey consumer = new ConsumerKey("9djdj82h48djs9d2", CONSUMER_SECRET);
+        RequestToken user = new RequestToken("kkk9d7dh3k39sjv7", TOKEN_SECRET);
+        OAuthSignatureCalculator calc = new OAuthSignatureCalculator(consumer, user);
+
+        String signatureBaseString = calc.signatureBaseString(//
+                request.getMethod(),//
+                request.getUri(),//
+                137131201,//
+                "7d8f3e4a",//
+                request.getFormParams(),//
+                request.getQueryParams()).toString();
+
+        assertEquals(signatureBaseString, "POST&" //
+                + "http%3A%2F%2Fexample.com%2Frequest" //
+                + "&a2%3Dr%2520b%26"//
+                + "a3%3D2%2520q%26" + "a3%3Da%26"//
+                + "b5%3D%253D%25253D%26"//
+                + "c%2540%3D%26"//
+                + "c2%3D%26"//
+                + "oauth_consumer_key%3D9djdj82h48djs9d2%26"//
+                + "oauth_nonce%3D7d8f3e4a%26"//
+                + "oauth_signature_method%3DHMAC-SHA1%26"//
+                + "oauth_timestamp%3D137131201%26"//
+                + "oauth_token%3Dkkk9d7dh3k39sjv7%26"//
+                + "oauth_version%3D1.0");
+    }
+
+    @Test(groups = "fast")
+    public void testSignatureBaseStringWithProperlyEncodedUri() {
+
+        Request request = new RequestBuilder("POST")//
+                .setUrl("http://example.com/request?b5=%3D%253D&a3=a&c%40=&a2=r%20b")//
+                .addFormParam("c2", "")//
+                .addFormParam("a3", "2 q")//
+                .build();
+
+        testSignatureBaseString(request);
+    }
+
+    @Test(groups = "fast")
+    public void testSignatureBaseStringWithRawUri() {
+
+        // note: @ is legal so don't decode it into %40 because it won't be
+        // encoded back
+        // note: we don't know how to fix a = that should have been encoded as
+        // %3D but who would be stupid enough to do that?
+        Request request = new RequestBuilder("POST")//
+                .setUrl("http://example.com/request?b5=%3D%253D&a3=a&c%40=&a2=r b")//
+                .addFormParam("c2", "")//
+                .addFormParam("a3", "2 q")//
+                .build();
+
+        testSignatureBaseString(request);
+    }
+
     // based on the reference test case from
     // http://oauth.pbwiki.com/TestCases
     @Test(groups = "fast")
@@ -99,16 +157,20 @@ public void testPostCalculateSignature() {
         formParams.add(new Param("file", "vacation.jpg"));
         formParams.add(new Param("size", "original"));
         String url = "http://photos.example.net/photos";
-        final Request req = new RequestBuilder("POST")
-                .setUri(Uri.create(url))
-                .setFormParams(formParams)
-                .setSignatureCalculator(calc).build();
+        final Request req = new RequestBuilder("POST")//
+                .setUri(Uri.create(url))//
+                .setFormParams(formParams)//
+                .setSignatureCalculator(calc)//
+                .build();
 
         // From the signature tester, POST should look like:
-        // normalized parameters: file=vacation.jpg&oauth_consumer_key=dpf43f3p2l4k3l03&oauth_nonce=kllo9940pd9333jh&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242096&oauth_token=nnch734d00sl2jdk&oauth_version=1.0&size=original
-        // signature base string: POST&http%3A%2F%2Fphotos.example.net%2Fphotos&file%3Dvacation.jpg%26oauth_consumer_key%3Ddpf43f3p2l4k3l03%26oauth_nonce%3Dkllo9940pd9333jh%26oauth_signature_method%3DHMAC-SHA1%26oauth_timestamp%3D1191242096%26oauth_token%3Dnnch734d00sl2jdk%26oauth_version%3D1.0%26size%3Doriginal
+        // normalized parameters:
+        // file=vacation.jpg&oauth_consumer_key=dpf43f3p2l4k3l03&oauth_nonce=kllo9940pd9333jh&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242096&oauth_token=nnch734d00sl2jdk&oauth_version=1.0&size=original
+        // signature base string:
+        // POST&http%3A%2F%2Fphotos.example.net%2Fphotos&file%3Dvacation.jpg%26oauth_consumer_key%3Ddpf43f3p2l4k3l03%26oauth_nonce%3Dkllo9940pd9333jh%26oauth_signature_method%3DHMAC-SHA1%26oauth_timestamp%3D1191242096%26oauth_token%3Dnnch734d00sl2jdk%26oauth_version%3D1.0%26size%3Doriginal
         // signature: wPkvxykrw+BTdCcGqKr+3I+PsiM=
-        // header: OAuth realm="",oauth_version="1.0",oauth_consumer_key="dpf43f3p2l4k3l03",oauth_token="nnch734d00sl2jdk",oauth_timestamp="1191242096",oauth_nonce="kllo9940pd9333jh",oauth_signature_method="HMAC-SHA1",oauth_signature="wPkvxykrw%2BBTdCcGqKr%2B3I%2BPsiM%3D"
+        // header: OAuth
+        // realm="",oauth_version="1.0",oauth_consumer_key="dpf43f3p2l4k3l03",oauth_token="nnch734d00sl2jdk",oauth_timestamp="1191242096",oauth_nonce="kllo9940pd9333jh",oauth_signature_method="HMAC-SHA1",oauth_signature="wPkvxykrw%2BBTdCcGqKr%2B3I%2BPsiM%3D"
 
         String authHeader = req.getHeaders().get("Authorization").get(0);
         Matcher m = Pattern.compile("oauth_signature=\"(.+?)\"").matcher(authHeader);
@@ -135,19 +197,23 @@ public void testGetWithRequestBuilder() {
         queryParams.add(new Param("size", "original"));
         String url = "http://photos.example.net/photos";
 
-        final Request req = new RequestBuilder("GET")
-                .setUri(Uri.create(url))
-                .setQueryParams(queryParams)
-                .setSignatureCalculator(calc).build();
+        final Request req = new RequestBuilder("GET")//
+                .setUri(Uri.create(url))//
+                .setQueryParams(queryParams)//
+                .setSignatureCalculator(calc)//
+                .build();
 
         final List<Param> params = req.getQueryParams();
         assertEquals(params.size(), 2);
-        
+
         // From the signature tester, the URL should look like:
-        //normalized parameters: file=vacation.jpg&oauth_consumer_key=dpf43f3p2l4k3l03&oauth_nonce=kllo9940pd9333jh&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242096&oauth_token=nnch734d00sl2jdk&oauth_version=1.0&size=original
-        //signature base string: GET&http%3A%2F%2Fphotos.example.net%2Fphotos&file%3Dvacation.jpg%26oauth_consumer_key%3Ddpf43f3p2l4k3l03%26oauth_nonce%3Dkllo9940pd9333jh%26oauth_signature_method%3DHMAC-SHA1%26oauth_timestamp%3D1191242096%26oauth_token%3Dnnch734d00sl2jdk%26oauth_version%3D1.0%26size%3Doriginal
-        //signature: tR3+Ty81lMeYAr/Fid0kMTYa/WM=
-        //Authorization header: OAuth realm="",oauth_version="1.0",oauth_consumer_key="dpf43f3p2l4k3l03",oauth_token="nnch734d00sl2jdk",oauth_timestamp="1191242096",oauth_nonce="kllo9940pd9333jh",oauth_signature_method="HMAC-SHA1",oauth_signature="tR3%2BTy81lMeYAr%2FFid0kMTYa%2FWM%3D"
+        // normalized parameters:
+        // file=vacation.jpg&oauth_consumer_key=dpf43f3p2l4k3l03&oauth_nonce=kllo9940pd9333jh&oauth_signature_method=HMAC-SHA1&oauth_timestamp=1191242096&oauth_token=nnch734d00sl2jdk&oauth_version=1.0&size=original
+        // signature base string:
+        // GET&http%3A%2F%2Fphotos.example.net%2Fphotos&file%3Dvacation.jpg%26oauth_consumer_key%3Ddpf43f3p2l4k3l03%26oauth_nonce%3Dkllo9940pd9333jh%26oauth_signature_method%3DHMAC-SHA1%26oauth_timestamp%3D1191242096%26oauth_token%3Dnnch734d00sl2jdk%26oauth_version%3D1.0%26size%3Doriginal
+        // signature: tR3+Ty81lMeYAr/Fid0kMTYa/WM=
+        // Authorization header: OAuth
+        // realm="",oauth_version="1.0",oauth_consumer_key="dpf43f3p2l4k3l03",oauth_token="nnch734d00sl2jdk",oauth_timestamp="1191242096",oauth_nonce="kllo9940pd9333jh",oauth_signature_method="HMAC-SHA1",oauth_signature="tR3%2BTy81lMeYAr%2FFid0kMTYa%2FWM%3D"
 
         String authHeader = req.getHeaders().get("Authorization").get(0);
         Matcher m = Pattern.compile("oauth_signature=\"(.+?)\"").matcher(authHeader);