fix: reject malformed mobile channel frames #21

Merged
graham merged 3 commits from fix/w4-mobile-frames into main 2026-09-15 16:33:02 -05:00
Owner

Fixes W4 from bugs.md.

  • adds a terminal handle_in/3 clause that replies with an error instead of raising FunctionClauseError on unknown events, missing keys, or mistyped payloads
  • guards search_callsign and subscribe_callsign with is_binary/1 so non-string values reach the catch-all rather than crashing byte_size/1 / String.trim/1
  • emits payload_rejected telemetry for rejected frames, logging only the event name and payload type — never frame contents

Verification: focused channel tests passed (65) including five new malformed-frame cases, full suite passed (2240), mix credo --strict, and commit hooks including Dialyzer passed.

Fixes W4 from bugs.md. - adds a terminal `handle_in/3` clause that replies with an error instead of raising `FunctionClauseError` on unknown events, missing keys, or mistyped payloads - guards `search_callsign` and `subscribe_callsign` with `is_binary/1` so non-string values reach the catch-all rather than crashing `byte_size/1` / `String.trim/1` - emits `payload_rejected` telemetry for rejected frames, logging only the event name and payload type — never frame contents Verification: focused channel tests passed (65) including five new malformed-frame cases, full suite passed (2240), `mix credo --strict`, and commit hooks including Dialyzer passed.
fix: reject malformed mobile channel frames
All checks were successful
Elixir CI / Dialyzer (pull_request) Successful in 40s
Elixir CI / Build and test (pull_request) Successful in 1m42s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: clean — 1 non-blocking note open
3d88443d3e
skippy-bot left a comment

🤖 Skippy PR review

1 finding — none blocking.

Severity Location Issue
🔵 Suggestion lib/aprsme_web/channels/mobile_channel.ex:245 Unmatched frames emit a warning log per frame, uncapped

Reviewed 3d88443d3e34. Comment skippy review to re-run.

### 🤖 Skippy PR review **1 finding** — none blocking. | Severity | Location | Issue | | --- | --- | --- | | 🔵 Suggestion | `lib/aprsme_web/channels/mobile_channel.ex:245` | Unmatched frames emit a warning log per frame, uncapped | <sub>Reviewed `3d88443d3e34`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -239,0 +242,4 @@
# payload itself is never logged or mirrored into telemetry — only its
# rough shape — so a hostile frame cannot inject log content.
def handle_in(event, payload, socket) do
emit_payload_rejected("unmatched_event", %{event: event, payload_type: inspect_type(payload)})
First-time contributor

🔵 Suggestion — Unmatched frames emit a warning log per frame, uncapped

This clause calls emit_payload_rejected/2, which does a Logger.warning for every frame that lands here, and unlike the four client-initiated handlers above it sits behind no check_rate_limit. Connects are capped at 30/min per IP, frames are not: one anonymous socket can push unknown events as fast as the transport allows, each one going through the single Logger process, so log volume becomes client-controlled.

This is not a regression — unsubscribe and unsubscribe_callsign already log uncapped — so treat it as optional hardening rather than a blocker. If you do want to close it, don't route the call through check_rate_limit/2: its deny branch logs a warning per frame too, so the volume stays. Keep the telemetry counter (cheap) and drop the unmatched-event Logger.warning to Logger.debug, or log once per socket.

**🔵 Suggestion — Unmatched frames emit a warning log per frame, uncapped** This clause calls `emit_payload_rejected/2`, which does a `Logger.warning` for **every** frame that lands here, and unlike the four client-initiated handlers above it sits behind no `check_rate_limit`. Connects are capped at 30/min per IP, frames are not: one anonymous socket can push unknown events as fast as the transport allows, each one going through the single Logger process, so log volume becomes client-controlled. This is not a regression — `unsubscribe` and `unsubscribe_callsign` already log uncapped — so treat it as optional hardening rather than a blocker. If you do want to close it, don't route the call through `check_rate_limit/2`: its deny branch logs a warning per frame too, so the volume stays. Keep the telemetry counter (cheap) and drop the unmatched-event `Logger.warning` to `Logger.debug`, or log once per socket.
skippy-bot marked this conversation as resolved
fix: log unmatched channel events at debug
Some checks failed
skippy-bot/review Skippy review: 2 blocking findings open — see the PR thread
Elixir CI / Build and test (pull_request) Failing after 47s
Elixir CI / Dialyzer (pull_request) Successful in 2m49s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
62630e5ee7
skippy-bot left a comment

🤖 Skippy PR review

2 findings — 2 blocking before merge.

Severity Location Issue
🟠 High lib/aprsme_web/channels/mobile_channel.ex:244 Unused inspect_type/1 fails mix compile --warnings-as-errors
🟡 Warning lib/aprsme_web/channels/mobile_channel.ex:248 Client-supplied event name is interpolated raw into the log line

Resolved 1 earlier finding: the uncapped per-frame warning is now Logger.debug + telemetry, as suggested. 1 new blocking finding (unused inspect_type/1) and 1 warning.

Reviewed 62630e5ee7a5. Comment skippy review to re-run.

### 🤖 Skippy PR review **2 findings** — 2 blocking before merge. | Severity | Location | Issue | | --- | --- | --- | | 🟠 High | `lib/aprsme_web/channels/mobile_channel.ex:244` | Unused `inspect_type/1` fails `mix compile --warnings-as-errors` | | 🟡 Warning | `lib/aprsme_web/channels/mobile_channel.ex:248` | Client-supplied event name is interpolated raw into the log line | Resolved 1 earlier finding: the uncapped per-frame warning is now Logger.debug + telemetry, as suggested. 1 new blocking finding (unused inspect_type/1) and 1 warning. <sub>Reviewed `62630e5ee7a5`. Comment `skippy review` to re-run.</sub> <!-- skippy-pr-review -->
@ -239,0 +241,4 @@
# crashing the channel (and minting an ErrorTracker row per frame). The
# payload itself is never logged or mirrored into telemetry — only its
# rough shape — so a hostile frame cannot inject log content.
def handle_in(event, payload, socket) do
First-time contributor

