From 315ead6ec9e24f6ea32980936cb91fbb91161219 Mon Sep 17 00:00:00 2001 From: Kerry Jones Date: Tue, 21 Jul 2020 21:52:54 +0100 Subject: [PATCH 1/2] Query parameters for S3 are already URI encoded, so don't encode/sanitize them again. --- lib/ex_aws/request/url.ex | 8 ++- test/ex_aws/request/url_test.exs | 119 ++++++++++++++++++++----------- 2 files changed, 82 insertions(+), 45 deletions(-) diff --git a/lib/ex_aws/request/url.ex b/lib/ex_aws/request/url.ex index 32a386db..21843612 100644 --- a/lib/ex_aws/request/url.ex +++ b/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 diff --git a/test/ex_aws/request/url_test.exs b/test/ex_aws/request/url_test.exs index 11ed9dd1..544c0a84 100644 --- a/test/ex_aws/request/url_test.exs +++ b/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 From 8f8cd35ac44a68e336a9710ad9882d5c4d59e811 Mon Sep 17 00:00:00 2001 From: Kerry Jones Date: Wed, 22 Jul 2020 17:41:54 +0100 Subject: [PATCH 2/2] Remove + replace for the path in sanitize, it's not needed --- lib/ex_aws/request/url.ex | 2 +- test/ex_aws/request/url_test.exs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/ex_aws/request/url.ex b/lib/ex_aws/request/url.ex index 21843612..a4d73bfa 100644 --- a/lib/ex_aws/request/url.ex +++ b/lib/ex_aws/request/url.ex @@ -63,7 +63,7 @@ defmodule ExAws.Request.Url do url |> URI.parse() |> Map.put(:fragment, nil) - |> Map.put(:path, "/" <> String.replace(new_path, "+", "%2B")) + |> Map.put(:path, "/" <> new_path) |> Map.put(:query, query_replace_spaces(query)) |> URI.to_string() end diff --git a/test/ex_aws/request/url_test.exs b/test/ex_aws/request/url_test.exs index 544c0a84..9f639014 100644 --- a/test/ex_aws/request/url_test.exs +++ b/test/ex_aws/request/url_test.exs @@ -102,8 +102,8 @@ defmodule ExAws.Request.UrlTest do 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" + url = "s3://my-bucket/uplo+ads/a key with ++" + assert Url.sanitize(url, :s3) == "s3://my-bucket/uplo%2Bads/a%20key%20with%20%2B%2B" end test "it URI encodes unicode characters in the path" do