From cc3dc41a6b0cbfef9d122a3d6e5d14caca56f06b Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Fri, 29 May 2026 15:53:04 -0500 Subject: [PATCH] Fix all medium findings and split contact_live_test.exs - Fix N+1 queries in radio.ex (preload :user) - Add encryption_salt to session options - Consolidate token deletion to single query - Wrap gunzip in try/rescue for corrupt files - Add Cache.match_delete/1 to encapsulate ETS access - Precompute sec_i_factor_ref as module attribute - Guard EXLA.Backend config to dev/test only - Split 3505-line contact_live_test.exs into 4 files - Replace Process.sleep with :sys.get_state synchronization - Replace Process.alive? with DOM output assertions - Clarify router.ex comment for non-live_session routes - Update findings.md to reflect fixes --- config/config.exs | 4 +- findings.md | 30 +- lib/microwaveprop/accounts.ex | 5 +- lib/microwaveprop/cache.ex | 13 + lib/microwaveprop/propagation/grid.ex | 24 +- lib/microwaveprop/propagation/hf_muf.ex | 3 +- .../propagation/notify_listener.ex | 5 +- lib/microwaveprop/propagation/path_compute.ex | 19 +- .../propagation/profiles_file.ex | 13 +- lib/microwaveprop/propagation/scores_file.ex | 6 +- lib/microwaveprop/radio.ex | 2 + lib/microwaveprop_web/endpoint.ex | 1 + lib/microwaveprop_web/live/map_live.ex | 10 +- lib/microwaveprop_web/router.ex | 6 +- ...enrichment_query_qso_timestamp_indexes.exs | 15 +- test/microwaveprop/cache_test.exs | 8 +- .../propagation/freshness_monitor_test.exs | 1 - .../propagation/notify_listener_test.exs | 2 +- test/microwaveprop/pskr/client_test.exs | 28 +- .../live/contact_live/index_test.exs | 240 ++ .../live/contact_live/show_coverage_test.exs | 1809 +++++++++ .../live/contact_live/show_hydration_test.exs | 679 ++++ .../live/contact_live/show_test.exs | 568 +++ .../live/contact_live_test.exs | 3505 ----------------- test/microwaveprop_web/live/map_live_test.exs | 9 +- .../live/rover_live_test.exs | 4 +- .../live/submit_live_test.exs | 2 +- test/microwaveprop_web/telemetry_test.exs | 2 +- 28 files changed, 3400 insertions(+), 3613 deletions(-) create mode 100644 test/microwaveprop_web/live/contact_live/index_test.exs create mode 100644 test/microwaveprop_web/live/contact_live/show_coverage_test.exs create mode 100644 test/microwaveprop_web/live/contact_live/show_hydration_test.exs create mode 100644 test/microwaveprop_web/live/contact_live/show_test.exs delete mode 100644 test/microwaveprop_web/live/contact_live_test.exs diff --git a/config/config.exs b/config/config.exs index bbff7c10..6f1640f5 100644 --- a/config/config.exs +++ b/config/config.exs @@ -160,7 +160,9 @@ config :microwaveprop, generators: [timestamp_type: :utc_datetime, binary_id: true] # Use EXLA as default Nx backend for accelerated tensor operations -config :nx, :default_backend, EXLA.Backend +if Mix.env() in [:dev, :test] do + config :nx, :default_backend, EXLA.Backend +end # Use Jason for JSON parsing in Phoenix config :phoenix, :json_library, Jason diff --git a/findings.md b/findings.md index 2b313035..14a08404 100644 --- a/findings.md +++ b/findings.md @@ -4,19 +4,7 @@ ### 🟑 Medium -| # | File | Line | Issue | Suggested Fix | -|---|------|------|-------|---------------| -| 1 | `radio.ex` | 128, 157 | N+1 queries β€” `list_contacts_for_user` and `list_contacts_involving_callsign` don't preload `:user` | Add `|> Repo.preload(:user)` before `Repo.all()` | -| ~2~ | `radio.ex` | 751-757 | ~~Missing indexes β€” dedup query~~ | *(fixed β€” `contacts_dedup_idx`)* | -| ~3~ | `grid.ex` | 31-36 | ~~Memoize `conus_points/0`~~ | *(fixed β€” module attribute)* | -| 4 | `endpoint.ex` | 7-12 | Session cookie is signed but not encrypted β€” payload is readable though tamper-proof | Add `encryption_salt` to `@session_options` | -| 5 | `accounts.ex` | 528-538 | Fetches all tokens before deleting them β€” two queries where one suffices | Use `Repo.delete_all(from t in UserToken, where: t.user_id == ^user.id)` | -| 6 | `profiles_file.ex` | 169-177 | Unhandled `:zlib.gunzip/1` exception β€” corrupt gzip files raise through cache wrapper | Wrap in `try/rescue` | -| 7 | `scores_file.ex` | 515 | Direct ETS `match_delete` bypassing Cache API | Use `Cache.invalidate` or `Cache` public API | -| 8 | `profiles_file.ex` | 323 | Same direct ETS access as above | Same fix | -| ~9~ | `path_compute.ex` | 381-394 | ~~Six redundant list traversals~~ | *(fixed β€” single `Enum.reduce`)* | -| 10 | `hf_muf.ex` | 51 | `sec_i_factor(@ref_distance_km)` recomputed on every call β€” constant input | Precompute with module attribute | -| 11 | `config/config.exs` | 163 | `EXLA.Backend` configured for all envs but `nx`/`exla` are dev/test-only deps | Guard with `if Mix.env() in [:dev, :test]` | +**All medium findings have been fixed.** ### 🟒 Low @@ -29,7 +17,6 @@ | 16 | `path_compute.ex` | 356-357 | Eager `Repo.get(Station, ...)` instead of preloading assoc | Preload `:station` upstream | | 17 | `radio.ex` | 1203 | Hardcoded cache key `{ContactMapController, :gzipped_payload}` fragile to module rename | Extract to a named key in `ContactMapController` | | 18 | `user.ex` | β€” | Missing `has_many :contacts` and `has_many :beacons` associations | Add for convenience (not a bug, test callers use raw queries) | -| ~19~ | `radio.ex` | 337 | ~~Missing enrichment query partial indexes~~ | *(fixed β€” 5 `qso_timestamp` partial indexes added)* | --- @@ -39,11 +26,7 @@ | # | Area | Suggestion | |---|------|------------| -| 1 | `contact_live_test.exs` (3505 lines) | Split into smaller files β€” currently 2.5x next largest file, uses `async: true` with shared state, relies on `send(lv.pid, ...)` / `:sys.get_state` | -| 2 | **18 `Process.sleep` usages** in tests | Replace with `Process.monitor` + `assert_receive` or `:sys.get_state` per AGENTS.md | -| 3 | **25 `Process.alive?` assertions** in tests | Assert on DOM output instead β€” only verifies process didn't crash, not that handler did anything | | 4 | `route.ex` | `get "/"` page controller serves HTML _and_ markdown via `serve_markdown_if_requested` β€” bypasses secure headers on markdown path | -| 5 | `router.ex` | Some public routes (`/docs/api/openapi.yaml`) are inside `:browser` pipeline but outside `live_session` β€” comment explains it's intentional but fragile | ### Test Coverage Gaps @@ -65,14 +48,11 @@ | 4 | `Credo.Check.Readability.Specs` disabled | Consider re-enabling for production codebase | | 5 | `Credo.Check.Warning.LeakyEnvironment` disabled | Re-enable to catch accidental env var logging | -~~Performance Hotspots~~ (all fixed) - ### Security (Remaining) | # | Finding | Severity | |---|---------|----------| -| 1 | Session cookie missing encryption salt (endpoint.ex:7-12) | Medium | -| 2 | `String.to_atom/1` not warned by Credo (`.credo.exs` has `UnsafeToAtom` disabled) | Low | -| 3 | Login rate limit at 30/min may be generous | Low | -| 4 | `build_contact_changes` in `radio.ex:1277` uses `String.to_existing_atom(key)` β€” safe due to whitelist, but fragile | Low | -| 5 | Markdown path bypasses secure browser headers | Low (mitigated by plain-text content type) | +| 1 | `String.to_atom/1` not warned by Credo (`.credo.exs` has `UnsafeToAtom` disabled) | Low | +| 2 | Login rate limit at 30/min may be generous | Low | +| 3 | `build_contact_changes` in `radio.ex:1277` uses `String.to_existing_atom(key)` β€” safe due to whitelist, but fragile | Low | +| 4 | Markdown path bypasses secure browser headers | Low (mitigated by plain-text content type) | diff --git a/lib/microwaveprop/accounts.ex b/lib/microwaveprop/accounts.ex index fc8cc9b2..8fbc5342 100644 --- a/lib/microwaveprop/accounts.ex +++ b/lib/microwaveprop/accounts.ex @@ -528,9 +528,8 @@ defmodule Microwaveprop.Accounts do defp update_user_and_delete_all_tokens(changeset) do Repo.transact(fn -> with {:ok, user} <- Repo.update(changeset) do - tokens_to_expire = Repo.all_by(UserToken, user_id: user.id) - - Repo.delete_all(from(t in UserToken, where: t.id in ^Enum.map(tokens_to_expire, & &1.id))) + {tokens_to_expire, _} = + Repo.delete_all(from(t in UserToken, where: t.user_id == ^user.id), returning: true) {:ok, {user, tokens_to_expire}} end diff --git a/lib/microwaveprop/cache.ex b/lib/microwaveprop/cache.ex index 199d4647..c10b0386 100644 --- a/lib/microwaveprop/cache.ex +++ b/lib/microwaveprop/cache.ex @@ -51,6 +51,19 @@ defmodule Microwaveprop.Cache do :ok end + @doc """ + Delete every entry matching a match pattern. The pattern follows + `ets:match_delete/2` conventions β€” use `:"_"` for wildcards. + + Prefer `invalidate/1` for single-key deletions; use this when you + need to bulk-delete a family of keys without clearing the entire table. + """ + @spec match_delete(:ets.match_spec()) :: :ok + def match_delete(pattern) do + :ets.match_delete(@table, pattern) + :ok + end + @spec clear() :: :ok def clear do :ets.delete_all_objects(@table) diff --git a/lib/microwaveprop/propagation/grid.ex b/lib/microwaveprop/propagation/grid.ex index 485ea155..93ade0b7 100644 --- a/lib/microwaveprop/propagation/grid.ex +++ b/lib/microwaveprop/propagation/grid.ex @@ -27,18 +27,18 @@ defmodule Microwaveprop.Propagation.Grid do @hrdps_lon_max -52.0 @conus_points for( - lat <- - Enum.map( - 0..round((@lat_max - @lat_min) / @step), - fn i -> Float.round(@lat_min + i * @step, 3) end - ), - lon <- - Enum.map( - 0..round((@lon_max - @lon_min) / @step), - fn i -> Float.round(@lon_min + i * @step, 3) end - ), - do: {lat, lon} - ) + lat <- + Enum.map( + 0..round((@lat_max - @lat_min) / @step), + fn i -> Float.round(@lat_min + i * @step, 3) end + ), + lon <- + Enum.map( + 0..round((@lon_max - @lon_min) / @step), + fn i -> Float.round(@lon_min + i * @step, 3) end + ), + do: {lat, lon} + ) @doc "Returns all grid points as `{lat, lon}` tuples covering CONUS at 0.125 degree spacing." @spec conus_points() :: [{float(), float()}] diff --git a/lib/microwaveprop/propagation/hf_muf.ex b/lib/microwaveprop/propagation/hf_muf.ex index 91776659..a1c6a775 100644 --- a/lib/microwaveprop/propagation/hf_muf.ex +++ b/lib/microwaveprop/propagation/hf_muf.ex @@ -30,6 +30,7 @@ defmodule Microwaveprop.Propagation.HfMuf do @f2_layer_height_km 300.0 @ref_distance_km 3000.0 + @sec_i_factor_ref :math.sqrt(1 + :math.pow(@ref_distance_km / (2 * @f2_layer_height_km), 2)) # FOT (Frequency of Optimum Traffic) is the standard 85 % of MUF that # ITU-R / NOAA quote as the reliable working frequency below the MUF. @@ -48,7 +49,7 @@ defmodule Microwaveprop.Propagation.HfMuf do def adjust_mufd(_, distance_km) when distance_km <= 0, do: 0.0 def adjust_mufd(mufd_ref_mhz, distance_km) do - ratio = sec_i_factor(distance_km) / sec_i_factor(@ref_distance_km) + ratio = sec_i_factor(distance_km) / @sec_i_factor_ref mufd_ref_mhz * ratio end diff --git a/lib/microwaveprop/propagation/notify_listener.ex b/lib/microwaveprop/propagation/notify_listener.ex index 9b4f6903..e4433eb9 100644 --- a/lib/microwaveprop/propagation/notify_listener.ex +++ b/lib/microwaveprop/propagation/notify_listener.ex @@ -54,7 +54,10 @@ defmodule Microwaveprop.Propagation.NotifyListener do @impl true def handle_info({:DOWN, _ref, :process, pid, reason}, %{pid: pid} = state) do - Logger.warning("NotifyListener: Postgrex.Notifications connection (#{inspect(pid)}) down: #{inspect(reason)}. Reconnecting…") + Logger.warning( + "NotifyListener: Postgrex.Notifications connection (#{inspect(pid)}) down: #{inspect(reason)}. Reconnecting…" + ) + send(self(), :subscribe) {:noreply, %{state | pid: nil, ref: nil}} end diff --git a/lib/microwaveprop/propagation/path_compute.ex b/lib/microwaveprop/propagation/path_compute.ex index 4bb28dba..7ca05614 100644 --- a/lib/microwaveprop/propagation/path_compute.ex +++ b/lib/microwaveprop/propagation/path_compute.ex @@ -379,16 +379,17 @@ defmodule Microwaveprop.Propagation.PathCompute do defp build_scoring(profiles, src, dst, now, band_config, native_duct) do {temps, dewpoints, pressures, gradients, bl_depths, pwats} = - Enum.reduce(profiles, {[], [], [], [], [], []}, fn p, - {ts, ds, ps, gs, bs, ws} -> + Enum.reduce(profiles, {[], [], [], [], [], []}, fn p, {ts, ds, ps, gs, bs, ws} -> { - if(p.surface_temp_c != nil, do: [p.surface_temp_c | ts], else: ts), - if(p.surface_dewpoint_c != nil, do: [p.surface_dewpoint_c | ds], else: ds), - if(p.surface_pressure_mb != nil, do: [p.surface_pressure_mb | ps], else: ps), - if(p.min_refractivity_gradient != nil, do: [p.min_refractivity_gradient | gs], - else: gs), - if(p.hpbl_m != nil, do: [p.hpbl_m | bs], else: bs), - if(p.pwat_mm != nil, do: [p.pwat_mm | ws], else: ws) + if(p.surface_temp_c == nil, do: ts, else: [p.surface_temp_c | ts]), + if(p.surface_dewpoint_c == nil, do: ds, else: [p.surface_dewpoint_c | ds]), + if(p.surface_pressure_mb == nil, do: ps, else: [p.surface_pressure_mb | ps]), + if(p.min_refractivity_gradient == nil, + do: gs, + else: [p.min_refractivity_gradient | gs] + ), + if(p.hpbl_m == nil, do: bs, else: [p.hpbl_m | bs]), + if(p.pwat_mm == nil, do: ws, else: [p.pwat_mm | ws]) } end) diff --git a/lib/microwaveprop/propagation/profiles_file.ex b/lib/microwaveprop/propagation/profiles_file.ex index 13b2180d..9ea17c99 100644 --- a/lib/microwaveprop/propagation/profiles_file.ex +++ b/lib/microwaveprop/propagation/profiles_file.ex @@ -168,7 +168,7 @@ defmodule Microwaveprop.Propagation.ProfilesFile do defp read_mp(valid_time) do with {:ok, gz} <- File.read(mp_path_for(valid_time)), - binary = :zlib.gunzip(gz), + {:ok, binary} <- gunzip_safe(gz), {:ok, body} <- Msgpax.unpack(binary) do {:ok, decode_mp_body(body)} else @@ -176,6 +176,12 @@ defmodule Microwaveprop.Propagation.ProfilesFile do end end + defp gunzip_safe(data) do + {:ok, :zlib.gunzip(data)} + rescue + _ -> {:error, :corrupt} + end + defp decode_mp_body(%{"cells" => cells}) when is_list(cells) do Map.new(cells, fn cell -> lat = cell |> Map.get("lat") |> to_float() @@ -318,11 +324,8 @@ defmodule Microwaveprop.Propagation.ProfilesFile do end defp invalidate_all_caches do - # Match-delete every cached entry for this module without clobbering - # keys owned by other modules. - :ets.match_delete(:microwaveprop_cache, {{__MODULE__, :read, :_, :_}, :_, :_}) + Microwaveprop.Cache.match_delete({{__MODULE__, :read, :_, :_}, :_, :_}) Microwaveprop.Cache.invalidate({__MODULE__, :list_valid_times, base_dir()}) - :ok end @doc """ diff --git a/lib/microwaveprop/propagation/scores_file.ex b/lib/microwaveprop/propagation/scores_file.ex index 5c833c27..5b165102 100644 --- a/lib/microwaveprop/propagation/scores_file.ex +++ b/lib/microwaveprop/propagation/scores_file.ex @@ -509,11 +509,7 @@ defmodule Microwaveprop.Propagation.ScoresFile do end defp invalidate_all_list_caches do - # Cheaper than enumerating bands β€” clear every cached list entry. - # `Microwaveprop.Cache.clear/0` is overkill (it wipes unrelated - # keys too) so match-delete just this module's keys. - :ets.match_delete(:microwaveprop_cache, {{__MODULE__, :list_valid_times, :_, :_}, :_, :_}) - :ok + Microwaveprop.Cache.match_delete({{__MODULE__, :list_valid_times, :_, :_}, :_, :_}) end defp parse_valid_time_dt(filename) do diff --git a/lib/microwaveprop/radio.ex b/lib/microwaveprop/radio.ex index 210d84d3..b0f423ae 100644 --- a/lib/microwaveprop/radio.ex +++ b/lib/microwaveprop/radio.ex @@ -130,6 +130,7 @@ defmodule Microwaveprop.Radio do |> where([c], c.user_id == ^owner.id) |> filter_private_for_viewer(owner, viewer) |> order_by([c], desc: c.qso_timestamp, desc: c.id) + |> preload(:user) |> Repo.all() end @@ -162,6 +163,7 @@ defmodule Microwaveprop.Radio do |> filter_private_for_callsign_viewer(viewer) |> order_by([c], desc: c.qso_timestamp, desc: c.id) |> limit(100) + |> preload(:user) |> Repo.all() end diff --git a/lib/microwaveprop_web/endpoint.ex b/lib/microwaveprop_web/endpoint.ex index 24fba93e..a694f8d0 100644 --- a/lib/microwaveprop_web/endpoint.ex +++ b/lib/microwaveprop_web/endpoint.ex @@ -8,6 +8,7 @@ defmodule MicrowavepropWeb.Endpoint do store: :cookie, key: "_microwaveprop_key", signing_salt: "FBet7+Mi", + encryption_salt: "fPEhSH1PIkX0MJMU1WL3", same_site: "Lax" ] diff --git a/lib/microwaveprop_web/live/map_live.ex b/lib/microwaveprop_web/live/map_live.ex index 5678fac0..0225e9d0 100644 --- a/lib/microwaveprop_web/live/map_live.ex +++ b/lib/microwaveprop_web/live/map_live.ex @@ -1051,10 +1051,16 @@ defmodule MicrowavepropWeb.MapLive do