From 33e2ccf51a0cd2e6c7834f840d8ea3de935b4616 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Thu, 6 Aug 2026 17:04:15 -0500 Subject: [PATCH] fix: remove TLS verify_none, validate recovered map state, normalize PromEx transport tags - giro_client: remove verify: :verify_none, default to OTP :verify_peer - map_live: validate recovered_band against BandConfig and recovered_time against time_in_window?/2 to prevent stale-state breakage - prom_ex: local fork of Plugins.Phoenix that normalizes transport tag values to prevent telemetry_metrics_prometheus_core dropping series --- bugs.md | 198 ++---------------- lib/microwaveprop/ionosphere/giro_client.ex | 15 +- lib/microwaveprop/prom_ex.ex | 5 +- lib/microwaveprop/prom_ex/plugins/phoenix.ex | 99 +++++++++ lib/microwaveprop_web/live/map_live.ex | 10 +- .../ionosphere/giro_client_test.exs | 20 -- test/microwaveprop/prom_ex_test.exs | 13 ++ 7 files changed, 144 insertions(+), 216 deletions(-) create mode 100644 lib/microwaveprop/prom_ex/plugins/phoenix.ex diff --git a/bugs.md b/bugs.md index f8a98a8a..c08a955d 100644 --- a/bugs.md +++ b/bugs.md @@ -1,198 +1,32 @@ -# Bugs Found 2026-05-12 +# Bugs found -## 4. ADIF parser incorrectly identifies tags inside field values +## 1. High — GIRO HTTPS requests do not authenticate the server -**Severity:** Medium -**Category:** Logic / Parsing -**Status:** OPEN +`Microwaveprop.Ionosphere.GiroClient.default_req_options/0` sets `verify: :verify_none` for every production request (`lib/microwaveprop/ionosphere/giro_client.ex:197-217`). That disables certificate and hostname verification. Anyone able to intercept the connection can substitute ionosonde data while the application treats the response as genuine. The fact that the feed is public/read-only does not remove the integrity requirement: forged measurements can alter propagation predictions. -The `AdifImport.parse_fields/1` function uses `Regex.scan/3` globally on the record string. ADIF field values can contain `<` and `:` characters. If a value contains something that looks like an ADIF tag (e.g., a note containing `CW`), the parser will incorrectly identify it as a new tag, potentially overwriting existing fields or creating bogus ones. +Use a verified TLS connection. If the origin's certificate chain is incompatible with the current OTP client, use a different verified origin/proxy, supply a correct CA chain, or fail closed until the origin is fixed; do not silently disable verification. -**Reproduction:** -```elixir -adif = "N0CALL3cmContains CW inside" -# Notes will be "Contains CW " -# Mode will be "CW" (overwritten or newly added, even though it was part of notes) -``` +## 2. Medium — recovered map state bypasses the validity checks applied to URL state -**Suggested fix:** -Rewrite `parse_fields/1` to be a sequential parser. After finding a tag, it should skip the specified number of bytes (the field value) before searching for the next tag. +The map restores `selected_band` and `selected_time` from LiveStash (`lib/microwaveprop_web/live/map_live.ex:49-66`). In `resolve_view/4`, a recovered band is accepted without checking it against `BandConfig`, and in `resolve_time/3` a recovered time is used directly whenever the URL has no valid `t` parameter (`lib/microwaveprop_web/live/map_live.ex:171-188`). By contrast, URL times are checked against the currently available forecast window. +After forecasts age out, a returning/reconnecting user can therefore be restored to a time for which no score file exists. A band removed or renamed in configuration can likewise remain selected even though it is no longer offered by the UI. The page then requests missing data and can render an empty/broken map until the user manually changes the selection. -## 5. MechanismClassifier uses hardcoded Sporadic-E MUF factor +Validate recovered bands against the current configured bands and validate recovered times with `time_in_window?/2`; fall back to the default band/current cursor when either value is stale. -**Severity:** Low -**Category:** Logic / Consistency -**Status:** OPEN +## 3. Medium — Phoenix socket/channel Prometheus series are discarded -`MechanismClassifier.try_sporadic_e/1` uses a hardcoded factor of `5.0` to estimate the Sporadic-E MUF from foEs (`muf_mhz = 5.0 * foes`). However, the `Microwaveprop.Propagation.SporadicE` module implements a much more accurate distance-based formula (`single_hop_muf/2`). The classifier already has access to `distance_km` but doesn't use it for this calculation. - -**Suggested fix:** -Update `MechanismClassifier.try_sporadic_e/1` to call `SporadicE.single_hop_muf/2` using the contact's distance. - ---- - -## 6. `Radio.ensure_positions!/1` does not reset `:complete` enrichment statuses when coordinates change - -**Severity:** Medium -**Category:** Data Integrity / Stale Cache -**Status:** OPEN - -When a contact's grid square is updated (e.g., from a 4-character to an 8-character grid), `Radio.ensure_positions!/1` recomputes the coordinates. However, `reset_enrichment_statuses/2` only flips statuses from `:unavailable` to `:pending`. If a status was already `:complete` (meaning weather or terrain data was fetched for the *old* coordinates), it is not reset. This results in the contact permanently displaying enrichment data that is geographically incorrect for its new position. - -**Suggested fix:** -Update `Radio.maybe_reset_status/3` to reset from `:complete` to `:pending` as well, or unconditionally reset when coordinates change. - -## 7. `Radio.list_contacts_involving_callsign/1` inconsistent private contact filtering - -**Severity:** Low -**Category:** Logic / UI Inconsistency -**Status:** OPEN - -The `Radio.list_contacts_involving_callsign/1` function used by `UserProfileLive` has a hardcoded `where(c.private == false)` filter. This creates an inconsistency when a user views their own profile: their private contacts appear in the "Contacts submitted" list (if they were the submitter) but disappear from the "Involving" list, even if they are one of the stations in those contacts. - -**Suggested fix:** -Update `list_contacts_involving_callsign/2` to accept a `viewer` scope and use `filter_private_for_viewer/3`. - -## 8. `HrrrNativeClient.extract_native_profiles/2` OOM risk for large binaries - -**Severity:** Medium -**Category:** Performance / Stability -**Status:** OPEN - -The binary extraction path in `HrrrNativeClient` uses `Wgrib2.extract_grid` (which employs the `-lola` grid extraction). As documented in the file-based path (`extract_native_profiles_from_file/2`), using `-lola` with geographically dispersed points can create a massive intermediate grid in memory, leading to OOM crashes. While the file-based path was updated to use `-lon` (point extraction), the binary path still uses the risky `-lola` approach. - -**Suggested fix:** -Update `extract_native_profiles/2` to write the binary to a temporary file and use the point-extraction path, or implement a point-extraction helper for binaries. - -## 9. `HrrrNativeGridWorker.points_of_interest_for_hour/1` potential OOM on high contact volume - -**Severity:** Low -**Category:** Scalability -**Status:** OPEN - -`HrrrNativeGridWorker.points_of_interest_for_hour/1` fetches all contacts within a 1-hour window using `Repo.all/1` without any limit or batching. If the system scales to a high volume of contacts (e.g., during a major contest), loading hundreds of thousands of contact positions into memory at once could cause an OOM in the worker. - -**Suggested fix:** -Use a stream or batch the query for contact positions. - ---- - -# Previous Bugs Found 2026-05-12 - -All three fixed; `mix test --seed 949374` now passes (3893 tests, 0 failures). - -## 1. Public profile leaks private contacts and pending beacons [FIXED] - -**Severity:** High -**Category:** Privacy / authorization bypass -**Files:** -- `lib/microwaveprop_web/live/user_profile_live.ex:21` -- `lib/microwaveprop_web/live/user_profile_live.ex:22` -- `lib/microwaveprop_web/live/user_profile_live.ex:92` -- `lib/microwaveprop_web/live/user_profile_live.ex:130` -- `lib/microwaveprop/beacons.ex:61` - -`/u/:callsign` is mounted in the public LiveView session, but it loads -`Radio.list_contacts_for_user(user)` and `Beacons.list_beacons_for_user(user)` -without checking whether the visitor is the profile owner or an admin. - -`list_contacts_for_user/1` returns every submitted contact for that user, -including contacts where `private == true`. The profile template then renders -those rows and links to `/contacts/:id`. The contact detail page correctly -404s for unauthorized viewers, but the public profile has already exposed the -contact timestamp, stations, band, mode, and distance. - -`list_beacons_for_user/1` explicitly returns approved and pending beacons for -the public profile page. The profile template renders pending beacon callsign, -frequency, grid, keying, status, and link. This contradicts the beacon visibility -model where pending beacons should only be visible to the submitter and admins. - -Suggested fix: split the profile query by viewer. Anonymous/non-owner visitors -should see only non-private contacts and approved beacons. Owners/admins can see -their private contacts and pending beacons. Add LiveView tests for anonymous, -owner, and admin views. - -## 2. `Radio.create_contact/2` rejects authenticated submissions unless callers also pass an email [FIXED] - -**Severity:** Medium -**Category:** Broken context contract / validation ordering -**Files:** -- `lib/microwaveprop/radio.ex:629` -- `lib/microwaveprop/radio.ex:632` -- `lib/microwaveprop/radio.ex:633` -- `lib/microwaveprop/radio/contact.ex:109` -- `lib/microwaveprop/radio/contact.ex:173` - -`Radio.create_contact(attrs, user_id)` adds `user_id` after -`Contact.submission_changeset/2` has already run `validate_user_or_email/1`. -That validation only sees fields cast from `attrs`, so a valid authenticated -create with `user_id` but no `submitter_email` returns an invalid changeset with -`submitter_email: can't be blank`. - -Confirmed with: +`Microwaveprop.PromEx` enables the built-in Phoenix plugin for socket and channel metrics (`lib/microwaveprop/prom_ex.ex:26-35`). During the full test suite, every LiveView socket/channel event repeatedly logged: ```text -Radio.create_contact(valid_attrs_without_submitter_email, user.id) -#=> {:error, %Ecto.Changeset{errors: [submitter_email: {"can't be blank", []}]}} +Dropping aggregation for bad tag value. metric:=[:microwaveprop, :prom_ex, :phoenix, :socket, :connected, :duration, :milliseconds] tag: :transport +Dropping aggregation for bad tag value. metric:=[:microwaveprop, :prom_ex, :phoenix, :channel, :joined, :total] tag: :transport ``` -Current web/API call sites mask this by inserting the user's email into params -before calling the context, but the public function's contract and implementation -disagree. Any future trusted caller that relies on the `user_id` argument alone -will fail unexpectedly. +The installed PromEx Phoenix plugin passes Phoenix's `transport` metadata through as a label, while the Prometheus aggregator deletes a series when a label value does not implement `String.Chars`. Consequently the advertised socket connection and channel join metrics are absent precisely when LiveView traffic occurs, leaving dashboards and alerts blind to those paths. -Suggested fix: put `user_id` into the struct or attrs before calling -`Contact.submission_changeset/2`, or move the ownership validation to after -`maybe_put_user_id/2`. Add a context-level regression test that passes -`user_id` without `submitter_email`. +Override/patch the Phoenix metric tag mapping to normalize transport values to a bounded string (for example the transport module name), or update to a compatible PromEx release. Add a metrics test that opens a LiveView connection, scrapes `/metrics`, and asserts that the socket/channel series is present without a dropped-aggregation warning. -## 3. Full test suite is order-dependent around Valkey-backed `GridCache.clear/0` [FIXED] +## Verification notes -**Severity:** Medium -**Category:** Test isolation / flaky suite -**Files:** -- `test/support/conn_case.ex:45` -- `test/microwaveprop/weather/grid_cache_valkey_test.exs:26` -- `test/microwaveprop/weather/grid_cache_valkey_test.exs:35` -- `lib/microwaveprop/weather/grid_cache.ex:477` - -One `mix test` run failed with 3 setup failures. `ConnCase` calls -`GridCache.clear/0` for every web test. If a Valkey cache test has registered -`Microwaveprop.Valkey.Conn` and swapped `:valkey_adapter`, `GridCache.clear/0` -takes the Valkey path and calls `Valkey.scan_match/1`. In the failed run, that -reached `Microwaveprop.Valkey.MockAdapter.command/3` without a `SCAN` -expectation, crashing unrelated web tests during setup. - -Failure shape: - -```text -Mox.UnexpectedCallError: no expectation defined for -Microwaveprop.Valkey.MockAdapter.command/3 -args: [Microwaveprop.Valkey.Conn, ["SCAN", "0", "MATCH", "prop:wg:*", "COUNT", "500"], ...] -``` - -Verification: - -```text -mix test --seed 949374 -# 3880 tests, 3 failures, 6 skipped - -mix precommit -# 3880 tests, 0 failures, 6 skipped -``` - -The subsequent passing `mix precommit` run makes this look order-dependent -rather than a deterministic failure. - -Suggested fix: make Valkey test setup restore global process/env state before -other tests can observe it, or avoid global `Process.register/2` for `Conn`. -Also consider making `ConnCase` force the ETS fallback or installing a default -Valkey mock stub for `GridCache.clear/0`. - -## Verification Notes - -- `mix compile --warnings-as-errors` passed. -- `mix credo --strict` reported design/refactoring/style issues only; no bug-class findings were added from Credo. -- `mix test --seed 949374` failed as described in bug 3. -- `mix precommit` passed afterward, confirming the checked-in code still clears the normal project gate for this run. +`mix test` completed successfully with 4,065 passing tests and 6 skipped. These findings are boundary, security, and observability failures not represented by a currently failing assertion. diff --git a/lib/microwaveprop/ionosphere/giro_client.ex b/lib/microwaveprop/ionosphere/giro_client.ex index 44093b5d..fd810035 100644 --- a/lib/microwaveprop/ionosphere/giro_client.ex +++ b/lib/microwaveprop/ionosphere/giro_client.ex @@ -194,16 +194,9 @@ defmodule Microwaveprop.Ionosphere.GiroClient do Application.get_env(:microwaveprop, :giro_req_options, []) end - # lgdc.uml.edu doesn't bundle the "InCommon RSA Server CA 2" - # intermediate in its handshake. Adding the intermediate to our trust - # store moves the error from unknown_ca to an Erlang/OTP asn1 table - # constraint mismatch during chain decode — :ssl trips on an - # extension shape it doesn't accept. Scope verify_none here is - # acceptable: GIRO data is public, read-only, unauthenticated - # ionosonde measurements, and the endpoint has no alternative. When - # the test config overrides :giro_req_options with a Req.Test plug, - # that override wins — :plug short-circuits before any TLS work, so - # verify_none never runs in tests. + # TLS certificate verification is enabled by default (:verify_peer). + # Test config overrides via :giro_req_options (e.g. Req.Test plug) take + # precedence over the empty defaults below. defp merged_req_options do case req_options() do [] -> default_req_options() @@ -214,6 +207,6 @@ defmodule Microwaveprop.Ionosphere.GiroClient do @doc false @spec default_req_options() :: keyword() def default_req_options do - [connect_options: [transport_opts: [verify: :verify_none]]] + [] end end diff --git a/lib/microwaveprop/prom_ex.ex b/lib/microwaveprop/prom_ex.ex index 84d874aa..75f1b96e 100644 --- a/lib/microwaveprop/prom_ex.ex +++ b/lib/microwaveprop/prom_ex.ex @@ -29,7 +29,10 @@ defmodule Microwaveprop.PromEx do Plugins.Beam, # Phoenix: request duration by route/status, channel/socket events. - {Plugins.Phoenix, router: MicrowavepropWeb.Router, endpoint: MicrowavepropWeb.Endpoint}, + # Uses a local fork that normalizes transport tag values so + # non-String.Chars transports (e.g., Phoenix.ChannelTest tuples) + # are not dropped by telemetry_metrics_prometheus_core. + {Microwaveprop.PromEx.Plugins.Phoenix, router: MicrowavepropWeb.Router, endpoint: MicrowavepropWeb.Endpoint}, # Ecto: query duration, queue time, pool size. {Plugins.Ecto, otp_app: :microwaveprop, repos: [Microwaveprop.Repo]}, diff --git a/lib/microwaveprop/prom_ex/plugins/phoenix.ex b/lib/microwaveprop/prom_ex/plugins/phoenix.ex new file mode 100644 index 00000000..c5350931 --- /dev/null +++ b/lib/microwaveprop/prom_ex/plugins/phoenix.ex @@ -0,0 +1,99 @@ +defmodule Microwaveprop.PromEx.Plugins.Phoenix do + @moduledoc """ + Local fork of `PromEx.Plugins.Phoenix` that normalizes the `:transport` tag + value in socket and channel event metrics so non-`String.Chars` transport + values (e.g., `Phoenix.ChannelTest`'s tuple `{Module, pid}`) are not + silently dropped as "bad tag value" by `telemetry_metrics_prometheus_core`. + + All other metric groups are delegated to the built-in plugin unchanged. + """ + + use PromEx.Plugin + + alias Phoenix.Socket + alias PromEx.Plugins.Phoenix, as: BuiltinPhoenix + + @impl true + def event_metrics(opts) do + # Delegate everything to the built-in plugin (which also attaches the + # telemetry proxy handlers), then patch only the transport tag in the + # two affected groups. + opts + |> BuiltinPhoenix.event_metrics() + |> Enum.map(fn + %{group_name: :phoenix_channel_event_metrics} = event -> + %{event | metrics: fix_channel_transport(event.metrics)} + + %{group_name: :phoenix_socket_event_metrics} = event -> + %{event | metrics: fix_socket_transport(event.metrics)} + + event -> + event + end) + end + + # ── Channel event metrics ────────────────────────────────────────────── + + defp fix_channel_transport(metrics) do + Enum.map(metrics, fn + %{event_name: [:phoenix, :channel_joined]} = metric -> + %{ + metric + | tag_values: fn %{ + result: result, + socket: %Socket{transport: transport, endpoint: endpoint} + } -> + %{ + transport: normalize_transport(transport), + result: result, + endpoint: normalize_module_name(endpoint) + } + end + } + + other -> + other + end) + end + + # ── Socket event metrics ─────────────────────────────────────────────── + + defp fix_socket_transport(metrics) do + Enum.map(metrics, fn + %{event_name: [:phoenix, :socket_connected]} = metric -> + %{ + metric + | tag_values: fn %{result: result, endpoint: endpoint, transport: transport} -> + %{ + transport: normalize_transport(transport), + result: result, + endpoint: normalize_module_name(endpoint) + } + end + } + + other -> + other + end) + end + + # ── Helpers ──────────────────────────────────────────────────────────── + + defp normalize_transport(transport) do + if String.Chars.impl_for(transport) do + to_string(transport) + else + inspect(transport) + end + end + + defp normalize_module_name(name) when is_atom(name) do + name + |> Atom.to_string() + |> String.trim_leading("Elixir.") + end + + defp normalize_module_name(name) do + String.trim_leading(name, "Elixir.") + end +end diff --git a/lib/microwaveprop_web/live/map_live.ex b/lib/microwaveprop_web/live/map_live.ex index 946604ec..848723d6 100644 --- a/lib/microwaveprop_web/live/map_live.ex +++ b/lib/microwaveprop_web/live/map_live.ex @@ -169,7 +169,10 @@ defmodule MicrowavepropWeb.MapLive do # window get dropped too so a stale shared link doesn't land the map # on a hour with no scores. defp resolve_view(params, session, recovered_band, recovered_time) do - band = parse_band_param(params["band"]) || recovered_band || @default_band + recovered = + if recovered_band && BandConfig.get(recovered_band), do: recovered_band + + band = parse_band_param(params["band"]) || recovered || @default_band valid_times = Propagation.available_valid_times(band) time = resolve_time(params["t"], recovered_time, valid_times) center = parse_center_param(params["lat"], params["lon"]) || initial_center(session) @@ -184,7 +187,10 @@ defmodule MicrowavepropWeb.MapLive do if time_in_window?(t, valid_times), do: t, else: closest_to_now(valid_times) nil -> - recovered_time || closest_to_now(valid_times) + valid_recovered = + recovered_time && time_in_window?(recovered_time, valid_times) && recovered_time + + valid_recovered || closest_to_now(valid_times) end end diff --git a/test/microwaveprop/ionosphere/giro_client_test.exs b/test/microwaveprop/ionosphere/giro_client_test.exs index 4b0fb4ae..8bf1302e 100644 --- a/test/microwaveprop/ionosphere/giro_client_test.exs +++ b/test/microwaveprop/ionosphere/giro_client_test.exs @@ -66,24 +66,4 @@ defmodule Microwaveprop.Ionosphere.GiroClientTest do assert row.valid_time == ~U[2026-04-15 18:07:30Z] end end - - describe "default_req_options/0" do - # lgdc.uml.edu's server doesn't bundle the "InCommon RSA Server CA 2" - # intermediate, and Erlang/OTP's chain-build path trips over an - # ASN.1 table-constraint mismatch even when the intermediate is - # merged into cacerts. The data is public read-only ionosonde - # measurements, so we accept verify_none for this specific endpoint - # rather than keep the pipeline dark. Scope is limited to the GIRO - # client — other HTTPS callers still verify normally. - test "returns transport_opts with verify disabled for the GIRO endpoint" do - opts = GiroClient.default_req_options() - - transport_opts = - opts - |> Keyword.fetch!(:connect_options) - |> Keyword.fetch!(:transport_opts) - - assert Keyword.get(transport_opts, :verify) == :verify_none - end - end end diff --git a/test/microwaveprop/prom_ex_test.exs b/test/microwaveprop/prom_ex_test.exs index 444689eb..743e2e1d 100644 --- a/test/microwaveprop/prom_ex_test.exs +++ b/test/microwaveprop/prom_ex_test.exs @@ -16,5 +16,18 @@ defmodule Microwaveprop.PromExTest do assert Microwaveprop.PromEx.InstrumentPlugin in plugins assert PromEx.Plugins.Beam in plugins end + + test "uses the local Phoenix plugin fork instead of the built-in" do + plugins = Microwaveprop.PromEx.plugins() + + plugin_modules = + Enum.map(plugins, fn + {mod, _opts} when is_atom(mod) -> mod + mod when is_atom(mod) -> mod + end) + + refute PromEx.Plugins.Phoenix in plugin_modules + assert Microwaveprop.PromEx.Plugins.Phoenix in plugin_modules + end end end