Skip to content
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

runtime checks for socket access in async functions #3161

Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 56 additions & 32 deletions lib/phoenix_live_view/async.ex
Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,47 @@ defmodule Phoenix.LiveView.Async do

alias Phoenix.LiveView.{AsyncResult, Socket, Channel}

defp validate_function_env(func, op, env) do
warn =
# TODO: Remove conditional once we require Elixir v1.14+
if Version.match?(System.version(), ">= 1.14.0") do
fn msg, env -> IO.warn(msg, env) end
else
fn msg, _env -> IO.warn(msg) end
end
defp warn_socket_access(op, warn) do
warn.("""
you are accessing the LiveView Socket inside a function given to #{op}.

# prevent false positives, for example
# start_async(socket, :foo, function_that_returns_the_anonymous_function(socket))
if match?({:&, _, _}, func) or match?({:fn, _, _}, func) do
Macro.prewalk(func, fn
{:socket, meta, nil} ->
warn.(
"""
you are accessing the LiveView Socket inside a function given to #{op}.
This is an expensive operation because the whole socket is copied to the new process.

This is an expensive operation because the whole socket is copied to the new process.
Instead of:

Instead of:
#{op}(socket, :key, fn ->
do_something(socket.assigns.my_assign)
end)

#{op}(socket, :key, fn ->
do_something(socket.assigns.my_assign)
end)
You should do:

You should do:
my_assign = socket.assigns.my_assign

my_assign = socket.assigns.my_assign
#{op}(socket, :key, fn ->
do_something(my_assign)
end)

