redirect: Change POST to GET on HTTP 301..303 (#236)

CHANGELOG.md

@@ -171,6 +171,10 @@ resp.body #=> %File.Stream{}
 
   * [`redirect`]: Rename `:location_trusted` option to `:redirect_trusted`.
 
+  * [`redirect`]: Change HTTP request method to GET only on POST requests that result in 301..303.
+
+    Previously we were changing the method to GET for all 3xx except 307 and 308.
+
   * [`decompress_body`]: Remove support for `deflate` compression
 
   * [`decompress_body`]: Don't crash on unknown codec

lib/req/steps.ex

@@ -1454,8 +1454,8 @@ defmodule Req.Steps do
     # assume put_params step already run so remove :params option so it's not applied again
     |> Req.Request.delete_option(:params)
     |> remove_credentials_if_untrusted(redirect_trusted, location_url)
-    |> put_redirect_request_method(response.status)
-    |> put_redirect_location(location_url)
+    |> put_redirect_method(response.status)
+    |> Map.replace!(:url, location_url)
   end
 
   defp log_redirect(false, _location), do: :ok
@@ -1464,13 +1464,20 @@ defmodule Req.Steps do
     Logger.log(level, ["redirecting to ", location])
   end
 
-  defp put_redirect_location(request, location_url) do
-    put_in(request.url, location_url)
+  # https://www.rfc-editor.org/rfc/rfc9110#name-301-moved-permanently and 302:
+  #
+  # > Note: For historical reasons, a user agent MAY change the request method from
+  # > POST to GET for the subsequent request.
+  #
+  # And my understanding is essentially same applies for 303.
+  # Also see https://everything.curl.dev/http/redirects
+  defp put_redirect_method(%{method: :post} = request, status) when status in 301..303 do
+    %{request | method: :get}
   end
 
-  defp put_redirect_request_method(request, status) when status in 307..308, do: request
-
-  defp put_redirect_request_method(request, _), do: %{request | method: :get}
+  defp put_redirect_method(request, _status) do
+    request
+  end
 
   defp remove_credentials_if_untrusted(request, true, _), do: request
 

test/req/steps_test.exs

@@ -698,32 +698,52 @@ defmodule Req.StepsTest do
              end) =~ "[debug] redirecting to /ok?a=1"
     end
 
-    test "301..303", c do
-      Bypass.expect(c.bypass, "POST", "/redirect", fn conn ->
-        redirect(conn, 301, c.url <> "/ok")
-      end)
+    test "change POST to GET to get on 301..303", c do
+      for status <- 301..303 do
+        Bypass.expect(c.bypass, "POST", "/redirect", fn conn ->
+          redirect(conn, status, c.url <> "/ok")
+        end)
 
-      Bypass.expect(c.bypass, "GET", "/ok", fn conn ->
-        Plug.Conn.send_resp(conn, 200, "ok")
-      end)
+        Bypass.expect(c.bypass, "GET", "/ok", fn conn ->
+          Plug.Conn.send_resp(conn, 200, "ok")
+        end)
 
-      assert ExUnit.CaptureLog.capture_log(fn ->
-               assert Req.post!(c.url <> "/redirect", body: "body").status == 200
-             end) =~ "[debug] redirecting to #{c.url}/ok"
+        assert ExUnit.CaptureLog.capture_log(fn ->
+                 assert Req.post!(c.url <> "/redirect", body: "body").status == 200
+               end) =~ "[debug] redirecting to #{c.url}/ok"
+      end
     end
 
-    test "307..308", c do
-      Bypass.expect(c.bypass, "POST", "/redirect", fn conn ->
-        redirect(conn, 307, c.url <> "/ok")
-      end)
+    test "do not change method on 307 and 308", c do
+      for status <- [307, 308] do
+        Bypass.expect(c.bypass, "POST", "/redirect", fn conn ->
+          redirect(conn, status, c.url <> "/ok")
+        end)
 
-      Bypass.expect(c.bypass, "POST", "/ok", fn conn ->
-        Plug.Conn.send_resp(conn, 200, "ok")
-      end)
+        Bypass.expect(c.bypass, "POST", "/ok", fn conn ->
+          Plug.Conn.send_resp(conn, 200, "ok")
+        end)
 
-      assert ExUnit.CaptureLog.capture_log(fn ->
-               assert Req.post!(c.url <> "/redirect", body: "body").status == 200
-             end) =~ "[debug] redirecting to #{c.url}/ok"
+        assert ExUnit.CaptureLog.capture_log(fn ->
+                 assert Req.post!(c.url <> "/redirect", body: "body").status == 200
+               end) =~ "[debug] redirecting to #{c.url}/ok"
+      end
+    end
+
+    test "never change HEAD requests", c do
+      for status <- [301, 302, 303, 307, 307] do
+        Bypass.expect(c.bypass, "HEAD", "/redirect", fn conn ->
+          redirect(conn, status, c.url <> "/ok")
+        end)
+
+        Bypass.expect(c.bypass, "HEAD", "/ok", fn conn ->
+          Plug.Conn.send_resp(conn, 200, "")
+        end)
+
+        assert ExUnit.CaptureLog.capture_log(fn ->
+                 assert Req.head!(c.url <> "/redirect").status == 200
+               end) =~ "[debug] redirecting to #{c.url}/ok"
+      end
     end
 
     test "auth same host", c do