Query parameters for S3 are already URI encoded, so don't encode/sanitize them again.

Author: kerryjj

lib/ex_aws/request/url.ex

@@ -63,10 +63,9 @@ defmodule ExAws.Request.Url do
     url
     |> URI.parse()
     |> Map.put(:fragment, nil)
-    |> Map.put(:path, "/" <> new_path)
-    |> Map.put(:query, query)
+    |> Map.put(:path, "/" <> String.replace(new_path, "+", "%2B"))
+    |> Map.put(:query, query_replace_spaces(query))
     |> URI.to_string()
-    |> String.replace("+", "%2B")
   end
 
   def sanitize(url, _), do: String.replace(url, "+", "%20")
@@ -106,6 +105,9 @@ defmodule ExAws.Request.Url do
 
   def uri_encode(url), do: URI.encode(url, &valid_path_char?/1)
 
+  defp query_replace_spaces(nil), do: nil
+  defp query_replace_spaces(query), do: String.replace(query, "+", "%20")
+
   # Space character
   defp valid_path_char?(?\ ), do: false
   defp valid_path_char?(?/), do: true

test/ex_aws/request/url_test.exs

@@ -18,58 +18,74 @@ defmodule ExAws.Request.UrlTest do
     {:ok, %{query: query, config: config}}
   end
 
-  test "it build urls for query operation", %{query: query, config: config} do
-    assert Url.build(query, config) == "https://example.com/path?foo=bar"
-  end
+  describe "build" do
+    test "it build urls for query operation", %{query: query, config: config} do
+      assert Url.build(query, config) == "https://example.com/path?foo=bar"
+    end
 
-  test "it allows setting custom port", %{query: query, config: config} do
-    config = config |> Map.put(:port, 4430)
-    assert Url.build(query, config) == "https://example.com:4430/path?foo=bar"
-  end
+    test "it allows setting custom port", %{query: query, config: config} do
+      config = config |> Map.put(:port, 4430)
+      assert Url.build(query, config) == "https://example.com:4430/path?foo=bar"
+    end
 
-  test "it converts the port to an integer if it is a string", %{query: query, config: config} do
-    config = config |> Map.put(:port, "4430")
-    assert Url.build(query, config) == "https://example.com:4430/path?foo=bar"
-  end
+    test "it converts the port to an integer if it is a string", %{query: query, config: config} do
+      config = config |> Map.put(:port, "4430")
+      assert Url.build(query, config) == "https://example.com:4430/path?foo=bar"
+    end
 
-  test "it allows passing scheme with trailing ://", %{query: query, config: config} do
-    config = config |> Map.put(:scheme, "https://")
-    assert Url.build(query, config) == "https://example.com/path?foo=bar"
-  end
+    test "it allows passing scheme with trailing ://", %{query: query, config: config} do
+      config = config |> Map.put(:scheme, "https://")
+      assert Url.build(query, config) == "https://example.com/path?foo=bar"
+    end
 
-  test "it accepts params as a list of keywords", %{query: query, config: config} do
-    query = query |> Map.put(:params, foo: :bar)
-    assert Url.build(query, config) == "https://example.com/path?foo=bar"
-  end
+    test "it accepts params as a list of keywords", %{query: query, config: config} do
+      query = query |> Map.put(:params, foo: :bar)
+      assert Url.build(query, config) == "https://example.com/path?foo=bar"
+    end
 
-  test "it does not have trailing ? when params is an empty map", %{query: query, config: config} do
-    query = query |> Map.put(:params, %{})
-    assert Url.build(query, config) == "https://example.com/path"
-  end
+    test "it does not have trailing ? when params is an empty map", %{
+      query: query,
+      config: config
+    } do
+      query = query |> Map.put(:params, %{})
+      assert Url.build(query, config) == "https://example.com/path"
+    end
 
-  test "it does not have trailing ? when params is an empty list", %{query: query, config: config} do
-    query = query |> Map.put(:params, [])
-    assert Url.build(query, config) == "https://example.com/path"
-  end
+    test "it does not have trailing ? when params is an empty list", %{
+      query: query,
+      config: config
+    } do
+      query = query |> Map.put(:params, [])
+      assert Url.build(query, config) == "https://example.com/path"
+    end
 
-  test "it accepts query without params key", %{query: query, config: config} do
-    query = query |> Map.delete(:params)
-    assert Url.build(query, config) == "https://example.com/path"
-  end
+    test "it accepts query without params key", %{query: query, config: config} do
+      query = query |> Map.delete(:params)
+      assert Url.build(query, config) == "https://example.com/path"
+    end
 
-  test "it cleans up excessive slashes in the path", %{query: query, config: config} do
-    query = query |> Map.put(:path, "//path///with/too/many//slashes//")
-    assert Url.build(query, config) == "https://example.com/path/with/too/many/slashes/?foo=bar"
-  end
+    test "it cleans up excessive slashes in the path", %{query: query, config: config} do
+      query = query |> Map.put(:path, "//path///with/too/many//slashes//")
+      assert Url.build(query, config) == "https://example.com/path/with/too/many/slashes/?foo=bar"
+    end
 
-  test "it ignores empty parameter key", %{query: query, config: config} do
-    query = query |> Map.put(:params, %{"foo" => "bar", "" => 1})
-    assert Url.build(query, config) == "https://example.com/path?foo=bar"
-  end
+    test "it ignores empty parameter key", %{query: query, config: config} do
+      query = query |> Map.put(:params, %{"foo" => "bar", "" => 1})
+      assert Url.build(query, config) == "https://example.com/path?foo=bar"
+    end
 
-  test "it ignores nil parameter key", %{query: query, config: config} do
-    query = query |> Map.put(:params, %{"foo" => "bar", nil => 1})
-    assert Url.build(query, config) == "https://example.com/path?foo=bar"
+    test "it ignores nil parameter key", %{query: query, config: config} do
+      query = query |> Map.put(:params, %{"foo" => "bar", nil => 1})
+      assert Url.build(query, config) == "https://example.com/path?foo=bar"
+    end
+
+    test "it URI encodes spaces, + and unicode characters in query parameters", %{
+      query: query,
+      config: config
+    } do
+      query = query |> Map.put(:params, %{"foo" => "put: it+й"})
+      assert Url.build(query, config) == "https://example.com/path?foo=put%3A+it%2B%D0%B9"
+    end
   end
 
   describe "get_path" do
@@ -83,4 +99,23 @@ defmodule ExAws.Request.UrlTest do
       assert Url.get_path(url) == "/uploads/invalid path but+valid//for"
     end
   end
+
+  describe "sanitize" do
+    test "it URI encodes spaces, + and unicode characters in the path" do
+      url = "s3://my-bucket/uploads/a key with ++"
+      assert Url.sanitize(url, :s3) == "s3://my-bucket/uploads/a%20key%20with%20%2B%2B"
+    end
+
+    test "it URI encodes unicode characters in the path" do
+      url = "s3://my-bucket/uploads/a key with й"
+      assert Url.sanitize(url, :s3) == "s3://my-bucket/uploads/a%20key%20with%20%D0%B9"
+    end
+
+    test "it doesn't re-encode query parameters" do
+      url = "s3://my-bucket/uploads/a key with й?foo=put%3A+it%2B%D0%B9"
+
+      assert Url.sanitize(url, :s3) ==
+               "s3://my-bucket/uploads/a%20key%20with%20%D0%B9?foo=put%3A%20it%2B%D0%B9"
+    end
+  end
 end