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.
This commit is contained in:
parent
d0b9513b41
commit
3948ae5894
3 changed files with 103 additions and 13 deletions
103
findings.md
Normal file
103
findings.md
Normal file
|
|
@ -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 `<h1>`)**: Dead template with bare `<h1>Packets</h1>` removed. The `/packets` route uses `PacketsLive.Index` LiveView, not this controller template.
|
||||
|
|
@ -1 +0,0 @@
|
|||
<h1>Packets</h1>
|
||||
|
|
@ -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
|
||||
Loading…
Add table
Reference in a new issue