From bc82777d34777816fb5bd0abaa2c4fe26edf8633 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Thu, 26 Mar 2026 12:36:50 -0500 Subject: [PATCH] fix: close review findings --- lib/aprsme/partition_manager.ex | 2 +- lib/aprsme_web/channels/mobile_channel.ex | 56 ++++++++++++++++++- lib/aprsme_web/plugs/api_csrf.ex | 12 ++-- test/aprsme/partition_manager_test.exs | 14 +++++ .../workers/packet_cleanup_worker_test.exs | 13 +++++ .../channels/mobile_channel_test.exs | 51 +++++++++++++++++ test/aprsme_web/plugs/api_csrf_test.exs | 39 +++++++++++++ 7 files changed, 176 insertions(+), 11 deletions(-) create mode 100644 test/aprsme_web/plugs/api_csrf_test.exs diff --git a/lib/aprsme/partition_manager.ex b/lib/aprsme/partition_manager.ex index 2227e77..9ca297b 100644 --- a/lib/aprsme/partition_manager.ex +++ b/lib/aprsme/partition_manager.ex @@ -91,7 +91,7 @@ defmodule Aprsme.PartitionManager do list_partitions() |> Enum.filter(fn name -> case partition_date(name) do - {:ok, date} -> Date.before?(date, cutoff_date) + {:ok, date} -> Date.compare(date, cutoff_date) != :gt _ -> false end end) diff --git a/lib/aprsme_web/channels/mobile_channel.ex b/lib/aprsme_web/channels/mobile_channel.ex index 8513f13..5d4121e 100644 --- a/lib/aprsme_web/channels/mobile_channel.ex +++ b/lib/aprsme_web/channels/mobile_channel.ex @@ -185,6 +185,8 @@ defmodule AprsmeWeb.MobileChannel do # Load historical packets for this callsign socket = load_callsign_history(socket, callsign, hours_back) + socket = ensure_callsign_subscription(socket) + # Store tracked callsign in socket socket = assign(socket, :tracked_callsign, callsign) @@ -197,7 +199,11 @@ defmodule AprsmeWeb.MobileChannel do if socket.assigns[:tracked_callsign] do callsign = socket.assigns.tracked_callsign - socket = assign(socket, :tracked_callsign, nil) + + socket = + socket + |> remove_callsign_subscription() + |> assign(:tracked_callsign, nil) Logger.info("Mobile client #{socket.assigns[:client_id]} unsubscribed from callsign: #{callsign}") @@ -209,6 +215,19 @@ defmodule AprsmeWeb.MobileChannel do @impl true def handle_info({:streaming_packet, packet}, socket) do + maybe_push_tracked_packet(socket, packet) + end + + @impl true + def handle_info({:postgres_packet, packet}, socket) do + maybe_push_tracked_packet(socket, packet) + end + + def handle_info(_message, socket) do + {:noreply, socket} + end + + defp maybe_push_tracked_packet(socket, packet) do # Check if packet matches tracked callsign (if any) should_send = if tracked_callsign = socket.assigns[:tracked_callsign] do @@ -237,6 +256,10 @@ defmodule AprsmeWeb.MobileChannel do Aprsme.StreamingPacketsPubSub.unsubscribe(self()) end + if callsign_subscription_active?(socket) do + Phoenix.PubSub.unsubscribe(Aprsme.PubSub, "postgres:aprsme_packets") + end + :ok end @@ -275,7 +298,14 @@ defmodule AprsmeWeb.MobileChannel do defp ensure_float(value) when is_integer(value), do: value * 1.0 defp ensure_float(value) when is_float(value), do: value - defp ensure_float(value) when is_binary(value), do: String.to_float(value) + + defp ensure_float(value) when is_binary(value) do + case Float.parse(String.trim(value)) do + {float, ""} -> float + _ -> value + end + end + defp ensure_float(value), do: value defp ensure_integer(value, _default) when is_integer(value), do: value @@ -521,4 +551,26 @@ defmodule AprsmeWeb.MobileChannel do {:reply, {:ok, %{bounds: bounds, message: "Bounds updated"}}, socket} end + + defp ensure_callsign_subscription(socket) do + if callsign_subscription_active?(socket) do + socket + else + Phoenix.PubSub.subscribe(Aprsme.PubSub, "postgres:aprsme_packets") + assign(socket, :callsign_subscription_active, true) + end + end + + defp remove_callsign_subscription(socket) do + if callsign_subscription_active?(socket) do + Phoenix.PubSub.unsubscribe(Aprsme.PubSub, "postgres:aprsme_packets") + assign(socket, :callsign_subscription_active, false) + else + socket + end + end + + defp callsign_subscription_active?(socket) do + Map.get(socket.assigns, :callsign_subscription_active, false) + end end diff --git a/lib/aprsme_web/plugs/api_csrf.ex b/lib/aprsme_web/plugs/api_csrf.ex index f994273..9d78d1c 100644 --- a/lib/aprsme_web/plugs/api_csrf.ex +++ b/lib/aprsme_web/plugs/api_csrf.ex @@ -1,6 +1,6 @@ defmodule AprsmeWeb.Plugs.ApiCSRF do @moduledoc """ - CSRF protection for API endpoints using X-Requested-With header or API tokens + CSRF protection for API endpoints using `X-Requested-With` or a valid CSRF token. """ import Phoenix.Controller import Plug.Conn @@ -22,16 +22,12 @@ defmodule AprsmeWeb.Plugs.ApiCSRF do end defp check_csrf_protection(conn) do - case {get_req_header(conn, "x-requested-with"), get_req_header(conn, "authorization")} do - {["XMLHttpRequest"], _} -> + case get_req_header(conn, "x-requested-with") do + ["XMLHttpRequest"] -> # Valid AJAX request conn - {_, ["Bearer " <> _token]} -> - # Has authorization header (API token) - would need validation in production - conn - - {_, _} -> + _ -> # Check for CSRF token in header case get_req_header(conn, "x-csrf-token") do [token] when byte_size(token) > 0 -> diff --git a/test/aprsme/partition_manager_test.exs b/test/aprsme/partition_manager_test.exs index d57bdf3..12e98ac 100644 --- a/test/aprsme/partition_manager_test.exs +++ b/test/aprsme/partition_manager_test.exs @@ -104,6 +104,20 @@ defmodule Aprsme.PartitionManagerTest do [today_name] ) end + + test "drops the partition at the retention cutoff boundary" do + cutoff_date = Date.add(Date.utc_today(), -7) + name = PartitionManager.partition_name(cutoff_date) + {from_dt, to_dt} = PartitionManager.partition_range(cutoff_date) + + Repo.query!( + "CREATE TABLE IF NOT EXISTS #{name} PARTITION OF packets FOR VALUES FROM ('#{DateTime.to_iso8601(from_dt)}') TO ('#{DateTime.to_iso8601(to_dt)}')" + ) + + {:ok, dropped} = PartitionManager.drop_old_partitions(7) + + assert name in dropped + end end describe "list_partitions/0" do diff --git a/test/aprsme/workers/packet_cleanup_worker_test.exs b/test/aprsme/workers/packet_cleanup_worker_test.exs index fd20b6f..7a86e76 100644 --- a/test/aprsme/workers/packet_cleanup_worker_test.exs +++ b/test/aprsme/workers/packet_cleanup_worker_test.exs @@ -37,6 +37,19 @@ defmodule Aprsme.Workers.PacketCleanupWorkerTest do assert today_name in PartitionManager.list_partitions() end + + test "drops the partition exactly at the cleanup cutoff" do + cutoff_date = Date.add(Date.utc_today(), -7) + name = PartitionManager.partition_name(cutoff_date) + {from_dt, to_dt} = PartitionManager.partition_range(cutoff_date) + + Repo.query!( + "CREATE TABLE IF NOT EXISTS #{name} PARTITION OF packets FOR VALUES FROM ('#{DateTime.to_iso8601(from_dt)}') TO ('#{DateTime.to_iso8601(to_dt)}')" + ) + + assert :ok = PacketCleanupWorker.perform(%{"cleanup_days" => 7}) + refute name in PartitionManager.list_partitions() + end end describe "perform/1 without cleanup_days" do diff --git a/test/aprsme_web/channels/mobile_channel_test.exs b/test/aprsme_web/channels/mobile_channel_test.exs index 1ff2a2e..69a9de1 100644 --- a/test/aprsme_web/channels/mobile_channel_test.exs +++ b/test/aprsme_web/channels/mobile_channel_test.exs @@ -126,6 +126,18 @@ defmodule AprsmeWeb.MobileChannelTest do assert is_float(bounds.east) assert is_float(bounds.west) end + + test "rejects invalid numeric strings without crashing", %{socket: socket} do + ref = + push(socket, "subscribe_bounds", %{ + "north" => "abc", + "south" => "33.0", + "east" => "-96.0", + "west" => "-96.2" + }) + + assert_reply ref, :error, %{message: "All bounds must be numeric"} + end end describe "update_bounds" do @@ -191,6 +203,28 @@ defmodule AprsmeWeb.MobileChannelTest do assert_reply ref, :error, %{message: "North must be greater than south"} end + + test "rejects invalid numeric strings on update without crashing", %{socket: socket} do + ref = + push(socket, "subscribe_bounds", %{ + "north" => 33.2, + "south" => 33.0, + "east" => -96.0, + "west" => -96.2 + }) + + assert_reply ref, :ok, _ + + ref = + push(socket, "update_bounds", %{ + "north" => "bad", + "south" => "32.9", + "east" => "-95.9", + "west" => "-96.3" + }) + + assert_reply ref, :error, %{message: "All bounds must be numeric"} + end end describe "unsubscribe" do @@ -409,6 +443,23 @@ defmodule AprsmeWeb.MobileChannelTest do refute_push "packet", _ end + test "receives live postgres packets after subscribing by callsign only", %{socket: socket} do + ref = push(socket, "subscribe_callsign", %{"callsign" => "W5ISP-9"}) + assert_reply ref, :ok, _ + + packet = %{ + "sender" => "W5ISP-9", + "lat" => 33.1, + "lon" => -96.1, + "received_at" => DateTime.truncate(DateTime.utc_now(), :second) + } + + send(socket.channel_pid, {:postgres_packet, packet}) + + assert_push "packet", pushed_packet + assert pushed_packet.callsign == "W5ISP-9" + end + test "wildcard callsign matches multiple SSIDs", %{socket: socket} do # Subscribe to wildcard ref = push(socket, "subscribe_callsign", %{"callsign" => "W5ISP*"}) diff --git a/test/aprsme_web/plugs/api_csrf_test.exs b/test/aprsme_web/plugs/api_csrf_test.exs new file mode 100644 index 0000000..aadf714 --- /dev/null +++ b/test/aprsme_web/plugs/api_csrf_test.exs @@ -0,0 +1,39 @@ +defmodule AprsmeWeb.Plugs.ApiCSRFTest do + use AprsmeWeb.ConnCase, async: true + + alias AprsmeWeb.Plugs.ApiCSRF + + test "allows JSON requests with XMLHttpRequest header", %{conn: conn} do + conn = + conn + |> init_test_session(%{}) + |> put_req_header("content-type", "application/json") + |> put_req_header("x-requested-with", "XMLHttpRequest") + |> ApiCSRF.call([]) + + refute conn.halted + end + + test "rejects JSON requests with arbitrary bearer token", %{conn: conn} do + conn = + conn + |> init_test_session(%{}) + |> put_req_header("content-type", "application/json") + |> put_req_header("authorization", "Bearer definitely-not-valid") + |> ApiCSRF.call([]) + + assert conn.halted + assert conn.status == 403 + assert Jason.decode!(conn.resp_body)["error"] == "CSRF protection failed" + end + + test "allows non-JSON requests without CSRF headers", %{conn: conn} do + conn = + conn + |> init_test_session(%{}) + |> put_req_header("content-type", "text/plain") + |> ApiCSRF.call([]) + + refute conn.halted + end +end