From 3948ae5894f6ca359981aacb42621ea1c2cb26ea Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Fri, 29 May 2026 15:50:23 -0500 Subject: [PATCH] Remove dead packets.html.heex template and create findings.md The unstyled

Packets

template was dead code - no route in the router renders it (/packets uses PacketsLive.Index LiveView instead). Removed the template and its test. Added findings.md with all audit results from comprehensive security and code quality review. --- findings.md | 103 ++++++++++++++++++ .../controllers/page_html/packets.html.heex | 1 - .../aprsme_web/controllers/page_html_test.exs | 12 -- 3 files changed, 103 insertions(+), 13 deletions(-) create mode 100644 findings.md delete mode 100644 lib/aprsme_web/controllers/page_html/packets.html.heex delete mode 100644 test/aprsme_web/controllers/page_html_test.exs diff --git a/findings.md b/findings.md new file mode 100644 index 0000000..f5250a9 --- /dev/null +++ b/findings.md @@ -0,0 +1,103 @@ +# Findings: aprs.me Code Review + +## Security Findings + +### CRITICAL: Production secrets exposed on disk +`.envrc` contains all production secrets in plaintext: APRS credentials, `DATABASE_URL`, `SECRET_KEY_BASE`, `RESEND_API_KEY`, `RELEASE_COOKIE`. Protected by `.gitignore` but accessible to anyone with filesystem access. + +### CRITICAL: APRS credentials in K8s manifest +`k8s/deployment.yaml:64-67,136-143` — APRS callsign and password hardcoded as plaintext env vars instead of Kubernetes Secrets. + +### HIGH: Secret key base in git history +`config/dev.exs:48`, `config/test.exs:40` — Development and test `secret_key_base` values committed since initial commit. + +### HIGH: SQL injection vector +`lib/aprsme/release.ex:162` — `SQL.query!` uses string interpolation with env var input: +```elixir +SQL.query!(repo, "SET statement_timeout = '#{timeout_seconds}s'", []) +``` + +### MEDIUM: Session signed but not encrypted +`lib/aprsme_web/endpoint.ex:7-13` — Session configured with `signing_salt` only, no `encryption_salt`. Session contents can be base64-decoded by anyone with cookie access. + +### MEDIUM: `raw()` HTML rendering in LiveView +`lib/aprsme_web/live/info_live/show.ex:537,542,545` — APRS symbol rendering uses `raw()` to inject HTML. While content is escaped, `raw()` bypasses HEEx auto-escaping at template level. + +### MEDIUM: `raw()` SVG rendering +`lib/aprsme_web/components/core_components.ex:50` — SVG content rendered via `raw()`. Risk is low (files are controlled vendored assets). + +### MEDIUM: `check_origin: false` in dev +`config/dev.exs:45` — WebSocket origin checks disabled in development. + +### MEDIUM: Anonymous WebSocket connections +`lib/aprsme_web/channels/mobile_user_socket.ex:21-29` — Mobile socket requires no authentication. Rate-limited to 30/min/IP but otherwise open. + +### MEDIUM: No auth on API endpoints +`lib/aprsme_web/router.ex:131-136` — API v1 (`/api/v1/callsign/:cs`, `/api/v1/weather/nearby`) is anonymous. Rate-limited but no API keys. + +### MEDIUM: APRS-IS connection uses plain TCP +`lib/aprsme/is/is.ex:215` — APRS passcode sent in cleartext over `:gen_tcp` (no SSL). + +### LOW: No `secure` flag on session cookie +`lib/aprsme_web/endpoint.ex:8-13` — No `secure: true` in session options. Handled by Phoenix in practice but not explicit. + +### LOW: Password policy too lax +`lib/aprsme/accounts/user.ex:73-75` — Character class requirements are commented out. Passwords only require 12+ characters. + +### LOW: Rate limiting not shared across cluster +`config/runtime.exs:133` — `Hammer.Backend.ETS` is per-node. Multi-node deployments can exhaust per-node limits independently. + +### INFO: CSP allows `unsafe-inline` and `unsafe-eval` +`lib/aprsme_web/router.ex:23-26` — Content Security Policy allows both, weakening XSS protection. + +### INFO: 27+ Sobelow findings pre-skipped +`.sobelow-skips` — Reviewed and deemed acceptable (parameterized spatial queries, symbol rendering, upstream TLS). + +--- + +## Code Quality & Refactoring Findings + +### HIGH: `map_live/index.ex` is 2,104 lines +Largest module in the project. Combines socket management, event handling, rendering, map state, and overlay logic. Extract into focused sub-modules (map_state, map_events, map_render). + +### HIGH: Duplicated `has_weather` logic +`lib/aprsme/packet.ex:159` and `lib/aprsme/packet_consumer.ex` both maintain parallel `set_has_weather/1` logic. If weather detection rules change, one path will be missed. + +### HIGH: 17 modules missing `@moduledoc` +Modules lacking documentation include: `Packet`, `PacketConsumer`, `DataExtended`, `BadPacket`, `Accounts.User`, `Accounts.UserNotifier`, `Packets`, and others. + +### MEDIUM: `require Logger` in function bodies (~75 occurrences) +Several modules duplicate `require Logger` at module level AND inside function bodies (e.g., `packets.ex:17,27`, `packet_consumer.ex`). Consolidate to module top-level. + +### MEDIUM: `packets.ex` is 849 lines +Mix of query functions, store logic, and validation. Consider splitting query/concerns into separate modules. + +### MEDIUM: Cache module has unnecessary GenServer overhead +`lib/aprsme/cache.ex` — Wraps ETS operations in GenServer calls. GenServer passes through `%{}` state with no meaningful state management. Direct ETS access would be faster for get/put. + +### MEDIUM: Encoding vs EncodingUtils boundary unclear +`encoding_utils.ex` (404 lines) wraps Gleam-based `Encoding` module but also contains substantive logic (`sanitize_packet`, `normalize_data_type`). Clarify responsibility boundary. + +### MEDIUM: Inline `import Ecto.Query` in function bodies +16 occurrences across `mobile_channel.ex`, `packet_utils.ex`, `info_live/show.ex`, `packets.ex`. Move to module top-level. + +### MEDIUM: Large inline SQL in LiveViews +`info_live/show.ex:369,392,484,491` — Complex ST_Distance calculations embedded in LiveView. Extract to query modules. + +### LOW: Geometry stored redundantly +`packet.ex` stores both scalar `lat`/`lon` AND PostGIS `location` geometry. `maybe_create_geometry_from_lat_lon/1` duplicates the data. Consider if both are needed. + +### LOW: Behaviour only covers subset of functions +`packets_behaviour.ex` defines 8 callbacks but `packets.ex` has far more public functions. Only partial mockability. + +### LOW: Test coverage at 91.13% (threshold 87%) +Only 4.13% above threshold. Focus on uncovered error/rescue paths and edge cases. + +### LOW: Performance TODOs not tracked in code +`TODO.md` items (table partitioning, ETS write path) not marked as `# TODO` in source code. Won't appear in IDE task lists. + +--- + +## Already Fixed + +- **`packets.html.heex` (unstyled `

`)**: Dead template with bare `

Packets

` removed. The `/packets` route uses `PacketsLive.Index` LiveView, not this controller template. diff --git a/lib/aprsme_web/controllers/page_html/packets.html.heex b/lib/aprsme_web/controllers/page_html/packets.html.heex deleted file mode 100644 index 70ae620..0000000 --- a/lib/aprsme_web/controllers/page_html/packets.html.heex +++ /dev/null @@ -1 +0,0 @@ -

Packets

diff --git a/test/aprsme_web/controllers/page_html_test.exs b/test/aprsme_web/controllers/page_html_test.exs deleted file mode 100644 index 253e11c..0000000 --- a/test/aprsme_web/controllers/page_html_test.exs +++ /dev/null @@ -1,12 +0,0 @@ -defmodule AprsmeWeb.PageHTMLTest do - use ExUnit.Case, async: true - - import Phoenix.LiveViewTest - - alias AprsmeWeb.PageHTML - - test "packets/1 renders the packets template" do - html = render_component(&PageHTML.packets/1, %{}) - assert html =~ "Packets" - end -end