refactor: address review comments for join view

CODE_GUIDELINES.md

@@ -86,7 +86,7 @@ lib/
 │   ├── membership.ex      # Domain definition
 │   ├── member.ex          # Member resource
 │   ├── join_request.ex    # JoinRequest (public join form, double opt-in)
-│   ├── join_request/      # JoinRequest changes (SetConfirmationToken, ConfirmRequest)
+│   ├── join_request/      # JoinRequest changes (SetConfirmationToken, FilterFormDataByAllowlist, ConfirmRequest)
 │   ├── custom_field.ex               # Custom field (definition) resource
 │   ├── custom_field_value.ex        # Custom field value resource
 │   ├── setting.ex         # Global settings (singleton resource; incl. join form config)

docs/onboarding-join-concept.md

@@ -42,7 +42,7 @@
 
 ### 2.3 Data Flow
 
-- **Input:** Only data explicitly allowed for the public form; field set is admin-configured (see §2.6). No internal or sensitive fields. **Server-side allowlist:** The set of accepted fields must be enforced on the server from the join-form settings (allowlist), not only in the UI, to prevent field injection or extra attributes from being stored.
+- **Input:** Only data explicitly allowed for the public form; field set is admin-configured (see §2.6). No internal or sensitive fields. **Server-side allowlist:** The set of accepted fields is enforced in the LiveView (`build_submit_attrs`) and in the resource via **`JoinRequest.Changes.FilterFormDataByAllowlist`** so that even direct API/submit_join_request calls only persist allowlisted form_data keys.
 - **On form submit:** **Create** a JoinRequest with status `pending_confirmation`, store confirmation token **hash** in the DB (raw token only in the email link), set `confirmation_token_expires_at` (e.g. 24h), store all allowlisted form data (see §2.3.2), then send confirmation email.
 - **On confirmation link click:** **Update** the JoinRequest (find by token hash): set status to `submitted`, set `submitted_at`, clear/invalidate token fields. If the record is already `submitted`, return success without changing it (idempotent).
 - **No creation** of Member or User in Prio 1; promotion to Member/User happens in a later step (e.g. after approval).
@@ -73,7 +73,7 @@
 - **Public paths:** `/join` and the confirmation route must be public (unauthenticated access returns 200).
   - **Explicit public path for `/join`:** Add **`/join`** (and if needed `/join/*`) to the page-permission plug’s **`public_path?/1`** so that the join page is reachable without login. Do not rely on the confirm path alone.
   - **Confirmation route:** Use **`/confirm_join/:token`** so that the existing whitelist (e.g. `String.starts_with?(path, "/confirm")`) already covers it; no extra plug change for confirm.
-- **Abuse:** **Honeypot** (MVP) plus **rate limiting** (MVP). Use Phoenix/Elixir standard options (e.g. **Hammer** with **Hammer.Plug**, ETS backend), scoped to the join flow (e.g. by IP). Verify library version and multi-node behaviour before or during implementation.
+- **Abuse:** **Honeypot** (MVP) plus **rate limiting** (MVP). Use Phoenix/Elixir standard options (e.g. **Hammer** with **Hammer.Plug**, ETS backend), scoped to the join flow (e.g. by IP). Client IP for rate limiting: prefer **X-Forwarded-For** / **X-Real-IP** when behind a reverse proxy (see Endpoint `connect_info: [:x_headers]` and `JoinLive.client_ip_from_socket/1`); fallback to peer_data with correct IPv4/IPv6 string via `:inet.ntoa/1`. Verify library version and multi-node behaviour before or during implementation.
 - **Data:** Minimal PII; no sensitive data on the public form; consider DSGVO when extending. If abuse signals are stored: only hashed or aggregated values; document classification and retention.
 - **Approval-only:** No automatic User/Member creation from the join form; approval (Step 2) or other trusted path creates identity.
 - **Ash policies and actor:** JoinRequest has **explicit public actions** allowed with `actor: nil` (e.g. `submit` for create, `confirm` for update). Model via **policies** that permit these actions when actor is nil; do **not** use `authorize?: false` unless documented and clearly not a privilege-escalation path.

lib/membership/join_request.ex

@@ -37,6 +37,7 @@ defmodule Mv.Membership.JoinRequest do
       accept [:email, :first_name, :last_name, :form_data, :schema_version]
 
       change Mv.Membership.JoinRequest.Changes.SetConfirmationToken
+      change Mv.Membership.JoinRequest.Changes.FilterFormDataByAllowlist
     end
 
     read :get_by_confirmation_token_hash do
@@ -77,6 +78,8 @@ defmodule Mv.Membership.JoinRequest do
   end
 
   validations do
+    # Format/formatting of email is not validated here; invalid addresses may fail at send time
+    # or can be enforced via an Ash change if needed.
     validate present(:email), on: [:create]
   end
 

lib/membership/join_request/changes/filter_form_data_by_allowlist.ex

