fix: address bugs from second code audit #39

Merged
graham merged 4 commits from fix/audit-round-2 into main 2026-10-04 22:45:53 +00:00
Owner

Second-round audit across the backend pipeline, LiveViews, web layer, and frontend. All findings verified against the code; each fix is in the commit message.

Highest impact:

  • PacketBatcher.latest_by_sender grew unboundedly per map tab — now capped at 5,000 senders with a 500-entry prune margin (regression test included)
  • PostgresNotifier could crash-loop the whole node on a DB outage — sync_connect: false + auto_reconnect: true
  • ShutdownHandler deadlocked on SIGTERM so System.stop(0) never ran — k8s always SIGKILLed at the grace period
  • Failed map tile loads tripped the LiveView error boundary and hid the map
  • LiveView rejoin never re-sent map_ready — the map stayed silent until the user panned
  • handle_params re-ran the full historical load on every pan (double queries + clear-and-redraw)
  • Info page "stations heard by" query could never return rows (first_digipeater = $1 vs $1 || *)
  • Dew point used the Celsius approximation on °F data; metric locales got hard-coded English imperial labels
  • Source-wide login bucket was reset on any successful sign-in, unbounding password spraying
  • IPv6 clients could rotate /64 addresses to dodge every per-IP limit
  • is_item was set on every non-object packet (Map.has_key? on a key the parser always emits)
  • Cluster mode: spatial broadcasts went cluster-wide, duplicating packets per node and cross-delivering to same-id remote subscribers — now local_broadcast

Verification: mix test 2274/2274, mix format --check-formatted, mix credo --strict, mix dialyzer all green (hooks re-ran on commit). Browser-smoked against a live server: map loads with tiles + LiveView connected, error boundary ignores synthetic tile error events, weather page renders 5 charts with correct labels and table rows, malformed login returns 400.

Second-round audit across the backend pipeline, LiveViews, web layer, and frontend. All findings verified against the code; each fix is in the commit message. **Highest impact:** - `PacketBatcher.latest_by_sender` grew unboundedly per map tab — now capped at 5,000 senders with a 500-entry prune margin (regression test included) - `PostgresNotifier` could crash-loop the whole node on a DB outage — `sync_connect: false` + `auto_reconnect: true` - `ShutdownHandler` deadlocked on SIGTERM so `System.stop(0)` never ran — k8s always SIGKILLed at the grace period - Failed map tile loads tripped the LiveView error boundary and hid the map - LiveView rejoin never re-sent `map_ready` — the map stayed silent until the user panned - `handle_params` re-ran the full historical load on every pan (double queries + clear-and-redraw) - Info page "stations heard by" query could never return rows (`first_digipeater = $1` vs `$1 || *`) - Dew point used the Celsius approximation on °F data; metric locales got hard-coded English imperial labels - Source-wide login bucket was reset on any successful sign-in, unbounding password spraying - IPv6 clients could rotate /64 addresses to dodge every per-IP limit - `is_item` was set on every non-object packet (`Map.has_key?` on a key the parser always emits) - Cluster mode: spatial broadcasts went cluster-wide, duplicating packets per node and cross-delivering to same-id remote subscribers — now `local_broadcast` **Verification:** `mix test` 2274/2274, `mix format --check-formatted`, `mix credo --strict`, `mix dialyzer` all green (hooks re-ran on commit). Browser-smoked against a live server: map loads with tiles + LiveView connected, error boundary ignores synthetic tile `error` events, weather page renders 5 charts with correct labels and table rows, malformed login returns 400.
fix: address bugs from second code audit
Some checks failed
Elixir CI / Build and test (pull_request) Failing after 2m8s
Elixir CI / Dialyzer (pull_request) Successful in 3m6s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
bb5c910bb9
Backend:
- PacketBatcher: cap latest_by_sender dedupe map (unbounded growth per
  long-lived map tab); prune 500 past a 5_000 cap by oldest timestamp
- PostgresNotifier: sync_connect: false + auto_reconnect: true so a DB
  outage can't crash-loop the node through the supervisor restart budget
- ShutdownHandler: drop synchronous Application.stop/1 deadlock; rely on
  System.stop(0) which stops applications in order
- SpatialPubSub: local_broadcast for per-subscriber spatial topics; in
  cluster mode a cluster-wide broadcast duplicates packets per node and
  can cross-deliver to a same-id remote subscriber
- PacketConsumer: log and broadcast inline when the shared task pool
  sheds work instead of silently dropping live packet broadcasts
- PacketConsumer: is_item now requires an actual itemname string —
  Map.has_key? flagged every non-object packet as an item
- DeviceCache: don't cache an empty device list for 24h after a failed
  or pre-population load; treat as a miss so next lookup retries
