From 05cbd833bccbe1e2c285ee0d23e51eb49e3be25e Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 23:12:07 +0100 Subject: [PATCH] Security: Fix critical deny-filter bug and improve authorization CRITICAL FIX: Deny-filter was allowing all records instead of denying Fix: User validation in Member now uses actor from changeset.context --- ...-_membership_fee_features_e886dc4b.plan.md | 318 ------------------ lib/membership/member.ex | 15 +- lib/mv/authorization/checks/has_permission.ex | 20 +- .../has_permission_integration_test.exs | 4 +- 4 files changed, 25 insertions(+), 332 deletions(-) delete mode 100644 code_review_fixes_-_membership_fee_features_e886dc4b.plan.md diff --git a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md b/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md deleted file mode 100644 index eebf419..0000000 --- a/code_review_fixes_-_membership_fee_features_e886dc4b.plan.md +++ /dev/null @@ -1,318 +0,0 @@ ---- -name: Code Review Fixes - Membership Fee Features -overview: Umsetzung der validen Code Review Punkte aus beiden Reviews mit Priorisierung nach Kritikalität. Fokus auf Transaktionssicherheit, Code-Qualität, Performance und UX-Verbesserungen. -todos: - - id: fix-after-action-tasks - content: "after_action mit Task.start → after_transaction + Task.Supervisor: Task.Supervisor zu application.ex hinzufügen, after_action Hooks in after_transaction umwandeln, Task.Supervisor.async_nolink verwenden" - status: pending - - id: reduce-code-duplication - content: "Code-Duplikation reduzieren: handle_cycle_generation/2 private Funktion extrahieren, alle drei Stellen (Create, Type Change, Date Change) verwenden" - status: pending - dependencies: - - fix-after-action-tasks - - id: fix-join-date-validation - content: "join_date Validierung: Entweder Validierung wieder hinzufügen (validate compare(:join_date, less_than_or_equal_to: &Date.utc_today/0)) oder Dokumentation anpassen" - status: pending - - id: fix-load-cycles-docs - content: "load_cycles_for_members: Entweder Dokumentation korrigieren (ehrlich machen) oder echte Filterung implementieren (z.B. nur letzte 2 Intervalle)" - status: pending - - id: fix-get-current-cycle-sort - content: "get_current_cycle nondeterministisch: Vor List.first() nach cycle_start sortieren (desc) in MembershipFeeHelpers.get_current_cycle" - status: pending - - id: fix-n1-query-member-count - content: "N+1 Query beheben: Aggregate auf MembershipFeeType definieren oder member_count einmalig vorab laden und in assigns cachen" - status: pending - - id: fix-assign-new-stale - content: "assign_new → assign: In MembershipFeesComponent.update/2 immer assign(:cycles, cycles) und assign(:available_fee_types, available_fee_types) setzen" - status: pending - - id: fix-regenerating-flag - content: "@regenerating auf true setzen: Direkt beim Event-Start in handle_event(\"regenerate_cycles\", ...) socket |> assign(:regenerating, true) setzen" - status: pending - - id: fix-create-cycle-parsing - content: "Create-cycle parsing Fix: Decimal.parse explizit behandeln und {:error, :invalid_amount} zurückgeben statt :error" - status: pending - - id: fix-delete-all-atomic - content: "Delete all cycles atomar: Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) statt Enum.map" - status: pending - - id: improve-async-error-handling - content: "Fehlerbehandlung bei async Tasks: Strukturierte Error-Logs mit Context, optional Retry-Mechanismus oder Event-System für Benachrichtigung" - status: pending - - id: improve-format-currency - content: "format_currency Robustheit: Number.Currency verwenden oder robusteres Pattern Matching + Tests für Edge Cases (negative Zahlen, sehr große Zahlen)" - status: pending - - id: add-missing-typespecs - content: "Fehlende Typespecs: @spec für SetDefaultMembershipFeeType.change/3 hinzufügen" - status: pending - - id: fix-race-condition - content: "Potenzielle Race Condition: Prüfen ob Ash doppelte Auslösung verhindert, ggf. Logik anpassen (beide Änderungen in einem Hook zusammenfassen)" - status: pending - - id: extract-magic-values - content: "Magic Numbers/Strings: Application.get_env(:mv, :sql_sandbox, false) in Konstante/Helper extrahieren (z.B. Mv.Config.sql_sandbox?/0)" - status: pending - - id: fix-domain-consistency - content: "Domain-Konsistenz: Überall in MembershipFeesComponent domain: MembershipFees explizit angeben" - status: pending - - id: fix-test-helper - content: "Test-Helper Fix: create_cycle/3 Helper - Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen" - status: pending - - id: fix-date-utc-today-param - content: "Date.utc_today() Parameter: today Parameter durchgeben in get_cycle_status_for_member und Helper-Funktionen" - status: pending - - id: fix-ui-locale-input - content: "UI/Locale Input Fix: type=\"number\" → type=\"text\" + inputmode=\"decimal\" + serverseitig \",\" → \".\" normalisieren" - status: pending - - id: fix-delete-confirmation - content: "Delete-all-Confirmation robuster: String.trim() + case-insensitive Vergleich oder \"type DELETE\" Pattern" - status: pending - - id: fix-warning-state - content: "Warning-State Fix: Bei Decimal.parse(:error) explizit hide_amount_warning(socket) aufrufen" - status: pending - - id: fix-double-toggle - content: "Toggle entfernen: Toggle-Button im Spalten-Header entfernen (nur in Toolbar behalten)" - status: pending - - id: fix-format-consistency - content: "Format-Konsistenz: Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren" - status: pending - dependencies: - - fix-ui-locale-input ---- - -# Code Review Fixes - Membership Fee Features - -## Kritische Probleme (Müssen vor Merge behoben werden) - -### 1. after_action mit Task.start - Transaktionsprobleme - -**Dateien:** `lib/membership/member.ex` (Zeilen 142, 279) - -**Problem:** `Task.start/1` wird innerhalb von `after_action` Hooks verwendet. `after_action` läuft innerhalb der DB-Transaktion, daher: - -- Tasks sehen möglicherweise noch nicht committed state -- Tasks werden auch bei Rollback gestartet -- Keine Supervision → Memory Leaks möglich - -**Lösung:** - -- `after_transaction` Hook verwenden (Ash Best Practice) -- `Task.Supervisor` zum Supervision Tree hinzufügen (`lib/mv/application.ex`) -- `Task.Supervisor.async_nolink/3` statt `Task.start/1` verwenden - -**Betroffene Stellen:** - -- Member Creation (Zeile 116-164) -- Join/Exit Date Change (Zeile 250-301) - -### 2. Code-Duplikation in Cycle-Generation-Logik - -**Datei:** `lib/membership/member.ex` - -**Problem:** Cycle-Generation-Logik ist dreimal dupliziert (Create, Type Change, Date Change) - -**Lösung:** Extrahiere in private Funktion `handle_cycle_generation/2` - -## Wichtige Probleme (Sollten behoben werden) - -### 3. join_date Validierung entfernt, aber Dokumentation behauptet Gegenteil - -**Datei:** `lib/membership/member.ex` (Zeile 27, 516-518) - -**Problem:** Dokumentation sagt "join_date not in future", aber Validierung fehlt - -**Lösung:** Dokumentation anpassen - -### 4. load_cycles_for_members overpromises - -**Datei:** `lib/mv_web/member_live/index/membership_fee_status.ex` (Zeile 36-40) - -**Problem:** Dokumentation sagt "Only loads the relevant cycle per member" und "Filters cycles at database level", aber lädt alle Cycles - -**Lösung:** echte Filterung implementieren (z.B. nur letzte 2 Intervalle) - -### 5. get_current_cycle nondeterministisch - -**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeile 178-182) - -**Problem:** `List.first()` ohne explizite Sortierung → Ergebnis hängt von Reihenfolge ab - -**Lösung:** Vor `List.first()` nach `cycle_start` sortieren (desc) - -### 6. N+1 Query durch get_member_count - -**Datei:** `lib/mv_web/live/membership_fee_type_live/index.ex` (Zeile 134-140) - -**Problem:** `get_member_count/1` wird pro Row aufgerufen → N+1 Query - -**Lösung:** Aggregate auf MembershipFeeType definieren oder einmalig vorab laden - -### 7. assign_new kann stale werden - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 402-403) - -**Problem:** `assign_new(:cycles, ...)` und `assign_new(:available_fee_types, ...)` werden nur gesetzt, wenn Assign noch nicht existiert - -**Lösung:** In `update/2` immer `assign(:cycles, cycles)` / `assign(:available_fee_types, available_fee_types)` setzen - -### 8. @regenerating wird nie auf true gesetzt - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 526-561) - -**Problem:** `regenerating` wird nur auf `false` gesetzt, nie auf `true` → Button/Spinner werden nie disabled - -**Lösung:** Direkt beim Event-Start `socket |> assign(:regenerating, true)` setzen - -### 9. Create-cycle parsing: invalid amount zeigt falsche Fehlermeldung - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 748-812) - -**Problem:** `Decimal.parse/1` gibt `:error` zurück, aber `with` behandelt es als `:error` → landet in "Invalid date format" Branch - -**Lösung:** Explizit `{:error, :invalid_amount}` zurückgeben: - -```elixir -amount = case Decimal.parse(amount_str) do - {d, _} -> {:ok, d} - :error -> {:error, :invalid_amount} -end -``` - -### 10. Delete all cycles: nicht atomar - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 666-714) - -**Problem:** `Enum.map(cycles, &Ash.destroy/1)` → nicht atomar, teilweise gelöscht möglich - -**Lösung:** Bulk Delete Query verwenden (Ash bulk destroy oder Query-basiert) - -### 11. Fehlerbehandlung bei async Tasks - -**Datei:** `lib/membership/member.ex` - -**Problem:** Bei Fehlern in async Tasks wird nur geloggt, aber der Benutzer erhält keine Rückmeldung. Die Member-Aktion wird als erfolgreich zurückgegeben, auch wenn die Cycle-Generierung fehlschlägt. Keine Retry-Logik oder Monitoring. - -**Lösung:** - -- Für kritische Fälle: synchron ausführen oder Retry-Mechanismus implementieren -- Für nicht-kritische Fälle: Event-System für spätere Benachrichtigung -- Strukturierte Error-Logs mit Context -- Optional: Error-Tracking (Sentry, etc.) - -### 12. format_currency Robustheit - -**Datei:** `lib/mv_web/helpers/membership_fee_helpers.ex` (Zeilen 27-51) - -**Problem:** Die Funktion verwendet String-Manipulation für Formatierung. Edge Cases könnten problematisch sein (z.B. sehr große Zahlen, negative Werte). - -**Lösung:** - -- `Number.Currency` oder ähnliche Bibliothek verwenden -- Oder: Robusteres Pattern Matching für Edge Cases -- Tests für Edge Cases hinzufügen (negative Zahlen, sehr große Zahlen) - -### 13. Fehlende Typespecs - -**Datei:** `lib/membership/member/changes/set_default_membership_fee_type.ex` - -**Problem:** Keine `@spec` für die `change/3` Funktion. - -**Lösung:** Typespecs hinzufügen für bessere Dokumentation und Dialyzer-Support. - -### 14. Potenzielle Race Condition - -**Datei:** `lib/membership/member.ex` (Zeile 250-301) - -**Problem:** Wenn `join_date` und `exit_date` gleichzeitig geändert werden, könnte die Cycle-Generierung zweimal ausgelöst werden (einmal pro Änderung). - -**Lösung:** Prüfen, ob Ash dies bereits verhindert, oder Logik anpassen (z.B. beide Änderungen in einem Hook zusammenfassen). - -### 15. Magic Numbers/Strings - -**Problem:** `Application.get_env(:mv, :sql_sandbox, false)` wird mehrfach verwendet. - -**Lösung:** Extrahiere in Konstante oder Helper-Funktion (z.B. `Mv.Config.sql_sandbox?/0`). - -## Mittlere Probleme (Nice-to-have) - -### 16. Inconsistent use of domain - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 819-821) - -**Problem:** Einige Actions verwenden `domain: MembershipFees`, andere nicht - -**Lösung:** Konsistent `domain` überall verwenden - -### 17. Tests: create_cycle/3 löscht jedes Mal alle Cycles - -**Datei:** `test/mv_web/member_live/index/membership_fee_status_test.exs` (Zeile 45-52) - -**Problem:** Helper löscht vor jedem Create alle Cycles → Tests prüfen nicht, was sie denken - -**Lösung:** Cleanup nur einmal im Setup oder gezielt nur auto-generierte löschen - -### 18. Tests/Design: Date.utc_today() macht Tests flaky - -**Problem:** Tests hängen von `Date.utc_today()` ab → nicht deterministisch - -**Lösung:** `today` Parameter durchgeben (z.B. `get_cycle_status_for_member(member, show_current, today \\ Date.utc_today())`) - -### 19. UI/Locale: input type="number" + Decimal/Komma - -**Problem:** `type="number"` funktioniert nicht zuverlässig mit Komma als Dezimaltrenner - -**Lösung:** `type="text"` + `inputmode="decimal"` + serverseitig "," → "." normalisieren - -### 20. Delete-all-Confirmation: String-Vergleich ist fragil - -**Datei:** `lib/mv_web/live/member_live/show/membership_fees_component.ex` (Zeile 296-298) - -**Problem:** String-Vergleich gegen `gettext("Yes")` und `"Yes"` → fragil bei Whitespace/Locale - -**Lösung:** `String.trim()` + case-insensitive Vergleich oder "type DELETE" Pattern - -### 21. MembershipFeeType Form: Warning-State kann hängen bleiben - -**Datei:** `lib/mv_web/live/membership_fee_type_live/form.ex` (Zeile 367-378) - -**Problem:** Bei `Decimal.parse(:error)` wird nur `socket` zurückgegeben → Warning kann stehen bleiben - -**Lösung:** Bei `:error` explizit `hide_amount_warning(socket)` aufrufen - -### 22. UI/UX: Toggle ist doppelt vorhanden - -**Datei:** `lib/mv_web/live/member_live/index.html.heex` (Zeile 45-72, 284-296) - -**Problem:** Toggle-Button sowohl in Toolbar als auch im Spalten-Header - -**Lösung:** Toggle im Spalten-Header entfernen (nur in Toolbar behalten) - -### 23. Konsistenz: format_currency vs Inputs - -**Problem:** `format_currency` formatiert deutsch (Komma), aber Inputs erwarten Punkt - -**Lösung:** Inputs ebenfalls auf Komma ausrichten oder serverseitig normalisieren - -## Implementierungsreihenfolge - -1. **Kritisch:** after_action → after_transaction + Task.Supervisor -2. **Kritisch:** Code-Duplikation reduzieren -3. **Wichtig:** join_date Validierung/Dokumentation -4. **Wichtig:** load_cycles_for_members Dokumentation/Implementierung -5. **Wichtig:** get_current_cycle Sortierung -6. **Wichtig:** N+1 Query beheben -7. **Wichtig:** assign_new → assign -8. **Wichtig:** @regenerating auf true setzen -9. **Wichtig:** Create-cycle parsing Fix -10. **Wichtig:** Delete all cycles atomar -11. **Wichtig:** Fehlerbehandlung bei async Tasks -12. **Wichtig:** format_currency Robustheit -13. **Wichtig:** Fehlende Typespecs -14. **Wichtig:** Potenzielle Race Condition prüfen/beheben -15. **Wichtig:** Magic Numbers/Strings extrahieren -16. **Mittel:** Domain-Konsistenz -17. **Mittel:** Test-Helper Fix -18. **Mittel:** Date.utc_today() Parameter -19. **Mittel:** UI/Locale Fixes -20. **Mittel:** String-Vergleich robuster -21. **Mittel:** Warning-State Fix -22. **Mittel:** Toggle entfernen -23. **Mittel:** Format-Konsistenz - diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 28cc712..27c1ce4 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -298,12 +298,12 @@ defmodule Mv.Membership.Member do # Authorization Policies # Order matters: Most specific policies first, then general permission check policies do - # SYSTEM OPERATIONS: Allow CRUD operations without actor + # SYSTEM OPERATIONS: Allow CRUD operations without actor (TEST ENVIRONMENT ONLY) # In test: All operations allowed (for test fixtures) - # In production: Only :create and :read allowed (enforced by NoActor.check) - # :read is needed for internal Ash lookups (e.g., relationship validation during user creation). + # In production/dev: ALL operations denied without actor (fail-closed for security) + # NoActor.check uses compile-time environment detection to prevent security issues bypass action_type([:create, :read, :update, :destroy]) do - description "Allow system operations without actor (seeds, tests, internal lookups)" + description "Allow system operations without actor (test environment only)" authorize_if Mv.Authorization.Checks.NoActor end @@ -397,8 +397,13 @@ defmodule Mv.Membership.Member do user_id = user_arg[:id] current_member_id = changeset.data.id + # Get actor from changeset context for authorization + # If no actor is present, this will fail in production (fail-closed) + actor = Map.get(changeset.context || %{}, :actor) + # Check the current state of the user in the database - case Ash.get(Mv.Accounts.User, user_id) do + # Pass actor to ensure proper authorization (User might have policies in future) + case Ash.get(Mv.Accounts.User, user_id, actor: actor) do # User is free to be linked {:ok, %{member_id: nil}} -> :ok diff --git a/lib/mv/authorization/checks/has_permission.ex b/lib/mv/authorization/checks/has_permission.ex index 3512ef0..68d83d1 100644 --- a/lib/mv/authorization/checks/has_permission.ex +++ b/lib/mv/authorization/checks/has_permission.ex @@ -131,13 +131,12 @@ defmodule Mv.Authorization.Checks.HasPermission do cond do is_nil(actor) -> # No actor - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - # This ensures no records are returned when actor is missing - [id: {:not, {:in, []}}] + # Return filter that never matches (id IN [] = never matches) + deny_filter() is_nil(action) -> # Cannot determine action - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() true -> auto_filter_with_permissions(actor, resource, action) @@ -169,16 +168,23 @@ defmodule Mv.Authorization.Checks.HasPermission do false -> # No permission - deny access (fail-closed) - # Return filter that never matches (using impossible condition) - [id: {:not, {:in, []}}] + deny_filter() end else _ -> # Error case (no role, invalid permission set, etc.) - deny access (fail-closed) - [id: {:not, {:in, []}}] + deny_filter() end end + # Helper function to return a filter that never matches (deny all records) + # Used when authorization should be denied (fail-closed) + # Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all) + # NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG! + defp deny_filter do + [id: {:in, []}] + end + # Helper to extract action type from authorizer # CRITICAL: Must use action_type, not action.name! # Action types: :create, :read, :update, :destroy diff --git a/test/mv/authorization/checks/has_permission_integration_test.exs b/test/mv/authorization/checks/has_permission_integration_test.exs index dad68ea..9ef5fd2 100644 --- a/test/mv/authorization/checks/has_permission_integration_test.exs +++ b/test/mv/authorization/checks/has_permission_integration_test.exs @@ -28,7 +28,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do end describe "Filter Expression Structure - :linked scope" do - test "Member filter uses user.id relationship path" do + test "Member filter uses actor.member_id (inverse relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.Member, :read) @@ -42,7 +42,7 @@ defmodule Mv.Authorization.Checks.HasPermissionIntegrationTest do assert is_list(filter) or is_map(filter) end - test "CustomFieldValue filter uses member.user.id relationship path" do + test "CustomFieldValue filter uses actor.member_id (via member relationship)" do actor = create_actor_with_role("own_data", member_id: "member-123") authorizer = create_authorizer(Mv.Membership.CustomFieldValue, :read)