@@ -0,0 +1,38 @@
+defmodule Mv.Membership.JoinRequest.Changes.FilterFormDataByAllowlist do
+  @moduledoc """
+  Filters form_data to only keys that are in the join form allowlist (server-side).
+
+  Ensures that even when submit_join_request/2 is called directly (e.g. from tests or API),
+  only allowlisted custom fields are persisted. Typed fields (email, first_name, last_name)
+  are not part of form_data; allowlist is join_form_field_ids minus those.
+  """
+  use Ash.Resource.Change
+
+  alias Mv.Membership
+
+  @typed_fields ["email", "first_name", "last_name"]
+
+  @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t()
+  def change(changeset, _opts, _context) do
+    form_data = Ash.Changeset.get_attribute(changeset, :form_data) || %{}
+
+    allowlist_ids =
+      case Membership.get_join_form_allowlist() do
+        list when is_list(list) ->
+          list
+          |> Enum.map(fn item -> item.id end)
+          |> MapSet.new()
+          |> MapSet.difference(MapSet.new(@typed_fields))
+
+        _ ->
+          MapSet.new()
+      end
+
+    filtered =
+      form_data
+      |> Enum.filter(fn {key, _} -> MapSet.member?(allowlist_ids, to_string(key)) end)
+      |> Map.new()
+
+    Ash.Changeset.force_change_attribute(changeset, :form_data, filtered)
+  end
+end

lib/mv_web/endpoint.ex

@@ -12,8 +12,8 @@ defmodule MvWeb.Endpoint do
   ]
 
   socket "/live", Phoenix.LiveView.Socket,
-    websocket: [connect_info: [:peer_data, session: @session_options]],
-    longpoll: [connect_info: [:peer_data, session: @session_options]]
+    websocket: [connect_info: [:peer_data, :x_headers, session: @session_options]],
+    longpoll: [connect_info: [:peer_data, :x_headers, session: @session_options]]
 
   # Serve at "/" the static files from "priv/static" directory.
   #

lib/mv_web/join_rate_limit.ex

