Merge pull request #1141 from basho/bugfix/v4-encoding-percent

Fix URL encoding bug around AWS v4 auth

Reviewed-by: kuenishi

client_tests/python/boto_tests/boto_test.py

@@ -164,6 +164,17 @@ def test_auth(self):
         conn = self.make_connection(bad_user)
         self.assertRaises(S3ResponseError, conn.get_canonical_user_id)
 
+    def test_auth_weird_query_param(self):
+        bucket = self.conn.create_bucket(self.bucket_name)
+        query_args = 'foo=bar%20baz'
+        response = bucket.connection.make_request('GET', bucket.name, "notfound",
+                                                  query_args=query_args)
+        body = response.read()
+        boto.log.debug(body)
+        if response.status != 404:
+            raise bucket.connection.provider.storage_response_error(
+                response.status, response.reason, body)
+
     def test_create_bucket(self):
         self.conn.create_bucket(self.bucket_name)
         self.assertIn(self.bucket_name,
@@ -317,7 +328,6 @@ def test_list_japanese_key(self):
         for u in uploads:
             self.assertEqual(u.key_name, key_name)
 
-
 def one_kb_string():
     "Return a 1KB string of all a's"
     return ''.join(['a' for _ in xrange(1024)])
@@ -665,14 +675,16 @@ class ObjectMetadataTest(S3ApiVerificationTestBase):
         "Expires": "Tue, 19 Jan 2038 03:14:07 GMT",
         "mtime": "1364742057",
         "UID": "0",
-        "with-hypen": "1"}
+        "with-hypen": "1",
+        "space-in-value": "abc xyz"}
 
     updated_metadata = {
         "Content-Disposition": 'attachment; filename="newname.txt"',
         "Cache-Control": "private",
         "Expires": "Tue, 19 Jan 2038 03:14:07 GMT",
         "mtime": "2222222222",
         "uid": "0",
+        "space-in-value": "ABC XYZ",
         "new-entry": "NEW"}
 
     def test_normal_object_metadata(self):
@@ -708,12 +720,14 @@ def assert_metadata(self, bucket, key_name):
         self.assertEqual(key.get_metadata("mtime"), "1364742057")
         self.assertEqual(key.get_metadata("uid"), "0")
         self.assertEqual(key.get_metadata("with-hypen"), "1")
+        self.assertEqual(key.get_metadata("space-in-value"), "abc xyz")
         # x-amz-meta-* headers should be normalized to lowercase
         self.assertEqual(key.get_metadata("Mtime"), None)
         self.assertEqual(key.get_metadata("MTIME"), None)
         self.assertEqual(key.get_metadata("Uid"), None)
         self.assertEqual(key.get_metadata("UID"), None)
         self.assertEqual(key.get_metadata("With-Hypen"), None)
+        self.assertEqual(key.get_metadata("Space-In-Value"), None)
 
     def change_metadata(self, bucket, key_name):
         key = Key(bucket, key_name)
@@ -730,6 +744,7 @@ def assert_updated_metadata(self, bucket, key_name):
                          'attachment; filename="newname.txt"')
         self.assertEqual(key.cache_control, "private")
         self.assertEqual(key.get_metadata("mtime"), "2222222222")
+        self.assertEqual(key.get_metadata("space-in-value"), "ABC XYZ")
         # removed
         self.assertEqual(key.content_encoding, None)
         self.assertEqual(key.get_metadata("with-hypen"), None)

src/riak_cs_s3_auth.erl

@@ -38,6 +38,14 @@
 -define(QS_KEYID, "AWSAccessKeyId").
 -define(QS_SIGNATURE, "Signature").
 
+-define(PERCENT, 37).  % $\%
+-define(FULLSTOP, 46). % $\.
+-define(QS_SAFE(C), ((C >= $a andalso C =< $z) orelse
+                     (C >= $A andalso C =< $Z) orelse
+                     (C >= $0 andalso C =< $9) orelse
+                     (C =:= ?FULLSTOP orelse C =:= $- orelse C =:= $~ orelse
+                      C =:= $_))).
+
 %% ===================================================================
 %% Public API
 %% ===================================================================
@@ -336,18 +344,39 @@ canonicalize_qs_v4([{K, V}|T], Acc) ->
 %% Because keys and values from raw query string that is URL-encoded at client
 %% side, they should be URL-decoded once and encoded back.
 strict_url_encode_for_qs_value(Value) ->
-    mochiweb_util:quote_plus(mochiweb_util:unquote(Value)).
+    quote_percent(mochiweb_util:unquote(Value)).
 
 %% Force string URL encoding for path part of URL.
 %% Contrary to query part, slashes MUST NOT encoded and left as is.
 strict_url_encode_for_path(Path) ->
     %% Use `binary:split/3' here instead of `string:tokens/2' because
     %% latter drops information about preceding and trailing slashes.
     Tokens = binary:split(list_to_binary(Path), <<"/">>, [global]),
-    EncodedTokens = [mochiweb_util:quote_plus(mochiweb_util:unquote(T)) ||
+    EncodedTokens = [quote_percent(mochiweb_util:unquote(T)) ||
                         T <- Tokens],
     string:join(EncodedTokens, "/").
 
+-spec quote_percent(string()) -> string().
+%% @doc URL safe encoding of the given string, keeping with the strict
+%% specification of AWS S3 because this is used for canonicalizing
+%% resource for signature.
+quote_percent(String) ->
+    quote_percent(String, []).
+
+quote_percent([], Acc) ->
+    lists:reverse(Acc);
+quote_percent([C | Rest], Acc) when ?QS_SAFE(C) ->
+    quote_percent(Rest, [C | Acc]);
+quote_percent([$\s | Rest], Acc) ->
+    quote_percent(Rest, [$0, $2, ?PERCENT | Acc]);
+quote_percent([C | Rest], Acc) ->
+    <<Hi:4, Lo:4>> = <<C>>,
+    quote_percent(Rest, [hexdigit(Lo), hexdigit(Hi), ?PERCENT | Acc]).
+
+hexdigit(C) when C < 10 -> $0 + C;
+hexdigit(C) when C < 16 -> $A + (C - 10).
+
+
 %% ===================================================================
 %% Eunit tests
 %% ===================================================================
@@ -532,4 +561,13 @@ example_unicode_keys() ->
     CalculatedSignature = calculate_signature_v2(KeyData, RD),
     test_fun("example unicode keys test", ExpectedSignature, CalculatedSignature).
 
+quote_percent_test() ->
+    ?assertEqual("", quote_percent("")),
+    ?assertEqual("abc", quote_percent("abc")),
+    ?assertEqual("%20", quote_percent(" ")),
+    ?assertEqual("abc%20%20xyz%20%0A", quote_percent("abc  xyz \n")),
+    ?assertEqual("%24%2A%3F%0A", quote_percent("$*?\n")),
+    ?assertEqual("foo%3B%26%3D", quote_percent("foo;&=")),
+    ok.
+
 -endif.