#{op}(socket, :key, fn ->
do_something(my_assign)
end)
For more information, see https://hexdocs.pm/elixir/1.16.1/process-anti-patterns.html#sending-unnecessary-data.
""")
end

For more information, see https://hexdocs.pm/elixir/1.16.1/process-anti-patterns.html#sending-unnecessary-data.
""",
Keyword.take(meta, [:line, :column]) ++ [line: env.line, file: env.file]
)
defp validate_function_env(func, op, env) do
# prevent false positives, for example
# start_async(socket, :foo, function_that_returns_the_anonymous_function(socket))
if match?({:&, _, _}, func) or match?({:fn, _, _}, func) do
Macro.prewalk(func, fn
{:socket, meta, nil} ->
warn_socket_access(op, fn msg ->
# TODO: Remove conditional once we require Elixir v1.14+
meta =
if Version.match?(System.version(), ">= 1.14.0") do
Keyword.take(meta, [:line, :column]) ++ [line: env.line, file: env.file]
else
[]
SteffenDE marked this conversation as resolved.
Show resolved Hide resolved
end

IO.warn(msg, meta)
end)

other ->
other
Expand All @@ -50,20 +53,36 @@ defmodule Phoenix.LiveView.Async do
:ok
end

defp validate_function_env(func, op) do
{:env, variables} = Function.info(func, :env)
SteffenDE marked this conversation as resolved.
Show resolved Hide resolved

if Enum.any?(variables, &match?(%Phoenix.LiveView.Socket{}, &1)) do
SteffenDE marked this conversation as resolved.
Show resolved Hide resolved
warn_socket_access(op, fn msg -> IO.warn(msg) end)
end
end

def start_async(socket, key, func, opts, env) do
validate_function_env(func, :start_async, env)

quote do
Phoenix.LiveView.Async.run_async_task(
Phoenix.LiveView.Async.start_async(
unquote(socket),
unquote(key),
unquote(func),
:start,
unquote(opts)
)
end
end

def start_async(%Socket{} = socket, key, func, opts) when is_function(func, 0) do
# runtime check
if Phoenix.LiveView.connected?(socket) do
validate_function_env(func, :start_async)
end

run_async_task(socket, key, func, :start, opts)
end

def assign_async(socket, key_or_keys, func, opts, env) do
validate_function_env(func, :assign_async, env)

Expand All @@ -80,6 +99,11 @@ defmodule Phoenix.LiveView.Async do
def assign_async(%Socket{} = socket, key_or_keys, func, opts)
when (is_atom(key_or_keys) or is_list(key_or_keys)) and
is_function(func, 0) do
# runtime check
if Phoenix.LiveView.connected?(socket) do
validate_function_env(func, :assign_async)
end

keys = List.wrap(key_or_keys)

# verifies result inside task
Expand Down
87 changes: 87 additions & 0 deletions test/phoenix_live_view/async_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
defmodule Phoenix.LiveView.AsyncTest do
# run with async: false to prevent other messages from being captured
use ExUnit.Case, async: false

import ExUnit.CaptureIO

describe "async operations" do
for fun <- [:assign_async, :start_async] do
test "warns when passing socket to #{fun} function" do
warnings =
capture_io(:stderr, fn ->
Code.eval_string("""
defmodule AssignAsyncSocket do
SteffenDE marked this conversation as resolved.
Show resolved Hide resolved
use Phoenix.LiveView

def mount(_params, _session, socket) do
{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)}
end

defp do_something(_socket), do: :ok
end
Copy link
Member

@josevalim josevalim Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably use eval_quoted. The if false is because we want the code expanded but not executed. :)

Suggested change
defmodule AssignAsyncSocket do
use Phoenix.LiveView
def mount(_params, _session, socket) do
{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)}
end
defp do_something(_socket), do: :ok
end
require Phoenix.LiveView
if false do
Phoenix.LiveView.#{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)
end

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No luck with eval_quoted.

ExUnit.start()

defmodule MyTest do
  use ExUnit.Case

  import ExUnit.CaptureIO

  for fun <- [:assign_async, :start_async] do
    test "warns when passing socket to #{fun} function" do
      warnings =
        capture_io(:stderr, fn ->
          Code.eval_quoted(
            quote bind_quoted: [fun: unquote(fun)] do
              require Phoenix.LiveView

              if false do
                fun(socket, :foo, fn ->
                  do_something(socket.assigns)
                end)
              end
            end
          )
        end)

      assert warnings =~
               "you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
    end
  end
end
mix run test.exs


  1) test warns when passing socket to start_async function (MyTest)
     test.exs:9
     ** (CompileError) nofile: cannot compile file (errors have been logged)
     stacktrace:
       (elixir 1.16.1) src/elixir_clauses.erl:45: :elixir_clauses.clause/6
       (elixir 1.16.1) src/elixir_clauses.erl:347: anonymous fn/7 in :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (stdlib 5.2) lists.erl:1707: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:351: :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:98: :elixir_clauses.case/4
       (elixir 1.16.1) src/elixir_expand.erl:692: :elixir_expand.expand_case/5
       (elixir 1.16.1) expanding macro: Kernel.if/2
       nofile:1: (file)



  2) test warns when passing socket to assign_async function (MyTest)
     test.exs:9
     ** (CompileError) nofile: cannot compile file (errors have been logged)
     stacktrace:
       (elixir 1.16.1) src/elixir_clauses.erl:45: :elixir_clauses.clause/6
       (elixir 1.16.1) src/elixir_clauses.erl:347: anonymous fn/7 in :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (stdlib 5.2) lists.erl:1707: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:351: :elixir_clauses.expand_clauses_origin/6
       (stdlib 5.2) lists.erl:1706: :lists.mapfoldl_1/3
       (elixir 1.16.1) src/elixir_clauses.erl:98: :elixir_clauses.case/4
       (elixir 1.16.1) src/elixir_expand.erl:692: :elixir_expand.expand_case/5
       (elixir 1.16.1) expanding macro: Kernel.if/2
       nofile:1: (file)


Finished in 0.01 seconds (0.01s on load, 0.00s async, 0.00s sync)
2 tests, 2 failures

Randomized with seed 369048

“errors have been logged”

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

          Code.eval_quoted(
            quote do
              require Phoenix.LiveView

              if false do
                unquote(fun)(socket, :foo, fn ->
                  do_something(socket.assigns)
                end)
              end
            end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, sorry:

        Code.eval_quoted(
            quote do
              require Phoenix.LiveView

              if false do
                Phoenix.LiveView.unquote(fun)(socket, :foo, fn ->
                  do_something(socket.assigns)
                end)
              end
            end

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. No luck with that either. But I know why I get no compiler errors... It's inside the capture_io.

Removing it yields

error: undefined variable "socket" (context MyTest)
└─ nofile:1

which kind of makes sense.

Besides that I think the defmodule code is actually alright. Might even be better to understand.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some eval_quoted variants as well. Let me know which ones you like better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

""")
end)

assert warnings =~
"you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
end

test "does not warn when accessing socket outside of function passed to #{fun}" do
warnings =
capture_io(:stderr, fn ->
Code.eval_string("""
defmodule AssignAsyncSocket do
use Phoenix.LiveView

def mount(_params, _session, socket) do
socket = assign(socket, :foo, :bar)
foo = socket.assigns.foo

{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(foo)
end)}
end

defp do_something(assigns), do: :ok
end
""")
end)

refute warnings =~
"you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
end

test "does not warn when argument is not a function (#{fun})" do
warnings =
capture_io(:stderr, fn ->
Code.eval_string("""
defmodule AssignAsyncSocket do
use Phoenix.LiveView

def mount(_params, _session, socket) do
socket = assign(socket, :foo, :bar)

{:ok, #{unquote(fun)}(socket, :foo, function_that_returns_the_func(socket))}
end

defp function_that_returns_the_func(socket) do
foo = socket.assigns.foo

fn ->
do_something(foo)
end
end

defp do_something(assigns), do: :ok
end
""")
end)

refute warnings =~
"you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
end
end
end
end
14 changes: 14 additions & 0 deletions test/phoenix_live_view/integrations/assign_async_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ defmodule Phoenix.LiveView.AssignAsyncTest do
import Phoenix.LiveViewTest
alias Phoenix.LiveViewTest.Endpoint

import ExUnit.CaptureIO

@endpoint Endpoint

setup do
Expand Down Expand Up @@ -83,6 +85,18 @@ defmodule Phoenix.LiveView.AssignAsyncTest do
assert render(lv)
assert_receive {:exit, _pid, :boom}, 1000
end

test "warns when accessing socket in function at runtime", %{conn: conn} do
warnings =
capture_io(:stderr, fn ->
{:ok, lv, _html} = live(conn, "/assign_async?test=socket_warning")

render_async(lv)
end)

assert warnings =~
"you are accessing the LiveView Socket inside a function given to assign_async"
end
end

describe "LiveComponent assign_async" do
Expand Down
16 changes: 15 additions & 1 deletion test/phoenix_live_view/integrations/start_async_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ defmodule Phoenix.LiveView.StartAsyncTest do
import Phoenix.LiveViewTest
alias Phoenix.LiveViewTest.Endpoint

import ExUnit.CaptureIO

@endpoint Endpoint

setup do
Expand Down Expand Up @@ -65,7 +67,7 @@ defmodule Phoenix.LiveView.StartAsyncTest do
Process.register(self(), :start_async_trap_exit_test)
{:ok, lv, _html} = live(conn, "/start_async?test=trap_exit")

assert render_async(lv, 200) =~ "result: :loading"
assert render_async(lv, 200) =~ "{:exit, :boom}"
assert render(lv)
assert_receive {:exit, _pid, :boom}, 1000
end
Expand Down Expand Up @@ -99,6 +101,18 @@ defmodule Phoenix.LiveView.StartAsyncTest do

assert render_async(lv) =~ "flash: hello"
end

test "warns when accessing socket in function at runtime", %{conn: conn} do
warnings =
capture_io(:stderr, fn ->
{:ok, lv, _html} = live(conn, "/start_async?test=socket_warning")

render_async(lv)
end)

assert warnings =~
"you are accessing the LiveView Socket inside a function given to start_async"
end
end

describe "LiveComponent start_async" do
Expand Down
83 changes: 0 additions & 83 deletions test/phoenix_live_view_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -301,87 +301,4 @@ defmodule Phoenix.LiveViewUnitTest do
end
end
end

describe "async operations" do
import ExUnit.CaptureIO

for fun <- [:assign_async, :start_async] do
test "warns when passing socket to #{fun} function" do
warnings =
capture_io(:stderr, fn ->
Code.eval_string("""
defmodule AssignAsyncSocket do
use Phoenix.LiveView

def mount(_params, _session, socket) do
{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(socket.assigns)
end)}
end

defp do_something(_socket), do: :ok
end
""")
end)

assert warnings =~
"you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
end

test "does not warn when accessing socket outside of function passed to #{fun}" do
warnings =
capture_io(:stderr, fn ->
Code.eval_string("""
defmodule AssignAsyncSocket do
use Phoenix.LiveView

def mount(_params, _session, socket) do
socket = assign(socket, :foo, :bar)
foo = socket.assigns.foo

{:ok, #{unquote(fun)}(socket, :foo, fn ->
do_something(foo)
end)}
end

defp do_something(_assigns), do: :ok
end
""")
end)

refute warnings =~
"you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
end

test "does not warn when argument is not a function (#{fun})" do
warnings =
capture_io(:stderr, fn ->
Code.eval_string("""
defmodule AssignAsyncSocket do
use Phoenix.LiveView

def mount(_params, _session, socket) do
socket = assign(socket, :foo, :bar)

{:ok, #{unquote(fun)}(socket, :foo, function_that_returns_the_func(socket))}
end

defp function_that_returns_the_func(socket) do
foo = socket.assigns.foo

fn ->
do_something(foo)
end
end

defp do_something(_assigns), do: :ok
end
""")
end)

refute warnings =~
"you are accessing the LiveView Socket inside a function given to #{unquote(fun)}"
end
end
end
end
Loading
Loading