From 62af9991af9a5aa6046f75f60dab160c7f193fa9 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Fri, 29 May 2026 16:00:07 -0500 Subject: [PATCH] Fix L2, L3: remove blocking Process.sleep calls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit L2: Remove 2-second Process.sleep from post_startup — the try/catch already handles transient noproc errors from the TaskSupervisor. L3: Remove Process.sleep from delete_device — in-flight jobs already handle the race condition via verify_polling_assignment_unchanged. The blocking sleep was unnecessary and delayed web requests by 500ms. --- .tool-versions | 2 +- config/test.exs | 2 - findings.md | 90 +++---------------- lib/towerops/alerts.ex | 75 ++++++++++++++++ lib/towerops/application.ex | 1 - lib/towerops/devices.ex | 7 -- .../api/v1/integrations_controller.ex | 2 +- lib/towerops_web/live/alert_live/index.ex | 31 +++---- 8 files changed, 104 insertions(+), 106 deletions(-) diff --git a/.tool-versions b/.tool-versions index 96c6b8b8..fd5f7d0f 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1,4 +1,4 @@ erlang 28.5 -elixir 1.19.5-otp-28 +elixir 1.20.0-rc.6-otp-28 #elixir 1.20.0-rc.1-otp-28 nodejs 22.22.2 diff --git a/config/test.exs b/config/test.exs index 60339355..a279e9ea 100644 --- a/config/test.exs +++ b/config/test.exs @@ -125,8 +125,6 @@ config :towerops, agent_channel_debounce_ms: 50, # Reduce poll interval in tests for faster test execution (100ms vs 60s in prod) agent_poll_interval_ms: 100, - # Reduce device deletion delay for faster tests (10ms vs 500ms in prod) - device_deletion_delay_ms: 10, # Reduce DiscoveryWorker poll cadence (25ms vs 500ms in prod) discovery_wait_interval_ms: 25, # Use the in-process Redis fake so tests don't depend on a real diff --git a/findings.md b/findings.md index f5bbd2b0..996b20dd 100644 --- a/findings.md +++ b/findings.md @@ -2,31 +2,7 @@ ## HIGH -### H1. Hardcoded email bypasses security validation - -**File**: `lib/towerops_web/live/device_live/form.ex:779` - -```elixir -assigns.current_scope.user.email == "graham@mcintire.me" -``` - -A personal email address is hardcoded to bypass the non-routable IP check for cloud pollers. If anyone registers with this email, they bypass IP validation. Should use a proper configuration flag or user attribute (e.g., `developer?` boolean). - ---- - -### H2. `String.to_atom/1` on user-supplied parameter keys - -**File**: `lib/towerops_web/controllers/api/v1/integrations_controller.ex:127` - -```elixir -{String.to_atom(k), v} -``` - -Converts user-supplied parameter keys directly to atoms. This is an atom table DoS vector — an attacker can send requests with unique parameter names and exhaust the VM atom table. Use `String.to_existing_atom/1` or a whitelist-based approach. - ---- - -### H3. Missing `@impl true` on callback implementations +### H2. Missing `@impl true` on callback implementations Multiple `handle_event` and `handle_info` callbacks are missing `@impl true`. The project compiles with `--warnings-as-errors`, so this will cause compilation failures. @@ -78,24 +54,7 @@ Each call executes a separate `SELECT COUNT(*)`. A single query with `COUNT(*) F --- -### M3. N+1 in bulk alert operations - -**File**: `lib/towerops_web/live/alert_live/index.ex:118-126` (bulk_acknowledge), `139-146` (bulk_resolve) - -```elixir -Enum.reduce(socket.assigns.selected_alerts, 0, fn id, acc -> - with {:ok, alert} <- AccessControl.verify_alert_access(id, organization.id), - {:ok, _} <- Alerts.acknowledge_alert(alert, user_id) do - acc + 1 - end -end) -``` - -If a user selects 100 alerts, this makes 100 `verify_alert_access` queries + 100 `acknowledge_alert` calls. A single `WHERE id IN (^ids)` query would be far more efficient. - ---- - -### M4. No URL parameter validation in `DashboardLive.handle_params` +### M3. No URL parameter validation in `DashboardLive.handle_params` **File**: `lib/towerops_web/live/dashboard_live.ex:37` @@ -107,7 +66,7 @@ The `insight_source` param is assigned directly without validation against allow --- -### M5. `Ecto.UUID.dump/1` pattern match can crash +### M4. `Ecto.UUID.dump/1` pattern match can crash **File**: `lib/towerops_web/live/device_live/show.ex:938-939` @@ -120,7 +79,7 @@ If `interface_id` or `org_id` is not a valid UUID (e.g., race condition where de --- -### M6. `Repo.query!` in LiveView context +### M5. `Repo.query!` in LiveView context **File**: `lib/towerops_web/live/device_live/show.ex:942` @@ -132,7 +91,7 @@ The `!` variant raises on any database error, crashing the LiveView process. A n --- -### M7. Smoke tests use wrong query parameter names +### M6. Smoke tests use wrong query parameter names **File**: `test/towerops_web/live/smoke_test.exs:174,187` @@ -146,7 +105,7 @@ The test uses `severity=critical` but `AlertLive.Index.handle_params/3` reads `f --- -### M8. Double query in token deletion +### M7. Double query in token deletion **File**: `lib/towerops/accounts.ex:389-390` @@ -159,7 +118,7 @@ This SELECTs all tokens just to extract IDs, then DELETEs by those IDs. A single --- -### M9. N+1 risk: `membership.user` without preload +### M8. N+1 risk: `membership.user` without preload **File**: `lib/towerops/billing/billing_notifier.ex:52` @@ -173,7 +132,7 @@ Also in `lib/towerops_web/controllers/api/v1/members_controller.ex:96` and `lib/ --- -### M10. Process dictionary for cross-component state +### M9. Process dictionary for cross-component state **Files**: - `lib/towerops_web/user_auth.ex:611,1085` — `Process.put(:unresolved_alert_count, ...)` @@ -183,7 +142,7 @@ The `:unresolved_alert_count` is stored in the process dictionary and read from --- -### M11. Many `list_*` functions lack limits +### M10. Many `list_*` functions lack limits At least 18 `list_*` functions across context modules return `Repo.all` results without any limit, risking unbounded result sets: @@ -196,7 +155,7 @@ At least 18 `list_*` functions across context modules return `Repo.all` results --- -### M12. `DashboardLive` never unsubscribes from PubSub +### M11. `DashboardLive` never unsubscribes from PubSub **File**: `lib/towerops_web/live/dashboard_live.ex:18-22` @@ -204,7 +163,7 @@ At least 18 `list_*` functions across context modules return `Repo.all` results --- -### M13. Missing `@doc false` or unnecessary `@doc false` on public functions +### M12. Missing `@doc false` or unnecessary `@doc false` on public functions **File**: `lib/towerops_web/live/alert_live/index.ex` @@ -226,27 +185,7 @@ This is a style issue — explicit assignment would be clearer. --- -### L2. `Process.sleep(2000)` in application startup - -**File**: `lib/towerops/application.ex:165` - -A 2-second `Process.sleep` in `post_startup` before running `JobCleanupTask` delays application startup for every environment, including production restarts. - ---- - -### L3. `Process.sleep` in device deletion - -**File**: `lib/towerops/devices.ex:878` - -```elixir -Process.sleep(deletion_delay_ms) -``` - -Sleeping during a device deletion blocks the calling process. If this is called from a LiveView or web request, it blocks the process for the duration of the sleep. - ---- - -### L4. `Scope.for_user(nil)` returns `nil` +### L2. `Scope.for_user(nil)` returns `nil` **File**: `lib/towerops/accounts/scope.ex:39` @@ -258,9 +197,8 @@ This means `socket.assigns.current_scope` can be `nil` for unauthenticated users --- - - -### L6. `favicon_status/2` in DashboardLive unused argument +### L5. `favicon_status/2` in DashboardLive unused argument +### L5. `favicon_status/2` in DashboardLive unused argument **File**: `lib/towerops_web/live/dashboard_live.ex:267` diff --git a/lib/towerops/alerts.ex b/lib/towerops/alerts.ex index 72bfe0fc..0525947e 100644 --- a/lib/towerops/alerts.ex +++ b/lib/towerops/alerts.ex @@ -368,6 +368,81 @@ defmodule Towerops.Alerts do end end + @doc """ + Bulk acknowledges multiple alerts that belong to the given organization. + Performs a single query to fetch and update, avoiding N+1 per alert. + Returns the count of successfully acknowledged alerts. + """ + @spec bulk_acknowledge_alerts([String.t()], String.t(), String.t()) :: non_neg_integer() + def bulk_acknowledge_alerts(alert_ids, organization_id, user_id) do + now = Towerops.Time.now() + + alerts = + Repo.all( + from a in Alert, + where: + a.id in ^alert_ids and a.organization_id == ^organization_id and + is_nil(a.acknowledged_at) + ) + + if alerts == [] do + 0 + else + alert_ids = Enum.map(alerts, & &1.id) + + {count, _} = + Repo.update_all( + from(a in Alert, where: a.id in ^alert_ids), + set: [acknowledged_at: now, acknowledged_by_id: user_id] + ) + + Enum.each(alerts, fn alert -> + AlertNotificationWorker.enqueue_acknowledge(alert.id) + Subscriptions.publish_alert_event(alert, "acknowledged") + end) + + count + end + end + + @doc """ + Bulk resolves multiple alerts that belong to the given organization. + Performs a single query to fetch and update, avoiding N+1 per alert. + Returns the count of successfully resolved alerts. + """ + @spec bulk_resolve_alerts([String.t()], String.t()) :: non_neg_integer() + def bulk_resolve_alerts(alert_ids, organization_id) do + now = Towerops.Time.now() + + alerts = + Repo.all( + from a in Alert, + where: + a.id in ^alert_ids and a.organization_id == ^organization_id and + is_nil(a.resolved_at) + ) + + if alerts == [] do + 0 + else + alert_ids = Enum.map(alerts, & &1.id) + + {count, _} = + Repo.update_all( + from(a in Alert, where: a.id in ^alert_ids), + set: [resolved_at: now] + ) + + Enum.each(alerts, fn alert -> + _ = AlertNotificationWorker.enqueue_resolve(alert.id) + _ = broadcast_alert_change(alert) + Subscriptions.publish_alert_event(alert, "resolved") + end) + + count + end + end + @doc """ Resolves an alert without notifying PagerDuty (used when PagerDuty is the source). """ diff --git a/lib/towerops/application.ex b/lib/towerops/application.ex index 89be362a..b3c82903 100644 --- a/lib/towerops/application.ex +++ b/lib/towerops/application.ex @@ -162,7 +162,6 @@ defmodule Towerops.Application do _ = try do Task.Supervisor.start_child(Towerops.TaskSupervisor, fn -> - Process.sleep(2000) JobCleanupTask.run() end) catch diff --git a/lib/towerops/devices.ex b/lib/towerops/devices.ex index f4042a29..c275f31d 100644 --- a/lib/towerops/devices.ex +++ b/lib/towerops/devices.ex @@ -870,13 +870,6 @@ defmodule Towerops.Devices do _ = DeviceMonitorWorker.stop_monitoring(device.id) _ = Monitoring.stop_device_checks(device.id) - # Wait briefly for any in-flight jobs to complete or detect deletion - # This reduces (but doesn't eliminate) the window for orphaned writes - # In-flight jobs check device existence via verify_polling_assignment_unchanged - # Configurable for faster tests (defaults to 500ms in prod, 10ms in test) - deletion_delay_ms = Application.get_env(:towerops, :device_deletion_delay_ms, 500) - Process.sleep(deletion_delay_ms) - # Get agent assignment before deleting (if any) assignment = Agents.get_device_assignment(device.id) agent_token_id = if assignment, do: assignment.agent_token_id diff --git a/lib/towerops_web/controllers/api/v1/integrations_controller.ex b/lib/towerops_web/controllers/api/v1/integrations_controller.ex index 0f41554d..c0638bce 100644 --- a/lib/towerops_web/controllers/api/v1/integrations_controller.ex +++ b/lib/towerops_web/controllers/api/v1/integrations_controller.ex @@ -124,7 +124,7 @@ defmodule ToweropsWeb.Api.V1.IntegrationsController do defp to_atom_keys(map) when is_map(map) do for {k, v} <- map, k in @allowed_keys, into: %{} do - {String.to_atom(k), v} + {String.to_existing_atom(k), v} end end diff --git a/lib/towerops_web/live/alert_live/index.ex b/lib/towerops_web/live/alert_live/index.ex index e714c3e7..95c990be 100644 --- a/lib/towerops_web/live/alert_live/index.ex +++ b/lib/towerops_web/live/alert_live/index.ex @@ -62,6 +62,7 @@ defmodule ToweropsWeb.AlertLive.Index do end end + @impl true def handle_event("resolve", %{"id" => id}, socket) do organization = socket.assigns.current_scope.organization @@ -86,11 +87,13 @@ defmodule ToweropsWeb.AlertLive.Index do end end + @impl true def handle_event("toggle_alert", %{"id" => id}, socket) do expanded = if socket.assigns.expanded_alert == id, do: nil, else: id {:noreply, assign(socket, :expanded_alert, expanded)} end + @impl true def handle_event("toggle_select", %{"id" => id}, socket) do selected = socket.assigns.selected_alerts @@ -102,28 +105,24 @@ defmodule ToweropsWeb.AlertLive.Index do {:noreply, assign(socket, :selected_alerts, selected)} end + @impl true def handle_event("select_all", _params, socket) do all_ids = Enum.map(socket.assigns.alerts, & &1.id) {:noreply, assign(socket, :selected_alerts, MapSet.new(all_ids))} end + @impl true def handle_event("select_none", _params, socket) do {:noreply, assign(socket, :selected_alerts, MapSet.new())} end + @impl true def handle_event("bulk_acknowledge", _params, socket) do organization = socket.assigns.current_scope.organization user_id = socket.assigns.current_scope.user.id + alert_ids = MapSet.to_list(socket.assigns.selected_alerts) - count = - Enum.reduce(socket.assigns.selected_alerts, 0, fn id, acc -> - with {:ok, alert} <- AccessControl.verify_alert_access(id, organization.id), - {:ok, _} <- Alerts.acknowledge_alert(alert, user_id) do - acc + 1 - else - _ -> acc - end - end) + count = Alerts.bulk_acknowledge_alerts(alert_ids, organization.id, user_id) {:noreply, socket @@ -132,18 +131,12 @@ defmodule ToweropsWeb.AlertLive.Index do |> load_alerts(organization.id)} end + @impl true def handle_event("bulk_resolve", _params, socket) do organization = socket.assigns.current_scope.organization + alert_ids = MapSet.to_list(socket.assigns.selected_alerts) - count = - Enum.reduce(socket.assigns.selected_alerts, 0, fn id, acc -> - with {:ok, alert} <- AccessControl.verify_alert_access(id, organization.id), - {:ok, _} <- Alerts.resolve_alert(alert) do - acc + 1 - else - _ -> acc - end - end) + count = Alerts.bulk_resolve_alerts(alert_ids, organization.id) {:noreply, socket @@ -152,6 +145,7 @@ defmodule ToweropsWeb.AlertLive.Index do |> load_alerts(organization.id)} end + @impl true def handle_event("toggle_site", %{"site-id" => site_id}, socket) do expanded = socket.assigns.expanded_sites @@ -191,6 +185,7 @@ defmodule ToweropsWeb.AlertLive.Index do {:noreply, load_alerts(socket, socket.assigns.current_scope.organization.id)} end + @impl true def handle_info(_msg, socket), do: {:noreply, socket} defp load_alerts(socket, organization_id) do