@@ -15,6 +15,7 @@ defmodule MvWeb.JoinRateLimit do
   - `{:deny, _retry_after_ms}` - rate limit exceeded
   """
   def check(key) when is_binary(key) do
+    # Read at runtime so config can be changed without restart (e.g. in tests).
     config = Application.get_env(:mv, :join_rate_limit, [])
     scale_ms = Keyword.get(config, :scale_ms, 60_000)
     limit = Keyword.get(config, :limit, 10)

lib/mv_web/live/join_live.ex

@@ -213,9 +213,7 @@ defmodule MvWeb.JoinLive do
       form_data =
         params
         |> Enum.filter(fn {key, _} -> key in allowlist_ids and key not in typed end)
-        |> Map.new()
-        |> Enum.map(fn {k, v} -> {k, String.trim(to_string(v))} end)
-        |> Map.new()
+        |> Map.new(fn {k, v} -> {k, String.trim(to_string(v))} end)
 
       attrs = %{attrs | form_data: form_data}
       {:ok, attrs}
@@ -227,13 +225,37 @@ defmodule MvWeb.JoinLive do
     if is_binary(v), do: String.trim(v), else: nil
   end
 
+  # Prefer X-Forwarded-For / X-Real-IP when behind a reverse proxy; fall back to peer_data.
+  # Uses :inet.ntoa/1 for correct IPv4 and IPv6 string representation.
   defp client_ip_from_socket(socket) do
-    case get_connect_info(socket, :peer_data) do
-      %{address: address} when is_tuple(address) ->
-        address |> Tuple.to_list() |> Enum.join(".")
+    with nil <- client_ip_from_headers(socket),
+         %{address: address} when is_tuple(address) <- get_connect_info(socket, :peer_data) do
+      address |> :inet.ntoa() |> to_string()
+    else
+      ip when is_binary(ip) -> ip
+      _ -> "unknown"
+    end
+  end
 
-      _ ->
-        "unknown"
+  defp client_ip_from_headers(socket) do
+    headers = get_connect_info(socket, :x_headers) || []
+    real_ip = header_value(headers, "x-real-ip")
+    forwarded = header_value(headers, "x-forwarded-for")
+
+    cond do
+      real_ip != nil -> real_ip
+      forwarded != nil -> String.split(forwarded, ~r/,\s*/) |> List.first() |> String.trim()
+      true -> nil
     end
   end
+
+  defp header_value(headers, name) do
+    name_lower = String.downcase(name)
+
+    headers
+    |> Enum.find_value(fn
+      {h, v} when is_binary(h) -> if String.downcase(h) == name_lower, do: String.trim(v)
+      _ -> nil
+    end)
+  end
 end

lib/mv_web/plugs/join_form_enabled.ex

@@ -24,6 +24,7 @@ defmodule MvWeb.Plugs.JoinFormEnabled do
     end
   end
 
+  # Same body as default ErrorHTML 404 (no custom error templates in this app).
   defp send_404(conn) do
     conn
     |> put_resp_content_type("text/html")

priv/gettext/de/LC_MESSAGES/default.po

@@ -513,6 +513,7 @@ msgstr "Zurück zur Mitgliederliste"
 msgid "Back to users list"
 msgstr "Zurück zur Benutzer*innen-Liste"
 
+#: lib/mv_web/components/layouts.ex
 #: lib/mv_web/components/layouts/sidebar.ex
 #, elixir-autogen, elixir-format
 msgid "Select language"
@@ -3429,11 +3430,6 @@ msgstr "Zu viele Anfragen. Bitte versuche es später erneut."
 msgid "We have saved your details. To complete your request, please click the link we sent to your email."
 msgstr "Wir haben deine Angaben gespeichert. Um deinen Antrag abzuschließen, klicke bitte auf den Link in der E-Mail, die wir dir geschickt haben."
 
-#: lib/mv_web/live/join_live.ex
-#, elixir-autogen, elixir-format
-msgid "Website"
-msgstr "Webseite"
-
 #: lib/mv_web/live/join_live.ex
 #, elixir-autogen, elixir-format
 msgid "By submitting your application you will receive an email with a confirmation link. Once you have confirmed your email address, your application will be reviewed."
@@ -3448,3 +3444,8 @@ msgstr "Bitte gib hier die Daten für deinen Mitgliedsantrag an."
 #, elixir-autogen, elixir-format
 msgid "Your details are only used to process your membership application and to contact you. To prevent abuse we also process technical data (e.g. IP address) only as necessary."
 msgstr "Deine Angaben werden nur zur Bearbeitung deines Mitgliedsantrags und zur Kontaktaufnahme genutzt. Zur Absicherung gegen Missbrauch verarbeiten wir zusätzlich technische Daten (z. B. IP-Adresse) nur im dafür nötigen Umfang."
+
+#: lib/mv_web/live/join_live.ex
+#, elixir-autogen, elixir-format
+msgid "Website"
+msgstr "Webseite"

priv/gettext/default.pot

@@ -3430,11 +3430,6 @@ msgstr ""
 msgid "We have saved your details. To complete your request, please click the link we sent to your email."
 msgstr ""
 
-#: lib/mv_web/live/join_live.ex
-#, elixir-autogen, elixir-format
-msgid "Website (leave empty)"
-msgstr ""
-
 #: lib/mv_web/live/join_live.ex
 #, elixir-autogen, elixir-format
 msgid "By submitting your application you will receive an email with a confirmation link. Once you have confirmed your email address, your application will be reviewed."
@@ -3449,3 +3444,8 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "Your details are only used to process your membership application and to contact you. To prevent abuse we also process technical data (e.g. IP address) only as necessary."
 msgstr ""
+
+#: lib/mv_web/live/join_live.ex
+#, elixir-autogen, elixir-format
+msgid "Website"
+msgstr ""

priv/gettext/en/LC_MESSAGES/default.po

@@ -514,6 +514,7 @@ msgstr ""
 msgid "Back to users list"
 msgstr ""
 
+#: lib/mv_web/components/layouts.ex
 #: lib/mv_web/components/layouts/sidebar.ex
 #, elixir-autogen, elixir-format, fuzzy
 msgid "Select language"
@@ -3429,11 +3430,6 @@ msgstr ""
 msgid "We have saved your details. To complete your request, please click the link we sent to your email."
 msgstr ""
 
-#: lib/mv_web/live/join_live.ex
-#, elixir-autogen, elixir-format
-msgid "Website (leave empty)"
-msgstr ""
-
 #: lib/mv_web/live/join_live.ex
 #, elixir-autogen, elixir-format
 msgid "By submitting your application you will receive an email with a confirmation link. Once you have confirmed your email address, your application will be reviewed."
@@ -3448,3 +3444,8 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "Your details are only used to process your membership application and to contact you. To prevent abuse we also process technical data (e.g. IP address) only as necessary."
 msgstr "Your details are only used to process your membership application and to contact you. To prevent abuse we also process technical data (e.g. IP address) only as necessary."
+
+#: lib/mv_web/live/join_live.ex
+#, elixir-autogen, elixir-format
+msgid "Website"
+msgstr ""

test/membership/join_request_test.exs

@@ -38,6 +38,21 @@ defmodule Mv.Membership.JoinRequestTest do
     end
 
     test "persists first_name, last_name and form_data when provided" do
+      # Allowlist must include custom fields so FilterFormDataByAllowlist persists them
+      {:ok, settings} = Membership.get_settings()
+
+      Mv.Membership.update_settings(settings, %{
+        join_form_enabled: true,
+        join_form_field_ids: ["email", "first_name", "last_name", "city", "notes"],
+        join_form_field_required: %{
+          "email" => true,
+          "first_name" => false,
+          "last_name" => false,
+          "city" => false,
+          "notes" => false
+        }
+      })
+
       attrs =
         @valid_submit_attrs
         |> Map.put(:confirmation_token, "token-#{System.unique_integer([:positive])}")
@@ -128,8 +143,8 @@ defmodule Mv.Membership.JoinRequestTest do
 
       Mv.Membership.update_settings(settings, %{
         join_form_enabled: true,
-        join_form_field_ids: ["email", "first_name"],
-        join_form_field_required: %{"email" => true, "first_name" => false}
+        join_form_field_ids: ["email", "first_name", "city"],
+        join_form_field_required: %{"email" => true, "first_name" => false, "city" => false}
       })
 
       attrs = %{