From 05194336441398a4525b4016930743b0e6e9183f Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 20 Jan 2026 15:55:08 +0100 Subject: [PATCH] feat: add custom boolean field state & URL-Parameter --- lib/mv/constants.ex | 42 ++++++++ lib/mv/membership/import/member_csv.ex | 12 ++- lib/mv_web/live/member_live/index.ex | 133 +++++++++++++++++++++++-- test/mv_web/member_live/index_test.exs | 117 +++++++++++++++++----- 4 files changed, 264 insertions(+), 40 deletions(-) diff --git a/lib/mv/constants.ex b/lib/mv/constants.ex index 73bfcd9..4ef355d 100644 --- a/lib/mv/constants.ex +++ b/lib/mv/constants.ex @@ -19,6 +19,12 @@ defmodule Mv.Constants do @custom_field_prefix "custom_field_" + @boolean_filter_prefix "bf_" + + @max_boolean_filters 50 + + @max_uuid_length 36 + @email_validator_checks [:html_input, :pow] def member_fields, do: @member_fields @@ -33,6 +39,42 @@ defmodule Mv.Constants do """ def custom_field_prefix, do: @custom_field_prefix + @doc """ + Returns the prefix used for boolean custom field filter URL parameters. + + ## Examples + + iex> Mv.Constants.boolean_filter_prefix() + "bf_" + """ + def boolean_filter_prefix, do: @boolean_filter_prefix + + @doc """ + Returns the maximum number of boolean custom field filters allowed per request. + + This limit prevents DoS attacks by restricting the number of filter parameters + that can be processed in a single request. + + ## Examples + + iex> Mv.Constants.max_boolean_filters() + 50 + """ + def max_boolean_filters, do: @max_boolean_filters + + @doc """ + Returns the maximum length of a UUID string (36 characters including hyphens). + + UUIDs in standard format (e.g., "550e8400-e29b-41d4-a716-446655440000") are + exactly 36 characters long. This constant is used for input validation. + + ## Examples + + iex> Mv.Constants.max_uuid_length() + 36 + """ + def max_uuid_length, do: @max_uuid_length + @doc """ Returns the email validator checks used for EctoCommons.EmailValidator. diff --git a/lib/mv/membership/import/member_csv.ex b/lib/mv/membership/import/member_csv.ex index 60cfadf..b4f4318 100644 --- a/lib/mv/membership/import/member_csv.ex +++ b/lib/mv/membership/import/member_csv.ex @@ -303,7 +303,9 @@ defmodule Mv.Membership.Import.MemberCSV do {inserted, failed, errors, _collected_error_count, truncated?} = Enum.reduce(chunk_rows_with_lines, {0, 0, [], 0, false}, fn {line_number, row_map}, - {acc_inserted, acc_failed, acc_errors, acc_error_count, acc_truncated?} -> + {acc_inserted, acc_failed, + acc_errors, acc_error_count, + acc_truncated?} -> current_error_count = existing_error_count + acc_error_count case process_row(row_map, line_number, custom_field_lookup) do @@ -325,7 +327,13 @@ defmodule Mv.Membership.Import.MemberCSV do end end) - {:ok, %{inserted: inserted, failed: failed, errors: Enum.reverse(errors), errors_truncated?: truncated?}} + {:ok, + %{ + inserted: inserted, + failed: failed, + errors: Enum.reverse(errors), + errors_truncated?: truncated? + }} end @doc """ diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index f63407a..1c257bc 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -30,6 +30,7 @@ defmodule MvWeb.MemberLive.Index do on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} require Ash.Query + require Logger import Ash.Expr import MvWeb.LiveHelpers, only: [current_actor: 1] @@ -43,6 +44,15 @@ defmodule MvWeb.MemberLive.Index do # Prefix used in sort field names for custom fields (e.g., "custom_field_") @custom_field_prefix Mv.Constants.custom_field_prefix() + # Prefix used for boolean custom field filter URL parameters (e.g., "bf_") + @boolean_filter_prefix Mv.Constants.boolean_filter_prefix() + + # Maximum number of boolean custom field filters allowed per request (DoS protection) + @max_boolean_filters Mv.Constants.max_boolean_filters() + + # Maximum length of UUID string (36 characters including hyphens) + @max_uuid_length Mv.Constants.max_uuid_length() + # Member fields that are loaded for the overview # Uses constants from Mv.Constants to ensure consistency # Note: :id is always included for member identification @@ -103,6 +113,7 @@ defmodule MvWeb.MemberLive.Index do |> assign_new(:sort_field, fn -> :first_name end) |> assign_new(:sort_order, fn -> :asc end) |> assign(:cycle_status_filter, nil) + |> assign(:boolean_custom_field_filters, %{}) |> assign(:selected_members, MapSet.new()) |> assign(:settings, settings) |> assign(:custom_fields_visible, custom_fields_visible) @@ -220,7 +231,8 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.sort_field, socket.assigns.sort_order, socket.assigns.cycle_status_filter, - new_show_current + new_show_current, + socket.assigns.boolean_custom_field_filters ) new_path = ~p"/members?#{query_params}" @@ -334,7 +346,8 @@ defmodule MvWeb.MemberLive.Index do existing_field_query, existing_sort_query, socket.assigns.cycle_status_filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) # Set the new path with params @@ -363,7 +376,8 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.sort_field, socket.assigns.sort_order, filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) new_path = ~p"/members?#{query_params}" @@ -478,6 +492,7 @@ defmodule MvWeb.MemberLive.Index do |> maybe_update_search(params) |> maybe_update_sort(params) |> maybe_update_cycle_status_filter(params) + |> maybe_update_boolean_filters(params) |> maybe_update_show_current_cycle(params) |> assign(:query, params["query"]) |> assign(:user_field_selection, final_selection) @@ -588,7 +603,8 @@ defmodule MvWeb.MemberLive.Index do field_str, Atom.to_string(order), socket.assigns.cycle_status_filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) new_path = ~p"/members?#{query_params}" @@ -618,7 +634,8 @@ defmodule MvWeb.MemberLive.Index do socket.assigns.sort_field, socket.assigns.sort_order, socket.assigns.cycle_status_filter, - socket.assigns.show_current_cycle + socket.assigns.show_current_cycle, + socket.assigns.boolean_custom_field_filters ) |> maybe_add_field_selection(socket.assigns[:user_field_selection]) @@ -636,12 +653,14 @@ defmodule MvWeb.MemberLive.Index do # Builds URL query parameters map including all filter/sort state. # Converts cycle_status_filter atom to string for URL. + # Adds boolean custom field filters as bf_=true|false. defp build_query_params( query, sort_field, sort_order, cycle_status_filter, - show_current_cycle + show_current_cycle, + boolean_filters ) do field_str = if is_atom(sort_field) do @@ -672,11 +691,19 @@ defmodule MvWeb.MemberLive.Index do end # Add show_current_cycle if true - if show_current_cycle do - Map.put(base_params, "show_current_cycle", "true") - else - base_params - end + base_params = + if show_current_cycle do + Map.put(base_params, "show_current_cycle", "true") + else + base_params + end + + # Add boolean custom field filters + Enum.reduce(boolean_filters, base_params, fn {custom_field_id, filter_value}, acc -> + param_key = "#{@boolean_filter_prefix}#{custom_field_id}" + param_value = if filter_value == true, do: "true", else: "false" + Map.put(acc, param_key, param_value) + end) end # Loads members from the database with custom field values and applies search/sort/payment filters. @@ -1135,6 +1162,90 @@ defmodule MvWeb.MemberLive.Index do defp determine_cycle_status_filter("unpaid"), do: :unpaid defp determine_cycle_status_filter(_), do: nil + # Updates boolean custom field filters from URL parameters if present. + # + # Parses all URL parameters with prefix @boolean_filter_prefix and validates them: + # - Extracts custom field ID from parameter name (explicitly removes prefix) + # - Validates filter value using determine_boolean_filter/1 + # - Whitelisting: Only custom field IDs that exist and have value_type: :boolean + # - Security: Limits to maximum @max_boolean_filters filters to prevent DoS attacks + # - Security: Validates UUID length (max @max_uuid_length characters) + # + # Returns socket with updated :boolean_custom_field_filters assign. + defp maybe_update_boolean_filters(socket, params) do + # Get all boolean custom fields for whitelisting (keyed by ID as string for consistency) + boolean_custom_fields = + socket.assigns.all_custom_fields + |> Enum.filter(&(&1.value_type == :boolean)) + |> Map.new(fn cf -> {to_string(cf.id), cf} end) + + # Parse all boolean filter parameters + prefix_length = String.length(@boolean_filter_prefix) + + filters = + params + |> Enum.filter(fn {key, _value} -> String.starts_with?(key, @boolean_filter_prefix) end) + |> Enum.reduce(%{}, fn {key, value_str}, acc -> + # Extract custom field ID from parameter name (explicitly remove prefix) + # This is more secure than String.replace_prefix which only removes first occurrence + custom_field_id_str = String.slice(key, prefix_length, String.length(key) - prefix_length) + + # Validate custom field ID length (UUIDs are max @max_uuid_length characters) + # This provides an additional security layer beyond UUID format validation + if String.length(custom_field_id_str) <= @max_uuid_length do + # Validate custom field ID exists and is boolean type + case Ecto.UUID.cast(custom_field_id_str) do + {:ok, _custom_field_id} -> + if Map.has_key?(boolean_custom_fields, custom_field_id_str) do + # Validate filter value + case determine_boolean_filter(value_str) do + nil -> acc + filter_value -> Map.put(acc, custom_field_id_str, filter_value) + end + else + acc + end + + :error -> + acc + end + else + acc + end + end) + + # Security: Limit number of filters to prevent DoS attacks + # Maximum @max_boolean_filters boolean filters allowed per request + filters = + if map_size(filters) > @max_boolean_filters do + Logger.warning( + "Too many boolean filters requested: #{map_size(filters)}, limiting to #{@max_boolean_filters}" + ) + + filters + |> Enum.take(@max_boolean_filters) + |> Enum.into(%{}) + else + filters + end + + assign(socket, :boolean_custom_field_filters, filters) + end + + # Determines valid boolean filter value from URL parameter. + # + # SECURITY: This function whitelists allowed filter values. Only "true" and "false" + # are accepted - all other input (including malicious strings) falls back to nil. + # This ensures no raw user input is ever passed to filter functions. + # + # Returns: + # - `:true` for "true" string + # - `:false` for "false" string + # - `nil` for any other value + defp determine_boolean_filter("true"), do: true + defp determine_boolean_filter("false"), do: false + defp determine_boolean_filter(_), do: nil + # Updates show_current_cycle from URL parameters if present. defp maybe_update_show_current_cycle(socket, %{"show_current_cycle" => "true"}) do assign(socket, :show_current_cycle, true) diff --git a/test/mv_web/member_live/index_test.exs b/test/mv_web/member_live/index_test.exs index 69d8972..7e7249e 100644 --- a/test/mv_web/member_live/index_test.exs +++ b/test/mv_web/member_live/index_test.exs @@ -696,26 +696,26 @@ defmodule MvWeb.MemberLive.IndexTest do assert state.socket.assigns.boolean_custom_field_filters == %{} end - test "handle_params parses boolean_filter_ values correctly", %{conn: conn} do + test "handle_params parses bf_ values correctly", %{conn: conn} do conn = conn_with_oidc_user(conn) boolean_field = create_boolean_custom_field() # Test true value {:ok, view1, _html} = - live(conn, "/members?boolean_filter_#{boolean_field.id}=true") + live(conn, "/members?bf_#{boolean_field.id}=true") state1 = :sys.get_state(view1.pid) filters1 = state1.socket.assigns.boolean_custom_field_filters - assert filters1[boolean_field.id] == :true + assert filters1[boolean_field.id] == true refute filters1[boolean_field.id] == "true" # Test false value {:ok, view2, _html} = - live(conn, "/members?boolean_filter_#{boolean_field.id}=false") + live(conn, "/members?bf_#{boolean_field.id}=false") state2 = :sys.get_state(view2.pid) filters2 = state2.socket.assigns.boolean_custom_field_filters - assert filters2[boolean_field.id] == :false + assert filters2[boolean_field.id] == false refute filters2[boolean_field.id] == "false" end @@ -724,7 +724,7 @@ defmodule MvWeb.MemberLive.IndexTest do fake_id = Ecto.UUID.generate() {:ok, view, _html} = - live(conn, "/members?boolean_filter_#{fake_id}=true") + live(conn, "/members?bf_#{fake_id}=true") state = :sys.get_state(view.pid) filters = state.socket.assigns.boolean_custom_field_filters @@ -739,7 +739,7 @@ defmodule MvWeb.MemberLive.IndexTest do string_field = create_string_custom_field() {:ok, view, _html} = - live(conn, "/members?boolean_filter_#{string_field.id}=true") + live(conn, "/members?bf_#{string_field.id}=true") state = :sys.get_state(view.pid) filters = state.socket.assigns.boolean_custom_field_filters @@ -758,7 +758,7 @@ defmodule MvWeb.MemberLive.IndexTest do for invalid_value <- invalid_values do {:ok, view, _html} = - live(conn, "/members?boolean_filter_#{boolean_field.id}=#{invalid_value}") + live(conn, "/members?bf_#{boolean_field.id}=#{invalid_value}") state = :sys.get_state(view.pid) filters = state.socket.assigns.boolean_custom_field_filters @@ -777,14 +777,14 @@ defmodule MvWeb.MemberLive.IndexTest do {:ok, view, _html} = live( conn, - "/members?boolean_filter_#{boolean_field1.id}=true&boolean_filter_#{boolean_field2.id}=false" + "/members?bf_#{boolean_field1.id}=true&bf_#{boolean_field2.id}=false" ) state = :sys.get_state(view.pid) filters = state.socket.assigns.boolean_custom_field_filters - assert filters[boolean_field1.id] == :true - assert filters[boolean_field2.id] == :false + assert filters[boolean_field1.id] == true + assert filters[boolean_field2.id] == false assert map_size(filters) == 2 end @@ -799,7 +799,7 @@ defmodule MvWeb.MemberLive.IndexTest do {:ok, view1, _html} = live( conn, - "/members?boolean_filter_#{boolean_field1.id}=true&boolean_filter_#{boolean_field2.id}=false" + "/members?bf_#{boolean_field1.id}=true&bf_#{boolean_field2.id}=false" ) # Trigger a search to see if filters are preserved in URL @@ -809,8 +809,8 @@ defmodule MvWeb.MemberLive.IndexTest do # Check that the patch includes boolean filters path1 = assert_patch(view1) - assert path1 =~ "boolean_filter_#{boolean_field1.id}=true" - assert path1 =~ "boolean_filter_#{boolean_field2.id}=false" + assert path1 =~ "bf_#{boolean_field1.id}=true" + assert path1 =~ "bf_#{boolean_field2.id}=false" # Test without filters (nil filters should not appear in URL) {:ok, view2, _html} = live(conn, "/members") @@ -820,9 +820,9 @@ defmodule MvWeb.MemberLive.IndexTest do |> element("[data-testid='search-input']") |> render_change(%{value: "test"}) - # Check that no boolean_filter params are in URL + # Check that no bf_ params are in URL path2 = assert_patch(view2) - refute path2 =~ "boolean_filter_" + refute path2 =~ "bf_" end test "boolean filters are preserved during navigation actions", %{conn: conn} do @@ -830,7 +830,7 @@ defmodule MvWeb.MemberLive.IndexTest do boolean_field = create_boolean_custom_field() {:ok, view, _html} = - live(conn, "/members?boolean_filter_#{boolean_field.id}=true") + live(conn, "/members?bf_#{boolean_field.id}=true") # Test sort toggle preserves filter view @@ -838,7 +838,7 @@ defmodule MvWeb.MemberLive.IndexTest do |> render_click() path1 = assert_patch(view) - assert path1 =~ "boolean_filter_#{boolean_field.id}=true" + assert path1 =~ "bf_#{boolean_field.id}=true" # Test search change preserves filter view @@ -846,7 +846,7 @@ defmodule MvWeb.MemberLive.IndexTest do |> render_change(%{value: "test"}) path2 = assert_patch(view) - assert path2 =~ "boolean_filter_#{boolean_field.id}=true" + assert path2 =~ "bf_#{boolean_field.id}=true" end test "boolean filters work together with cycle_status_filter", %{conn: conn} do @@ -856,14 +856,14 @@ defmodule MvWeb.MemberLive.IndexTest do {:ok, view, _html} = live( conn, - "/members?cycle_status_filter=paid&boolean_filter_#{boolean_field.id}=true" + "/members?cycle_status_filter=paid&bf_#{boolean_field.id}=true" ) state = :sys.get_state(view.pid) filters = state.socket.assigns.boolean_custom_field_filters # Both filters should be set - assert filters[boolean_field.id] == :true + assert filters[boolean_field.id] == true assert state.socket.assigns.cycle_status_filter == :paid # Both should be in URL when triggering search @@ -873,7 +873,7 @@ defmodule MvWeb.MemberLive.IndexTest do path = assert_patch(view) assert path =~ "cycle_status_filter=paid" - assert path =~ "boolean_filter_#{boolean_field.id}=true" + assert path =~ "bf_#{boolean_field.id}=true" end test "handle_params removes filter when custom field is deleted", %{conn: conn} do @@ -882,18 +882,18 @@ defmodule MvWeb.MemberLive.IndexTest do # Set up filter via URL {:ok, view, _html} = - live(conn, "/members?boolean_filter_#{boolean_field.id}=true") + live(conn, "/members?bf_#{boolean_field.id}=true") state_before = :sys.get_state(view.pid) filters_before = state_before.socket.assigns.boolean_custom_field_filters - assert filters_before[boolean_field.id] == :true + assert filters_before[boolean_field.id] == true # Delete the custom field Ash.destroy!(boolean_field) # Navigate again - filter should be removed since custom field no longer exists {:ok, view2, _html} = - live(conn, "/members?boolean_filter_#{boolean_field.id}=true") + live(conn, "/members?bf_#{boolean_field.id}=true") state_after = :sys.get_state(view2.pid) filters_after = state_after.socket.assigns.boolean_custom_field_filters @@ -911,14 +911,77 @@ defmodule MvWeb.MemberLive.IndexTest do encoded_id = URI.encode(boolean_field.id) {:ok, view, _html} = - live(conn, "/members?boolean_filter_#{encoded_id}=true") + live(conn, "/members?bf_#{encoded_id}=true") state = :sys.get_state(view.pid) filters = state.socket.assigns.boolean_custom_field_filters # Filter should work with URL-encoded ID # Phoenix should decode it automatically, so we check with original ID - assert filters[boolean_field.id] == :true + assert filters[boolean_field.id] == true + end + + test "handle_params ignores malformed prefix (bf_bf_)", %{conn: conn} do + conn = conn_with_oidc_user(conn) + boolean_field = create_boolean_custom_field() + + # Try to send parameter with double prefix + {:ok, view, _html} = + live(conn, "/members?bf_bf_#{boolean_field.id}=true") + + state = :sys.get_state(view.pid) + filters = state.socket.assigns.boolean_custom_field_filters + + # Should not parse as valid filter (UUID validation should fail) + refute Map.has_key?(filters, boolean_field.id) + assert filters == %{} + end + + test "handle_params limits number of boolean filters to prevent DoS", %{conn: conn} do + conn = conn_with_oidc_user(conn) + + # Create 60 boolean custom fields (more than the limit) + boolean_fields = Enum.map(1..60, fn _ -> create_boolean_custom_field() end) + + # Build URL with all 60 filters + filter_params = + boolean_fields + |> Enum.map(fn cf -> "bf_#{cf.id}=true" end) + |> Enum.join("&") + + {:ok, view, _html} = live(conn, "/members?#{filter_params}") + + state = :sys.get_state(view.pid) + filters = state.socket.assigns.boolean_custom_field_filters + + # Should limit to maximum 50 filters + assert map_size(filters) <= 50 + # All filters in the result should be valid + Enum.each(filters, fn {id, value} -> + assert value in [true, false] + # Verify the ID corresponds to one of our boolean fields + assert id in Enum.map(boolean_fields, &to_string(&1.id)) + end) + end + + test "handle_params ignores extremely long custom field IDs", %{conn: conn} do + conn = conn_with_oidc_user(conn) + boolean_field = create_boolean_custom_field() + + # Create a fake ID that's way too long (UUIDs are max 36 chars) + fake_long_id = String.duplicate("a", 100) + + {:ok, view, _html} = + live(conn, "/members?bf_#{fake_long_id}=true") + + state = :sys.get_state(view.pid) + filters = state.socket.assigns.boolean_custom_field_filters + + # Should not accept the extremely long ID + refute Map.has_key?(filters, fake_long_id) + # Valid boolean field should still work + refute Map.has_key?(filters, boolean_field.id) + assert filters == %{} end end end