- ConnectionMonitor: sample cpu_sup in the long-lived process — calling
  it from a fresh Task returns a lifetime average, not an interval
- Packets.get_callsign_history: order DESC + LIMIT so the newest rows
  survive truncation, then reverse to the documented ascending order
- PromEx plugin: sum() not counter() for metrics carrying per-batch
  measurements (errors counted batches, not packets)
- InfoLive stations_heard_by SQL: first_digipeater CASE returned ||'*'
  but filters compare to  — always empty; also comma-delimit the match
  so W5ABC* doesn't match KW5ABC*
- LoginThrottle: don't reset the source-wide ip bucket on successful
  sign-in — it's the only bound on cross-account password spraying
- IPv6 rate-limit keys masked to /64 (RateLimiter.bucket_ip) across all
  throttles, the plug, and the mobile socket
- Browser pipeline gets its own rate-limit prefix (browser_rate_limit)
  instead of sharing 'rate_limit' with the :api pipeline
- MobileChannel: reject out-of-float-range integer bounds instead of
  ArithmeticError crashing the channel
- UserSessionController: 400 on non-string email/password instead of
  crashing Accounts
- UserSettingsLive: set changeset action :insert so password-change
  failures render the error banner

LiveView map:
- handle_params no longer re-runs the historical load on every
  self-issued patch (each pan cost two redundant DB queries + a
  clear-and-redraw)
- cleanup_old_packets recomputes the cutoff each sweep instead of using
  the frozen mount-time threshold
- HistoricalLoader resets historical_packets on a new generation so old
  windows can't merge into the unfiltered heat map
- BoundsUpdater sets initial_bounds_loaded so clear-on-pan is reachable
- PacketBatcher starts only for the connected mount (dead-render
  batchers idled for the HTTP keep-alive lifetime)

Frontend:
- ErrorBoundary ignores non-ErrorEvent resource-load failures — every
  failed tile <img> hid the whole map
- Tile retry keyed per tile element via WeakMap (keying by mutating src
  never reached the 3-attempt cap); strips old _retry params, skips
  detached tiles
- reconnected() re-sends map_ready + map state after LiveView rejoin so
  the fresh server process resumes streaming
- pushHistoryPatch called with the event arg LiveView 1.x expects
- removeMarker/withoutTrail no longer skip removal while the cluster
  group is detached (heat-map mode left permanent ghost markers)
- Trail hover: cancel debounce timer only when re-arming it — every
  second mousemove killed the RF-path hover permanently
- Weather charts read data-chart-labels (matching dataset key and JS
  key names); rain/gust/solar labels now sent server-side; dew point
  uses the °F formula on °F data; wind_gust serialized in history JSON
- InfoMap container marked phx-update=ignore so LiveView patches no
  longer wipe the Leaflet DOM
skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity Location Issue
🟡 Warning lib/aprsme_web/plugs/rate_limiter.ex:110 Masking this key splits it from the LiveView-side buckets (IPv6 only)

Reviewed bb5c910bb911. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟡 Warning | `lib/aprsme_web/plugs/rate_limiter.ex:110` | Masking this key splits it from the LiveView-side buckets (IPv6 only) | <sub>Reviewed `bb5c910bb911`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -108,3 +108,3 @@
# resolved client IP. Reading raw forwarded headers here would let an
# untrusted client spoof its rate-limit bucket.
conn.remote_ip |> :inet.ntoa() |> to_string()
Aprsme.RateLimiter.bucket_ip(conn.remote_ip)
First-time contributor

🟡 Warning — Masking this key splits it from the LiveView-side buckets (IPv6 only)

Masking conn.remote_ip makes the :api pipeline key rate_limit:<masked /64>, but extract_socket_ip/1 in this same module (line 80) still returns :inet.ntoa(resolved) |> to_string() unmasked, and the LiveView-side limiters build their keys from that. ApiDocsLive documents that it deliberately reuses the rate_limit:<ip> key "so a client cannot reset its 100/min allowance by switching from REST to this LiveView tester", and MapLive.Events.check_event_rate/3 keys lv_event:<event>:<ip> the same way. For IPv4 the two sides still coincide; for an IPv6 client they now diverge, so that documented invariant is false: rate_limit_socket still caps one socket at 20/min, but each fresh /64 address buys a fresh 100/min tester allowance and a fresh lv_event:* budget - the same rotation this PR closes on the accounts/API paths. Fix is one line in extract_socket_ip/1: |> Aprsme.RateLimiter.bucket_ip(). Safe because bucket_ip/1 is idempotent (re-masking an already-masked string yields the same key), so the Accounts.* callers that pass client_ip into the throttles are unaffected.

