fix: audit round 4 (~50 defects across map, ingestion, storage, accounts, pages, API) #46

graham · 2026-10-25T19:09:04Z

graham commented

2026-10-25 19:09:04 +00:00

Implements the OpenSpec change fix-aprsme-audit-round-4 (estate store). One commit per task group.

Highest impact

Map: zooming past level 11 threw in zoomend, cancelling moveend and tripping the error boundary (trailLayer was a LayerGroup).
Error alert emails never sent (notifier listened to an event ErrorTracker never emits).
/dashboard and /errors: CSP nonce wired; admin re-checked on every live connection.
Client IP behind Cloudflare: CF-Connecting-IP is authoritative over HTTP; WebSockets resolve to the address next to the edge.
Ultimeter $ULTW packets failed batch insert (float wind direction); only fields 1-5 are stored.
Batch insert errors: only row-level errors fall back to row-by-row.
Partition retention no longer stops when one creation fails.
Tracked trail line survives at zoom <= 8; Locate/set_location/URL jumps apply zoom transitions, load history for the destination viewport, and set_location updates the URL.
"Heard directly" credits the real digipeater, not WIDE1; identical beacons no longer collide.
Fuzzy de/es/fr translations corrected.

Full list: CHANGELOG.md.

Verification

Suite: 2378 passed on this base; main CI was green at fcdf808c. Group 2 (accounts/security) tests confirmed red against pre-fix lib/, as were the callsign API/docs tests. Browser (harness Chromium, dev server): zoom 10->11->10->11 (red with old bundle), tracked callsign at z6, clear/track without panning, Locate from z5, page-load sweep incl. /dashboard and /errors with no console errors.

Not verified

24h trails and the zoom-8 boundary in a browser; a 10-minute live session.
OMS removal, trail hover, trail-colour ref-counting (client, untested).
EXPLAIN ANALYZE on prod-sized data (dev DB only, ~515k rows, 335ms cold).
Relative-time localization in a browser (Intl output only).
Callsign subscribe: test asserts subscribe-before-history ordering, not a packet mid-load.
tsc has ~142 pre-existing errors; none attributable to this change beyond that class.
Not every other group's tests were re-run against old code.

Notes

Locate/set_location previously left the URL stale; now fixed.
Changelog/docs edits are in the last two commits rather than per group.

Implements the OpenSpec change `fix-aprsme-audit-round-4` (estate store). One commit per task group. ## Highest impact - Map: zooming past level 11 threw in `zoomend`, cancelling `moveend` and tripping the error boundary (`trailLayer` was a `LayerGroup`). - Error alert emails never sent (notifier listened to an event ErrorTracker never emits). - `/dashboard` and `/errors`: CSP nonce wired; admin re-checked on every live connection. - Client IP behind Cloudflare: `CF-Connecting-IP` is authoritative over HTTP; WebSockets resolve to the address next to the edge. - Ultimeter `$ULTW` packets failed batch insert (float wind direction); only fields 1-5 are stored. - Batch insert errors: only row-level errors fall back to row-by-row. - Partition retention no longer stops when one creation fails. - Tracked trail line survives at zoom <= 8; Locate/`set_location`/URL jumps apply zoom transitions, load history for the destination viewport, and `set_location` updates the URL. - "Heard directly" credits the real digipeater, not `WIDE1`; identical beacons no longer collide. - Fuzzy de/es/fr translations corrected. Full list: CHANGELOG.md. ## Verification Suite: 2378 passed on this base; main CI was green at fcdf808c. Group 2 (accounts/security) tests confirmed red against pre-fix `lib/`, as were the callsign API/docs tests. Browser (harness Chromium, dev server): zoom 10->11->10->11 (red with old bundle), tracked callsign at z6, clear/track without panning, Locate from z5, page-load sweep incl. `/dashboard` and `/errors` with no console errors. ## Not verified - 24h trails and the zoom-8 boundary in a browser; a 10-minute live session. - OMS removal, trail hover, trail-colour ref-counting (client, untested). - `EXPLAIN ANALYZE` on prod-sized data (dev DB only, ~515k rows, 335ms cold). - Relative-time localization in a browser (Intl output only). - Callsign subscribe: test asserts subscribe-before-history ordering, not a packet mid-load. - `tsc` has ~142 pre-existing errors; none attributable to this change beyond that class. - Not every other group's tests were re-run against old code. ## Notes - Locate/`set_location` previously left the URL stale; now fixed. - Changelog/docs edits are in the last two commits rather than per group.

graham added 10 commits

