Harden DataTable sort and selection helpers against forged client params (#46)

Whitelist sort columns via sort_columns, ignore invalid sort_by without
crashing, and only accept row ids that exist in the current rows assign.

Author: karim-semmoud

lib/components/data_table.ex

@@ -99,7 +99,11 @@ defmodule Corex.DataTable do
   # mount
   socket
   |> assign(:users, users)
-  |> Corex.DataTable.Sort.assign_for_sort(:users, default_sort_by: :id, default_sort_order: :asc)
+  |> Corex.DataTable.Sort.assign_for_sort(:users,
+    default_sort_by: :id,
+    default_sort_order: :asc,
+    sort_columns: [:id, :name]
+  )
 
   # handle_event("sort", params, socket)
   {:noreply, Corex.DataTable.Sort.handle_sort(socket, params, :users)}

lib/components/data_table/selection.ex

@@ -74,22 +74,34 @@ defmodule Corex.DataTable.Selection do
   Use in `handle_event("select", params, socket)` (same name as `on_select`) and return
   `{:noreply, Corex.DataTable.Selection.handle_select(socket, params, :users)}`.
 
-  `params` must contain `"id"` (checkbox DOM id) and `"checked"`. `rows_assign`
-  is the assign key passed to [`data_table/1`](Corex.DataTable.html#data_table/1) as `rows` (e.g. `:users`).
+  `params` must contain `"id"` (checkbox DOM id) and `"checked"`. Only row ids derived from
+  current `rows` via `selection_row_id` are added to `:selected`; forged ids are ignored when
+  checking a row. `rows_assign` is the assign key passed to [`data_table/1`](Corex.DataTable.html#data_table/1) as `rows` (e.g. `:users`).
   """
   def handle_select(socket, %{"id" => checkbox_id, "checked" => checked}, rows_assign) do
     table_id = socket.assigns.selection_table_id
+    row_id_fn = socket.assigns.selection_row_id
     row_id = String.replace(checkbox_id, "#{table_id}-select-", "")
     rows = socket.assigns[rows_assign] || []
+    valid_ids = valid_row_ids(rows, row_id_fn)
 
     selected =
-      if checked do
-        [row_id | socket.assigns.selected] |> Enum.uniq()
-      else
-        List.delete(socket.assigns.selected, row_id)
+      cond do
+        checked and MapSet.member?(valid_ids, row_id) ->
+          [row_id | socket.assigns.selected] |> Enum.uniq()
+
+        checked ->
+          socket.assigns.selected
+
+        true ->
+          List.delete(socket.assigns.selected, row_id)
       end
 
-    all_selected = length(selected) == length(rows)
+    selected = Enum.filter(selected, &MapSet.member?(valid_ids, &1))
+
+    all_selected =
+      MapSet.subset?(MapSet.new(selected), valid_ids) and
+        length(selected) == MapSet.size(valid_ids)
 
     socket
     |> assign(:selected, selected)
@@ -129,4 +141,10 @@ defmodule Corex.DataTable.Selection do
 
     socket
   end
+
+  defp valid_row_ids(rows, row_id_fn) when is_function(row_id_fn, 1) do
+    rows
+    |> Enum.map(row_id_fn)
+    |> MapSet.new()
+  end
 end

lib/components/data_table/sort.ex

@@ -15,7 +15,11 @@ defmodule Corex.DataTable.Sort do
         socket =
           socket
           |> assign(:users, fetch_users())
-          |> Corex.DataTable.Sort.assign_for_sort(:users, default_sort_by: :id, default_sort_order: :asc)
+          |> Corex.DataTable.Sort.assign_for_sort(:users,
+            default_sort_by: :id,
+            default_sort_order: :asc,
+            sort_columns: [:id, :name]
+          )
 
         {:ok, socket}
       end
@@ -47,19 +51,24 @@ defmodule Corex.DataTable.Sort do
 
   - `:default_sort_by` – atom; column `name` on [`data_table/1`](Corex.DataTable.html#data_table/1) (e.g. `:id`)
   - `:default_sort_order` – `:asc` or `:desc`, default `:asc`
+  - `:sort_columns` – list of atoms the client may sort by (e.g. `[:id, :name]`). When set,
+    [`handle_sort/3`](#handle_sort/3) ignores unknown or disallowed `"sort_by"` values instead of
+    raising. Always set this in production LiveViews.
 
   The socket must already have an assign at `rows_assign` (e.g. `:users`) with the same list
   passed as `rows` to [`data_table/1`](Corex.DataTable.html#data_table/1). Adds `:sort_by`,
-  `:sort_order`, and replaces the rows assign with the sorted list.
+  `:sort_order`, `:sort_columns`, and replaces the rows assign with the sorted list.
   """
   def assign_for_sort(socket, rows_assign, opts \\ []) do
     sort_by = Keyword.get(opts, :default_sort_by)
     sort_order = Keyword.get(opts, :default_sort_order, :asc)
+    sort_columns = Keyword.get(opts, :sort_columns)
     rows = socket.assigns[rows_assign] || []
 
     socket
     |> assign(:sort_by, sort_by)
     |> assign(:sort_order, sort_order)
+    |> assign(:sort_columns, sort_columns)
     |> assign(rows_assign, sort_rows(rows, sort_by, sort_order))
   end
 
@@ -69,11 +78,19 @@ defmodule Corex.DataTable.Sort do
   Use in `handle_event("sort", params, socket)` (same name as `on_sort`) and return
   `{:noreply, Corex.DataTable.Sort.handle_sort(socket, params, :users)}`.
 
-  `params` must contain `"sort_by"` (string, e.g. `"id"`). It is converted to an atom.
+  `params` must contain `"sort_by"` (string, e.g. `"id"`). When `:sort_columns` was set via
+  [`assign_for_sort/3`](#assign_for_sort/3), only those columns are accepted; other values are
+  ignored and the socket is returned unchanged. Unknown atoms never crash the LiveView process.
   `rows_assign` is the assign key passed to [`data_table/1`](Corex.DataTable.html#data_table/1) as `rows`.
   """
   def handle_sort(socket, %{"sort_by" => sort_by_param}, rows_assign) do
-    sort_by = String.to_existing_atom(sort_by_param)
+    case parse_sort_by(sort_by_param, socket.assigns[:sort_columns]) do
+      {:ok, sort_by} -> apply_sort(socket, sort_by, rows_assign)
+      :error -> socket
+    end
+  end
+
+  defp apply_sort(socket, sort_by, rows_assign) do
     current_sort_by = socket.assigns.sort_by
     current_sort_order = socket.assigns.sort_order
 
@@ -92,6 +109,22 @@ defmodule Corex.DataTable.Sort do
     |> assign(rows_assign, sort_rows(rows, sort_by, sort_order))
   end
 
+  defp parse_sort_by(param, columns) when is_list(columns) do
+    with {:ok, sort_by} <- safe_existing_atom(param), true <- sort_by in columns do
+      {:ok, sort_by}
+    else
+      _ -> :error
+    end
+  end
+
+  defp parse_sort_by(param, _columns), do: safe_existing_atom(param)
+
+  defp safe_existing_atom(param) when is_binary(param) do
+    {:ok, String.to_existing_atom(param)}
+  rescue
+    ArgumentError -> :error
+  end
+
   defp toggle_sort_order(:asc), do: :desc
   defp toggle_sort_order(:desc), do: :asc
 

test/components/data_table_selection_test.exs

@@ -49,6 +49,26 @@ defmodule Corex.DataTable.SelectionTest do
 
       assert Enum.sort(socket.assigns.selected) == ["1", "2"]
     end
+
+    test "ignores forged row id when checking" do
+      socket = base_socket()
+
+      socket =
+        Selection.handle_select(socket, %{"id" => "tbl-select-forged", "checked" => true}, :users)
+
+      assert socket.assigns.selected == []
+    end
+
+    test "drops stale selected ids not in current rows" do
+      socket =
+        base_socket()
+        |> assign(:selected, ["1", "stale"])
+
+      socket =
+        Selection.handle_select(socket, %{"id" => "tbl-select-2", "checked" => true}, :users)
+
+      assert Enum.sort(socket.assigns.selected) == ["1", "2"]
+    end
   end
 
   describe "handle_select_all/3" do

test/components/data_table_sort_test.exs

@@ -14,10 +14,15 @@ defmodule Corex.DataTable.SortTest do
       socket =
         %Phoenix.LiveView.Socket{}
         |> assign(:users, rows())
-        |> Sort.assign_for_sort(:users, default_sort_by: :id, default_sort_order: :asc)
+        |> Sort.assign_for_sort(:users,
+          default_sort_by: :id,
+          default_sort_order: :asc,
+          sort_columns: [:id, :name]
+        )
 
       assert socket.assigns.sort_by == :id
       assert socket.assigns.sort_order == :asc
+      assert socket.assigns.sort_columns == [:id, :name]
       assert Enum.map(socket.assigns.users, & &1.id) == [1, 2]
     end
 
@@ -34,11 +39,16 @@ defmodule Corex.DataTable.SortTest do
   end
 
   describe "handle_sort/3" do
+    defp sorted_socket(opts \\ []) do
+      defaults = [default_sort_by: :id, default_sort_order: :asc, sort_columns: [:id, :name]]
+
+      %Phoenix.LiveView.Socket{}
+      |> assign(:users, rows())
+      |> Sort.assign_for_sort(:users, Keyword.merge(defaults, opts))
+    end
+
     test "sorts by new column ascending" do
-      socket =
-        %Phoenix.LiveView.Socket{}
-        |> assign(:users, rows())
-        |> Sort.assign_for_sort(:users, default_sort_by: :id, default_sort_order: :asc)
+      socket = sorted_socket()
 
       socket = Sort.handle_sort(socket, %{"sort_by" => "name"}, :users)
 
@@ -48,14 +58,28 @@ defmodule Corex.DataTable.SortTest do
     end
 
     test "toggles order when sorting same column" do
-      socket =
-        %Phoenix.LiveView.Socket{}
-        |> assign(:users, rows())
-        |> Sort.assign_for_sort(:users, default_sort_by: :name, default_sort_order: :asc)
+      socket = sorted_socket(default_sort_by: :name)
 
       socket = Sort.handle_sort(socket, %{"sort_by" => "name"}, :users)
 
       assert socket.assigns.sort_order == :desc
     end
+
+    test "ignores unknown sort_by atom" do
+      socket = sorted_socket()
+
+      assert socket == Sort.handle_sort(socket, %{"sort_by" => "not_a_column"}, :users)
+    end
+
+    test "ignores sort_by not in sort_columns whitelist" do
+      socket = sorted_socket(sort_columns: [:id])
+
+      before = socket.assigns
+      socket = Sort.handle_sort(socket, %{"sort_by" => "name"}, :users)
+
+      assert socket.assigns.sort_by == before.sort_by
+      assert socket.assigns.sort_order == before.sort_order
+      assert socket.assigns.users == before.users
+    end
   end
 end