Hi,
in the process of securing my liveview callbacks, I’m hardening my handle_event callbacks checking that actions are allowed for the user submitting it.
My question is : is it ok to use raise as in the following excerpt ?
def handle_event("delete_board", %{"id" => id} = params, %{assigns: assigns} = socket) do
id = to_integer(id)
row = Enum.find(assigns.boards, & &1.id == id)
if !row , do: raise("Access to forbidden data detected")
if !user_rights(row, :delete_board?), do: raise("Improper right access detected")
{:ok, res} = Repo.query("select sp_delete_board($1);", [id])
boards = for b <- assigns.boards, b.id != id, do: b
broadcast_from(self(), @boards_topic, "delete_board", %{id: id})
{:noreply, assign(socket, boards: boards)}
end
Or should I go we nested case/if returning non modified socket object so that nothing is done where action is not validated. How do you handle it in your apps ?
I would say it depends on your application. A few considerations:
Do you log errors, and do you want to be able to inspect those logs? If so, raising might be ok, assuming that an unauthorized access is an unexpected exceptional behavior.
How likely it is that a normal user would get into the situation where they try to perform an action that they are not authorized to perform? If it can happen, I would rather skip the action, and add some error in the assigns, so an explanatory message can be presented to the user.
A final consideration, but here I don’t know enough about LiveView to determine if it’s a valid concern, so it’s more like a question to the LiveView experts: is it possible for an attacker to disrupt service by causing many of these errors in a short time, therefore hitting the supervisor max_restarts?
Do you log errors, and do you want to be able to inspect those logs? If so, raising might be ok, assuming that an unauthorized access is an unexpected exceptional behavior.
Yep, this is exceptional behaviour as it sould be only the case for people trying to hack the application. My initial concern is illustrated in this thread :
A final consideration, but here I don’t know enough about LiveView to determine if it’s a valid concern, so it’s more like a question to the LiveView experts: is it possible for an attacker to disrupt service by causing many of these errors in a short time, therefore hitting the supervisor max_restarts ?
Yes that’s probably the main reason I’m asking here. I don’t want that the choices I do now makes the application failed later !
@chrismccord, @josevalim I would really appreciate to have your thoughts on this as this is a showstopper for me in order to use liveview in real applications (I mean that goes in the wild where all humans are not always with good intentions…).
No, it is not possible. Channels and LiveViews are marked as temporary. They are never restarted (without a client doing a new request) and they never count towards max failures.
Raising in a LiveView is completely fine and that’s what I do for scenarios I don’t expect to be triggered via the UI - i.e. when someone is trying to hack.
