fix: address bugs from second code audit #39
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/audit-round-2"
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?
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_sendergrew unboundedly per map tab — now capped at 5,000 senders with a 500-entry prune margin (regression test included)PostgresNotifiercould crash-loop the whole node on a DB outage —sync_connect: false+auto_reconnect: trueShutdownHandlerdeadlocked on SIGTERM soSystem.stop(0)never ran — k8s always SIGKILLed at the grace periodmap_ready— the map stayed silent until the user pannedhandle_paramsre-ran the full historical load on every pan (double queries + clear-and-redraw)first_digipeater = $1vs$1 || *)is_itemwas set on every non-object packet (Map.has_key?on a key the parser always emits)local_broadcastVerification:
mix test2274/2274,mix format --check-formatted,mix credo --strict,mix dialyzerall green (hooks re-ran on commit). Browser-smoked against a live server: map loads with tiles + LiveView connected, error boundary ignores synthetic tileerrorevents, weather page renders 5 charts with correct labels and table rows, malformed login returns 400.🤖 Skippy PR review
1 finding — 1 blocking before merge.
lib/aprsme_web/plugs/rate_limiter.ex:110Reviewed
bb5c910bb911. Commentskippy reviewto re-run.@ -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)🟡 Warning — Masking this key splits it from the LiveView-side buckets (IPv6 only)
Masking
conn.remote_ipmakes the:apipipeline keyrate_limit:<masked /64>, butextract_socket_ip/1in this same module (line 80) still returns:inet.ntoa(resolved) |> to_string()unmasked, and the LiveView-side limiters build their keys from that.ApiDocsLivedocuments that it deliberately reuses therate_limit:<ip>key "so a client cannot reset its 100/min allowance by switching from REST to this LiveView tester", andMapLive.Events.check_event_rate/3keyslv_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_socketstill caps one socket at 20/min, but each fresh /64 address buys a fresh 100/min tester allowance and a freshlv_event:*budget - the same rotation this PR closes on the accounts/API paths. Fix is one line inextract_socket_ip/1:|> Aprsme.RateLimiter.bucket_ip(). Safe becausebucket_ip/1is idempotent (re-masking an already-masked string yields the same key), so theAccounts.*callers that passclient_ipinto the throttles are unaffected.Resolved 1 of 1 earlier finding:
extract_socket_ip/1(lib/aprsme_web/plugs/rate_limiter.ex) now keys throughAprsme.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 in13063d22..9b18664d.