Skip to content

Commit

Permalink
Update HTTPoison and fix PATCH 301 handling
Browse files Browse the repository at this point in the history
Updated HTTPoison to 1.8 which includes edgurgel/httpoison#424,
implementing proper handling of redirects. Additionally, we now
follow redirects for all HTTP methods (not just PATCH).

NOTES (in order of importance):
- POST requests which receive a 303 respose are followed internally by
  HTTPoison but with the GET method, thus if the server replies 303 to a
  POST request the tus upload will not work properly
- there is no redirect loop detection, it may make sense to add an
  option (with a default) to limit the max number of redirects
- edgurgel/httpoison#453 is manually worked around
  • Loading branch information
Gianluca Nitti committed Feb 1, 2022
1 parent 13d22f3 commit 1710e32
Show file tree
Hide file tree
Showing 11 changed files with 144 additions and 33 deletions.
2 changes: 1 addition & 1 deletion lib/tus_client/head.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defmodule TusClient.Head do
def request(url, headers \\ [], opts \\ []) do
url
|> HTTPoison.head(headers, Utils.httpoison_opts([], opts))
|> parse()
|> Utils.maybe_follow_redirect(&parse/1, &request(&1, headers, opts))
end

defp parse({:ok, %{status_code: status} = resp}) when status in [200, 204] do
Expand Down
2 changes: 1 addition & 1 deletion lib/tus_client/options.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ defmodule TusClient.Options do
def request(url, headers \\ [], opts \\ []) do
url
|> HTTPoison.options(headers, Utils.httpoison_opts([], opts))
|> parse()
|> Utils.maybe_follow_redirect(&parse/1, &request(&1, headers, opts))
end

defp parse({:ok, %{status_code: status} = resp}) when status in [200, 204] do
Expand Down
27 changes: 4 additions & 23 deletions lib/tus_client/patch.ex
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,10 @@ defmodule TusClient.Patch do

url
|> HTTPoison.patch(data, hdrs, Utils.httpoison_opts([], opts))
|> case do
{:ok,
%HTTPoison.AsyncResponse{
id: {:maybe_redirect, 301, red_headers, _client}
}} ->
case extract_loc(red_headers) do
{:ok, new_loc} ->
do_request({:ok, data}, new_loc, offset, headers, opts)

_ ->
{:error, :protocol}
end

response ->
parse(response)
end
|> Utils.maybe_follow_redirect(
&parse/1,
&do_request({:ok, data}, &1, offset, headers, opts)
)
end

defp do_request({:error, _} = err, _url, _offset, _headers, _opts), do: err
Expand Down Expand Up @@ -104,11 +92,4 @@ defmodule TusClient.Patch do
defp add_custom_headers(hdrs1, hdrs2) do
hdrs1 ++ hdrs2
end

defp extract_loc(headers) do
case Utils.get_header(headers, "location") do
nil -> :error
location -> {:ok, location}
end
end
end
5 changes: 4 additions & 1 deletion lib/tus_client/post.ex
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ defmodule TusClient.Post do

url
|> HTTPoison.post("", hdrs, Utils.httpoison_opts([], opts))
|> parse()
|> Utils.maybe_follow_redirect(
&parse/1,
&do_request({:ok, size}, &1, headers, opts)
)
end

defp do_request(res, _url, _headers, _opts) do
Expand Down
26 changes: 26 additions & 0 deletions lib/tus_client/utils.ex
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,32 @@ defmodule TusClient.Utils do
http_opts ++ [ssl: ssl_opts] ++ [follow_redirect: follow_redirect]
end

@doc false
def maybe_follow_redirect(
{:ok, %HTTPoison.MaybeRedirect{redirect_url: new_loc}},
_parse_fn,
rereq_fn
)
when is_binary(new_loc) do
rereq_fn.(new_loc)
end

# TODO remove this when https://github.com/edgurgel/httpoison/issues/453 is fixed
def maybe_follow_redirect(
{:ok, %HTTPoison.MaybeRedirect{headers: headers}} = resp,
parse_fn,
rereq_fn
) do
case get_header(headers, "location") do
new_loc when is_binary(new_loc) -> rereq_fn.(new_loc)
nil -> parse_fn.(resp)
end
end

def maybe_follow_redirect(response, parse_fn, _rereq_fn) do
parse_fn.(response)
end