2026-10-25 19:09:04 +00:00

fix: send error alert emails from ErrorTracker's new-error event; drop dead notifier and metric 03b8ee7349

fix: admin dashboards under CSP with live re-auth; CDN-safe client IP; non-enumerating email change; session revocation on password change; login email prefill; digit-prefix callsigns ae0b4705b5

fix: store Ultimeter weather, drop out-of-range positions, isolate row vs batch insert errors, rank device patterns, reset IS buffer on disconnect 0577cfabc5

fix: keep partition retention running through creation failures; most-recent other SSIDs 5baf44756a

fix: antimeridian viewports in one grid cell, retry failed viewport registration, serve draining views, one cluster-active definition b62b4d6388

fix: map zoom crash at z11, trail duration on load, tracked trail at low zoom, programmatic zoom transitions, live low-zoom refresh, tracking reload, popup link encoding, batcher restart 6bbfe169a3

fix: station page data - first-hop digipeater credit, (id, received_at) identity, symbol and sender links, weather precision and luminosity, localized relative times, bad-packet refresh, status packet rate 3ffb155a68

fix: callsign API accepts every valid APRS identifier, docs match responses, mobile callsign subscribe has no history gap 699cf49e14

docs: record audit round 4 behavior changes 5168c363de

fix: review follow-ups - Ultimeter field mapping, WebSocket client IP, Locate bounds and URL, real ErrorTracker test, drop_partition idempotence, fuzzy translations

Elixir CI / Dialyzer (pull_request) Successful in 59s

Details

Elixir CI / Build and test (pull_request) Successful in 1m41s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: 1 blocking finding open — see the PR thread

Details

d074b8b490

skippy-bot reviewed

2026-10-25 19:17:21 +00:00

skippy-bot left a comment

🤖 Skippy PR review

1 finding — 1 blocking before merge.

Severity	Location	Issue
🟠 High	`lib/aprsme_web/user_auth.ex:117`	Password change/reset does not disconnect live sockets: nothing subscribes to the topic

_{Reviewed d074b8b4902e. Comment skippy review to re-run.}

### 🤖 Skippy PR review **1 finding** — 1 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/aprsme_web/user_auth.ex:117` | Password change/reset does not disconnect live sockets: nothing subscribes to the topic | <sub>Reviewed `d074b8b4902e`. Comment `skippy review` to re-run.</sub>

lib/aprsme_web/user_auth.ex

					
				@ -100,0 +114,4 @@

				    except = Keyword.get(opts, :except)

				    for token <- tokens, id = live_socket_id(token), id != except do

				      _ = AprsmeWeb.Endpoint.broadcast(id, "disconnect", %{})

🟠 High — Password change/reset does not disconnect live sockets: nothing subscribes to the topic

This broadcast reaches no process. Phoenix.LiveView never subscribes a socket to session["live_socket_id"] - in the pinned phoenix_live_view 1.2.13 the only subscribe in channel.ex is the "live_view" live-reload topic, and the string disconnect appears nowhere in the dep - and the app has no subscriber either (grep -rn users_sessions lib/ finds only live_socket_id/1 itself). So a socket opened with a revoked token keeps its current_user assign and full access until it happens to reconnect; update_user_password/reset_user_password only stop new mounts, which is the situation this function was added to fix. The two new tests assert_receive %Broadcast{event: "disconnect"} after Endpoint.subscribe("users_sessions:...") in the test process, so they prove the message is emitted, not that any LiveView closed - they stay green with the feature inert. (log_out_user/1 has the same hole, so logout never closes live sockets either.) Fix: subscribe the socket and close it from the shared hook, e.g. in mount_current_user/2:

if connected?(socket) and is_binary(id = session["live_socket_id"]) do
  AprsmeWeb.Endpoint.subscribe(id)

  Phoenix.LiveView.attach_hook(socket, :session_disconnect, :handle_info, fn
    %Phoenix.Socket.Broadcast{event: "disconnect"}, s -> {:halt, push_navigate(s, to: ~p"/users/log_in")}
    _, s -> {:cont, s}
  end)
else
  socket
end

