From 1710e323056ae5b6e553b96a0df93c8d90194ff8 Mon Sep 17 00:00:00 2001 From: Gianluca Nitti Date: Wed, 26 Jan 2022 14:47:09 +0100 Subject: [PATCH] Update HTTPoison and fix PATCH 301 handling 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 --- lib/tus_client/head.ex | 2 +- lib/tus_client/options.ex | 2 +- lib/tus_client/patch.ex | 27 ++++------------------- lib/tus_client/post.ex | 5 ++++- lib/tus_client/utils.ex | 26 ++++++++++++++++++++++ mix.exs | 2 +- mix.lock | 12 +++++------ test/tus_client/head_test.exs | 22 +++++++++++++++++++ test/tus_client/options_test.exs | 21 ++++++++++++++++++ test/tus_client/patch_test.exs | 37 ++++++++++++++++++++++++++++++++ test/tus_client/post_test.exs | 21 ++++++++++++++++++ 11 files changed, 144 insertions(+), 33 deletions(-) diff --git a/lib/tus_client/head.ex b/lib/tus_client/head.ex index 2ce01db..500eef5 100644 --- a/lib/tus_client/head.ex +++ b/lib/tus_client/head.ex @@ -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 diff --git a/lib/tus_client/options.ex b/lib/tus_client/options.ex index 54df492..258d184 100644 --- a/lib/tus_client/options.ex +++ b/lib/tus_client/options.ex @@ -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 diff --git a/lib/tus_client/patch.ex b/lib/tus_client/patch.ex index 678daab..dde3eb5 100644 --- a/lib/tus_client/patch.ex +++ b/lib/tus_client/patch.ex @@ -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 @@ -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 diff --git a/lib/tus_client/post.ex b/lib/tus_client/post.ex index 4ec8698..6080ec7 100644 --- a/lib/tus_client/post.ex +++ b/lib/tus_client/post.ex @@ -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 diff --git a/lib/tus_client/utils.ex b/lib/tus_client/utils.ex index 8f37316..b0d5e9d 100644 --- a/lib/tus_client/utils.ex +++ b/lib/tus_client/utils.ex @@ -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 diff --git a/mix.exs b/mix.exs index e58a731..8aeb0b8 100644 --- a/mix.exs +++ b/mix.exs @@ -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}, diff --git a/mix.lock b/mix.lock index 9167ce5..1b87ad0 100644 --- a/mix.lock +++ b/mix.lock @@ -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"}, @@ -11,15 +11,15 @@ "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"}, @@ -27,5 +27,5 @@ "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"}, } diff --git a/test/tus_client/head_test.exs b/test/tus_client/head_test.exs index f68c80f..79cb8b4 100644 --- a/test/tus_client/head_test.exs +++ b/test/tus_client/head_test.exs @@ -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 diff --git a/test/tus_client/options_test.exs b/test/tus_client/options_test.exs index e2643f3..8cfc30f 100644 --- a/test/tus_client/options_test.exs +++ b/test/tus_client/options_test.exs @@ -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 diff --git a/test/tus_client/patch_test.exs b/test/tus_client/patch_test.exs index b83835b..6ef286a 100644 --- a/test/tus_client/patch_test.exs +++ b/test/tus_client/patch_test.exs @@ -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 diff --git a/test/tus_client/post_test.exs b/test/tus_client/post_test.exs index e76e997..74b2bad 100644 --- a/test/tus_client/post_test.exs +++ b/test/tus_client/post_test.exs @@ -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, "

go follow the location header

") + 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