Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue where query parameters for S3 are double encoded #704

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix issue where query parameters for S3 are double encoded #704

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
315ead6
Query parameters for S3 are already URI encoded, so don't encode/sani…
kerryjj Jul 21, 2020
8f8cd35
Remove + replace for the path in sanitize, it's not needed
kerryjj Jul 22, 2020
ba68369
Merge branch 'ex-aws:master' into master
kerryjj Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/ex_aws/request/url.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,8 @@ defmodule ExAws.Request.Url do
|> URI.parse()
|> Map.put(:fragment, nil)
|> Map.put(:path, "/" <> new_path)
|> Map.put(:query, query)
|> Map.put(:query, query_replace_spaces(query))
|> URI.to_string()
|> String.replace("+", "%2B")
end

def sanitize(url, _), do: String.replace(url, "+", "%20")
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's %2B in one place and %20 in another. This seems wrong?

Copy link
Author

@kerryjj kerryjj Jul 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that whole String.replace(new_path, "+", "%2B") isn't needed and has no effect. The new_path gets uri_encoded a little higher up.

In the S3 operation code path, the url that gets passed to sanize has already had the query params encoded, but the reset of the URL hasn't been. So if new_path in this case contains a +, then it is actually a + and not a space.

s3.ex:29 calls ExAws.Request.Url.build(config) which is what encodes the query and not the path.

It feels a little bit inconsistent to encode one earlier and the other later. Maybe it would be better to shift the encoding to one place in the code. And perhaps it's best to encode both the path and the query in Url.sanitize() because ExAws.Operation.{JSON,Query,RestQuery} also call ExAws.Request.Url.build and therefore have their query and not path encoded if they include any.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a quick change taking out that replace. Let me know if you think it'd be a good idea for me to try to move the uri_encoding out of ExAws.Request.Url.build(config) and to encode both the path and query in ExAws.Request.Url.sanitize instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query_replace_spaces appears to actually be replacing +'s rather than spaces .. and I think that could more simply use URI.encode_www_form/1 instead of URI.encode.

On a related note, the query string parse is redundant with the URI.parse/1 call which also separates out the query string.

It would be nice to see this fix get into the repo, but perhaps with a bit more cleanup?


# Space character
defp valid_path_char?(?\ ), do: false
defp valid_path_char?(?/), do: true
Expand Down
119 changes: 77 additions & 42 deletions test/ex_aws/request/url_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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/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
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