Graham McIntire
3948ae5894
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.
103 lines
5.6 KiB
Markdown
103 lines
5.6 KiB
Markdown
# 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 `<h1>`)**: Dead template with bare `<h1>Packets</h1>` removed. The `/packets` route uses `PacketsLive.Index` LiveView, not this controller template.
|