fix: reject malformed mobile channel frames #21
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/w4-mobile-frames"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Fixes W4 from bugs.md.
handle_in/3clause that replies with an error instead of raisingFunctionClauseErroron unknown events, missing keys, or mistyped payloadssearch_callsignandsubscribe_callsignwithis_binary/1so non-string values reach the catch-all rather than crashingbyte_size/1/String.trim/1payload_rejectedtelemetry for rejected frames, logging only the event name and payload type — never frame contentsVerification: focused channel tests passed (65) including five new malformed-frame cases, full suite passed (2240),
mix credo --strict, and commit hooks including Dialyzer passed.🤖 Skippy PR review
1 finding — none blocking.
lib/aprsme_web/channels/mobile_channel.ex:245Reviewed
3d88443d3e34. Commentskippy reviewto re-run.@ -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) doemit_payload_rejected("unmatched_event", %{event: event, payload_type: inspect_type(payload)})🔵 Suggestion — Unmatched frames emit a warning log per frame, uncapped
This clause calls
emit_payload_rejected/2, which does aLogger.warningfor every frame that lands here, and unlike the four client-initiated handlers above it sits behind nocheck_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 —
unsubscribeandunsubscribe_callsignalready log uncapped — so treat it as optional hardening rather than a blocker. If you do want to close it, don't route the call throughcheck_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-eventLogger.warningtoLogger.debug, or log once per socket.🤖 Skippy PR review
2 findings — 2 blocking before merge.
lib/aprsme_web/channels/mobile_channel.ex:244inspect_type/1failsmix compile --warnings-as-errorslib/aprsme_web/channels/mobile_channel.ex:248Resolved 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. Commentskippy reviewto re-run.@ -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🟠 High — Unused
inspect_type/1failsmix compile --warnings-as-errorsinspect_type/1(lines 259-265) lost its only caller when this clause stopped passingpayload_typeand now has zero call sites;payloadis also unused in this head. Elixir emitsfunction AprsmeWeb.MobileChannel.inspect_type/1 is unusedandvariable "payload" is unused, and CI compiles with warnings as errors (.forgejo/workflows/ci.ymland.github/workflows/ci.yml, stepmix compile --warnings-as-errors), so this build goes red even though the localprecommitalias (format/credo/test, no--warnings-as-errors) stays green. Fix: delete the seveninspect_type/1clauses and rename the head arg to_payload.@ -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}")🟡 Warning — Client-supplied event name is interpolated raw into the log line
eventis 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 throughinspect/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 passeventas metadata so the formatter escapes it.skippy review
Resolved 2 earlier findings in
bdd1753f5770—inspect_type/1is called again from the terminal clause (andpayloadis used), and the unmatched-event log now goes throughinspect/1so a hostile event name cannot forge log content. 0 still open, nothing new in62630e5..bdd1753.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.debugwithinspect(event)and keeps thepayload_typetelemetry, andinspect_type/1is live again (called from the catch-all's metadata), so the compile-with-warnings-as-errors gate is clean.Check finalized green.