Raise if there are too many redirects without a render (#3670)

* Raise if there are too many redirects without a render

In circumstances where handle_params does a patch redirect, it is
possible for a user to get stuck in an infinite loop. This can be
expensive if the application performs any querying inside of
handle_params/3

For a dead render, the browser will handle this, as it will give an
`ERR_TOO_MANY_REDIRECTS` - however for a patch, the browser never
actually handles the redirect.

This commit simply stores a counter every time a patch occurs, and if
there's no render between patches, then it will raise, giving the
developer a clue where to look for the loop. A limit of 20 was chosen to
match what chrome uses for its `ERR_TOO_MANY_REDIRECTS` implementation.

```
Mix.install([
  {:phoenix_playground, "~> 0.1.6"},
  {:phoenix_live_view, "~> 1.0.4"}
])

defmodule DemoLive do
  use Phoenix.LiveView

  def mount(_params, _session, socket) do
    {:ok, assign(socket, permitted: true, count: 0)}
  end

  def handle_params(params, _uri, socket) do
    socket = assign(socket, :count, socket.assigns.count + 1)
    if socket.assigns.permitted do
      {:noreply, socket}
    else
      {:noreply, push_patch(socket, to: "/?count=#{socket.assigns.count}")}
    end
  end

  def handle_event("toggle", _params, socket) do
    {:noreply, assign(socket, permitted: !socket.assigns.permitted)}
  end

  def handle_event("patch", _params, socket) do
      {:noreply, push_patch(socket, to: "/?count=#{socket.assigns.count}")}
  end

  def render(assigns) do
    ~H"""
    <div>
      {if @permitted, do: "Permitted", else: "Not Permitted"}
      <button phx-click="toggle">{if @permitted, do: "Disable", else: "Enable"}</button>
      <button phx-click="patch">Patch Redirect</button>
    </div>
    """
  end
end

PhoenixPlayground.start(live: DemoLive, port: 4200)
```

* set low reload jitter for test

* Store redirect count on state instead of in process dictionary

---------

Co-authored-by: Steffen Deusch <[email protected]>

Author: Gazler

lib/phoenix_live_view/channel.ex

@@ -812,6 +812,7 @@ defmodule Phoenix.LiveView.Channel do
       {:diff, diff, new_state} ->
         {:noreply,
          new_state
+         |> clear_live_patch_counter()
          |> push_live_patch(pending_live_patch)
          |> push_diff(diff, ref)}
 
@@ -820,6 +821,24 @@ defmodule Phoenix.LiveView.Channel do
     end
   end
 
+  defp check_patch_redirect_limit!(state) do
+    current = state.redirect_count
+
+    if current == 20 do
+      raise RuntimeError, """
+      too many redirects for #{inspect(state.socket.view)} on action #{inspect(state.socket.assigns.live_action)}
+
+      Check the `handle_params/3` callback for an infinite patch redirect loop
+      """
+    else
+      %{state | redirect_count: current + 1}
+    end
+  end
+
+  defp clear_live_patch_counter(state) do
+    %{state | redirect_count: 0}
+  end
+
   defp handle_redirect(new_state, result, flash, ref) do
     %{socket: new_socket} = new_state
     root_pid = new_socket.root_pid
@@ -857,6 +876,7 @@ defmodule Phoenix.LiveView.Channel do
 
         new_state
         |> drop_redirect()
+        |> check_patch_redirect_limit!()
         |> Map.update!(:socket, &Utils.replace_flash(&1, flash))
         |> sync_handle_params_with_live_redirect(params, action, opts, ref)
 
@@ -1377,6 +1397,7 @@ defmodule Phoenix.LiveView.Channel do
       topic: phx_socket.topic,
       components: Diff.new_components(),
       fingerprints: Diff.new_fingerprints(),
+      redirect_count: 0,
       upload_names: %{},
       upload_pids: %{}
     }

test/e2e/support/navigation.ex

@@ -9,7 +9,11 @@ defmodule Phoenix.LiveViewTest.E2E.Navigation.Layout do
     <script type="module">
       import {LiveSocket} from "/assets/phoenix_live_view/phoenix_live_view.esm.js"
       let csrfToken = document.querySelector("meta[name='csrf-token']").getAttribute("content");
-      let liveSocket = new LiveSocket("/live", window.Phoenix.Socket, {params: {_csrf_token: csrfToken}})
+      let liveSocket = new LiveSocket("/live", window.Phoenix.Socket, {
+        params: {_csrf_token: csrfToken},
+        reloadJitterMin: 50,
+        reloadJitterMax: 500
+      })
       liveSocket.connect()
       window.liveSocket = liveSocket
 
@@ -185,6 +189,36 @@ defmodule Phoenix.LiveViewTest.E2E.Navigation.Dead do
   end
 end
 
+defmodule Phoenix.LiveViewTest.E2E.Navigation.RedirectLoopLive do
+  use Phoenix.LiveView
+
+  @impl Phoenix.LiveView
+  def mount(params, _session, socket) do
+    if params["loop"] do
+      {:ok, assign(socket, message: "Too many redirects", loop: false)}
+    else
+      {:ok, assign(socket, message: nil, loop: true)}
+    end
+  end
+
+  @impl Phoenix.LiveView
+  def handle_params(params, _uri, socket) do
+    if params["loop"] && socket.assigns.loop do
+      {:noreply, push_patch(socket, to: "/navigation/redirectloop?loop=true")}
+    else
+      {:noreply, socket}
+    end
+  end
+
+  @impl Phoenix.LiveView
+  def render(assigns) do
+    ~H"""
+    <div :if={@message} id="message">{@message}</div>
+    <.link patch="/navigation/redirectloop?loop=true">Redirect Loop</.link>
+    """
+  end
+end
+
 defmodule Phoenix.LiveViewTest.E2E.Navigation.DeadHTML do
   use Phoenix.Component
 

test/e2e/test_helper.exs

@@ -172,6 +172,7 @@ defmodule Phoenix.LiveViewTest.E2E.Router do
       live "/a", Phoenix.LiveViewTest.E2E.Navigation.ALive
       live "/b", Phoenix.LiveViewTest.E2E.Navigation.BLive, :index
       live "/b/:id", Phoenix.LiveViewTest.E2E.Navigation.BLive, :show
+      live "/redirectloop", Phoenix.LiveViewTest.E2E.Navigation.RedirectLoopLive, :index
       get "/dead", Phoenix.LiveViewTest.E2E.Navigation.Dead, :index
     end
   end

test/e2e/tests/navigation.spec.js

@@ -64,6 +64,24 @@ test("can navigate between LiveViews in the same live session over websocket", a
   ]))
 })
 
+test("handles live redirect loops", async ({page}) => {
+  await page.goto("/navigation/redirectloop")
+  await syncLV(page)
+
+  await page.getByRole("link", {name: "Redirect Loop"}).click()
+
+  await expect(async () => {
+    expect(webSocketEvents).toEqual(expect.arrayContaining([
+      expect.objectContaining({type: "received", payload: expect.stringContaining("phx_error")}),
+    ]))
+  }).toPass()
+
+  // We need to wait for the LV to reconnect
+  await syncLV(page)
+  const message = page.locator("#message")
+  await expect(message).toHaveText("Too many redirects")
+})
+
 test("popstate", async ({page}) => {
   await page.goto("/navigation/a")
   await syncLV(page)