**🟡 Warning — Masking this key splits it from the LiveView-side buckets (IPv6 only)** Masking `conn.remote_ip` makes the `:api` pipeline key `rate_limit:<masked /64>`, but `extract_socket_ip/1` in this same module (line 80) still returns `:inet.ntoa(resolved) |> to_string()` unmasked, and the LiveView-side limiters build their keys from that. `ApiDocsLive` documents that it deliberately reuses the `rate_limit:<ip>` key "so a client cannot reset its 100/min allowance by switching from REST to this LiveView tester", and `MapLive.Events.check_event_rate/3` keys `lv_event:<event>:<ip>` the same way. For IPv4 the two sides still coincide; for an IPv6 client they now diverge, so that documented invariant is false: `rate_limit_socket` still caps one socket at 20/min, but each fresh /64 address buys a fresh 100/min tester allowance and a fresh `lv_event:*` budget - the same rotation this PR closes on the accounts/API paths. Fix is one line in `extract_socket_ip/1`: `|> Aprsme.RateLimiter.bucket_ip()`. Safe because `bucket_ip/1` is idempotent (re-masking an already-masked string yields the same key), so the `Accounts.*` callers that pass `client_ip` into the throttles are unaffected.
skippy-bot marked this conversation as resolved
test: pin regressions for audit fixes
Some checks failed
Elixir CI / Dialyzer (pull_request) Successful in 1m3s
Elixir CI / Build and test (pull_request) Failing after 1m59s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
fb659ade98
Each new/updated test fails on main and passes on this branch:
- RateLimiter.bucket_ip/1: IPv4 passthrough, IPv6 /64 masking, mapped-IPv4
  collapse, non-IP binaries, nil
- LoginThrottle.reset/2: leaves the source-wide ip bucket intact so a
  successful self-login can't wipe the password-spray budget
- get_callsign_history/3: the newest N rows survive the limit, oldest first
- InfoLive stations-heard-by: digipeater path 'K5ABC-1*' lists the sender
  on /info/K5ABC-1 (first_digipeater compared to $1, not $1||'*')
- UserSessionController: nested user[email] params get 400, not a crash
- MobileChannel subscribe_bounds: integer beyond float range gets an
  error reply, not ArithmeticError
- PacketConsumer: itemname: nil no longer marks every non-object is_item
- WeatherLive dew point: Fahrenheit approximation on °F data
- PacketBatcher dedupe map bound (cap + prune margin)
chore: bump mint and lazy_html for new CVEs
Some checks failed
Elixir CI / Build and test (pull_request) Successful in 2m35s
Elixir CI / Dialyzer (pull_request) Successful in 2m45s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread
13063d22f5
mix deps.audit fails CI on mint 1.10.1 (EEF-CVE-2026-94194,
EEF-CVE-2026-91043, EEF-CVE-2026-92103) and lazy_html 0.1.12
(EEF-CVE-2026-92106). Bumped to mint 1.11.0, lazy_html 0.1.13
(hpax 1.1.0 pulled along); audit is clean and the suite still passes.
fix: mask socket-side rate-limit IPs to /64 too
All checks were successful
Elixir CI / Dialyzer (pull_request) Successful in 58s
Elixir CI / Build and test (pull_request) Successful in 1m15s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: clean — no open findings
9b18664da7
Skippy flagged that bucket_ip in the plug splits IPv6 clients from the
LiveView/channel-side buckets, which still keyed on the full address
(ApiDocsLive's rate_limit:<ip>, auth LiveView throttles, map events,
mobile socket). extract_socket_ip/1 now returns the masked form so
both sides share one bucket; the throttles already re-normalize, so
this is idempotent.
First-time contributor

Resolved 1 of 1 earlier finding: extract_socket_ip/1 (lib/aprsme_web/plugs/rate_limiter.ex) now keys through Aprsme.RateLimiter.bucket_ip/1, so every LiveView-side bucket (api_docs, map events, registration/settings/forgot-password/confirmation) shares the HTTP plug's /64 mask again. Nothing new in 13063d22..9b18664d.

Resolved 1 of 1 earlier finding: `extract_socket_ip/1` (`lib/aprsme_web/plugs/rate_limiter.ex`) now keys through `Aprsme.RateLimiter.bucket_ip/1`, so every LiveView-side bucket (api_docs, map events, registration/settings/forgot-password/confirmation) shares the HTTP plug's /64 mask again. Nothing new in `13063d22..9b18664d`. <!-- skippy-pr-review -->
graham merged commit bda6f58615 into main 2026-10-04 22:45:53 +00:00
graham deleted branch fix/audit-round-2 2026-10-04 22:45:53 +00:00
Sign in to join this conversation.
No description provided.