Graham McIntire
5ca5edd554
- mechanism_classifier: add nil guards on get_lat/get_lon (no crash on bad radar lookup) - path_compute: use Map.get with fallback for abs_humidity (no nil * dist_km crash) - beacon_live/index: fix path_with_prefix double-slash with trim_leading - map_live: inline flash rendering instead of calling Layouts.flash_group - runtime.exs: raise on missing EMAIL_SERVER (prevents silent TLS failure) - notify_listener: wrap Task.start in try/rescue (prevents linked crash on GenServer) - notify_listener: add Process.monitor on PG notifications (detect connection loss) - findings.md: remove fixed high-severity items
85 lines
6 KiB
Markdown
85 lines
6 KiB
Markdown
# Findings — Bugs & Improvements (Non-Critical)
|
|
|
|
## Bugs
|
|
|
|
### 🟡 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 filters on `band`, `qso_timestamp`, `flagged_invalid` with no covering index | Add partial index on `(band, qso_timestamp) WHERE flagged_invalid = false` |
|
|
| 3 | `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()` |
|
|
| 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 — 12 passes over 9 profiles for what could be 1 `Enum.reduce` | Single pass with `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]` |
|
|
|
|
### 🟢 Low
|
|
|
|
| # | File | Line | Issue | Suggested Fix |
|
|
|---|------|------|-------|---------------|
|
|
| 12 | `router.ex` | 165 | Login rate limit 30/min per IP may be generous for brute-force | Consider lowering to 10-15/min |
|
|
| 13 | `application.ex` | 107-109 | Dev/test model loading blocks supervisor start | Wrap in `Task.start` |
|
|
| 14 | `scorer.ex` | 126-138 | `classify_time_period` guard boundaries have gap at exactly `-3.0` | Switch to `cond` with inclusive/exclusive clarity |
|
|
| 15 | `scorer.ex` | 636 | `||` for default when `//` is more intention-revealing | Replace `contact.pos1["lon"] \|\| -97.0` with `//` |
|
|
| 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 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:31` — `conus_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) |
|