Normalize URI before redirect in order to avoid :badarg failure from nil port in URI

[bfolkens]

Per #327 - URI.merge/2 overwrites port attribute with nil during merge with relative redirect URI. Since URI.merge/2 correctly follows RFC 3986 5.2 Relative Resolution, a separate normalization step is needed to include RFC 3986 6.2.3 Scheme-Based Normalization.

Note: a test was not included since Bypass runs on a port other than 80 or 443, and we would explicitly need to test to see if the port will be defaulted to 80/443 (depending on the original URI scheme). However, we were able to confirm a test watching for :badarg exit fails prior to the patch and succeeds after the patch. This test has been excluded from the PR since it hangs the tests while waiting for a connection timeout in the success case.

[wojtekmach]

Thank you. Could you look into testing it using the :plug feature, without bypass?

[bfolkens]

I tried to implement a test using the :plug feature via the following:

test "inherit port", c do
  relative_redirect = fn
    %Plug.Conn{host: "redirected", port: nil} ->
      flunk("port cannot be nil")

    %Plug.Conn{host: "redirected", port: 80} = conn ->
      Plug.Conn.send_resp(conn, 200, "ok")

    conn ->
      conn
      |> Plug.Conn.put_resp_header("location", "//redirected")
      |> Plug.Conn.send_resp(301, "")
  end

  assert ExUnit.CaptureLog.capture_log(fn ->
           assert Req.get!(c.url, plug: relative_redirect).status == 200
         end) =~ "[debug] redirecting to //redirected"
end

However, in the underlying Plug.Adapters.Test.Conn.conn/4 port is coalesced into 80 (https://github.com/elixir-plug/plug/blob/2eda50618296b1c0e5fd9d0066e382c34b28ea0a/lib/plug/adapters/test/conn.ex#L39), so it masks the failure case and the above test will always pass.

The above test at least ensures that there's no fatal error with the normalize_redirect_uri/1 code, though I don't think it's entirely useful. Happy to entertain any other ideas...

[wojtekmach]

Thank you!

[wojtekmach]

@bfolkens fyi turns out there was an existing similar test, I was able to adjust it to fail before your change and pass afterwards. b303130