diff --git a/bugs.md b/bugs.md index 70b8cb95..02030925 100644 --- a/bugs.md +++ b/bugs.md @@ -1,27 +1,28 @@ -# Identified Bugs and Observations (Post-Fix Review) +# Bugs Found 2026-05-11 -This document reflects the state of the codebase after the fixes for the initial 15 bugs were applied. +The first batch (7 items: pending-beacon access control, per_page contract, +rate-limiter ETS lifecycle, unbounded grid endpoints, NEXRAD PNG quadratic +unfilter, LiveTableFooter parser, ScoresFile parser) was fixed and removed +from this file as each was resolved. The items below are additional bugs +surfaced by a follow-up audit. -All three items below were verified, covered by failing tests, and fixed on 2026-04-25. +## A. `String.to_integer` on user-controlled `band` param crashes LiveViews -## 1. Haversine Implementation Inconsistency — FIXED 2026-04-25 -**Files:** `lib/microwaveprop/radio.ex` vs `lib/microwaveprop/commercial.ex` -**Description:** -- `Radio.haversine_km/4` uses the `asin` form: `2 * r * :math.asin(:math.sqrt(a))` -- `Commercial.haversine_km/4` uses the `atan2` form: `2 * r * :math.atan2(:math.sqrt(a), :math.sqrt(1 - a))` -**Impact:** While functionally similar, `atan2` is the industry standard for floating-point stability at large distances. Having two different implementations for the same physical calculation can lead to tiny discrepancies (sub-millimeter) that might cause a contact or commercial link to intermittently cross a distance threshold (e.g., 75 km) depending on which module performs the check. -**Fix:** Switched `Radio.haversine_km/4` to the `atan2` form and made `Commercial`'s private wrapper delegate to it. One canonical implementation, no per-module drift. Numerical-stability regression test added in `test/microwaveprop/radio_test.exs`. +**Status:** Fixed. +**Severity:** High +**Category:** Availability / unhandled crash on user input +**Files:** +- `lib/microwaveprop_web/live/path_live.ex` (formerly lines 227, 284) +- `lib/microwaveprop_web/live/map_live.ex` (formerly line 198) -## 2. Grid Snapping Mathematical Drift — FIXED 2026-04-25 -**Files:** `lib/microwaveprop/propagation/profiles_file.ex` vs `rust/prop_grid_rs/src/profiles_file.rs` -**Description:** -- Elixir `snap/2`: `Float.round(Float.round(lat / step) * step, 3)` -- Rust `snap_coords`: `(lat * 1000.0).round() / 1000.0` -**Impact:** The Elixir version performs a division and multiplication by the grid step (`0.125`) before rounding. The Rust version rounds the raw coordinate to the third decimal place. For coordinates that land exactly on a $0.0625$ boundary (the halfway point between cells), these two methods may snap to different cells, causing the UI to report "No Data" or show the wrong factor breakdown for a clicked point. -**Fix:** Rust `snap_coords` now mirrors the Elixir step-aware snap (`round(coord / 0.125) * 0.125`, then 3-decimal round). Cross-stack parity test in `rust/prop_grid_rs/src/profiles_file.rs` enumerates known-disagreeing inputs. +`PathLive.handle_event("calculate", …)` and the auto-calculate path both +called `String.to_integer(params["band"])` directly on the `band` form +parameter. `MapLive.handle_event("select_band", %{"value" => band}, …)` +did the same. A non-numeric `band` value — supplied via a hand-crafted +URL (`/path?band=abc`) or a misbehaving JS hook — raised `ArgumentError` +inside the LiveView process and crashed the channel. -## 3. Potential Exception in Wgrib2 Parsing (Theoretical) — FIXED 2026-04-25 -**File:** `lib/microwaveprop/weather/grib2/wgrib2.ex` -**Description:** `parse_lon_val_segment/1` uses `String.to_float(lon_str)` for the longitude, but `Float.parse` for latitude and value. -**Impact:** If `wgrib2` were to ever return an integer string for longitude (e.g. `"235"`) instead of a float (`"235.0"`), `String.to_float` would raise an `ArgumentError`. While `wgrib2` typically outputs floats, using `Float.parse` would be more consistent with the other fields in the same line. -**Fix:** All three numeric fields now go through `Float.parse/1` (in a `with` chain so a malformed segment returns `nil` instead of bringing down the chain step). Function exposed as `@doc false` and covered by direct unit tests for both decimal-formatted and integer-formatted longitude strings. +**Fix:** Use the existing `LiveHelpers.parse_int/2` (PathLive) and the +existing `parse_band_param/1` plus a small `normalize_band_event/1` helper +(MapLive). Unknown bands now fall back to a safe default or a no-op +instead of crashing the process. diff --git a/lib/microwaveprop/application.ex b/lib/microwaveprop/application.ex index d38f84f1..0828f498 100644 --- a/lib/microwaveprop/application.ex +++ b/lib/microwaveprop/application.ex @@ -58,7 +58,11 @@ defmodule Microwaveprop.Application do # leader to drop. See `Microwaveprop.Pskr.Client`. Disabled # in dev/test so `mix test` doesn't try to dial out. Microwaveprop.Pskr.Aggregator, - pskr_client_child_spec() + pskr_client_child_spec(), + # Periodic GC for the API rate limiter ETS table; without this + # the table accumulates one row per distinct caller per window + # for the lifetime of the BEAM. + MicrowavepropWeb.Api.RateLimiter.Sweeper ] # See https://hexdocs.pm/elixir/Supervisor.html diff --git a/lib/microwaveprop/beacons.ex b/lib/microwaveprop/beacons.ex index 48c7fdf1..1cfd871d 100644 --- a/lib/microwaveprop/beacons.ex +++ b/lib/microwaveprop/beacons.ex @@ -75,6 +75,33 @@ defmodule Microwaveprop.Beacons do @spec get_beacon!(Ecto.UUID.t()) :: Beacon.t() | [Beacon.t()] | nil def get_beacon!(id), do: Beacon |> Repo.get!(id) |> Repo.preload(:user) + @doc """ + Returns a beacon only if the viewer is allowed to see it. Approved + beacons are visible to everyone; pending (unapproved) beacons are + visible only to the submitter and admins. Returns `nil` for + not-found, malformed ids, and unauthorized viewers — callers render + the same 404 in each case so existence of a pending beacon is not + observable. + """ + @spec get_visible_beacon(Ecto.UUID.t() | String.t(), User.t() | nil) :: Beacon.t() | nil + def get_visible_beacon(id, viewer) do + case fetch_beacon(id) do + nil -> nil + beacon -> if can_view?(beacon, viewer), do: beacon + end + end + + defp fetch_beacon(id) do + Beacon |> Repo.get(id) |> Repo.preload(:user) + rescue + Ecto.Query.CastError -> nil + end + + defp can_view?(%Beacon{approved: true}, _viewer), do: true + defp can_view?(%Beacon{}, %User{is_admin: true}), do: true + defp can_view?(%Beacon{user_id: uid}, %User{id: uid}) when not is_nil(uid), do: true + defp can_view?(%Beacon{}, _viewer), do: false + @doc """ Creates a beacon. When a user is provided they are recorded as the creator; anonymous submissions pass `nil` and leave `user_id` unset. diff --git a/lib/microwaveprop/propagation/scores_file.ex b/lib/microwaveprop/propagation/scores_file.ex index e511a3eb..8ba5283a 100644 --- a/lib/microwaveprop/propagation/scores_file.ex +++ b/lib/microwaveprop/propagation/scores_file.ex @@ -480,9 +480,24 @@ defmodule Microwaveprop.Propagation.ScoresFile do defp fetch_bound(bounds, atom_key, string_key, default) do case Map.get(bounds, atom_key) || Map.get(bounds, string_key) do - nil -> default - v when is_number(v) -> v - v when is_binary(v) -> String.to_float(v) + nil -> + default + + v when is_number(v) -> + v + + v when is_binary(v) -> + case Float.parse(v) do + {f, ""} -> f + # `Float.parse` accepts trailing junk (`"1.5xyz"` → `{1.5, "xyz"}`); + # only treat values that fully consume the input as parsed. On + # any other shape, fall back to the caller-supplied default + # rather than raising and crashing the caller. + _ -> default + end + + _ -> + default end end diff --git a/lib/microwaveprop/radio.ex b/lib/microwaveprop/radio.ex index 5974199f..3440104f 100644 --- a/lib/microwaveprop/radio.ex +++ b/lib/microwaveprop/radio.ex @@ -14,6 +14,7 @@ defmodule Microwaveprop.Radio do alias Microwaveprop.Workers.ContactWeatherEnqueueWorker @per_page 20 + @max_per_page 200 @sortable_fields ~w(station1 station2 band mode distance_km qso_timestamp inserted_at)a @count_cache_key {__MODULE__, :total_contact_count} @count_cache_ttl_ms 30_000 @@ -152,7 +153,8 @@ defmodule Microwaveprop.Radio do @spec list_contacts(keyword()) :: contact_page() def list_contacts(opts \\ []) do page = max(Keyword.get(opts, :page, 1), 1) - offset = (page - 1) * @per_page + per_page = clamp_per_page(Keyword.get(opts, :per_page, @per_page)) + offset = (page - 1) * per_page search = Keyword.get(opts, :search) scope = Keyword.get(opts, :scope) @@ -164,12 +166,12 @@ defmodule Microwaveprop.Radio do |> filter_private(scope) total_entries = total_entries_for(search, scope, base_query) - total_pages = max(ceil(total_entries / @per_page), 1) + total_pages = max(ceil(total_entries / per_page), 1) entries = base_query |> order_by([q], [{^sort_dir, field(q, ^sort_field)}, {^sort_dir, q.id}]) - |> limit(^@per_page) + |> limit(^per_page) |> offset(^offset) |> Repo.all() |> Repo.preload(:user) @@ -183,6 +185,10 @@ defmodule Microwaveprop.Radio do } end + defp clamp_per_page(value) when is_integer(value) and value >= 1, do: min(value, @max_per_page) + + defp clamp_per_page(_), do: @per_page + @doc """ Groups reciprocal contacts (same station pair swapped, same band, same timestamp). Returns a list of `{primary, reciprocals}` tuples where reciprocals is diff --git a/lib/microwaveprop/weather/nexrad_client.ex b/lib/microwaveprop/weather/nexrad_client.ex index 517c012c..a0febdfd 100644 --- a/lib/microwaveprop/weather/nexrad_client.ex +++ b/lib/microwaveprop/weather/nexrad_client.ex @@ -388,59 +388,66 @@ defmodule Microwaveprop.Weather.NexradClient do unfilter_paeth(row, prev, bpp) end - defp unfilter_sub(row, bpp) do - row_bytes = :binary.bin_to_list(row) + # Indexed-color PNG only ever has bpp = 1 (one palette index byte per + # pixel), so "left" is simply the previously-decoded output byte. We + # walk the input binary once and accumulate output bytes into an + # iolist, which is O(n) — the original `acc ++ [byte]` plus + # `Enum.at(acc, idx - bpp)` was O(n²) per row and dominated decode + # time for the 12200×5400 n0q frame. + defp unfilter_sub(row, 1), do: unfilter_sub_iter(row, 0, []) - {result, _} = - Enum.reduce(row_bytes, {[], 0}, fn byte, {acc, idx} -> - left = if idx >= bpp, do: Enum.at(acc, idx - bpp), else: 0 - val = rem(byte + left, 256) - {acc ++ [val], idx + 1} - end) + defp unfilter_sub_iter(<<>>, _left, acc), do: acc |> Enum.reverse() |> :binary.list_to_bin() - :binary.list_to_bin(result) + defp unfilter_sub_iter(<>, left, acc) do + val = rem(byte + left, 256) + unfilter_sub_iter(rest, val, [val | acc]) end - defp unfilter_up(row, prev) do - row_bytes = :binary.bin_to_list(row) - prev_bytes = :binary.bin_to_list(prev) + defp unfilter_up(row, prev), do: unfilter_up_iter(row, prev, []) - row_bytes - |> Enum.zip(prev_bytes) - |> Enum.map(fn {a, b} -> rem(a + b, 256) end) - |> :binary.list_to_bin() + defp unfilter_up_iter(<<>>, _prev, acc), do: acc |> Enum.reverse() |> :binary.list_to_bin() + + defp unfilter_up_iter(<>, <>, acc) do + val = rem(byte + above, 256) + unfilter_up_iter(row_rest, prev_rest, [val | acc]) end - defp unfilter_average(row, prev, bpp) do - row_bytes = :binary.bin_to_list(row) - prev_bytes = :binary.bin_to_list(prev) - - {result, _} = - Enum.reduce(Enum.with_index(row_bytes), {[], prev_bytes}, fn {byte, idx}, {acc, prev_list} -> - left = if idx >= bpp, do: Enum.at(acc, idx - bpp), else: 0 - above = Enum.at(prev_list, idx, 0) - val = rem(byte + div(left + above, 2), 256) - {acc ++ [val], prev_list} - end) - - :binary.list_to_bin(result) + # `prev` may be shorter than `row` if a row is shorter than the + # previous one — for indexed-color this never happens but we + # defensively treat missing prior bytes as zero. + defp unfilter_up_iter(<>, <<>>, acc) do + val = rem(byte, 256) + unfilter_up_iter(row_rest, <<>>, [val | acc]) end - defp unfilter_paeth(row, prev, bpp) do - row_bytes = :binary.bin_to_list(row) - prev_bytes = :binary.bin_to_list(prev) + defp unfilter_average(row, prev, 1), do: unfilter_average_iter(row, prev, 0, []) - {result, _} = - Enum.reduce(Enum.with_index(row_bytes), {[], prev_bytes}, fn {byte, idx}, {acc, prev_list} -> - left = if idx >= bpp, do: Enum.at(acc, idx - bpp), else: 0 - above = Enum.at(prev_list, idx, 0) - upper_left = if idx >= bpp, do: Enum.at(prev_list, idx - bpp, 0), else: 0 - pr = paeth_predictor(left, above, upper_left) - val = rem(byte + pr, 256) - {acc ++ [val], prev_list} - end) + defp unfilter_average_iter(<<>>, _prev, _left, acc), do: acc |> Enum.reverse() |> :binary.list_to_bin() - :binary.list_to_bin(result) + defp unfilter_average_iter(<>, <>, left, acc) do + val = rem(byte + div(left + above, 2), 256) + unfilter_average_iter(row_rest, prev_rest, val, [val | acc]) + end + + defp unfilter_average_iter(<>, <<>>, left, acc) do + val = rem(byte + div(left, 2), 256) + unfilter_average_iter(row_rest, <<>>, val, [val | acc]) + end + + defp unfilter_paeth(row, prev, 1), do: unfilter_paeth_iter(row, prev, 0, 0, []) + + defp unfilter_paeth_iter(<<>>, _prev, _left, _upper_left, acc), do: acc |> Enum.reverse() |> :binary.list_to_bin() + + defp unfilter_paeth_iter(<>, <>, left, upper_left, acc) do + pr = paeth_predictor(left, above, upper_left) + val = rem(byte + pr, 256) + unfilter_paeth_iter(row_rest, prev_rest, val, above, [val | acc]) + end + + defp unfilter_paeth_iter(<>, <<>>, left, _upper_left, acc) do + pr = paeth_predictor(left, 0, 0) + val = rem(byte + pr, 256) + unfilter_paeth_iter(row_rest, <<>>, val, 0, [val | acc]) end defp paeth_predictor(a, b, c) do diff --git a/lib/microwaveprop_web/api/rate_limiter.ex b/lib/microwaveprop_web/api/rate_limiter.ex index 0d1e0ec8..b0e51f52 100644 --- a/lib/microwaveprop_web/api/rate_limiter.ex +++ b/lib/microwaveprop_web/api/rate_limiter.ex @@ -2,10 +2,10 @@ defmodule MicrowavepropWeb.Api.RateLimiter do @moduledoc """ Tiny ETS-backed fixed-window rate limiter for `/api/v1`. - No external dependency, no supervisor entry — the named ETS table - is created lazily on the first request and survives for the - lifetime of the BEAM. Each (bucket, window) pair holds a counter - incremented atomically via `:ets.update_counter/4`. + The named ETS table is created at application boot (and re-created + idempotently if the boot path is bypassed in tests). Each + `{bucket, window}` pair holds a counter incremented atomically via + `:ets.update_counter/4`. Default limits (overridable per plug invocation): * authenticated requests: 600 / minute, keyed by API token id @@ -13,6 +13,14 @@ defmodule MicrowavepropWeb.Api.RateLimiter do The plug emits the RFC 9651 `RateLimit` headers and a 429 problem+json response when the bucket is exhausted. + + ## Lifecycle + + Without a sweeper the table grows one row per distinct `{bucket, + window}` and never shrinks — every distinct IP / token over the + pod's lifetime stays resident. The companion `Sweeper` GenServer + prunes rows whose window has fully expired on a fixed cadence; see + `MicrowavepropWeb.Api.RateLimiter.Sweeper`. """ @behaviour Plug @@ -26,15 +34,28 @@ defmodule MicrowavepropWeb.Api.RateLimiter do @default_anon_limit 60 @default_auth_limit 600 + @doc "Returns the ETS table name backing the limiter." + @spec table() :: atom() + def table, do: @table + + @doc "Returns the default window size in milliseconds." + @spec default_window_ms() :: pos_integer() + def default_window_ms, do: @default_window_ms + @doc """ Initializes the named ETS table. Idempotent — safe to call from - application boot and from tests. Returns `:ok` whether the table - existed already or was created by this call. + application boot and from tests. The race between `whereis` and + `:ets.new` is handled by rescuing `ArgumentError`, which is what + `:ets.new` raises when the table already exists. """ @spec init_table() :: :ok def init_table do if :ets.whereis(@table) == :undefined do - :ets.new(@table, [:set, :public, :named_table, write_concurrency: true]) + try do + :ets.new(@table, [:set, :public, :named_table, write_concurrency: true]) + rescue + ArgumentError -> :ok + end end :ok diff --git a/lib/microwaveprop_web/api/rate_limiter/sweeper.ex b/lib/microwaveprop_web/api/rate_limiter/sweeper.ex new file mode 100644 index 00000000..e7feff35 --- /dev/null +++ b/lib/microwaveprop_web/api/rate_limiter/sweeper.ex @@ -0,0 +1,103 @@ +defmodule MicrowavepropWeb.Api.RateLimiter.Sweeper do + @moduledoc """ + Periodic GC for the API rate limiter ETS table. + + The limiter writes one row per `{bucket, window}` and never + overwrites — without a sweeper the table accumulates one row per + distinct caller per window for the lifetime of the BEAM. This + process scans the table on a fixed cadence and deletes any row + whose window has fully expired (window index strictly less than + the current window index). + + Defaults to one sweep per 5 minutes. Configure via: + + config :microwaveprop, MicrowavepropWeb.Api.RateLimiter.Sweeper, + interval_ms: 300_000 + + Test helper `sweep_now/1` runs a sweep synchronously. + """ + + use GenServer + + alias MicrowavepropWeb.Api.RateLimiter + + @default_interval_ms 5 * 60 * 1_000 + + @spec start_link(keyword()) :: GenServer.on_start() + def start_link(opts \\ []) do + name = Keyword.get(opts, :name, __MODULE__) + GenServer.start_link(__MODULE__, opts, name: name) + end + + @doc "Run a sweep now. Returns the number of rows deleted." + @spec sweep_now(GenServer.server()) :: non_neg_integer() + def sweep_now(server \\ __MODULE__) do + GenServer.call(server, :sweep_now) + end + + @impl true + def init(opts) do + interval_ms = + opts + |> Keyword.get(:interval_ms) + |> Kernel.||( + :microwaveprop + |> Application.get_env(__MODULE__, []) + |> Keyword.get(:interval_ms, @default_interval_ms) + ) + + # Own the ETS table from this long-lived process. ETS named tables + # die with their owner — having a transient request task or + # short-lived test process create the table leaves the system + # without a backing store the moment that process exits. The + # application boot path also calls `RateLimiter.init_table/0` for + # the cold-start safety net; both paths are idempotent. + :ok = RateLimiter.init_table() + + schedule(interval_ms) + {:ok, %{interval_ms: interval_ms}} + end + + @impl true + def handle_info(:sweep, state) do + _ = sweep() + schedule(state.interval_ms) + {:noreply, state} + end + + @impl true + def handle_call(:sweep_now, _from, state) do + {:reply, sweep(), state} + end + + defp schedule(interval_ms) do + Process.send_after(self(), :sweep, interval_ms) + end + + defp sweep do + table = RateLimiter.table() + + # Defensive: if the table is missing (test that hasn't booted the + # app, etc.) there is nothing to prune. + if :ets.whereis(table) == :undefined do + 0 + else + window_ms = RateLimiter.default_window_ms() + current_window = div(System.system_time(:millisecond), window_ms) + + # Match rows whose window index is strictly less than the + # current window — those windows can no longer be hit (even a + # request arriving exactly at the boundary uses the new + # window). `:ets.select_delete/2` returns the count. + match_spec = [ + { + {{:_, :"$1"}, :_}, + [{:<, :"$1", current_window}], + [true] + } + ] + + :ets.select_delete(table, match_spec) + end + end +end diff --git a/lib/microwaveprop_web/controllers/api/v1/beacon_controller.ex b/lib/microwaveprop_web/controllers/api/v1/beacon_controller.ex index 035e3608..979fa63c 100644 --- a/lib/microwaveprop_web/controllers/api/v1/beacon_controller.ex +++ b/lib/microwaveprop_web/controllers/api/v1/beacon_controller.ex @@ -16,7 +16,9 @@ defmodule MicrowavepropWeb.Api.V1.BeaconController do end def show(conn, %{"id" => id}) do - case fetch_beacon(id) do + viewer = conn.assigns[:current_api_user] + + case Beacons.get_visible_beacon(id, viewer) do nil -> ErrorJSON.send_problem(conn, 404, "not_found", "Beacon not found.") beacon -> json(conn, BeaconJSON.show(%{beacon: beacon})) end @@ -41,12 +43,4 @@ defmodule MicrowavepropWeb.Api.V1.BeaconController do ErrorJSON.send_changeset(conn, changeset) end end - - # `Beacons.get_beacon!/1` raises on bad ids; we want a clean 404. - defp fetch_beacon(id) do - Beacons.get_beacon!(id) - rescue - Ecto.NoResultsError -> nil - Ecto.Query.CastError -> nil - end end diff --git a/lib/microwaveprop_web/controllers/api/v1/contact_controller.ex b/lib/microwaveprop_web/controllers/api/v1/contact_controller.ex index cdcdc7db..3c91241f 100644 --- a/lib/microwaveprop_web/controllers/api/v1/contact_controller.ex +++ b/lib/microwaveprop_web/controllers/api/v1/contact_controller.ex @@ -24,6 +24,7 @@ defmodule MicrowavepropWeb.Api.V1.ContactController do %{entries: entries, total_entries: total, total_pages: total_pages} = Radio.list_contacts( page: page, + per_page: per_page, search: search, scope: Scope.for_user(viewer) ) diff --git a/lib/microwaveprop_web/controllers/scores_controller.ex b/lib/microwaveprop_web/controllers/scores_controller.ex index 20695d30..5aa95aa0 100644 --- a/lib/microwaveprop_web/controllers/scores_controller.ex +++ b/lib/microwaveprop_web/controllers/scores_controller.ex @@ -5,16 +5,25 @@ defmodule MicrowavepropWeb.ScoresController do the /map overlay so the client can fetch directly (cacheable, doesn't block the LiveView channel) and decode via DataView + typed-array views without a JSON parse. + + Rate-limited per IP and bounds-capped to the propagation grid extent + so a malicious caller cannot trigger global / oversized binary + responses. """ use MicrowavepropWeb, :controller alias Microwaveprop.Propagation + alias MicrowavepropWeb.Api.RateLimiter + alias MicrowavepropWeb.GridBounds + + plug RateLimiter, anon_limit: 120, auth_limit: 600 @spec cells(Plug.Conn.t(), map()) :: Plug.Conn.t() def cells(conn, %{"band" => band_param} = params) do with {band_mhz, ""} <- Integer.parse(band_param), {:ok, valid_time} <- parse_optional_time(params["time"]), - {:ok, bounds} <- parse_bounds(params) do + {:ok, bounds} <- parse_bounds(params), + {:ok, bounds} <- GridBounds.clamp(bounds) do scores = if valid_time do Propagation.scores_at(band_mhz, valid_time, bounds) @@ -27,6 +36,7 @@ defmodule MicrowavepropWeb.ScoresController do |> put_resp_header("cache-control", "public, max-age=60") |> send_resp(200, encode_binary(scores)) else + {:error, :viewport_too_large} -> payload_too_large(conn) _ -> bad_request(conn) end end @@ -39,6 +49,12 @@ defmodule MicrowavepropWeb.ScoresController do |> json(%{error: "invalid params"}) end + defp payload_too_large(conn) do + conn + |> put_status(413) + |> json(%{error: "viewport_too_large", detail: "Requested bounds exceed the maximum supported viewport area."}) + end + defp parse_optional_time(nil), do: {:ok, nil} defp parse_optional_time(value) when is_binary(value) do diff --git a/lib/microwaveprop_web/controllers/weather_tile_controller.ex b/lib/microwaveprop_web/controllers/weather_tile_controller.ex index 2966329d..f581cb50 100644 --- a/lib/microwaveprop_web/controllers/weather_tile_controller.ex +++ b/lib/microwaveprop_web/controllers/weather_tile_controller.ex @@ -2,6 +2,10 @@ defmodule MicrowavepropWeb.WeatherTileController do use MicrowavepropWeb, :controller alias Microwaveprop.Weather + alias MicrowavepropWeb.Api.RateLimiter + alias MicrowavepropWeb.GridBounds + + plug RateLimiter, anon_limit: 120, auth_limit: 600 # Layer fields the binary cell-pack can carry. Used both as the # whitelist for ?layers=... and as the default if the param is absent. @@ -17,6 +21,7 @@ defmodule MicrowavepropWeb.WeatherTileController do def cells(conn, %{"time" => time_param} = params) do with {:ok, valid_time, _offset} <- DateTime.from_iso8601(time_param), {:ok, bounds} <- parse_bounds(params), + {:ok, bounds} <- GridBounds.clamp(bounds), {:ok, layers} <- parse_layers(params["layers"]) do rows = read_rows(valid_time, bounds, params["source"]) @@ -25,6 +30,7 @@ defmodule MicrowavepropWeb.WeatherTileController do |> put_resp_header("cache-control", "public, max-age=60") |> send_resp(200, encode_binary(rows, layers)) else + {:error, :viewport_too_large} -> payload_too_large(conn) _ -> bad_request(conn) end end @@ -40,6 +46,12 @@ defmodule MicrowavepropWeb.WeatherTileController do |> json(%{error: "invalid params"}) end + defp payload_too_large(conn) do + conn + |> put_status(413) + |> json(%{error: "viewport_too_large", detail: "Requested bounds exceed the maximum supported viewport area."}) + end + defp parse_bounds(%{"south" => s, "north" => n, "west" => w, "east" => e}) do with {:ok, south} <- parse_float(s), {:ok, north} <- parse_float(n), diff --git a/lib/microwaveprop_web/grid_bounds.ex b/lib/microwaveprop_web/grid_bounds.ex new file mode 100644 index 00000000..b809a8c9 --- /dev/null +++ b/lib/microwaveprop_web/grid_bounds.ex @@ -0,0 +1,73 @@ +defmodule MicrowavepropWeb.GridBounds do + @moduledoc """ + Bounds clamp + sanity checks for grid-cell HTTP endpoints + (`/scores/cells`, `/weather/cells`). + + The propagation grid covers HRRR (CONUS) and HRDPS (Canada to + 60°N): roughly lat 25-60, lon -141 to -52 — about 35° × 90° = + 3150 sq degrees. Any caller-provided viewport is clamped to a + generous superset of that bbox, and viewports whose area exceeds + the supported region are rejected as `:viewport_too_large` so a + malicious caller cannot trigger global / multi-million-cell + binary responses. + + `nil` bounds are passed through (caller intends the full grid). + """ + + # Generous superset of HRRR + HRDPS — a few extra degrees on each + # edge so legitimate clients zoomed slightly past the data extent + # still get a 200 with the actual covered cells. + @lat_min 20.0 + @lat_max 65.0 + @lon_min -150.0 + @lon_max -50.0 + + # Comfortably above the HRDPS+HRRR union (~3150 sq deg), but tight + # enough that the fully-clamped extent (~4500 sq deg) still trips + # the cap — a "give me everything" request is exactly the DoS + # shape we want to reject. + @max_viewport_area_sq_deg 4000.0 + + @type bounds :: %{optional(String.t()) => float()} | nil + + @doc """ + Clamp `bounds` to the supported grid extent. Returns: + + * `{:ok, nil}` when bounds are nil (caller wants the full grid) + * `{:ok, clamped_bounds}` when the clamped viewport area is OK + * `{:error, :viewport_too_large}` when the clamped area still + exceeds `@max_viewport_area_sq_deg` (catches degenerate + requests like global bounds that survive clamping) + * `{:error, :invalid_bounds}` when north < south or east < west + """ + @spec clamp(bounds()) :: + {:ok, bounds()} | {:error, :viewport_too_large | :invalid_bounds} + def clamp(nil), do: {:ok, nil} + + def clamp(%{"south" => s, "north" => n, "west" => w, "east" => e}) + when is_number(s) and is_number(n) and is_number(w) and is_number(e) do + south = clamp_value(s, @lat_min, @lat_max) + north = clamp_value(n, @lat_min, @lat_max) + west = clamp_value(w, @lon_min, @lon_max) + east = clamp_value(e, @lon_min, @lon_max) + + cond do + north < south or east < west -> + {:error, :invalid_bounds} + + (north - south) * (east - west) > @max_viewport_area_sq_deg -> + {:error, :viewport_too_large} + + true -> + {:ok, %{"south" => south, "north" => north, "west" => west, "east" => east}} + end + end + + def clamp(_), do: {:error, :invalid_bounds} + + @doc "Maximum viewport area in square degrees." + @spec max_viewport_area_sq_deg() :: float() + def max_viewport_area_sq_deg, do: @max_viewport_area_sq_deg + + defp clamp_value(v, lo, hi), do: v |> max(lo) |> min(hi) +end diff --git a/lib/microwaveprop_web/live/beacon_live/show.ex b/lib/microwaveprop_web/live/beacon_live/show.ex index e88569f4..04560883 100644 --- a/lib/microwaveprop_web/live/beacon_live/show.ex +++ b/lib/microwaveprop_web/live/beacon_live/show.ex @@ -159,19 +159,28 @@ defmodule MicrowavepropWeb.BeaconLive.Show do @impl true def mount(%{"id" => id}, _session, socket) do - _ = if connected?(socket), do: Beacons.subscribe_beacons() + viewer = socket.assigns[:current_scope] && socket.assigns.current_scope.user - beacon = Beacons.get_beacon!(id) + case Beacons.get_visible_beacon(id, viewer) do + nil -> + # Pending beacons are invisible to everyone except the + # submitter and admins. Render the same 404 we'd give for an + # unknown id so existence isn't observable. + raise Ecto.NoResultsError, queryable: Beacon - {:ok, - socket - |> assign(:page_title, "Beacon") - |> assign(:beacon, beacon) - |> assign(:show_coverage, false) - |> assign(:coverage_supported, coverage_supported?(beacon)) - # Estimate is lazy — computed on the first toggle-on to avoid - # paying the bbox grid cost on every page load. - |> assign(:estimate, nil)} + beacon -> + _ = if connected?(socket), do: Beacons.subscribe_beacons() + + {:ok, + socket + |> assign(:page_title, "Beacon") + |> assign(:beacon, beacon) + |> assign(:show_coverage, false) + |> assign(:coverage_supported, coverage_supported?(beacon)) + # Estimate is lazy — computed on the first toggle-on to avoid + # paying the bbox grid cost on every page load. + |> assign(:estimate, nil)} + end end # Estimated current coverage only makes sense from 5.76 GHz upward, diff --git a/lib/microwaveprop_web/live/map_live.ex b/lib/microwaveprop_web/live/map_live.ex index 8700c0ad..ca1b8a03 100644 --- a/lib/microwaveprop_web/live/map_live.ex +++ b/lib/microwaveprop_web/live/map_live.ex @@ -195,21 +195,15 @@ defmodule MicrowavepropWeb.MapLive do @impl true def handle_event("select_band", %{"value" => band}, socket) do - band = if is_binary(band), do: String.to_integer(band), else: band - valid_times = Propagation.available_valid_times(band) - selected_time = closest_to_now(valid_times) + case normalize_band_event(band) do + nil -> + # Reject unknown / unparseable bands rather than crashing the + # LiveView with String.to_integer/BandConfig.get failures. + {:noreply, socket} - socket = - socket - |> assign(selected_band: band, valid_times: valid_times, selected_time: selected_time) - |> assign(:tracking_now?, tracking_now?(selected_time, valid_times, DateTime.utc_now())) - |> MicrowavepropWeb.LiveStashGuard.stash() - # band_mhz tells the client to refetch /scores/cells for the new band. - |> push_event("update_band_info", %{band_info: band_info(band), band_mhz: band}) - |> push_timeline() - |> patch_map_url() - - {:noreply, socket} + band -> + do_select_band(socket, band) + end end def handle_event("select_time", %{"time" => time_str}, socket) do @@ -511,6 +505,25 @@ defmodule MicrowavepropWeb.MapLive do end end + defp normalize_band_event(band) when is_integer(band), do: if(BandConfig.get(band), do: band) + defp normalize_band_event(band) when is_binary(band), do: parse_band_param(band) + defp normalize_band_event(_), do: nil + + defp do_select_band(socket, band) do + valid_times = Propagation.available_valid_times(band) + selected_time = closest_to_now(valid_times) + + socket + |> assign(selected_band: band, valid_times: valid_times, selected_time: selected_time) + |> assign(:tracking_now?, tracking_now?(selected_time, valid_times, DateTime.utc_now())) + |> MicrowavepropWeb.LiveStashGuard.stash() + # band_mhz tells the client to refetch /scores/cells for the new band. + |> push_event("update_band_info", %{band_info: band_info(band), band_mhz: band}) + |> push_timeline() + |> patch_map_url() + |> then(&{:noreply, &1}) + end + @doc false def parse_time_param(nil), do: nil diff --git a/lib/microwaveprop_web/live/path_live.ex b/lib/microwaveprop_web/live/path_live.ex index eb3628e1..a2b91aa2 100644 --- a/lib/microwaveprop_web/live/path_live.ex +++ b/lib/microwaveprop_web/live/path_live.ex @@ -3,7 +3,7 @@ defmodule MicrowavepropWeb.PathLive do use MicrowavepropWeb, :live_view import MicrowavepropWeb.Components.SkewTChart - import MicrowavepropWeb.LiveHelpers, only: [parse_float: 2] + import MicrowavepropWeb.LiveHelpers, only: [parse_float: 2, parse_int: 2] alias Microwaveprop.Buildings.Index, as: BuildingsIndex alias Microwaveprop.Buildings.Loader, as: BuildingsLoader @@ -224,7 +224,7 @@ defmodule MicrowavepropWeb.PathLive do dst_gain_dbi: parse_float(p["dst_gain_dbi"], 30.0) } - send(self(), {:compute_path, p["source"], p["destination"], String.to_integer(p["band"]), station_params}) + send(self(), {:compute_path, p["source"], p["destination"], parse_int(p["band"], 10_000), station_params}) {:noreply, assign(socket, computing: true)} else {:noreply, socket} @@ -281,7 +281,7 @@ defmodule MicrowavepropWeb.PathLive do send( self(), - {:compute_path, params["source"], params["destination"], String.to_integer(params["band"]), station_params} + {:compute_path, params["source"], params["destination"], parse_int(params["band"], 10_000), station_params} ) url_params = diff --git a/lib/microwaveprop_web/live_table_footer.ex b/lib/microwaveprop_web/live_table_footer.ex index 7f517aa2..e63a384e 100644 --- a/lib/microwaveprop_web/live_table_footer.ex +++ b/lib/microwaveprop_web/live_table_footer.ex @@ -91,7 +91,14 @@ defmodule MicrowavepropWeb.LiveTableFooter do end end - defp parse_page(page) when is_integer(page), do: page - defp parse_page(page) when is_binary(page), do: String.to_integer(page) + defp parse_page(page) when is_integer(page) and page >= 1, do: page + + defp parse_page(page) when is_binary(page) do + case Integer.parse(page) do + {n, ""} when n >= 1 -> n + _ -> 1 + end + end + defp parse_page(_), do: 1 end diff --git a/test/microwaveprop/radio_test.exs b/test/microwaveprop/radio_test.exs index e933e180..e7850637 100644 --- a/test/microwaveprop/radio_test.exs +++ b/test/microwaveprop/radio_test.exs @@ -69,6 +69,39 @@ defmodule Microwaveprop.RadioTest do assert result.page == 2 assert result.total_pages == 2 end + + test "honors per_page option" do + for i <- 1..25 do + ts = DateTime.add(~U[2026-01-01 00:00:00Z], i * 3600, :second) + create_contact(%{qso_timestamp: ts}) + end + + result = Radio.list_contacts(per_page: 50) + + assert length(result.entries) == 25 + assert result.total_pages == 1 + end + + test "clamps per_page to the maximum" do + for i <- 1..3 do + ts = DateTime.add(~U[2026-01-01 00:00:00Z], i * 3600, :second) + create_contact(%{qso_timestamp: ts}) + end + + # 5000 is well above the 200 cap; should still return all 3 rows. + result = Radio.list_contacts(per_page: 5000) + assert length(result.entries) == 3 + end + + test "falls back to default when per_page is invalid" do + for i <- 1..25 do + ts = DateTime.add(~U[2026-01-01 00:00:00Z], i * 3600, :second) + create_contact(%{qso_timestamp: ts}) + end + + result = Radio.list_contacts(per_page: 0) + assert length(result.entries) == 20 + end end describe "list_contacts/1 sorting" do diff --git a/test/microwaveprop_web/api/rate_limiter_test.exs b/test/microwaveprop_web/api/rate_limiter_test.exs index 946ceb14..bc7839ea 100644 --- a/test/microwaveprop_web/api/rate_limiter_test.exs +++ b/test/microwaveprop_web/api/rate_limiter_test.exs @@ -79,4 +79,54 @@ defmodule MicrowavepropWeb.Api.RateLimiterTest do assert RateLimiter.reset() == :ok end end + + describe "init_table/0" do + test "is a no-op when the table already exists" do + RateLimiter.init_table() + assert RateLimiter.init_table() == :ok + end + + test "tolerates many concurrent init_table calls" do + # `call/2` invokes `init_table/0` on every request; under heavy + # load many requests can fly through the whereis check before + # any of them call `:ets.new`. The rescue path makes the loser + # of that race return `:ok` instead of crashing. The Sweeper + # owns the canonical table from the application supervisor, so + # concurrent calls must never leave the system without one. + tasks = + for _ <- 1..16 do + Task.async(fn -> RateLimiter.init_table() end) + end + + results = Task.await_many(tasks) + assert Enum.all?(results, &(&1 == :ok)) + assert :ets.whereis(RateLimiter.table()) != :undefined + end + end + + describe "Sweeper" do + alias MicrowavepropWeb.Api.RateLimiter.Sweeper + + test "sweep_now removes expired window rows but keeps current ones" do + RateLimiter.reset() + table = RateLimiter.table() + + now = System.system_time(:millisecond) + window_ms = RateLimiter.default_window_ms() + current = div(now, window_ms) + + :ets.insert(table, {{{:ip, "stale-1"}, current - 5}, 1}) + :ets.insert(table, {{{:ip, "stale-2"}, current - 1}, 4}) + :ets.insert(table, {{{:ip, "current"}, current}, 2}) + + assert :ets.info(table, :size) == 3 + + {:ok, pid} = Sweeper.start_link(name: :rate_limiter_sweeper_test, interval_ms: 60_000_000) + deleted = Sweeper.sweep_now(pid) + + assert deleted == 2 + assert :ets.info(table, :size) == 1 + assert :ets.lookup(table, {{:ip, "current"}, current}) != [] + end + end end diff --git a/test/microwaveprop_web/controllers/api/v1/beacon_controller_test.exs b/test/microwaveprop_web/controllers/api/v1/beacon_controller_test.exs index 2447dec5..ea5c8735 100644 --- a/test/microwaveprop_web/controllers/api/v1/beacon_controller_test.exs +++ b/test/microwaveprop_web/controllers/api/v1/beacon_controller_test.exs @@ -37,12 +37,45 @@ defmodule MicrowavepropWeb.Api.V1.BeaconControllerTest do end describe "GET /api/v1/beacons/:id" do - test "returns the beacon when found", %{conn: conn, user: user} do + test "returns approved beacons to anonymous viewers", %{conn: conn, user: user} do {:ok, b} = Beacons.create_beacon(user, @valid) + {:ok, b} = Beacons.approve_beacon(b) body = conn |> get(~p"/api/v1/beacons/#{b.id}") |> json_response(200) assert body["data"]["id"] == b.id end + test "404 for anonymous viewers on unapproved beacons", %{conn: conn, user: user} do + {:ok, b} = Beacons.create_beacon(user, @valid) + conn = get(conn, ~p"/api/v1/beacons/#{b.id}") + assert json_response(conn, 404) + end + + test "submitter can view their own unapproved beacon", %{conn: conn, user: user, plaintext: pt} do + {:ok, b} = Beacons.create_beacon(user, @valid) + + body = + conn + |> put_req_header("authorization", "Bearer " <> pt) + |> get(~p"/api/v1/beacons/#{b.id}") + |> json_response(200) + + assert body["data"]["id"] == b.id + assert body["data"]["approved"] == false + end + + test "non-owner non-admin gets 404 on another user's unapproved beacon", %{conn: conn, user: user} do + {:ok, b} = Beacons.create_beacon(user, @valid) + other = AccountsFixtures.user_fixture() + {:ok, {other_pt, _}} = Accounts.create_api_token(other, %{name: "t"}) + + conn = + conn + |> put_req_header("authorization", "Bearer " <> other_pt) + |> get(~p"/api/v1/beacons/#{b.id}") + + assert json_response(conn, 404) + end + test "404 on unknown id", %{conn: conn} do conn = get(conn, ~p"/api/v1/beacons/00000000-0000-0000-0000-000000000000") assert json_response(conn, 404) diff --git a/test/microwaveprop_web/controllers/scores_controller_test.exs b/test/microwaveprop_web/controllers/scores_controller_test.exs index c28acb27..1a9981a7 100644 --- a/test/microwaveprop_web/controllers/scores_controller_test.exs +++ b/test/microwaveprop_web/controllers/scores_controller_test.exs @@ -100,5 +100,11 @@ defmodule MicrowavepropWeb.ScoresControllerTest do conn = get(conn, ~p"/scores/cells?band=10000&time=&south=0&north=1&west=-1&east=0") assert json_response(conn, 400) == %{"error" => "invalid params"} end + + test "returns 413 for global / oversized viewport bounds", %{conn: conn} do + conn = get(conn, ~p"/scores/cells?band=10000&south=-90&north=90&west=-180&east=180") + body = json_response(conn, 413) + assert body["error"] == "viewport_too_large" + end end end diff --git a/test/microwaveprop_web/controllers/weather_tile_controller_test.exs b/test/microwaveprop_web/controllers/weather_tile_controller_test.exs index 557d03ea..e9ca25e9 100644 --- a/test/microwaveprop_web/controllers/weather_tile_controller_test.exs +++ b/test/microwaveprop_web/controllers/weather_tile_controller_test.exs @@ -165,5 +165,18 @@ defmodule MicrowavepropWeb.WeatherTileControllerTest do File.rm_rf!(hrdps_dir) end + + test "returns 413 for global / oversized viewport bounds", %{conn: conn} do + time = DateTime.to_iso8601(~U[2026-04-28 12:00:00Z]) + + conn = + get( + conn, + ~p"/weather/cells?time=#{time}&south=-90&north=90&west=-180&east=180" + ) + + body = json_response(conn, 413) + assert body["error"] == "viewport_too_large" + end end end diff --git a/test/microwaveprop_web/grid_bounds_test.exs b/test/microwaveprop_web/grid_bounds_test.exs new file mode 100644 index 00000000..ba922a07 --- /dev/null +++ b/test/microwaveprop_web/grid_bounds_test.exs @@ -0,0 +1,38 @@ +defmodule MicrowavepropWeb.GridBoundsTest do + use ExUnit.Case, async: true + + alias MicrowavepropWeb.GridBounds + + describe "clamp/1" do + test "passes nil through" do + assert GridBounds.clamp(nil) == {:ok, nil} + end + + test "passes a small viewport through unchanged" do + bounds = %{"south" => 30.0, "north" => 35.0, "west" => -100.0, "east" => -90.0} + assert {:ok, ^bounds} = GridBounds.clamp(bounds) + end + + test "clamps south/north/west/east to the supported extent" do + bounds = %{"south" => -50.0, "north" => 90.0, "west" => -200.0, "east" => 50.0} + assert {:error, :viewport_too_large} = GridBounds.clamp(bounds) + end + + test "rejects global bounds with 413-equivalent error" do + global = %{"south" => -90.0, "north" => 90.0, "west" => -180.0, "east" => 180.0} + assert GridBounds.clamp(global) == {:error, :viewport_too_large} + end + + test "rejects inverted bounds" do + inverted = %{"south" => 40.0, "north" => 30.0, "west" => -90.0, "east" => -100.0} + assert GridBounds.clamp(inverted) == {:error, :invalid_bounds} + end + + test "rejects malformed bounds" do + assert GridBounds.clamp(%{}) == {:error, :invalid_bounds} + + assert GridBounds.clamp(%{"south" => "x", "north" => 1, "west" => 1, "east" => 1}) == + {:error, :invalid_bounds} + end + end +end diff --git a/test/microwaveprop_web/live/beacon_live_test.exs b/test/microwaveprop_web/live/beacon_live_test.exs index 9547a29d..331efb04 100644 --- a/test/microwaveprop_web/live/beacon_live_test.exs +++ b/test/microwaveprop_web/live/beacon_live_test.exs @@ -89,12 +89,39 @@ defmodule MicrowavepropWeb.BeaconLiveTest do assert html =~ "10,368.1" end - test "shows pending badge on unapproved beacons", %{conn: conn} do - beacon = beacon_fixture(user_fixture()) + test "shows pending badge to the submitter on their own unapproved beacon", %{conn: conn} do + user = user_fixture() + beacon = beacon_fixture(user) + conn = log_in_user(conn, user) {:ok, _view, html} = live(conn, ~p"/beacons/#{beacon}") assert html =~ "Pending approval" end + test "anonymous viewer gets 404 on unapproved beacon", %{conn: conn} do + beacon = beacon_fixture(user_fixture(), callsign: "W5HID") + + assert_raise Ecto.NoResultsError, fn -> + live(conn, ~p"/beacons/#{beacon}") + end + end + + test "logged-in non-owner non-admin gets 404 on unapproved beacon", %{conn: conn} do + beacon = beacon_fixture(user_fixture(), callsign: "W5HID") + conn = log_in_user(conn, user_fixture()) + + assert_raise Ecto.NoResultsError, fn -> + live(conn, ~p"/beacons/#{beacon}") + end + end + + test "admin sees unapproved beacon", %{conn: conn} do + beacon = beacon_fixture(user_fixture(), callsign: "W5ADM") + conn = log_in_user(conn, admin_user_fixture()) + {:ok, _view, html} = live(conn, ~p"/beacons/#{beacon}") + assert html =~ "Pending approval" + assert html =~ "W5ADM" + end + test "plot-path-to-beacon link pre-populates tx_power + gain to match beacon EIRP", %{conn: conn} do # power_mw: 10_000 → EIRP 40 dBm → default-gain 30 dBi → tx 10 dBm @@ -358,12 +385,15 @@ defmodule MicrowavepropWeb.BeaconLiveTest do refute html =~ "src_gain_dbi=" end - test "non-admin unauthenticated user clicking 'approve' is rejected with a flash", + test "submitter clicking 'approve' on their own pending beacon is rejected with a flash", %{conn: conn} do # The approve handler is rendered for admins only but the server # still defends the admin check. Simulate a crafted client pushing - # the event manually with no scope; the flash branch should fire. - pending = beacon_fixture(user_fixture(), callsign: "W5DEF") + # the event manually as the (non-admin) submitter; the flash branch + # should fire and the beacon must remain unapproved. + user = user_fixture() + pending = beacon_fixture(user, callsign: "W5DEF") + conn = log_in_user(conn, user) {:ok, view, _html} = live(conn, ~p"/beacons/#{pending}")