From b4657cae23ed1e0275bab19a52551ffcf29f449d Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 23 Jan 2026 14:00:18 +0100 Subject: [PATCH] fix: resolve pr remarks --- .../components/member_filter_component.ex | 38 ++++----- lib/mv_web/live/member_live/index.ex | 85 +++++++++++++------ priv/gettext/de/LC_MESSAGES/default.po | 20 ++--- priv/gettext/default.pot | 10 --- priv/gettext/en/LC_MESSAGES/default.po | 31 +++---- test/mv_web/member_live/index_test.exs | 38 +++++++++ 6 files changed, 139 insertions(+), 83 deletions(-) diff --git a/lib/mv_web/live/components/member_filter_component.ex b/lib/mv_web/live/components/member_filter_component.ex index 657cb02..f5f99ea 100644 --- a/lib/mv_web/live/components/member_filter_component.ex +++ b/lib/mv_web/live/components/member_filter_component.ex @@ -9,7 +9,8 @@ defmodule MvWeb.Components.MemberFilterComponent do - Uses `div` panel instead of `ul.menu/li` structure to avoid DaisyUI menu styles (padding, display, hover, font sizes) that would interfere with form controls. - - Filter controls are form elements (fieldset, radio inputs), not menu items. + - Filter controls are form elements (fieldset with legend, radio inputs), not menu items. + Uses semantic `
` and `` for proper accessibility and form structure. - Dropdown stays open when clicking filter segments to allow multiple filter changes. - Uses `phx-change` on form for radio inputs instead of individual `phx-click` events. @@ -88,7 +89,8 @@ defmodule MvWeb.Components.MemberFilterComponent do @@ -107,11 +109,11 @@ defmodule MvWeb.Components.MemberFilterComponent do
{gettext("Payments")}
-
-
+ +
@@ -166,20 +168,14 @@ defmodule MvWeb.Components.MemberFilterComponent do {gettext("Custom Fields")}
-
- -
+ +
-
-
+
+ diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index 9fc9cdf..41ef275 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/lib/mv_web/live/member_live/index.ex @@ -778,9 +778,32 @@ defmodule MvWeb.MemberLive.Index do |> Ash.Query.new() |> Ash.Query.select(@overview_fields) - # Load custom field values for visible custom fields (based on user selection) + # Load custom field values for visible custom fields AND active boolean filters + # This ensures boolean filters work even when the custom field is not visible in overview visible_custom_field_ids = socket.assigns[:visible_custom_field_ids] || [] - query = load_custom_field_values(query, visible_custom_field_ids) + + # Get IDs of active boolean filters (whitelisted against boolean_custom_fields) + # Convert boolean_custom_fields list to map for efficient lookup (consistent with maybe_update_boolean_filters) + boolean_custom_fields_map = + socket.assigns.boolean_custom_fields + |> Map.new(fn cf -> {to_string(cf.id), cf} end) + + active_boolean_filter_ids = + socket.assigns.boolean_custom_field_filters + |> Map.keys() + |> Enum.filter(fn id_str -> + # Validate UUID format and check against whitelist + String.length(id_str) <= @max_uuid_length && + match?({:ok, _}, Ecto.UUID.cast(id_str)) && + Map.has_key?(boolean_custom_fields_map, id_str) + end) + + # Union of visible IDs and active filter IDs + ids_to_load = + (visible_custom_field_ids ++ active_boolean_filter_ids) + |> Enum.uniq() + + query = load_custom_field_values(query, ids_to_load) # Load membership fee cycles for status display query = MembershipFeeStatus.load_cycles_for_members(query, socket.assigns.show_current_cycle) @@ -1233,35 +1256,43 @@ defmodule MvWeb.MemberLive.Index do |> Map.new(fn cf -> {to_string(cf.id), cf} end) # Parse all boolean filter parameters + # Security: Use reduce_while to abort early after @max_boolean_filters to prevent DoS attacks + # This protects CPU/Parsing costs, not just memory/state + # We count processed parameters (not just valid filters) to protect against parsing DoS prefix_length = String.length(@boolean_filter_prefix) - filters = + {filters, total_processed} = params |> Enum.filter(fn {key, _value} -> String.starts_with?(key, @boolean_filter_prefix) end) - |> Enum.reduce(%{}, fn {key, value_str}, acc -> - process_boolean_filter_param( - key, - value_str, - prefix_length, - boolean_custom_fields, - acc - ) + |> Enum.reduce_while({%{}, 0}, fn {key, value_str}, {acc, count} -> + if count >= @max_boolean_filters do + Logger.warning( + "Too many boolean filter parameters in request (#{count} processed), limiting to #{@max_boolean_filters} to prevent DoS" + ) + + {:halt, {acc, count}} + else + new_acc = + process_boolean_filter_param( + key, + value_str, + prefix_length, + boolean_custom_fields, + acc + ) + + # Increment counter for each processed parameter (DoS protection) + # Note: We count processed params, not just valid filters, to protect parsing costs + {:cont, {new_acc, count + 1}} + 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 + # Log additional warning if we hit the limit + if total_processed >= @max_boolean_filters do + Logger.warning( + "Boolean filter limit reached: processed #{total_processed} parameters, accepted #{map_size(filters)} valid filters (max: #{@max_boolean_filters})" + ) + end assign(socket, :boolean_custom_field_filters, filters) end @@ -1340,8 +1371,8 @@ defmodule MvWeb.MemberLive.Index do # This ensures no raw user input is ever passed to filter functions. # # Returns: - # - `:true` for "true" string - # - `:false` for "false" string + # - `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 diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 83d8ed0..3369548 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1925,11 +1925,6 @@ msgstr "Validierung fehlgeschlagen: %{message}" msgid "Close" msgstr "Schließen" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format -msgid "Filter by %{name}" -msgstr "Filtern nach %{name}" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format msgid "Filter members" @@ -1945,12 +1940,17 @@ msgstr "Mitgliedsfilter" msgid "Payment Status" msgstr "Bezahlstatus" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format, fuzzy -msgid "Payment status filter" -msgstr "Bezahlstatusfilter" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format msgid "Reset" msgstr "Zurücksetzen" + +#~ #: lib/mv_web/live/components/member_filter_component.ex +#~ #, elixir-autogen, elixir-format +#~ msgid "Filter by %{name}" +#~ msgstr "Filtern nach %{name}" + +#~ #: lib/mv_web/live/components/member_filter_component.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Payment status filter" +#~ msgstr "Bezahlstatusfilter" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 87acf6b..fb50446 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1926,11 +1926,6 @@ msgstr "" msgid "Close" msgstr "" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format -msgid "Filter by %{name}" -msgstr "" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format msgid "Filter members" @@ -1946,11 +1941,6 @@ msgstr "" msgid "Payment Status" msgstr "" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format -msgid "Payment status filter" -msgstr "" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format msgid "Reset" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 81135b2..37a7a0d 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -780,6 +780,7 @@ msgstr "" msgid "Payment Data" msgstr "" +#: lib/mv_web/live/components/member_filter_component.ex #: lib/mv_web/live/member_live/form.ex #, elixir-autogen, elixir-format msgid "Payments" @@ -1925,11 +1926,6 @@ msgstr "" msgid "Close" msgstr "" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format -msgid "Filter by %{name}" -msgstr "" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format msgid "Filter members" @@ -1940,21 +1936,11 @@ msgstr "" msgid "Member filter" msgstr "" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format, fuzzy -msgid "Payment" -msgstr "" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format, fuzzy msgid "Payment Status" msgstr "" -#: lib/mv_web/live/components/member_filter_component.ex -#, elixir-autogen, elixir-format, fuzzy -msgid "Payment status filter" -msgstr "" - #: lib/mv_web/live/components/member_filter_component.ex #, elixir-autogen, elixir-format msgid "Reset" @@ -1986,6 +1972,11 @@ msgstr "" #~ msgid "Regular" #~ msgstr "" +#~ #: lib/mv_web/live/components/member_filter_component.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Payment" +#~ msgstr "" + #~ #: lib/mv_web/live/contribution_period_live/show.ex #~ #, elixir-autogen, elixir-format #~ msgid "Current" @@ -2207,6 +2198,11 @@ msgstr "" #~ msgid "Contributions for %{name}" #~ msgstr "" +#~ #: lib/mv_web/live/components/member_filter_component.ex +#~ #, elixir-autogen, elixir-format, fuzzy +#~ msgid "Payment status filter" +#~ msgstr "" + #~ #: lib/mv_web/live/contribution_type_live/index.ex #~ #, elixir-autogen, elixir-format #~ msgid "Family" @@ -2251,3 +2247,8 @@ msgstr "" #~ #, elixir-autogen, elixir-format #~ msgid "About Contribution Types" #~ msgstr "" + +#~ #: lib/mv_web/live/components/member_filter_component.ex +#~ #, elixir-autogen, elixir-format +#~ msgid "Filter by %{name}" +#~ msgstr "" diff --git a/test/mv_web/member_live/index_test.exs b/test/mv_web/member_live/index_test.exs index daa377f..3391b86 100644 --- a/test/mv_web/member_live/index_test.exs +++ b/test/mv_web/member_live/index_test.exs @@ -1526,6 +1526,44 @@ defmodule MvWeb.MemberLive.IndexTest do refute html =~ "FalseMember" end + test "boolean filter works even when custom field is not visible in overview", %{conn: conn} do + conn = conn_with_oidc_user(conn) + + # Create boolean field with show_in_overview: false + boolean_field = create_boolean_custom_field(%{show_in_overview: false}) + + _member_with_true = + create_member_with_boolean_value(%{first_name: "TrueMember"}, boolean_field, true) + + _member_with_false = + create_member_with_boolean_value(%{first_name: "FalseMember"}, boolean_field, false) + + {:ok, _member_without_value} = + Mv.Membership.Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "NoValue", + last_name: "Member", + email: "novalue.member.#{System.unique_integer([:positive])}@example.com" + }) + |> Ash.create() + + # Test that filter works even though field is not visible in overview + {:ok, _view, html_true} = + live(conn, "/members?bf_#{boolean_field.id}=true") + + assert html_true =~ "TrueMember" + refute html_true =~ "FalseMember" + refute html_true =~ "NoValue" + + # Test false filter + {:ok, _view, html_false} = + live(conn, "/members?bf_#{boolean_field.id}=false") + + assert html_false =~ "FalseMember" + refute html_false =~ "TrueMember" + refute html_false =~ "NoValue" + end + test "boolean custom field appears in filter dropdown after being added", %{conn: conn} do conn = conn_with_oidc_user(conn)