prop/findings.md
Graham McIntire 3bdc4b6a11
Fix 4 critical bugs and document remaining findings
- TOCTOU race: add partial unique index + rescue constraint violation on
  concurrent contact insert (radio.ex:737-768)
- Mass assignment: remove :user_id from @optional_fields (contact.ex:85)
- Stale path_live result: clear result when URL params change (path_live.ex:105)
- Blocking migrations: wrap Release.migrate() in Task.start (application.ex:79)
- Rate limiter: fix guard for monitor tokens (not is_nil vs is_binary)
- findings.md: document remaining non-critical bugs and improvements
2026-05-29 15:27:33 -05:00

7.5 KiB

Findings — Bugs & Improvements (Non-Critical)

Bugs

🟠 High

# File Line Issue Suggested Fix
1 mechanism_classifier.ex 314-317 get_lat/get_lon crash on nil — nil / 1.0 raises ArithmeticError if radar lookup returns nil coords Add pattern match for nil or guard before division
2 path_compute.ex 433 conditions.abs_humidity nil crash — nil * dist_km raises if conditions map is missing :abs_humidity Use Map.get(conditions, :abs_humidity, 7.5) with fallback
3 beacon_live/index.ex 119 path_with_prefix creates //beacons URLs — "/" <> path doubles slash on already-prefixed paths String.trim_leading(path, "/")
4 map_live.ex 1053 flash_group called outside layouts.ex — violates AGENTS.md rule Include equivalent in-line FlashGroup component or restructure to use Layouts.app
5 config/runtime.exs 142, 386 EMAIL_SERVER not validated — unset produces empty SNI causing silent TLS handshake failure Raise at boot when required env vars are missing (like DATABASE_URL does)
6 notify_listener.ex 119-126 Task.start/1 linked to caller — crash in task kills the GenServer, contradicting moduledoc Use Task.Supervisor.start_child or wrap body in try/rescue
7 notify_listener.ex 36, 40-51 No Process.monitor on Postgrex.Notifications — if PG notify connection crashes, GenServer never learns Add Process.monitor and handle {:DOWN, ...} in handle_info

🟡 Medium

# File Line Issue Suggested Fix
8 radio.ex 128, 157 N+1 queries — list_contacts_for_user and list_contacts_involving_callsign don't preload :user Add `
9 radio.ex 751-757 Missing indexes — dedup query filters on band, qso_timestamp, flagged_invalid with no covering index Add partial index on (band, qso_timestamp) WHERE flagged_invalid = false
10 grid.ex 31-36 conus_points/0 recomputes ~94k elements on every call — pure function, never memoized Memoize with module attribute @conus_points conus_points()
11 endpoint.ex 7-12 Session cookie is signed but not encrypted — payload is readable though tamper-proof Add encryption_salt to @session_options
12 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)
13 profiles_file.ex 169-177 Unhandled :zlib.gunzip/1 exception — corrupt gzip files raise through cache wrapper Wrap in try/rescue
14 scores_file.ex 515 Direct ETS match_delete bypassing Cache API Use Cache.invalidate or Cache public API
15 profiles_file.ex 323 Same direct ETS access as above Same fix
16 path_compute.ex 381-394 Six redundant list traversals — 12 passes over 9 profiles for what could be 1 Enum.reduce Single pass with Enum.reduce
17 hf_muf.ex 51 sec_i_factor(@ref_distance_km) recomputed on every call — constant input Precompute with module attribute
18 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]

🟢 Low

# File Line Issue Suggested Fix
19 router.ex 165 Login rate limit 30/min per IP may be generous for brute-force Consider lowering to 10-15/min
20 application.ex 107-109 Dev/test model loading blocks supervisor start Wrap in Task.start
21 scorer.ex 126-138 classify_time_period guard boundaries have gap at exactly -3.0 Switch to cond with inclusive/exclusive clarity
22 scorer.ex 636 `
23 path_compute.ex 356-357 Eager Repo.get(Station, ...) instead of preloading assoc Preload :station upstream
24 radio.ex 1203 Hardcoded cache key {ContactMapController, :gzipped_payload} fragile to module rename Extract to a named key in ContactMapController
25 user.ex Missing has_many :contacts and has_many :beacons associations Add for convenience (not a bug, test callers use raw queries)
26 radio.ex 337 Missing partial indexes for enrichment queries Create filtered indexes on (qso_timestamp) WHERE weather_status IN ('pending','failed')

Improvements

Architecture & Design

# 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

Module Status
lib/microwaveprop/ionosphere.ex Untested
lib/microwaveprop/mailer.ex Untested
lib/microwaveprop/repo.ex Untested
lib/microwaveprop/space_weather.ex Untested
about_live.ex Real DB query logic, untested

Config & Tooling

# Finding Suggested Fix
1 LIVE_VIEW_SIGNING_SALT hardcoded in config.exs, not read from env Read from System.get_env in runtime.exs
2 :precommit uses deps.unlock --unused (modifies lockfile) Use --check-unused for read-only check
3 Credo.Check.Warning.UnsafeToAtom disabled Re-enable to catch String.to_atom(user_input) DOS vector
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

# Area Impact
1 grid.ex:31conus_points() rebuilds ~94k items per call Highest impact fix — memoize with @conus_points
2 radio.ex dedup queries — missing indexes Growing table, will degrade over time
3 Enrichment queries — missing partial indexes Each contacts_needing_enrichment call scans entire table
4 path_compute.ex:381-394 — 12 list traversals for 9-element profile lists Small today, but unnecessary overhead in hot path

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)