diff --git a/config/dev.exs b/config/dev.exs index 6170594..63c49fb 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -61,9 +61,9 @@ config :aprsme, AprsmeWeb.Endpoint, live_reload: [ web_console_logger: true, patterns: [ - ~r"priv/static/(?!uploads/).*(js|css|png|jpeg|jpg|gif|svg)$", - ~r"priv/gettext/.*(po)$", - ~r"lib/aprsme_web/(?:controllers|live|components|router)/?.*\.(ex|heex)$" + ~r"priv/static/(?!uploads/).*(js|css|png|jpeg|jpg|gif|svg)$"E, + ~r"priv/gettext/.*(po)$"E, + ~r"lib/aprsme_web/(?:controllers|live|components|router)/?.*\.(ex|heex)$"E ] ] diff --git a/config/test.exs b/config/test.exs index 48fb3bc..4cd63da 100644 --- a/config/test.exs +++ b/config/test.exs @@ -10,7 +10,7 @@ pool_size = if System.get_env("MIX_TEST_COVERAGE") do 2 else - System.schedulers_online() * 16 + min(System.schedulers_online() * 2, 20) end # In test we don't send emails. diff --git a/findings.md b/findings.md deleted file mode 100644 index f5250a9..0000000 --- a/findings.md +++ /dev/null @@ -1,103 +0,0 @@ -# Findings: aprs.me Code Review - -## Security Findings - -### CRITICAL: Production secrets exposed on disk -`.envrc` contains all production secrets in plaintext: APRS credentials, `DATABASE_URL`, `SECRET_KEY_BASE`, `RESEND_API_KEY`, `RELEASE_COOKIE`. Protected by `.gitignore` but accessible to anyone with filesystem access. - -### CRITICAL: APRS credentials in K8s manifest -`k8s/deployment.yaml:64-67,136-143` — APRS callsign and password hardcoded as plaintext env vars instead of Kubernetes Secrets. - -### HIGH: Secret key base in git history -`config/dev.exs:48`, `config/test.exs:40` — Development and test `secret_key_base` values committed since initial commit. - -### HIGH: SQL injection vector -`lib/aprsme/release.ex:162` — `SQL.query!` uses string interpolation with env var input: -```elixir -SQL.query!(repo, "SET statement_timeout = '#{timeout_seconds}s'", []) -``` - -### MEDIUM: Session signed but not encrypted -`lib/aprsme_web/endpoint.ex:7-13` — Session configured with `signing_salt` only, no `encryption_salt`. Session contents can be base64-decoded by anyone with cookie access. - -### MEDIUM: `raw()` HTML rendering in LiveView -`lib/aprsme_web/live/info_live/show.ex:537,542,545` — APRS symbol rendering uses `raw()` to inject HTML. While content is escaped, `raw()` bypasses HEEx auto-escaping at template level. - -### MEDIUM: `raw()` SVG rendering -`lib/aprsme_web/components/core_components.ex:50` — SVG content rendered via `raw()`. Risk is low (files are controlled vendored assets). - -### MEDIUM: `check_origin: false` in dev -`config/dev.exs:45` — WebSocket origin checks disabled in development. - -### MEDIUM: Anonymous WebSocket connections -`lib/aprsme_web/channels/mobile_user_socket.ex:21-29` — Mobile socket requires no authentication. Rate-limited to 30/min/IP but otherwise open. - -### MEDIUM: No auth on API endpoints -`lib/aprsme_web/router.ex:131-136` — API v1 (`/api/v1/callsign/:cs`, `/api/v1/weather/nearby`) is anonymous. Rate-limited but no API keys. - -### MEDIUM: APRS-IS connection uses plain TCP -`lib/aprsme/is/is.ex:215` — APRS passcode sent in cleartext over `:gen_tcp` (no SSL). - -### LOW: No `secure` flag on session cookie -`lib/aprsme_web/endpoint.ex:8-13` — No `secure: true` in session options. Handled by Phoenix in practice but not explicit. - -### LOW: Password policy too lax -`lib/aprsme/accounts/user.ex:73-75` — Character class requirements are commented out. Passwords only require 12+ characters. - -### LOW: Rate limiting not shared across cluster -`config/runtime.exs:133` — `Hammer.Backend.ETS` is per-node. Multi-node deployments can exhaust per-node limits independently. - -### INFO: CSP allows `unsafe-inline` and `unsafe-eval` -`lib/aprsme_web/router.ex:23-26` — Content Security Policy allows both, weakening XSS protection. - -### INFO: 27+ Sobelow findings pre-skipped -`.sobelow-skips` — Reviewed and deemed acceptable (parameterized spatial queries, symbol rendering, upstream TLS). - ---- - -## Code Quality & Refactoring Findings - -### HIGH: `map_live/index.ex` is 2,104 lines -Largest module in the project. Combines socket management, event handling, rendering, map state, and overlay logic. Extract into focused sub-modules (map_state, map_events, map_render). - -### HIGH: Duplicated `has_weather` logic -`lib/aprsme/packet.ex:159` and `lib/aprsme/packet_consumer.ex` both maintain parallel `set_has_weather/1` logic. If weather detection rules change, one path will be missed. - -### HIGH: 17 modules missing `@moduledoc` -Modules lacking documentation include: `Packet`, `PacketConsumer`, `DataExtended`, `BadPacket`, `Accounts.User`, `Accounts.UserNotifier`, `Packets`, and others. - -### MEDIUM: `require Logger` in function bodies (~75 occurrences) -Several modules duplicate `require Logger` at module level AND inside function bodies (e.g., `packets.ex:17,27`, `packet_consumer.ex`). Consolidate to module top-level. - -### MEDIUM: `packets.ex` is 849 lines -Mix of query functions, store logic, and validation. Consider splitting query/concerns into separate modules. - -### MEDIUM: Cache module has unnecessary GenServer overhead -`lib/aprsme/cache.ex` — Wraps ETS operations in GenServer calls. GenServer passes through `%{}` state with no meaningful state management. Direct ETS access would be faster for get/put. - -### MEDIUM: Encoding vs EncodingUtils boundary unclear -`encoding_utils.ex` (404 lines) wraps Gleam-based `Encoding` module but also contains substantive logic (`sanitize_packet`, `normalize_data_type`). Clarify responsibility boundary. - -### MEDIUM: Inline `import Ecto.Query` in function bodies -16 occurrences across `mobile_channel.ex`, `packet_utils.ex`, `info_live/show.ex`, `packets.ex`. Move to module top-level. - -### MEDIUM: Large inline SQL in LiveViews -`info_live/show.ex:369,392,484,491` — Complex ST_Distance calculations embedded in LiveView. Extract to query modules. - -### LOW: Geometry stored redundantly -`packet.ex` stores both scalar `lat`/`lon` AND PostGIS `location` geometry. `maybe_create_geometry_from_lat_lon/1` duplicates the data. Consider if both are needed. - -### LOW: Behaviour only covers subset of functions -`packets_behaviour.ex` defines 8 callbacks but `packets.ex` has far more public functions. Only partial mockability. - -### LOW: Test coverage at 91.13% (threshold 87%) -Only 4.13% above threshold. Focus on uncovered error/rescue paths and edge cases. - -### LOW: Performance TODOs not tracked in code -`TODO.md` items (table partitioning, ETS write path) not marked as `# TODO` in source code. Won't appear in IDE task lists. - ---- - -## Already Fixed - -- **`packets.html.heex` (unstyled `

