Merge pull request #1316 from basho/bugfix-rcs-350

Corrected fix for RCS-350 Reviewed-by: JeetKunDoug
basho · Jun 8, 2016 · 27bc090 · 27bc090
2 parents 64eb8de + f69aea8
commit 27bc090
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 52 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -17,7 +17,7 @@ During the update to 2.1.2, a '==' omitted upload ID might be passed to a Riak C
 ##Changes
 
 * Due to a recent [Product Advisory](http://docs.basho.com/riak/latest/community/product-advisories/codeinjectioninitfiles/), node_package was bumped to version 3.0.0 to prevent a potential code injection on the riak init file. [[Issue 1297](https://github.com/basho/riak_cs/issues/1297), [PR 1306](https://github.com/basho/riak_cs/pull/1306), & [PR 109](https://github.com/basho/stanchion/pull/109)]
-* Multipart upload IDs no longer contain trailing '=' characters, which caused trouble for some clients. This change also makes upload IDs URL-safe. This commit removes several unused functions. [[PR 1293](https://github.com/basho/riak_cs/pull/1293)]
+* Multipart upload IDs no longer contain trailing '=' characters, which caused trouble for some clients. This change also makes upload IDs URL-safe. [[PR 1316](https://github.com/basho/riak_cs/pull/1316)]
 * When Riak is unavailable due to network partition or node being offline, a 500 error is returned. [[PR 1298](https://github.com/basho/riak_cs/pull/1298)]
 
 ## Bugs Fixed

diff --git a/riak_test/tests/mp_upload_test.erl b/riak_test/tests/mp_upload_test.erl
@@ -1,6 +1,6 @@
-%% ---------------------------------------------------------------------
+%% -------------------------------------------------------------------
 %%
-%% Copyright (c) 2007-2013 Basho Technologies, Inc.  All Rights Reserved.
+%% Copyright (c) 2007-2016 Basho Technologies, Inc.
 %%
 %% This file is provided to you under the Apache License,
 %% Version 2.0 (the "License"); you may not use this file
@@ -16,7 +16,7 @@
 %% specific language governing permissions and limitations
 %% under the License.
 %%
-%% ---------------------------------------------------------------------
+%% -------------------------------------------------------------------
 
 -module(mp_upload_test).
 
@@ -154,6 +154,7 @@ aborted_upload_test_case(Bucket, Key, Config) ->
     lager:info("Initiating multipart upload"),
     InitUploadRes = erlcloud_s3_multipart:initiate_upload(Bucket, Key, [], [], Config),
     UploadId = erlcloud_s3_multipart:upload_id(InitUploadRes),
+    lager:info("Upload ID: ~p", [UploadId]),
 
     %% Verify the upload id is in list_uploads results and
     %% that the bucket information is correct
@@ -216,6 +217,7 @@ basic_upload_test_case(Bucket, Key, Config) ->
     lager:info("Initiating multipart upload"),
     InitUploadRes = erlcloud_s3_multipart:initiate_upload(Bucket, Key, [], [], Config),
     UploadId = erlcloud_s3_multipart:upload_id(InitUploadRes),
+    lager:info("Upload ID: ~p", [UploadId]),
 
     %% Verify the upload id is in list_uploads results and
     %% that the bucket information is correct
@@ -274,6 +276,7 @@ parts_too_small_test_case(Bucket, Key, Config) ->
     lager:info("Initiating multipart upload (bad)"),
     InitUploadRes = erlcloud_s3_multipart:initiate_upload(Bucket, Key, [], [], Config),
     UploadId = erlcloud_s3_multipart:upload_id(InitUploadRes),
+    lager:info("Upload ID: ~p", [UploadId]),
 
     lager:info("Uploading parts (bad)"),
     EtagList = upload_and_assert_parts(Bucket,

diff --git a/src/base64url.erl b/src/base64url.erl
@@ -1,6 +1,6 @@
-%% ---------------------------------------------------------------------
+%% -------------------------------------------------------------------
 %%
-%% Copyright (c) 2007-2013 Basho Technologies, Inc.  All Rights Reserved.
+%% Copyright (c) 2007-2016 Basho Technologies, Inc.
 %%
 %% This file is provided to you under the Apache License,
 %% Version 2.0 (the "License"); you may not use this file
@@ -16,7 +16,7 @@
 %% specific language governing permissions and limitations
 %% under the License.
 %%
-%% ---------------------------------------------------------------------
+%% -------------------------------------------------------------------
 
 %% @doc base64url is a wrapper around the base64 module to produce
 %%      base64-compatible encodings that are URL safe.
@@ -27,46 +27,66 @@
 
 -module(base64url).
 
--include_lib("eunit/include/eunit.hrl").
-
 -export([decode/1,
          decode_to_string/1,
          encode/1,
          encode_to_string/1]).
 
+-ifdef(TEST).
+-compile(export_all).
+-include_lib("eunit/include/eunit.hrl").
+-endif.
+
 decode(Base64url) ->
-    base64:decode(amend_equal(urldecode(Base64url))).
+    base64:decode(append_equals(urldecode(Base64url))).
 
 decode_to_string(Base64url) ->
-    base64:decode_to_string(amend_equal(urldecode(Base64url))).
+    base64:decode_to_string(append_equals(urldecode(Base64url))).
 
 encode(Data) ->
-    urlencode(strip_equal(base64:encode(Data))).
+    urlencode(strip_equals(base64:encode(Data))).
 
 encode_to_string(Data) ->
-    urlencode(strip_equal(base64:encode_to_string(Data))).
-
--spec strip_equal(binary() | string()) -> binary()|string().
-strip_equal(Encoded) when is_list(Encoded) ->
-    hd(string:tokens(Encoded, "="));
-strip_equal(Encoded) when is_binary(Encoded) ->
-    LCS = binary:longest_common_suffix([Encoded, <<"===">>]),
-    binary:part(Encoded, 0, byte_size(Encoded)-LCS).
-
-%% @doc complements '=' if it doesn't have 4*n length
--spec amend_equal(binary()|string()) -> binary()|string().
-amend_equal(Encoded) when is_list(Encoded) ->
-    Suffix = case length(Encoded) rem 4 of
-                 0 -> "";
-                 L -> [$=||_<-lists:seq(1,L)]
-             end,
-    lists:flatten([Encoded, Suffix]);
-amend_equal(Bin) when is_binary(Bin) ->
+    urlencode(strip_equals(base64:encode_to_string(Data))).
+
+-spec strip_equals(binary() | string()) -> binary()|string().
+%% @private Strip off trailing '=' characters.
+strip_equals(Str) when is_list(Str) ->
+    string:strip(Str, right, $=);
+strip_equals(Bin) when is_binary(Bin) ->
+    LCS = binary:longest_common_suffix([Bin, <<"===">>]),
+    binary:part(Bin, 0, byte_size(Bin)-LCS).
+
+-spec append_equals(binary()|string()) -> binary()|string().
+%% @private Append trailing '=' characters to make result legal Base64 length.
+%% The most common use case will be a B64-encoded UUID, requiring the addition
+%% of 2 characters, so that's the first check. We assume 0 and 3 are equally
+%% likely.
+%% Because B64 encoding spans all bytes across two characters, the remainder
+%% of (length / 4) can never be 1 with a valid encoding, so we throw a badarg
+%% with the argument here rather than letting it percolate up from stdlib with
+%% no worthwhile information.
+append_equals(Str) when is_list(Str) ->
+    case length(Str) rem 4 of
+        2 ->
+            Str ++ "==";
+        0 ->
+            Str;
+        3 ->
+            Str ++ "=";
+        1 ->
+            erlang:error(badarg, [Str])
+    end;
+append_equals(Bin) when is_binary(Bin) ->
     case byte_size(Bin) rem 4 of
-        0 -> Bin;
-        1 -> <<Bin/binary, "===">>;
-        2 -> <<Bin/binary, "==">>;
-        3 -> <<Bin/binary, "=">>
+        2 ->
+            <<Bin/binary, "==">>;
+        0 ->
+            Bin;
+        3 ->
+            <<Bin/binary, "=">>;
+        1 ->
+            erlang:error(badarg, [Bin])
     end.
 
 urlencode(Base64) when is_list(Base64) ->
@@ -88,20 +108,64 @@ urldecode_digit($-) -> $+;
 urldecode_digit(D)  -> D.
 
 -ifdef(TEST).
-equal_strip_amend_test() ->
-    %% TODO: rewrite this with EQC
-    _ = [begin
-             UUID = druuid:v4(),
-             Encoded = base64url:encode(UUID),
-             ?assertEqual(nomatch, binary:match(Encoded, [<<"=">>, <<"+">>, <<"/">>])),
-             ?assertEqual(UUID, base64url:decode(Encoded))
-         end || _<- lists:seq(1, 1024)],
-    _ = [begin
-             UUID = druuid:v4(),
-             Encoded = base64url:encode_to_string(UUID),
-             ?assertEqual(nomatch, re:run(Encoded, "(=\\+\\/)")),
-             ?assertEqual(UUID, base64url:decode(Encoded))
-         end || _<- lists:seq(1, 1024)],
-    ok.
+
+illegal_char_REs() ->
+    BinRE = binary:compile_pattern([<<"+">>, <<"/">>, <<"=">>]),
+    {ok, StrRE} = re:compile("[\\+/=]"),
+    {BinRE, StrRE}.
+
+encode_decode_test() ->
+    crypto:start(),
+    % Make sure Rand is at least twice as long as the highest count, but
+    % because we're depleting the entropy pool don't go overboard!
+    RandLen = 2050,
+    % crypto:rand:bytes/1 would be fine for us, but it's gone in OTP-19,
+    % so use the good stuff rather than bothering with a version check.
+    BinRand = crypto:strong_rand_bytes(RandLen),
+    % Swap halves so the binary and string tests don't use the same sequences.
+    HalfLen = (RandLen div 2),
+    RandLo  = binary_part(BinRand, 0, HalfLen),
+    RandHi  = binary_part(BinRand, HalfLen, HalfLen),
+    StrRand = << RandHi/binary, RandLo/binary >>,
+    {BinRE, StrRE} = illegal_char_REs(),
+    test_encode_decode_uuid(256,    BinRE, StrRE),
+    test_encode_decode_binary(1024, BinRE, StrRE, BinRand),
+    test_encode_decode_string(384,  BinRE, StrRE, StrRand).
+
+test_encode_decode_uuid(0, _, _) ->
+    ok;
+test_encode_decode_uuid(Count, BinRE, StrRE) ->
+    test_encode_decode(druuid:v4(), BinRE, StrRE),
+    test_encode_decode_uuid((Count - 1), BinRE, StrRE).
+
+test_encode_decode_binary(0, _, _, _) ->
+    ok;
+test_encode_decode_binary(Count, BinRE, StrRE, Rand) ->
+    test_encode_decode(binary:part(Rand, Count, Count), BinRE, StrRE),
+    test_encode_decode_binary((Count - 1), BinRE, StrRE, Rand).
+
+test_encode_decode_string(0, _, _, _) ->
+    ok;
+test_encode_decode_string(Count, BinRE, StrRE, Rand) ->
+    test_encode_decode(binary:bin_to_list(Rand, Count, Count), BinRE, StrRE),
+    test_encode_decode_string((Count - 1), BinRE, StrRE, Rand).
+
+test_encode_decode(Data, BinRE, StrRE) when is_binary(Data) ->
+    EncBin = base64url:encode(Data),
+    EncStr = base64url:encode_to_string(Data),
+    ?assertEqual(EncStr, binary_to_list(EncBin)),
+    ?assertEqual(nomatch, binary:match(EncBin, BinRE)),
+    ?assertEqual(nomatch, re:run(EncStr, StrRE)),
+    ?assertEqual(Data, base64url:decode(EncBin)),
+    ?assertEqual(Data, base64url:decode(EncStr));
+
+test_encode_decode(Data, BinRE, StrRE) when is_list(Data) ->
+    EncBin = base64url:encode(Data),
+    EncStr = base64url:encode_to_string(Data),
+    ?assertEqual(EncStr, binary_to_list(EncBin)),
+    ?assertEqual(nomatch, binary:match(EncBin, BinRE)),
+    ?assertEqual(nomatch, re:run(EncStr, StrRE)),
+    ?assertEqual(Data, base64url:decode_to_string(EncBin)),
+    ?assertEqual(Data, base64url:decode_to_string(EncStr)).
 
 -endif.
diff --git a/src/riak_cs_utils.erl b/src/riak_cs_utils.erl
@@ -1,6 +1,6 @@
-%% ---------------------------------------------------------------------
+%% -------------------------------------------------------------------
 %%
-%% Copyright (c) 2007-2014 Basho Technologies, Inc.  All Rights Reserved.
+%% Copyright (c) 2007-2016 Basho Technologies, Inc.
 %%
 %% This file is provided to you under the Apache License,
 %% Version 2.0 (the "License"); you may not use this file
@@ -16,7 +16,7 @@
 %% specific language governing permissions and limitations
 %% under the License.
 %%
-%% ---------------------------------------------------------------------
+%% -------------------------------------------------------------------
 
 %% @doc riak_cs utility functions