Remove dead packets.html.heex template and create findings.md

The unstyled <h1>Packets</h1> 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.

2026-05-29 15:50:23 -05:00

5.6 KiB

Raw Blame History

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:

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 <h1>): Dead template with bare <h1>Packets</h1> removed. The /packets route uses PacketsLive.Index LiveView, not this controller template.

5.6 KiB Raw Blame History