`)**: Dead template with bare `

Packets

` removed. The `/packets` route uses `PacketsLive.Index` LiveView, not this controller template. diff --git a/lib/aprsme/application.ex b/lib/aprsme/application.ex index 05da032..2ee01e3 100644 --- a/lib/aprsme/application.ex +++ b/lib/aprsme/application.ex @@ -29,8 +29,6 @@ defmodule Aprsme.Application do Aprsme.CircuitBreaker, # Start regex cache for performance Aprsme.RegexCache, - # Start cache manager - Aprsme.Cache, # Start device cache manager Aprsme.DeviceCache, # Start broadcast task supervisor for async operations @@ -184,8 +182,7 @@ defmodule Aprsme.Application do defp redis_children do Logger.info("Starting ETS-based caching and rate limiting") - # Create ETS tables for caching — use :public so the Cache GenServer (a separate process) - # can write to these tables; write_concurrency improves throughput under concurrent writes. + # Create public ETS tables so callers can use concurrent reads and writes directly. _ = :ets.new(:query_cache, [:set, :public, :named_table, read_concurrency: true, write_concurrency: true]) _ = :ets.new(:device_cache, [:set, :public, :named_table, read_concurrency: true, write_concurrency: true]) _ = :ets.new(:symbol_cache, [:set, :public, :named_table, read_concurrency: true, write_concurrency: true]) diff --git a/lib/aprsme/cache.ex b/lib/aprsme/cache.ex index 48556d8..6df59a3 100644 --- a/lib/aprsme/cache.ex +++ b/lib/aprsme/cache.ex @@ -8,22 +8,30 @@ defmodule Aprsme.Cache do Expired entries are lazily evicted on read. """ - use GenServer - - def start_link(opts) do - GenServer.start_link(__MODULE__, opts, name: __MODULE__) - end - - @impl true - def init(_opts) do - {:ok, %{}} - end - @doc """ Get a value from cache. Returns `{:ok, nil}` for expired entries. """ def get(cache_name, key) do - GenServer.call(__MODULE__, {:get, cache_name, key}) + case :ets.lookup(cache_name, key) do + [{^key, value, :infinity}] -> + {:ok, value} + + [{^key, value, expires_at}] -> + if System.monotonic_time(:millisecond) < expires_at do + {:ok, value} + else + :ets.delete(cache_name, key) + {:ok, nil} + end + + [{^key, value}] -> + {:ok, value} + + [] -> + {:ok, nil} + end + rescue + ArgumentError -> {:error, :no_cache} end @doc """ @@ -33,28 +41,47 @@ defmodule Aprsme.Cache do * `:ttl` - Time to live in milliseconds. Use `Cache.to_timeout/1` for convenience. """ def put(cache_name, key, value, opts \\ []) do - GenServer.call(__MODULE__, {:put, cache_name, key, value, opts}) + expires_at = + case Keyword.get(opts, :ttl) do + nil -> :infinity + ttl when is_integer(ttl) and ttl > 0 -> System.monotonic_time(:millisecond) + ttl + _ -> :infinity + end + + :ets.insert(cache_name, {key, value, expires_at}) + {:ok, true} + rescue + ArgumentError -> {:error, :no_cache} end @doc """ Delete a key from cache """ def del(cache_name, key) do - GenServer.call(__MODULE__, {:del, cache_name, key}) + :ets.delete(cache_name, key) + {:ok, true} + rescue + ArgumentError -> {:error, :no_cache} end @doc """ Clear all keys from cache """ def clear(cache_name) do - GenServer.call(__MODULE__, {:clear, cache_name}) + :ets.delete_all_objects(cache_name) + {:ok, true} + rescue + ArgumentError -> {:error, :no_cache} end @doc """ Get cache statistics (simplified for ETS) """ def stats(cache_name) do - GenServer.call(__MODULE__, {:stats, cache_name}) + case :ets.info(cache_name) do + :undefined -> {:error, :no_cache} + info when is_list(info) -> {:ok, %{size: Keyword.get(info, :size, 0)}} + end end @doc """ @@ -72,7 +99,14 @@ defmodule Aprsme.Cache do Get TTL for a key. Returns remaining milliseconds or nil if no TTL. """ def ttl(cache_name, key) do - GenServer.call(__MODULE__, {:ttl, cache_name, key}) + case :ets.lookup(cache_name, key) do + [{^key, _value, :infinity}] -> {:ok, nil} + [{^key, _value, expires_at}] -> {:ok, max(0, expires_at - System.monotonic_time(:millisecond))} + [{^key, _value}] -> {:ok, nil} + [] -> {:ok, nil} + end + rescue + ArgumentError -> {:ok, nil} end @doc """ @@ -92,108 +126,4 @@ defmodule Aprsme.Cache do {:milliseconds, n}, acc -> acc + n end) end - - @impl true - def handle_call({:get, cache_name, key}, _from, state) do - result = - try do - case :ets.lookup(cache_name, key) do - [{^key, value, :infinity}] -> - {:ok, value} - - [{^key, value, expires_at}] -> - if System.monotonic_time(:millisecond) < expires_at do - {:ok, value} - else - :ets.delete(cache_name, key) - {:ok, nil} - end - - [{^key, value}] -> - {:ok, value} - - [] -> - {:ok, nil} - end - rescue - ArgumentError -> - {:error, :no_cache} - end - - {:reply, result, state} - end - - @impl true - def handle_call({:put, cache_name, key, value, opts}, _from, state) do - result = - try do - expires_at = - case Keyword.get(opts, :ttl) do - nil -> :infinity - ttl when is_integer(ttl) and ttl > 0 -> System.monotonic_time(:millisecond) + ttl - _ -> :infinity - end - - :ets.insert(cache_name, {key, value, expires_at}) - {:ok, true} - rescue - ArgumentError -> {:error, :no_cache} - end - - {:reply, result, state} - end - - @impl true - def handle_call({:del, cache_name, key}, _from, state) do - result = - try do - :ets.delete(cache_name, key) - {:ok, true} - rescue - ArgumentError -> {:error, :no_cache} - end - - {:reply, result, state} - end - - @impl true - def handle_call({:clear, cache_name}, _from, state) do - result = - try do - :ets.delete_all_objects(cache_name) - {:ok, true} - rescue - ArgumentError -> {:error, :no_cache} - end - - {:reply, result, state} - end - - @impl true - def handle_call({:stats, cache_name}, _from, state) do - result = - case :ets.info(cache_name) do - :undefined -> {:error, :no_cache} - info when is_list(info) -> {:ok, %{size: Keyword.get(info, :size, 0)}} - end - - {:reply, result, state} - end - - @impl true - def handle_call({:ttl, cache_name, key}, _from, state) do - result = - try do - case :ets.lookup(cache_name, key) do - [{^key, _value, :infinity}] -> {:ok, nil} - [{^key, _value, expires_at}] -> {:ok, max(0, expires_at - System.monotonic_time(:millisecond))} - [{^key, _value}] -> {:ok, nil} - [] -> {:ok, nil} - end - rescue - ArgumentError -> {:ok, nil} - end - - {:reply, result, state} - end end diff --git a/lib/aprsme_web/live/info_live/show.ex b/lib/aprsme_web/live/info_live/show.ex index a09c9dc..af1ac6a 100644 --- a/lib/aprsme_web/live/info_live/show.ex +++ b/lib/aprsme_web/live/info_live/show.ex @@ -86,6 +86,8 @@ defmodule AprsmeWeb.InfoLive.Show do |> assign(:callsign, normalized_callsign) |> assign(:packet, packet) |> assign(:neighbors, neighbors) + |> assign(:heard_by_stations, []) + |> assign(:stations_heard_by, []) |> assign(:page_title, "APRS station #{normalized_callsign}") |> assign(:has_weather_packets, has_weather_packets) |> assign(:other_ssids, other_ssids) @@ -398,11 +400,11 @@ defmodule AprsmeWeb.InfoLive.Show do MIN(pp.received_at) as first_heard, MAX(pp.received_at) as last_heard, COUNT(*) as packet_count, - MAX(lp.longest_path_packet_id) as longest_path_packet_id + lp.longest_path_packet_id FROM parsed_paths pp LEFT JOIN longest_paths lp ON lp.first_digipeater = pp.first_digipeater WHERE pp.first_digipeater IS NOT NULL - GROUP BY pp.first_digipeater + GROUP BY pp.first_digipeater, lp.longest_path_packet_id ) SELECT ds.digipeater, diff --git a/lib/aprsme_web/live/map_live/index.ex b/lib/aprsme_web/live/map_live/index.ex index 56e25bc..421a399 100644 --- a/lib/aprsme_web/live/map_live/index.ex +++ b/lib/aprsme_web/live/map_live/index.ex @@ -36,8 +36,6 @@ defmodule AprsmeWeb.MapLive.Index do @impl true def mount(params, session, socket) do - socket = setup_subscriptions(socket) - # Basic setup deployed_at = Aprsme.Release.deployed_at() one_day_ago = TimeUtils.hours_ago(24) @@ -86,6 +84,7 @@ defmodule AprsmeWeb.MapLive.Index do # Calculate initial bounds initial_bounds = BoundsUtils.calculate_bounds_from_center_and_zoom(final_map_center, final_map_zoom) + socket = setup_subscriptions(socket, initial_bounds) # Final socket assignment {:ok, @@ -100,11 +99,11 @@ defmodule AprsmeWeb.MapLive.Index do })} end - defp setup_subscriptions(socket) do - do_setup_subscriptions(socket, connected?(socket)) + defp setup_subscriptions(socket, initial_bounds) do + do_setup_subscriptions(socket, initial_bounds, connected?(socket)) end - defp do_setup_subscriptions(socket, true) do + defp do_setup_subscriptions(socket, initial_bounds, true) do # Check if we should accept new connections if Application.get_env(:aprsme, :cluster_enabled, false) and not Aprsme.ConnectionMonitor.accepting_connections?() do @@ -125,10 +124,10 @@ defmodule AprsmeWeb.MapLive.Index do Phoenix.PubSub.subscribe(Aprsme.PubSub, "connection:drain:#{Node.self()}") end - # Register with spatial PubSub (will get viewport later) - # Start with a default viewport that will be updated when we get actual bounds - default_bounds = %{north: 90.0, south: -90.0, east: 180.0, west: -180.0} - {:ok, spatial_topic} = Aprsme.SpatialPubSub.register_viewport(client_id, default_bounds) + # Register the actual initial viewport. Registering world bounds here used to + # allocate roughly 65,000 one-degree grid cells for every new connection, + # only to tear them down again after the client sent its real bounds. + {:ok, spatial_topic} = Aprsme.SpatialPubSub.register_viewport(client_id, initial_bounds) # Subscribe to the spatial topic for this client :ok = Phoenix.PubSub.subscribe(Aprsme.PubSub, spatial_topic) @@ -140,7 +139,7 @@ defmodule AprsmeWeb.MapLive.Index do :ok = Phoenix.PubSub.subscribe(Aprsme.PubSub, "deployment_events") # Subscribe to StreamingPacketsPubSub with initial bounds - _ = Aprsme.StreamingPacketsPubSub.subscribe_to_bounds(self(), default_bounds) + _ = Aprsme.StreamingPacketsPubSub.subscribe_to_bounds(self(), initial_bounds) _ = Process.send_after(self(), :cleanup_old_packets, 60_000) @@ -150,7 +149,7 @@ defmodule AprsmeWeb.MapLive.Index do end end - defp do_setup_subscriptions(socket, false), do: socket + defp do_setup_subscriptions(socket, _initial_bounds, false), do: socket defp setup_additional_subscriptions(socket) do do_setup_additional_subscriptions(socket, connected?(socket)) diff --git a/test/aprsme/connection_prevention_test.exs b/test/aprsme/connection_prevention_test.exs index 2600d4a..059f79d 100644 --- a/test/aprsme/connection_prevention_test.exs +++ b/test/aprsme/connection_prevention_test.exs @@ -19,13 +19,13 @@ defmodule Aprsme.ConnectionPreventionTest do test "APRS configuration is safe for testing" do # Verify server configuration is neutralized - server = Application.get_env(:aprsme, :aprsme_is_server) + server = Application.get_env(:aprsme, :aprs_is_server) # credo:disable-for-next-line Jump.CredoChecks.ConditionalAssertion assert server == nil or server == "mock.aprs.test" # server config is legitimately nil or mock value in test # Verify test credentials are used - login_id = Application.get_env(:aprsme, :aprsme_is_login_id) + login_id = Application.get_env(:aprsme, :aprs_is_login_id) assert login_id == "TEST" # Verify no real APRS servers in config