-
Notifications
You must be signed in to change notification settings - Fork 959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Raise if there are too many redirects without a render #3670
Conversation
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) ```
14b5301
to
f9a0d31
Compare
@Gazler wouldn't there be the same issue for push_navigate as well? I think this may need to be solved in the JS instead. |
It never makes it to the JavaScript though, |
Hmm right. Users can still make it behave badly when they do stuff with 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: false, count: 0)}
end
def handle_params(params, _uri, socket) do
if connected?(socket) do
socket =
assign(
socket,
:count,
(Map.get(params, "count", to_string(socket.assigns.count)) |> String.to_integer()) + 1
)
if socket.assigns.permitted do
{:noreply, socket}
else
{:noreply, push_navigate(socket, to: "/?count=#{socket.assigns.count}")}
end
else
{:noreply, socket}
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_navigate(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) |
Yeah, I think to handle something like that, we'd need to handle it outside of the LiveView, since the LiveView doesn't persist between navigates. |
🙌🏻 |
* 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]>
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.