🟠 High — Unused inspect_type/1 fails mix compile --warnings-as-errors

inspect_type/1 (lines 259-265) lost its only caller when this clause stopped passing payload_type and now has zero call sites; payload is also unused in this head. Elixir emits function AprsmeWeb.MobileChannel.inspect_type/1 is unused and variable "payload" is unused, and CI compiles with warnings as errors (.forgejo/workflows/ci.yml and .github/workflows/ci.yml, step mix compile --warnings-as-errors), so this build goes red even though the local precommit alias (format/credo/test, no --warnings-as-errors) stays green. Fix: delete the seven inspect_type/1 clauses and rename the head arg to _payload.

**🟠 High — Unused `inspect_type/1` fails `mix compile --warnings-as-errors`** `inspect_type/1` (lines 259-265) lost its only caller when this clause stopped passing `payload_type` and now has zero call sites; `payload` is also unused in this head. Elixir emits `function AprsmeWeb.MobileChannel.inspect_type/1 is unused` and `variable "payload" is unused`, and CI compiles with warnings as errors (`.forgejo/workflows/ci.yml` and `.github/workflows/ci.yml`, step `mix compile --warnings-as-errors`), so this build goes red even though the local `precommit` alias (`format`/`credo`/`test`, no `--warnings-as-errors`) stays green. Fix: delete the seven `inspect_type/1` clauses and rename the head arg to `_payload`.
skippy-bot marked this conversation as resolved
@ -239,0 +245,4 @@
# Telemetry stays (cheap, aggregated); the log drops to debug because this
# clause sits behind no rate limit — one socket can push unknown events at
# line rate, and a warning per frame hands log volume to the client.
Logger.debug("Mobile channel unmatched event: #{event}")
First-time contributor

🟡 Warning — Client-supplied event name is interpolated raw into the log line

event is the unvalidated event string straight off the wire, so #{event} hands an anonymous socket control of the log line: embedded newlines forge entries that look like this module's own output, ANSI escapes land in terminal/log tailers, and the message length is unbounded. The metadata path this replaced formatted the value through inspect/1, which escaped it, and the comment three lines up still promises a hostile frame cannot inject log content. Fix: Logger.debug("Mobile channel unmatched event: #{inspect(event)}"), or pass event as metadata so the formatter escapes it.

**🟡 Warning — Client-supplied event name is interpolated raw into the log line** `event` is the unvalidated event string straight off the wire, so `#{event}` hands an anonymous socket control of the log line: embedded newlines forge entries that look like this module's own output, ANSI escapes land in terminal/log tailers, and the message length is unbounded. The metadata path this replaced formatted the value through `inspect/1`, which escaped it, and the comment three lines up still promises a hostile frame cannot inject log content. Fix: `Logger.debug("Mobile channel unmatched event: #{inspect(event)}")`, or pass `event` as metadata so the formatter escapes it.
skippy-bot marked this conversation as resolved
fix: address review on unmatched-frame logging
All checks were successful
Elixir CI / Dialyzer (pull_request) Successful in 40s
Elixir CI / Build and test (pull_request) Successful in 1m37s
Elixir CI / Build and Push Docker Image (pull_request) Has been skipped
skippy-bot/review Skippy review: clean — no open findings
bdd1753f57
- inspect the event name in the debug log so client-controlled newlines
  or escape sequences cannot forge log content
- restore payload_type to the rejection telemetry, using inspect_type/1
  (unused since 62630e5e, which failed compile --warnings-as-errors)
Author
Owner

skippy review

skippy review
First-time contributor

Resolved 2 earlier findings in bdd1753f5770inspect_type/1 is called again from the terminal clause (and payload is used), and the unmatched-event log now goes through inspect/1 so a hostile event name cannot forge log content. 0 still open, nothing new in 62630e5..bdd1753.

Resolved 2 earlier findings in `bdd1753f5770` — `inspect_type/1` is called again from the terminal clause (and `payload` is used), and the unmatched-event log now goes through `inspect/1` so a hostile event name cannot forge log content. 0 still open, nothing new in `62630e5..bdd1753`. <!-- skippy-pr-review -->
First-time contributor

Re-review at bdd1753f5770 (same head as the last pass, full diff re-read): 0 findings open, nothing new.

All three earlier findings stay resolved: the terminal clause logs at Logger.debug with inspect(event) and keeps the payload_type telemetry, and inspect_type/1 is live again (called from the catch-all's metadata), so the compile-with-warnings-as-errors gate is clean.

Check finalized green.

Re-review at `bdd1753f5770` (same head as the last pass, full diff re-read): 0 findings open, nothing new. All three earlier findings stay resolved: the terminal clause logs at `Logger.debug` with `inspect(event)` and keeps the `payload_type` telemetry, and `inspect_type/1` is live again (called from the catch-all's metadata), so the compile-with-warnings-as-errors gate is clean. Check finalized green. <!-- skippy-pr-review -->
graham merged commit 358cc23ed7 into main 2026-09-15 16:33:02 -05:00
graham deleted branch fix/w4-mobile-frames 2026-09-15 16:33:02 -05:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
graham/aprs.me!21
No description provided.