defp get_header_impl(headers, header_name) do
headers
|> Enum.filter(fn
Expand Down
2 changes: 1 addition & 1 deletion mix.exs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ defmodule TusClient.MixProject do
# Run "mix help deps" to learn about dependencies.
defp deps do
[
{:httpoison, "~> 1.7"},
{:httpoison, "~> 1.8"},
# development stuff,
{:bypass, "~> 2.1.0-rc.0", only: :test},
{:excoveralls, "~> 0.13", only: :test, runtime: false},
Expand Down
12 changes: 6 additions & 6 deletions mix.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
%{
"bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"},
"bypass": {:hex, :bypass, "2.1.0-rc.0", "3581b77401b1e34e90a355c3946dca7b154666966f945a1118c221ea8fb7bc0a", [:mix], [{:plug, "~> 1.7", [hex: :plug, repo: "hexpm", optional: false]}, {:plug_cowboy, "~> 1.0 or ~> 2.0", [hex: :plug_cowboy, repo: "hexpm", optional: false]}, {:ranch, "~> 1.3", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "61f04e917f2556893eb6d44e1d9506063a8104b307a1055a702ca08f94e04332"},
"certifi": {:hex, :certifi, "2.5.2", "b7cfeae9d2ed395695dd8201c57a2d019c0c43ecaf8b8bcb9320b40d6662f340", [:rebar3], [{:parse_trans, "~>3.3", [hex: :parse_trans, repo: "hexpm", optional: false]}], "hexpm", "3b3b5f36493004ac3455966991eaf6e768ce9884693d9968055aeeeb1e575040"},
"certifi": {:hex, :certifi, "2.8.0", "d4fb0a6bb20b7c9c3643e22507e42f356ac090a1dcea9ab99e27e0376d695eba", [:rebar3], [], "hexpm", "6ac7efc1c6f8600b08d625292d4bbf584e14847ce1b6b5c44d983d273e1097ea"},
"cowboy": {:hex, :cowboy, "2.8.0", "f3dc62e35797ecd9ac1b50db74611193c29815401e53bac9a5c0577bd7bc667d", [:rebar3], [{:cowlib, "~> 2.9.1", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "~> 1.7.1", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "4643e4fba74ac96d4d152c75803de6fad0b3fa5df354c71afdd6cbeeb15fac8a"},
"cowboy_telemetry": {:hex, :cowboy_telemetry, "0.3.1", "ebd1a1d7aff97f27c66654e78ece187abdc646992714164380d8a041eda16754", [:rebar3], [{:cowboy, "~> 2.7", [hex: :cowboy, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "3a6efd3366130eab84ca372cbd4a7d3c3a97bdfcfb4911233b035d117063f0af"},
"cowlib": {:hex, :cowlib, "2.9.1", "61a6c7c50cf07fdd24b2f45b89500bb93b6686579b069a89f88cb211e1125c78", [:rebar3], [], "hexpm", "e4175dc240a70d996156160891e1c62238ede1729e45740bdd38064dad476170"},
Expand All @@ -11,21 +11,21 @@
"excoveralls": {:hex, :excoveralls, "0.13.3", "edc5f69218f84c2bf61b3609a22ddf1cec0fbf7d1ba79e59f4c16d42ea4347ed", [:mix], [{:hackney, "~> 1.16", [hex: :hackney, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "cc26f48d2f68666380b83d8aafda0fffc65dafcc8d8650358e0b61f6a99b1154"},
"exjsx": {:hex, :exjsx, "4.0.0", "60548841e0212df401e38e63c0078ec57b33e7ea49b032c796ccad8cde794b5c", [:mix], [{:jsx, "~> 2.8.0", [hex: :jsx, repo: "hexpm", optional: false]}], "hexpm"},
"file_system": {:hex, :file_system, "0.2.9", "545b9c9d502e8bfa71a5315fac2a923bd060fd9acb797fe6595f54b0f975fd32", [:mix], [], "hexpm", "3cf87a377fe1d93043adeec4889feacf594957226b4f19d5897096d6f61345d8"},
"hackney": {:hex, :hackney, "1.16.0", "5096ac8e823e3a441477b2d187e30dd3fff1a82991a806b2003845ce72ce2d84", [:rebar3], [{:certifi, "2.5.2", [hex: :certifi, repo: "hexpm", optional: false]}, {:idna, "6.0.1", [hex: :idna, repo: "hexpm", optional: false]}, {:metrics, "1.0.1", [hex: :metrics, repo: "hexpm", optional: false]}, {:mimerl, "~>1.1", [hex: :mimerl, repo: "hexpm", optional: false]}, {:parse_trans, "3.3.0", [hex: :parse_trans, repo: "hexpm", optional: false]}, {:ssl_verify_fun, "1.1.6", [hex: :ssl_verify_fun, repo: "hexpm", optional: false]}], "hexpm", "3bf0bebbd5d3092a3543b783bf065165fa5d3ad4b899b836810e513064134e18"},
"httpoison": {:hex, :httpoison, "1.7.0", "abba7d086233c2d8574726227b6c2c4f6e53c4deae7fe5f6de531162ce9929a0", [:mix], [{:hackney, "~> 1.16", [hex: :hackney, repo: "hexpm", optional: false]}], "hexpm", "975cc87c845a103d3d1ea1ccfd68a2700c211a434d8428b10c323dc95dc5b980"},
"idna": {:hex, :idna, "6.0.1", "1d038fb2e7668ce41fbf681d2c45902e52b3cb9e9c77b55334353b222c2ee50c", [:rebar3], [{:unicode_util_compat, "0.5.0", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm", "a02c8a1c4fd601215bb0b0324c8a6986749f807ce35f25449ec9e69758708122"},
"hackney": {:hex, :hackney, "1.18.0", "c4443d960bb9fba6d01161d01cd81173089686717d9490e5d3606644c48d121f", [:rebar3], [{:certifi, "~>2.8.0", [hex: :certifi, repo: "hexpm", optional: false]}, {:idna, "~>6.1.0", [hex: :idna, repo: "hexpm", optional: false]}, {:metrics, "~>1.0.0", [hex: :metrics, repo: "hexpm", optional: false]}, {:mimerl, "~>1.1", [hex: :mimerl, repo: "hexpm", optional: false]}, {:parse_trans, "3.3.1", [hex: :parse_trans, repo: "hexpm", optional: false]}, {:ssl_verify_fun, "~>1.1.0", [hex: :ssl_verify_fun, repo: "hexpm", optional: false]}, {:unicode_util_compat, "~>0.7.0", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm", "9afcda620704d720db8c6a3123e9848d09c87586dc1c10479c42627b905b5c5e"},
"httpoison": {:hex, :httpoison, "1.8.0", "6b85dea15820b7804ef607ff78406ab449dd78bed923a49c7160e1886e987a3d", [:mix], [{:hackney, "~> 1.17", [hex: :hackney, repo: "hexpm", optional: false]}], "hexpm", "28089eaa98cf90c66265b6b5ad87c59a3729bea2e74e9d08f9b51eb9729b3c3a"},
"idna": {:hex, :idna, "6.1.1", "8a63070e9f7d0c62eb9d9fcb360a7de382448200fbbd1b106cc96d3d8099df8d", [:rebar3], [{:unicode_util_compat, "~>0.7.0", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm", "92376eb7894412ed19ac475e4a86f7b413c1b9fbb5bd16dccd57934157944cea"},
"jason": {:hex, :jason, "1.2.2", "ba43e3f2709fd1aa1dce90aaabfd039d000469c05c56f0b8e31978e03fa39052", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "18a228f5f0058ee183f29f9eae0805c6e59d61c3b006760668d8d18ff0d12179"},
"jsx": {:hex, :jsx, "2.8.3", "a05252d381885240744d955fbe3cf810504eb2567164824e19303ea59eef62cf", [:mix, :rebar3], [], "hexpm"},
"metrics": {:hex, :metrics, "1.0.1", "25f094dea2cda98213cecc3aeff09e940299d950904393b2a29d191c346a8486", [:rebar3], [], "hexpm", "69b09adddc4f74a40716ae54d140f93beb0fb8978d8636eaded0c31b6f099f16"},
"mime": {:hex, :mime, "1.4.0", "5066f14944b470286146047d2f73518cf5cca82f8e4815cf35d196b58cf07c47", [:mix], [], "hexpm", "75fa42c4228ea9a23f70f123c74ba7cece6a03b1fd474fe13f6a7a85c6ea4ff6"},
"mimerl": {:hex, :mimerl, "1.2.0", "67e2d3f571088d5cfd3e550c383094b47159f3eee8ffa08e64106cdf5e981be3", [:rebar3], [], "hexpm", "f278585650aa581986264638ebf698f8bb19df297f66ad91b18910dfc6e19323"},
"parse_trans": {:hex, :parse_trans, "3.3.0", "09765507a3c7590a784615cfd421d101aec25098d50b89d7aa1d66646bc571c1", [:rebar3], [], "hexpm", "17ef63abde837ad30680ea7f857dd9e7ced9476cdd7b0394432af4bfc241b960"},
"parse_trans": {:hex, :parse_trans, "3.3.1", "16328ab840cc09919bd10dab29e431da3af9e9e7e7e6f0089dd5a2d2820011d8", [:rebar3], [], "hexpm", "07cd9577885f56362d414e8c4c4e6bdf10d43a8767abb92d24cbe8b24c54888b"},
"plug": {:hex, :plug, "1.11.0", "f17217525597628298998bc3baed9f8ea1fa3f1160aa9871aee6df47a6e4d38e", [:mix], [{:mime, "~> 1.0", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_crypto, "~> 1.1.1 or ~> 1.2", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "2d9c633f0499f9dc5c2fd069161af4e2e7756890b81adcbb2ceaa074e8308876"},
"plug_cowboy": {:hex, :plug_cowboy, "2.4.1", "779ba386c0915027f22e14a48919a9545714f849505fa15af2631a0d298abf0f", [:mix], [{:cowboy, "~> 2.7", [hex: :cowboy, repo: "hexpm", optional: false]}, {:cowboy_telemetry, "~> 0.3", [hex: :cowboy_telemetry, repo: "hexpm", optional: false]}, {:plug, "~> 1.7", [hex: :plug, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "d72113b6dff7b37a7d9b2a5b68892808e3a9a752f2bf7e503240945385b70507"},
"plug_crypto": {:hex, :plug_crypto, "1.2.0", "1cb20793aa63a6c619dd18bb33d7a3aa94818e5fd39ad357051a67f26dfa2df6", [:mix], [], "hexpm", "a48b538ae8bf381ffac344520755f3007cc10bd8e90b240af98ea29b69683fc2"},
"poison": {:hex, :poison, "3.1.0", "d9eb636610e096f86f25d9a46f35a9facac35609a7591b3be3326e99a0484665", [:mix], [], "hexpm"},
"ranch": {:hex, :ranch, "1.7.1", "6b1fab51b49196860b733a49c07604465a47bdb78aa10c1c16a3d199f7f8c881", [:rebar3], [], "hexpm", "451d8527787df716d99dc36162fca05934915db0b6141bbdac2ea8d3c7afc7d7"},
"ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.6", "cf344f5692c82d2cd7554f5ec8fd961548d4fd09e7d22f5b62482e5aeaebd4b0", [:make, :mix, :rebar3], [], "hexpm", "bdb0d2471f453c88ff3908e7686f86f9be327d065cc1ec16fa4540197ea04680"},
"telemetry": {:hex, :telemetry, "0.4.2", "2808c992455e08d6177322f14d3bdb6b625fbcfd233a73505870d8738a2f4599", [:rebar3], [], "hexpm", "2d1419bd9dda6a206d7b5852179511722e2b18812310d304620c7bd92a13fcef"},
"unicode_util_compat": {:hex, :unicode_util_compat, "0.5.0", "8516502659002cec19e244ebd90d312183064be95025a319a6c7e89f4bccd65b", [:rebar3], [], "hexpm", "d48d002e15f5cc105a696cf2f1bbb3fc72b4b770a184d8420c8db20da2674b38"},
"unicode_util_compat": {:hex, :unicode_util_compat, "0.7.0", "bc84380c9ab48177092f43ac89e4dfa2c6d62b40b8bd132b1059ecc7232f9a78", [:rebar3], [], "hexpm", "25eee6d67df61960cf6a794239566599b09e17e668d3700247bc498638152521"},
}
22 changes: 22 additions & 0 deletions test/tus_client/head_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,28 @@ defmodule TusClient.HeadTest do
assert {:error, :not_found} = Head.request(file_url(bypass.port))
end

test "request/1 302", %{bypass: bypass} do
Bypass.expect_once(bypass, "HEAD", "/files/foobar", fn conn ->
conn
|> put_resp_header(
"location",
"http://localhost:#{bypass.port}/somewhere/foobar"
)
|> resp(302, "")
end)

Bypass.expect_once(bypass, "HEAD", "/somewhere/foobar", fn conn ->
conn
|> put_resp_header("upload-offset", "0")
|> put_resp_header("upload-length", "1234")
|> put_resp_header("cache-control", "no-store")
|> resp(200, "")
end)

assert {:ok, %{upload_offset: 0, upload_length: 1234}} =
Head.request(file_url(bypass.port), [], follow_redirect: true)
end

defp file_url(port), do: endpoint_url(port) <> "/foobar"
defp endpoint_url(port), do: "http://localhost:#{port}/files"
end
21 changes: 21 additions & 0 deletions test/tus_client/options_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -83,5 +83,26 @@ defmodule TusClient.OptionsTest do
Options.request(endpoint_url(bypass.port))
end

test "request/1 307", %{bypass: bypass} do
Bypass.expect_once(bypass, "OPTIONS", "/files", fn conn ->
conn
|> put_resp_header("location", "http://localhost:#{bypass.port}/files2")
|> resp(307, "")
end)

Bypass.expect_once(bypass, "OPTIONS", "/files2", fn conn ->
conn
|> put_resp_header("tus-version", "1.0.0")
|> put_resp_header("tus-max-size", "1234")
|> put_resp_header("tus-extension", "creation,expiration")
|> resp(200, "")
end)

assert {:ok, %{max_size: 1234, extensions: ["creation", "expiration"]}} =
Options.request(endpoint_url(bypass.port), [],
follow_redirect: true
)
end

defp endpoint_url(port), do: "http://localhost:#{port}/files"
end
37 changes: 37 additions & 0 deletions test/tus_client/patch_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,43 @@ defmodule TusClient.PatchTest do
)
end

test "request/3 301", %{bypass: bypass, tmp_file: path} do
data = "yaddayaddamohmoh"
filen = random_file_name()
:ok = File.write!(path, data)

Bypass.expect_once(bypass, "PATCH", "/files/#{filen}", fn conn ->
conn
|> put_resp_header(
"location",
"http://localhost:#{bypass.port}/actual_files/#{filen}"
)
|> resp(301, "")
end)

Bypass.expect_once(bypass, "PATCH", "/actual_files/#{filen}", fn conn ->
protocol_assertions(conn)

{:ok, body, conn} = read_body(conn)
assert data == body

len = data |> String.length() |> to_string()

conn
|> put_resp_header("upload-offset", len)
|> resp(204, "")
end)

assert {:ok, String.length(data)} ==
Patch.request(
endpoint_url(bypass.port, "#{filen}"),
0,
path,
[],
follow_redirect: true
)
end

defp endpoint_url(port, fname), do: "http://localhost:#{port}/files/#{fname}"

defp protocol_assertions(conn) do
Expand Down
21 changes: 21 additions & 0 deletions test/tus_client/post_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,27 @@ defmodule TusClient.PostTest do
Post.request(endpoint_url(bypass.port), path, [{"foo", "bar"}])
end

test "request/1 301", %{bypass: bypass, tmp_file: path} do
Bypass.expect_once(bypass, "POST", "/files", fn conn ->
conn
|> put_resp_header("location", "http://localhost:#{bypass.port}/here")
|> resp(301, "<h1>go follow the location header</h1>")
end)

Bypass.expect_once(bypass, "POST", "/here", fn conn ->
conn
|> assert_upload_len()
|> assert_version()
|> put_resp_header("location", endpoint_url(bypass.port) <> "/foofile")
|> resp(201, "")
end)

assert {:ok, %{location: endpoint_url(bypass.port) <> "/foofile"}} ==
Post.request(endpoint_url(bypass.port), path, [],
follow_redirect: true
)
end

test "request/1 with metadata", %{bypass: bypass, tmp_file: path} do
Bypass.expect_once(bypass, "POST", "/files", fn conn ->
conn
Expand Down

0 comments on commit 1710e32

Please sign in to comment.