diff --git a/bugs.md b/bugs.md index 89a9f904..218c9b32 100644 --- a/bugs.md +++ b/bugs.md @@ -6,66 +6,6 @@ ## HIGH -### H1. N+1 Queries in MobileController - -**File:** `lib/towerops_web/controllers/api/mobile_controller.ex:38-46,78-84` - -**Severity:** HIGH — 3N + 2M extra queries per request - -**Description:** For each organization: 3 count queries (sites, devices, active alerts). For each site: 2 count queries (total devices, down devices). 1 + 3N + 2M DB round-trips. - -**Fix:** Use batch count queries as already partially implemented in `devices.ex:98`. - ---- - -### H2. N+1 Oban `cancel_all_jobs` per Check - -**File:** `lib/towerops/monitoring.ex:463-473` - -**Severity:** HIGH — One DB round-trip per check when stopping device checks - -**Description:** Loops through check_ids calling `Oban.cancel_all_jobs` individually. A device with 50 checks = 50 queries. Should batch all check_ids into a single query using `ANY(?)`. - -**Fix:** Use `fragment("args->>'check_id' = ANY(?)", ^check_ids)` in a single query. - ---- - -### H3. N+1 Alert Resolution - -**File:** `lib/towerops/alerts.ex:381-388` - -**Severity:** HIGH — One DB write per alert when resolving - -**Description:** Loads all alerts then `Enum.each` calls `resolve_alert_silent()` per alert. For 1000 alerts in a storm: 1000 individual `Repo.update` calls + 1000 PubSub broadcasts. - -**Fix:** Use `Repo.update_all` with a single query. - ---- - -### H4. Race Condition in `create_check` (Auto-Discovery) - -**File:** `lib/towerops/monitoring.ex:152-184` - -**Severity:** HIGH — Concurrent discoveries crash the job with Ecto.ConstraintError - -**Description:** Two concurrent discoveries can both get `nil` from `get_by` and both attempt `insert`. The unique index prevents duplicates but one crashes with unhandled `Ecto.ConstraintError`. - -**Fix:** Use `Repo.insert(..., on_conflict: :nothing)` or wrap in transaction with `FOR UPDATE`. - ---- - -### H5. Missing Transaction in `apply_agent_to_all_equipment` - -**File:** `lib/towerops/sites.ex:223-252` - -**Severity:** HIGH — Partial data loss on failure - -**Description:** `delete_all` runs before `insert_all`. If insert fails, assignments are lost without rollback. - -**Fix:** Wrap in `Repo.transaction`. - ---- - ### H6. N+1 Interface Capacity Queries **File:** `lib/towerops/capacity.ex:53-83` @@ -115,18 +55,6 @@ --- -### H10. Missing HSTS Header - -**File:** `lib/towerops_web/plugs/security_headers.ex:22-29` - -**Severity:** HIGH — SSL stripping attacks possible - -**Description:** Security headers plug sets CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy — but NOT `Strict-Transport-Security`. - -**Fix:** Add `strict-transport-security: max-age=63072000; includeSubDomains; preload`. - ---- - ### H11. Hardcoded Session Salts (8 bytes) **File:** `lib/towerops_web/endpoint.ex:11-12` @@ -165,47 +93,6 @@ --- -### H14. `String.to_integer/1` on Unvalidated SNMP OID Components - -**Files:** -- `lib/towerops/snmp/neighbor_discovery.ex:202,299,129` -- `lib/towerops/snmp/discovery.ex:1368` -- `lib/towerops/snmp/profiles/base.ex:1332` - -**Severity:** HIGH — Malicious SNMP responses crash GenServers (DoS) - -**Description:** OID suffixes extracted from SNMP responses are passed directly to `String.to_integer/1`. An attacker-controlled device can send non-numeric OID fragments, crashing the poller process. - -**Fix:** Replace with `Integer.parse/1` + pattern match. Never trust OID suffixes from network data. - ---- - -### H15. `String.to_existing_atom/1` on User Input in ActivityFeed - -**File:** `lib/towerops/activity_feed_live.ex:55` - -**Severity:** HIGH — Any user can crash the activity feed LiveView (DoS) - -**Description:** `String.to_existing_atom(type_str)` where `type_str` comes from client event params. If the atom doesn't exist, `ArgumentError` kills the LiveView process. - -**Fix:** Use a whitelist of allowed types. - ---- - -### H16. `String.to_integer/1` on User-Controlled Page Param - -**Files:** -- `lib/towerops_web/live/user_settings_live.ex:63` -- `lib/towerops_web/live/admin/dashboard_live.ex:19` - -**Severity:** HIGH — Non-integer page param crashes LiveView (DoS) - -**Description:** `params |> Map.get("page", "1") |> String.to_integer()` — passing `?page=abc` raises an error killing the process. - -**Fix:** Use `Integer.parse` with fallback to 1. - ---- - ### H17. Reports LiveView — Missing Org Scope on Individual Operations **File:** `lib/towerops/reports_live.ex:73-101` @@ -331,18 +218,6 @@ --- -### H27. Backslash-Wildcard Injection in Search - -**File:** `lib/towerops/search.ex:90-94` - -**Severity:** HIGH — Search bypass via backslash injection leaks wildcard matching - -**Description:** `sanitize/1` escapes `%` and `_` but NOT `\` first. A search for `\%%` becomes `\\%\\%` — PG interprets `\\` as literal `\` followed by `%` wildcard, leaking wildcard matching. - -**Fix:** Escape `\` first, or reuse `Towerops.QueryHelpers.sanitize_like/1`. - ---- - ### H28. Nominatim API Response Injected via innerHTML **File:** `assets/js/hooks/coverage_hooks.ts:635-645` @@ -367,18 +242,6 @@ --- -### H30. JobCleanupTask Cancels Executing Jobs - -**File:** `lib/towerops/workers/job_cleanup_task.ex:47-63` - -**Severity:** HIGH — Rolling deployments kill currently executing poll jobs - -**Description:** `cancel_all_jobs` filters on `["available", "scheduled", "executing", "retryable"]` states. In-progress polling/monitoring is discarded. - -**Fix:** Exclude `"executing"` state so active jobs complete naturally. - ---- - ## MEDIUM ### M1. Resource Scoping Reveals Org Membership diff --git a/lib/towerops/alerts.ex b/lib/towerops/alerts.ex index cde20ec7..2a0df6f4 100644 --- a/lib/towerops/alerts.ex +++ b/lib/towerops/alerts.ex @@ -161,6 +161,25 @@ defmodule Towerops.Alerts do ) end + @doc """ + Batch count active (unresolved) device_down alerts for multiple organizations. + + Returns a map of `%{organization_id => count}`. Organizations with zero + active alerts are omitted; callers should use `Map.get(map, org_id, 0)`. + """ + @spec batch_count_active_alerts([String.t()]) :: %{String.t() => non_neg_integer()} + def batch_count_active_alerts(organization_ids) when is_list(organization_ids) do + from(a in Alert, + where: a.organization_id in ^organization_ids, + where: a.alert_type == "device_down", + where: is_nil(a.resolved_at), + group_by: a.organization_id, + select: {a.organization_id, count(a.id)} + ) + |> Repo.all() + |> Map.new() + end + @doc "Returns the count of active alerts for devices at a specific site." def count_site_active_alerts(site_id) do Repo.aggregate( @@ -379,12 +398,26 @@ defmodule Towerops.Alerts do spurious "resolved" event. """ def resolve_active_alerts_for_device(device_id) do - from(a in Alert, - where: a.device_id == ^device_id, - where: is_nil(a.resolved_at) - ) - |> Repo.all() - |> Enum.each(&resolve_alert_silent/1) + now = Towerops.Time.now() + + {_count, org_ids} = + Repo.update_all( + from(a in Alert, where: a.device_id == ^device_id, where: is_nil(a.resolved_at), select: a.organization_id), + set: [resolved_at: now, updated_at: now] + ) + + org_ids + |> Enum.reject(&is_nil/1) + |> Enum.uniq() + |> Enum.each(fn org_id -> + Phoenix.PubSub.broadcast( + Towerops.PubSub, + "organization:#{org_id}:alerts", + {:alert_changed, org_id} + ) + end) + + :ok end # Performs the database update for resolving an alert diff --git a/lib/towerops/devices.ex b/lib/towerops/devices.ex index 4ccc5e53..f4042a29 100644 --- a/lib/towerops/devices.ex +++ b/lib/towerops/devices.ex @@ -80,6 +80,23 @@ defmodule Towerops.Devices do |> Repo.aggregate(:count) end + @doc """ + Batch count devices for multiple organizations. + + Returns a map of `%{organization_id => count}`. Organizations with zero + devices are omitted; callers should use `Map.get(map, org_id, 0)`. + """ + @spec batch_count_organization_devices([String.t()]) :: %{String.t() => non_neg_integer()} + def batch_count_organization_devices(organization_ids) when is_list(organization_ids) do + from(d in DeviceSchema, + where: d.organization_id in ^organization_ids, + group_by: d.organization_id, + select: {d.organization_id, count(d.id)} + ) + |> Repo.all() + |> Map.new() + end + @doc """ Returns the count of devices for a site. """ diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index 4e34d508..70aaca63 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -149,25 +149,9 @@ defmodule Towerops.Monitoring do instead of creating duplicates. """ def create_check(attrs \\ %{}) do - # Use upsert for auto-discovery checks to avoid duplicates result = if Map.get(attrs, :source_type) == "auto_discovery" and Map.has_key?(attrs, :source_id) do - # For partial unique indexes, find existing record and update it - case Repo.get_by(Check, - device_id: attrs.device_id, - check_type: attrs.check_type, - source_id: attrs.source_id - ) do - nil -> - %Check{} - |> Check.changeset(attrs) - |> Repo.insert() - - existing_check -> - existing_check - |> Check.changeset(attrs) - |> Repo.update() - end + upsert_auto_discovery_check(attrs) else %Check{} |> Check.changeset(attrs) @@ -183,6 +167,22 @@ defmodule Towerops.Monitoring do result end + # Atomic upsert against the partial unique index on + # (device_id, check_type, source_id) WHERE source_type = 'auto_discovery' + # AND source_id IS NOT NULL. Avoids the get-then-insert race where two + # concurrent discoveries would both see `nil` and one would crash with + # Ecto.ConstraintError. + defp upsert_auto_discovery_check(attrs) do + %Check{} + |> Check.changeset(attrs) + |> Repo.insert( + on_conflict: {:replace_all_except, [:id, :inserted_at]}, + conflict_target: + {:unsafe_fragment, + "(device_id, check_type, source_id) WHERE source_type = 'auto_discovery' AND source_id IS NOT NULL"} + ) + end + @doc """ Updates a check. """ @@ -452,25 +452,27 @@ defmodule Towerops.Monitoring do Used when deleting a device to clean up scheduled check executions. """ def stop_device_checks(device_id) do - # Get all check IDs for this device check_ids = device_id |> CheckQuery.for_device() |> select([c], c.id) |> Repo.all() - # Cancel all scheduled jobs for these checks - Enum.each(check_ids, fn check_id -> - Oban.cancel_all_jobs( - from(j in Oban.Job, - where: j.queue == "check_executors", - where: j.state in ["available", "scheduled", "retryable"], - where: fragment("? @> ?", j.args, ^%{"check_id" => check_id}) - ) - ) - end) + case check_ids do + [] -> + :ok - :ok + ids -> + Oban.cancel_all_jobs( + from(j in Oban.Job, + where: j.queue == "check_executors", + where: j.state in ["available", "scheduled", "retryable"], + where: fragment("?->>'check_id' = ANY(?)", j.args, ^ids) + ) + ) + + :ok + end end @doc """ diff --git a/lib/towerops/search.ex b/lib/towerops/search.ex index f3f22608..3d2bcbdc 100644 --- a/lib/towerops/search.ex +++ b/lib/towerops/search.ex @@ -87,9 +87,5 @@ defmodule Towerops.Search do end) end - defp sanitize(query) do - query - |> String.replace("%", "\\%") - |> String.replace("_", "\\_") - end + defp sanitize(query), do: Towerops.QueryHelpers.sanitize_like(query) end diff --git a/lib/towerops/sites.ex b/lib/towerops/sites.ex index 98b51097..f6760b0a 100644 --- a/lib/towerops/sites.ex +++ b/lib/towerops/sites.ex @@ -57,6 +57,23 @@ defmodule Towerops.Sites do |> Repo.aggregate(:count) end + @doc """ + Batch count sites for multiple organizations. + + Returns a map of `%{organization_id => count}`. Organizations with zero sites + are omitted from the map; callers should use `Map.get(map, org_id, 0)`. + """ + @spec batch_count_organization_sites([String.t()]) :: %{String.t() => non_neg_integer()} + def batch_count_organization_sites(organization_ids) when is_list(organization_ids) do + from(s in Site, + where: s.organization_id in ^organization_ids, + group_by: s.organization_id, + select: {s.organization_id, count(s.id)} + ) + |> Repo.all() + |> Map.new() + end + @doc """ Returns the list of root sites (sites without a parent) for an organization. Ordered by custom display_order (if set), then alphabetically by name. @@ -222,33 +239,40 @@ defmodule Towerops.Sites do """ def apply_agent_to_all_equipment(site_id) do site = Repo.get!(Site, site_id) - - # device IDs for this site device_ids = Repo.all(from(e in Device, where: e.site_id == ^site_id, select: e.id)) - if site.agent_token_id do - # Delete existing assignments - Repo.delete_all(from(a in AgentAssignment, where: a.device_id in ^device_ids)) + # Wrap delete + insert in a transaction so an insert_all failure doesn't + # leave the site with empty assignments — either all old assignments are + # replaced atomically or the prior state is preserved. + {:ok, result} = + Repo.transaction(fn -> + Repo.delete_all(from(a in AgentAssignment, where: a.device_id in ^device_ids)) + reassign_devices(device_ids, site.agent_token_id) + end) - # Create new assignments - now = Towerops.Time.now() + result + end - assignments = - Enum.map(device_ids, fn device_id -> - %{ - device_id: device_id, - agent_token_id: site.agent_token_id, - inserted_at: now, - updated_at: now - } - end) + defp reassign_devices(device_ids, nil) do + # No agent set; assignments stay deleted (devices inherit from org or use cloud). + {length(device_ids), nil} + end - {count, _} = Repo.insert_all(AgentAssignment, assignments) - {count, nil} - else - # No agent set, delete all assignments (they will inherit from org or use cloud) - Repo.delete_all(from(a in AgentAssignment, where: a.device_id in ^device_ids)) - end + defp reassign_devices(device_ids, agent_token_id) do + now = Towerops.Time.now() + + assignments = + Enum.map(device_ids, fn device_id -> + %{ + device_id: device_id, + agent_token_id: agent_token_id, + inserted_at: now, + updated_at: now + } + end) + + {count, _} = Repo.insert_all(AgentAssignment, assignments) + {count, nil} end ## Display Order Management diff --git a/lib/towerops/snmp/discovery.ex b/lib/towerops/snmp/discovery.ex index 9a4ea4e1..5879eaf6 100644 --- a/lib/towerops/snmp/discovery.ex +++ b/lib/towerops/snmp/discovery.ex @@ -1318,12 +1318,21 @@ defmodule Towerops.Snmp.Discovery do |> Repo.all() |> Map.new(&{&1.storage_index, &1}) - # Get the set of discovered storage_indices (convert string index to integer) - discovered_indices = - MapSet.new(discovered_storage, fn s -> - normalize_storage_index(s.storage_index) + # Drop entries with non-integer storage_index (malformed OIDs from network) + {valid_storage, invalid_count} = + Enum.reduce(discovered_storage, {[], 0}, fn entry, {acc, dropped} -> + case normalize_storage_index(entry.storage_index) do + nil -> {acc, dropped + 1} + idx -> {[Map.put(entry, :storage_index, idx) | acc], dropped} + end end) + if invalid_count > 0 do + Logger.warning("Dropped #{invalid_count} storage entries with malformed storage_index") + end + + discovered_indices = MapSet.new(valid_storage, & &1.storage_index) + # Delete storage that no longer exists on the device existing_indices = MapSet.new(Map.keys(existing_storage)) removed_indices = MapSet.difference(existing_indices, discovered_indices) @@ -1340,11 +1349,8 @@ defmodule Towerops.Snmp.Discovery do end # Upsert each discovered storage - Enum.each(discovered_storage, fn storage_data -> - storage_index = normalize_storage_index(storage_data.storage_index) - storage_attrs = Map.put(storage_data, :storage_index, storage_index) - - case Map.get(existing_storage, storage_index) do + Enum.each(valid_storage, fn storage_attrs -> + case Map.get(existing_storage, storage_attrs.storage_index) do nil -> # New storage - insert %Storage{} @@ -1359,13 +1365,22 @@ defmodule Towerops.Snmp.Discovery do end end) - Logger.debug("Synced #{length(discovered_storage)} storage entries for device") + Logger.debug("Synced #{length(valid_storage)} storage entries for device") :ok end - # Normalize storage index to integer (Base.discover_storage may return string from OID parsing) + # Normalize storage index to integer (Base.discover_storage may return string from OID parsing). + # Returns nil for malformed values — OID suffixes come from SNMP responses and must not be trusted. defp normalize_storage_index(index) when is_integer(index), do: index - defp normalize_storage_index(index) when is_binary(index), do: String.to_integer(index) + + defp normalize_storage_index(index) when is_binary(index) do + case Integer.parse(index) do + {n, ""} -> n + _ -> nil + end + end + + defp normalize_storage_index(_), do: nil @spec sync_mempools(Device.t(), [map()]) :: :ok @doc """ diff --git a/lib/towerops/snmp/neighbor_discovery.ex b/lib/towerops/snmp/neighbor_discovery.ex index 2cc4fc30..3d25ea9f 100644 --- a/lib/towerops/snmp/neighbor_discovery.ex +++ b/lib/towerops/snmp/neighbor_discovery.ex @@ -109,32 +109,52 @@ defmodule Towerops.Snmp.NeighborDiscovery do # Parse IPv4 management address (addr_subtype = 1) defp parse_management_address(local_port, rem_index, "1", [_len | addr_bytes]) when length(addr_bytes) >= 4 do - # Take first 4 bytes for IPv4 - ip = - addr_bytes - |> Enum.take(4) - |> Enum.join(".") + case parse_address_bytes(addr_bytes, 4) do + nil -> + nil - {"#{local_port}.#{rem_index}", ip} + bytes -> + {"#{local_port}.#{rem_index}", Enum.join(bytes, ".")} + end end # Parse IPv6 management address (addr_subtype = 2) defp parse_management_address(local_port, rem_index, "2", [_len | addr_bytes]) when length(addr_bytes) >= 16 do - # Format IPv6 address - ip = - addr_bytes - |> Enum.take(16) - |> Enum.chunk_every(2) - |> Enum.map_join(":", fn [a, b] -> - Integer.to_string(String.to_integer(a) * 256 + String.to_integer(b), 16) - end) + case parse_address_bytes(addr_bytes, 16) do + nil -> + nil - {"#{local_port}.#{rem_index}", ip} + bytes -> + ip = + bytes + |> Enum.chunk_every(2) + |> Enum.map_join(":", fn [a, b] -> Integer.to_string(a * 256 + b, 16) end) + + {"#{local_port}.#{rem_index}", ip} + end end # Unknown address type or malformed defp parse_management_address(_local_port, _rem_index, _addr_subtype, _addr_parts), do: nil + # Take `count` byte values from a list of OID-component strings, returning + # a list of integers in 0..255 or nil if any component is malformed. SNMP + # responses come from the network and must never be trusted. + defp parse_address_bytes(parts, count) do + parts + |> Enum.take(count) + |> Enum.reduce_while([], fn part, acc -> + case Integer.parse(part) do + {n, ""} when n >= 0 and n <= 255 -> {:cont, [n | acc]} + _ -> {:halt, nil} + end + end) + |> case do + nil -> nil + bytes -> Enum.reverse(bytes) + end + end + defp parse_lldp_neighbors(entries, interfaces, mgmt_addresses) do # Convert map from Client.walk to list of maps entry_list = Enum.map(entries, fn {oid, value} -> %{oid: oid, value: value} end) @@ -199,7 +219,11 @@ defmodule Towerops.Snmp.NeighborDiscovery do defp extract_local_port(acc, oid) do parts = String.split(oid, ".") [_rem_index, local_port | _] = Enum.reverse(parts) - Map.put(acc, :local_port, String.to_integer(local_port)) + + case Integer.parse(local_port) do + {n, ""} -> Map.put(acc, :local_port, n) + _ -> acc + end end defp build_neighbor_record(_protocol, _neighbor_map, _interfaces, nil), do: nil @@ -296,7 +320,11 @@ defmodule Towerops.Snmp.NeighborDiscovery do defp extract_cdp_local_interface(acc, oid) do parts = String.split(oid, ".") [_device_idx, if_idx | _] = Enum.reverse(parts) - Map.put(acc, :local_if_index, String.to_integer(if_idx)) + + case Integer.parse(if_idx) do + {n, ""} -> Map.put(acc, :local_if_index, n) + _ -> acc + end end # Helper Functions diff --git a/lib/towerops/snmp/profiles/base.ex b/lib/towerops/snmp/profiles/base.ex index 73589941..ac0440cf 100644 --- a/lib/towerops/snmp/profiles/base.ex +++ b/lib/towerops/snmp/profiles/base.ex @@ -1291,19 +1291,25 @@ defmodule Towerops.Snmp.Profiles.Base do supplies = type_results |> Enum.map(fn {oid, type_code} -> - index = extract_printer_supply_index(oid) - type_int = parse_integer(type_code) + case extract_printer_supply_index(oid) do + nil -> + nil - %{ - supply_index: to_string(index), - supply_type: map_supply_type(type_int), - supply_description: to_string_or_nil(Map.get(description_map, index)), - supply_unit: map_supply_unit(parse_integer(Map.get(unit_map, index))), - max_capacity: parse_supply_capacity(Map.get(max_capacity_map, index)), - current_level: parse_supply_capacity(Map.get(level_map, index)), - color_name: to_string_or_nil(Map.get(color_map, index)) - } + index -> + type_int = parse_integer(type_code) + + %{ + supply_index: to_string(index), + supply_type: map_supply_type(type_int), + supply_description: to_string_or_nil(Map.get(description_map, index)), + supply_unit: map_supply_unit(parse_integer(Map.get(unit_map, index))), + max_capacity: parse_supply_capacity(Map.get(max_capacity_map, index)), + current_level: parse_supply_capacity(Map.get(level_map, index)), + color_name: to_string_or_nil(Map.get(color_map, index)) + } + end end) + |> Enum.reject(&is_nil/1) |> Enum.sort_by(& &1.supply_index) Logger.debug("Discovered #{length(supplies)} printer supplies from Printer-MIB") @@ -1313,23 +1319,30 @@ defmodule Towerops.Snmp.Profiles.Base do defp walk_printer_attribute(client_opts, oid) do case Client.walk(client_opts, oid) do {:ok, results} when is_map(results) -> - Map.new(results, fn {result_oid, value} -> - index = extract_printer_supply_index(result_oid) - {index, value} - end) + results + |> Enum.flat_map(&printer_attribute_entry/1) + |> Map.new() _ -> %{} end end + defp printer_attribute_entry({result_oid, value}) do + case extract_printer_supply_index(result_oid) do + nil -> [] + index -> [{index, value}] + end + end + defp extract_printer_supply_index(oid) when is_binary(oid) do - # Extract the last two parts: table.entry.column.HR_INDEX.SUPPLY_INDEX - # e.g., "1.3.6.1.2.1.43.11.1.1.3.1.1" -> "1" - oid - |> String.split(".") - |> List.last() - |> String.to_integer() + # Extract the last OID component: table.entry.column.HR_INDEX.SUPPLY_INDEX + # e.g., "1.3.6.1.2.1.43.11.1.1.3.1.1" -> 1. Returns nil for malformed OIDs; + # SNMP responses come from the network and must never be trusted. + case oid |> String.split(".") |> List.last() |> Integer.parse() do + {n, ""} -> n + _ -> nil + end end defp map_supply_type(type_code) when is_integer(type_code) do diff --git a/lib/towerops/workers/job_cleanup_task.ex b/lib/towerops/workers/job_cleanup_task.ex index 148f9203..d9d2cdb1 100644 --- a/lib/towerops/workers/job_cleanup_task.ex +++ b/lib/towerops/workers/job_cleanup_task.ex @@ -45,11 +45,15 @@ defmodule Towerops.Workers.JobCleanupTask do end defp cancel_all_polling_jobs do + # Exclude "executing" so in-flight polls/monitors complete naturally; + # cancelling them mid-run discards work and can leave partial state. + cancellable_states = ["available", "scheduled", "retryable"] + {:ok, poller_count} = Oban.cancel_all_jobs( from(j in Oban.Job, where: j.worker == "Towerops.Workers.DevicePollerWorker", - where: j.state in ["available", "scheduled", "executing", "retryable"] + where: j.state in ^cancellable_states ) ) @@ -57,7 +61,7 @@ defmodule Towerops.Workers.JobCleanupTask do Oban.cancel_all_jobs( from(j in Oban.Job, where: j.worker == "Towerops.Workers.DeviceMonitorWorker", - where: j.state in ["available", "scheduled", "executing", "retryable"] + where: j.state in ^cancellable_states ) ) diff --git a/lib/towerops_web/controllers/api/mobile_controller.ex b/lib/towerops_web/controllers/api/mobile_controller.ex index e2e5c8b5..2a836315 100644 --- a/lib/towerops_web/controllers/api/mobile_controller.ex +++ b/lib/towerops_web/controllers/api/mobile_controller.ex @@ -31,17 +31,21 @@ defmodule ToweropsWeb.Api.MobileController do """ def list_organizations(conn, _params) do user = conn.assigns.current_user + orgs = Organizations.list_user_organizations(user.id) + org_ids = Enum.map(orgs, & &1.id) + + site_counts = Sites.batch_count_organization_sites(org_ids) + device_counts = Devices.batch_count_organization_devices(org_ids) + alert_counts = Alerts.batch_count_active_alerts(org_ids) organizations = - user.id - |> Organizations.list_user_organizations() - |> Enum.map(fn org -> + Enum.map(orgs, fn org -> %{ id: org.id, name: org.name, - sites_count: Sites.count_organization_sites(org.id), - device_count: Devices.count_organization_devices(org.id), - active_alerts_count: Alerts.count_active_alerts(org.id) + sites_count: Map.get(site_counts, org.id, 0), + device_count: Map.get(device_counts, org.id, 0), + active_alerts_count: Map.get(alert_counts, org.id, 0) } end) @@ -71,16 +75,20 @@ defmodule ToweropsWeb.Api.MobileController do case verify_organization_access(user, org_id) do {:ok, _org} -> + site_list = Sites.list_organization_sites(org_id) + site_ids = Enum.map(site_list, & &1.id) + counts = Devices.batch_count_site_devices(site_ids) + sites = - org_id - |> Sites.list_organization_sites() - |> Enum.map(fn site -> + Enum.map(site_list, fn site -> + %{total: total, down: down} = Map.get(counts, site.id, %{total: 0, down: 0}) + %{ id: site.id, name: site.name, location: site.location, - device_count: Devices.count_site_devices(site.id), - equipment_down_count: Devices.count_site_devices_down(site.id) + device_count: total, + equipment_down_count: down } end) diff --git a/lib/towerops_web/live/activity_feed_live.ex b/lib/towerops_web/live/activity_feed_live.ex index ac548888..7f103fa7 100644 --- a/lib/towerops_web/live/activity_feed_live.ex +++ b/lib/towerops_web/live/activity_feed_live.ex @@ -52,22 +52,27 @@ defmodule ToweropsWeb.ActivityFeedLive do @impl true def handle_event("toggle_type", %{"type" => type_str}, socket) do - type = String.to_existing_atom(type_str) - active = socket.assigns.active_types + case parse_activity_type(type_str) do + nil -> + {:noreply, socket} - new_active = - if type in active do - List.delete(active, type) - else - [type | active] - end + type -> + active = socket.assigns.active_types - {:noreply, - socket - |> assign(:active_types, new_active) - |> assign(:limit, @default_limit) - |> assign(:has_more, true) - |> load_activity()} + new_active = + if type in active do + List.delete(active, type) + else + [type | active] + end + + {:noreply, + socket + |> assign(:active_types, new_active) + |> assign(:limit, @default_limit) + |> assign(:has_more, true) + |> load_activity()} + end end @impl true @@ -153,6 +158,15 @@ defmodule ToweropsWeb.ActivityFeedLive do # --- Helpers --- + # Lookup against the static @all_types whitelist. Never use + # `String.to_existing_atom/1` on user-controlled input — an unknown atom + # raises ArgumentError and crashes the LiveView. + defp parse_activity_type(type_str) when is_binary(type_str) do + Enum.find(@all_types, fn t -> Atom.to_string(t) == type_str end) + end + + defp parse_activity_type(_), do: nil + defp filter_options do [ {:config_change, "Config", "hero-wrench-screwdriver"}, diff --git a/lib/towerops_web/live/admin/dashboard_live.ex b/lib/towerops_web/live/admin/dashboard_live.ex index 39ac62d8..afc270f0 100644 --- a/lib/towerops_web/live/admin/dashboard_live.ex +++ b/lib/towerops_web/live/admin/dashboard_live.ex @@ -16,7 +16,12 @@ defmodule ToweropsWeb.Admin.DashboardLive do @impl true def handle_params(params, _url, socket) do - page = params |> Map.get("page", "1") |> String.to_integer() + page = + case Integer.parse(Map.get(params, "page", "1")) do + {n, _} when n >= 1 -> n + _ -> 1 + end + per_page = 10 user_count = Admin.count_users() diff --git a/lib/towerops_web/live/user_settings_live.ex b/lib/towerops_web/live/user_settings_live.ex index e0089e7c..0253ca4a 100644 --- a/lib/towerops_web/live/user_settings_live.ex +++ b/lib/towerops_web/live/user_settings_live.ex @@ -59,8 +59,12 @@ defmodule ToweropsWeb.UserSettingsLive do # Default to "personal" tab if no tab param is provided tab = Map.get(params, "tab", "personal") - # Get page from params, default to 1 - page = params |> Map.get("page", "1") |> String.to_integer() + # Get page from params, default to 1 (defensive against non-integer input) + page = + case Integer.parse(Map.get(params, "page", "1")) do + {n, _} when n >= 1 -> n + _ -> 1 + end # Read modal state from URL modal = params["modal"] diff --git a/priv/repo/migrations/20260512105203_restore_auto_discovery_check_unique_index.exs b/priv/repo/migrations/20260512105203_restore_auto_discovery_check_unique_index.exs new file mode 100644 index 00000000..a338d252 --- /dev/null +++ b/priv/repo/migrations/20260512105203_restore_auto_discovery_check_unique_index.exs @@ -0,0 +1,58 @@ +defmodule Towerops.Repo.Migrations.RestoreAutoDiscoveryCheckUniqueIndex do + use Ecto.Migration + + # The partial unique index on (device_id, check_type, source_id) WHERE + # source_type = 'auto_discovery' was dropped in 20260404000002 because + # pg_stat reported zero scans. That removed DB-level uniqueness, leaving a + # race in Towerops.Monitoring.create_check/1 where two concurrent auto + # discoveries would each `get_by` returning nil and both insert a row, + # producing duplicate checks. + # + # Restore the index so we can use atomic ON CONFLICT upsert. + + @disable_ddl_transaction true + @disable_migration_lock true + + def up do + # Dedupe any duplicates that accumulated while the index was missing. + execute("SET statement_timeout = '10min'") + + execute(""" + DELETE FROM checks + WHERE id IN ( + SELECT id FROM ( + SELECT id, + ROW_NUMBER() OVER ( + PARTITION BY device_id, check_type, source_id + ORDER BY inserted_at ASC + ) AS rn + FROM checks + WHERE source_type = 'auto_discovery' + AND source_id IS NOT NULL + ) ranked + WHERE rn > 1 + ) + """) + + execute("RESET statement_timeout") + + execute("DROP INDEX IF EXISTS checks_device_type_source_unique_index") + + create( + unique_index(:checks, [:device_id, :check_type, :source_id], + where: "source_type = 'auto_discovery' AND source_id IS NOT NULL", + name: :checks_device_type_source_unique_index, + concurrently: true + ) + ) + end + + def down do + drop_if_exists( + index(:checks, [:device_id, :check_type, :source_id], + name: :checks_device_type_source_unique_index, + concurrently: true + ) + ) + end +end