**🟠 High — Password change/reset does not disconnect live sockets: nothing subscribes to the topic** This broadcast reaches no process. `Phoenix.LiveView` never subscribes a socket to `session["live_socket_id"]` - in the pinned phoenix_live_view 1.2.13 the only `subscribe` in `channel.ex` is the `"live_view"` live-reload topic, and the string `disconnect` appears nowhere in the dep - and the app has no subscriber either (`grep -rn users_sessions lib/` finds only `live_socket_id/1` itself). So a socket opened with a revoked token keeps its `current_user` assign and full access until it happens to reconnect; `update_user_password`/`reset_user_password` only stop *new* mounts, which is the situation this function was added to fix. The two new tests `assert_receive %Broadcast{event: "disconnect"}` after `Endpoint.subscribe("users_sessions:...")` in the *test* process, so they prove the message is emitted, not that any LiveView closed - they stay green with the feature inert. (`log_out_user/1` has the same hole, so logout never closes live sockets either.) Fix: subscribe the socket and close it from the shared hook, e.g. in `mount_current_user/2`: ```elixir if connected?(socket) and is_binary(id = session["live_socket_id"]) do AprsmeWeb.Endpoint.subscribe(id) Phoenix.LiveView.attach_hook(socket, :session_disconnect, :handle_info, fn %Phoenix.Socket.Broadcast{event: "disconnect"}, s -> {:halt, push_navigate(s, to: ~p"/users/log_in")} _, s -> {:cont, s} end) else socket end ```

skippy-bot marked this conversation as resolved

graham added 1 commit

2026-10-25 19:38:01 +00:00

fix: handle session revocation in authenticated LiveViews

Elixir CI / Dialyzer (pull_request) Successful in 1m0s

Details

Elixir CI / Build and test (pull_request) Successful in 1m27s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Superseded by 34207e8055df — the newer push is reviewed instead

Details

7f85e09b60

graham added 1 commit

2026-10-25 19:42:01 +00:00

revert: keep session revocation owned by Phoenix transport

Elixir CI / Dialyzer (pull_request) Successful in 1m3s

Details

Elixir CI / Build and test (pull_request) Successful in 1m20s

Details

Elixir CI / Build and Push Docker Image (pull_request) Has been skipped

Details

skippy-bot/review Skippy review: clean — no open findings

Details

34207e8055

graham commented

2026-10-25 19:42:21 +00:00

Regarding the blocking session-disconnect finding: the finding is a false positive; the subscriber is in Phoenix's transport, not the LiveView channel.

Pinned dependencies: Phoenix 1.8.16 / LiveView 1.2.13.

LiveView.Socket.id/1 returns the session's live_socket_id.
Phoenix.Socket.init/1 subscribes the transport to that ID. info/2 handles the disconnect broadcast by stopping the transport with {:shutdown, :disconnected}.
The app supplies session connect_info for both WebSocket and longpoll (lib/aprsme_web/endpoint.ex:28-30), supplies _csrf_token (assets/js/app.ts:282-284), and stores the same topic in the login session (user_auth.ex:320-327).

I initially added the suggested hook in 7f85e09b, but corrected that mistake in 34207e80: removed the redundant subscription/redirect hook and restored the original tests and changelog. A full login redirect is not the existing behavior; the transport closes and public pages can reconnect anonymously.

Verified without that hook, using real local HTTP login requests and mounted LiveViews over actual WebSockets (not LiveViewTest), with dummy accounts inside a rolled-back SQL sandbox transaction:

Logout: authenticated WebSocket closed normally (1000); revoked cookie cannot reopen settings.
Password change: other session's authenticated WebSocket closed normally (1000); revoked cookie cannot reopen settings; submitting transport stayed open for the trigger-action POST; new credentials successfully issued an authenticated session.
Password reset: authenticated WebSocket closed normally (1000); revoked cookie cannot reopen settings.

The earlier failing-before/passing-after LiveViewTest assertions were misleading: ClientProxy constructs a socket directly and bypasses transport initialization/subscription. Their failure did not establish a production bug.

After the corrective revert: all 2,378 tests passed; format, strict Credo, dependency audit, and Dialyzer passed. Please withdraw the blocking finding based on the transport source and real-transport verification above.

Regarding [the blocking session-disconnect finding](https://git.mcintire.me/graham/aprs.me/pulls/46#issuecomment-23834): **the finding is a false positive; the subscriber is in Phoenix's transport, not the LiveView channel.** Pinned dependencies: Phoenix 1.8.16 / LiveView 1.2.13. - [LiveView.Socket.id/1](https://github.com/phoenixframework/phoenix_live_view/blob/v1.2.13/lib/phoenix_live_view/socket.ex#L109-L115) returns the session's live_socket_id. - [Phoenix.Socket.__init__/1](https://github.com/phoenixframework/phoenix/blob/v1.8.16/lib/phoenix/socket.ex#L549-L552) subscribes the transport to that ID. [__info__/2](https://github.com/phoenixframework/phoenix/blob/v1.8.16/lib/phoenix/socket.ex#L571-L573) handles the disconnect broadcast by stopping the transport with {:shutdown, :disconnected}. - The app supplies session connect_info for both WebSocket and longpoll (lib/aprsme_web/endpoint.ex:28-30), supplies _csrf_token (assets/js/app.ts:282-284), and stores the same topic in the login session (user_auth.ex:320-327). I initially added the suggested hook in 7f85e09b, but corrected that mistake in **34207e80**: removed the redundant subscription/redirect hook and restored the original tests and changelog. A full login redirect is not the existing behavior; the transport closes and public pages can reconnect anonymously. **Verified without that hook**, using real local HTTP login requests and mounted LiveViews over actual WebSockets (not LiveViewTest), with dummy accounts inside a rolled-back SQL sandbox transaction: - Logout: authenticated WebSocket closed normally (1000); revoked cookie cannot reopen settings. - Password change: other session's authenticated WebSocket closed normally (1000); revoked cookie cannot reopen settings; submitting transport stayed open for the trigger-action POST; new credentials successfully issued an authenticated session. - Password reset: authenticated WebSocket closed normally (1000); revoked cookie cannot reopen settings. The earlier failing-before/passing-after LiveViewTest assertions were misleading: [ClientProxy constructs a socket directly](https://github.com/phoenixframework/phoenix_live_view/blob/v1.2.13/lib/phoenix_live_view/test/client_proxy.ex#L259-L271) and bypasses transport initialization/subscription. Their failure did not establish a production bug. After the corrective revert: all 2,378 tests passed; format, strict Credo, dependency audit, and Dialyzer passed. Please withdraw the blocking finding based on the transport source and real-transport verification above.

skippy-bot commented

2026-10-25 19:44:32 +00:00

First-time contributor

Resolved 1 of 1 earlier findings, as a false positive. Nothing new in 7f85e09b..34207e80.

lib/aprsme_web/user_auth.ex (broadcast reaches nothing): retracted, graham is right. The subscriber is in the transport, not the LiveView channel. Phoenix.LiveView.Socket.id/1 returns socket.private.connect_info[:session]["live_socket_id"], Phoenix.Socket.__init__/1 does _ = id && endpoint.subscribe(id) (so the transport process is subscribed), and Phoenix.Socket.__info__(%Broadcast{event: "disconnect"}) stops it with {:shutdown, :disconnected}, 1001. Both present in the pinned Phoenix; endpoint.ex:28-30 passes the session through for websocket and longpoll, so the id is there. My grep stopped at phoenix_live_view/channel.ex and missed phoenix/socket.ex.

So the app-side hook added in 7f85e09b was redundant, and revert 34207e80 is correct: logout, password change and reset all close the live sockets through the transport, and public pages reconnect anonymously instead of being bounced to login. The tests asserting the broadcast is emitted are the right level for app code, since the closing itself is Phoenix's.

Resolved 1 of 1 earlier findings, as a false positive. Nothing new in `7f85e09b..34207e80`. `lib/aprsme_web/user_auth.ex` (broadcast reaches nothing): retracted, graham is right. The subscriber is in the transport, not the LiveView channel. `Phoenix.LiveView.Socket.id/1` returns `socket.private.connect_info[:session]["live_socket_id"]`, `Phoenix.Socket.__init__/1` does `_ = id && endpoint.subscribe(id)` (so the transport process is subscribed), and `Phoenix.Socket.__info__(%Broadcast{event: "disconnect"})` stops it with `{:shutdown, :disconnected}, 1001`. Both present in the pinned Phoenix; `endpoint.ex:28-30` passes the session through for websocket and longpoll, so the id is there. My grep stopped at `phoenix_live_view/channel.ex` and missed `phoenix/socket.ex`. So the app-side hook added in `7f85e09b` was redundant, and revert `34207e80` is correct: logout, password change and reset all close the live sockets through the transport, and public pages reconnect anonymously instead of being bounced to login. The tests asserting the broadcast is emitted are the right level for app code, since the closing itself is Phoenix's.

graham merged commit 2f150f6648 into main

2026-10-25 19:45:40 +00:00

graham deleted branch fix/audit-round-4

2026-10-25 19:45:40 +00:00

graham referenced this pull request from a commit

2026-10-25 19:45:42 +00:00

fix: audit round 4 (~50 defects across map, ingestion, storage, accounts, pages, API) (#46)

Sign in to join this conversation.

No reviewers