diff --git a/assets/js/hooks/coverage_hooks.ts b/assets/js/hooks/coverage_hooks.ts index af72da2f..cab35e82 100644 --- a/assets/js/hooks/coverage_hooks.ts +++ b/assets/js/hooks/coverage_hooks.ts @@ -642,7 +642,11 @@ export const MultiCoverageMap = { if (!Number.isFinite(lat) || !Number.isFinite(lon)) return this.map.setView([lat, lon], 16) if (this.searchMarker) this.map.removeLayer(this.searchMarker) - this.searchMarker = L.marker([lat, lon]).addTo(this.map).bindPopup(data[0].display_name).openPopup() + // Bind via a text node, not an HTML string — Nominatim's display_name + // is third-party data and would otherwise execute as HTML/JS in the popup. + const popupEl = document.createElement("div") + popupEl.textContent = data[0].display_name + this.searchMarker = L.marker([lat, lon]).addTo(this.map).bindPopup(popupEl).openPopup() } catch (err) { console.error("Coverage search failed:", err) } diff --git a/bugs.md b/bugs.md index 0e486509..f94934d6 100644 --- a/bugs.md +++ b/bugs.md @@ -121,18 +121,6 @@ --- -### H23. Sync Workers Silently Swallow Errors - -**Files:** All sync workers (uisp, preseem, cn_maestro, sonar, splynx, visp, netbox, gaiia) - -**Severity:** HIGH — Transient failures NEVER retried; data silently stale - -**Description:** All sync workers log errors but return `:ok`. Network timeouts, API rate limits, DNS failures are all silently accepted. Data goes stale until next cron cycle. - -**Fix:** Distinguish permanent (return `:ok`/`:discard`) from transient (return `{:error, reason}` for Oban retry). - ---- - ### H24. WeatherSyncWorker Never Starts **File:** `lib/towerops/workers/weather_sync_worker.ex` @@ -157,42 +145,6 @@ --- -### H26. N+1 Latency Queries per Site Device - -**File:** `lib/towerops_web/live/site_live/show.ex:101-128` - -**Severity:** HIGH — 50+ queries for large sites - -**Description:** Loops through devices calling `Monitoring.get_latency_data` per device. For 50+ devices at a site, 50+ individual queries. - -**Fix:** Batch the latency query or add pagination/limit. - ---- - -### H28. Nominatim API Response Injected via innerHTML - -**File:** `assets/js/hooks/coverage_hooks.ts:635-645` - -**Severity:** HIGH — Third-party API response flows into innerHTML - -**Description:** Nominatim search API `display_name` is passed directly to `marker.bindPopup()` (innerHTML). If Nominatim is compromised or DNS poisoned, arbitrary JS executes. - -**Fix:** Sanitize `display_name` with `escapeHtml()` before `bindPopup()`. - ---- - -### H29. OID Injection via Substring Match in Profiles - -**File:** `lib/towerops/profiles/yaml_profiles.ex:788` - -**Severity:** HIGH — Incorrect profile matching due to partial OID match - -**Description:** `String.contains?(sys_object_id, block.oid)` matches partial octets. OID `1.3.6.1.4.1.9` would incorrectly match `1.3.6.1.4.1.99` because "9" is a substring of "99". - -**Fix:** Use `String.starts_with?(sys_object_id <> ".", block.oid <> ".")` for dot-boundary matching. - ---- - ## MEDIUM ### M1. Resource Scoping Reveals Org Membership diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index 70aaca63..2831fff5 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -646,4 +646,16 @@ defmodule Towerops.Monitoring do @doc false def get_latency_data(_device_or_site_id, _opts), do: [] + + @doc """ + Batch counterpart to `get_latency_data/2`. Returns a map of + `%{device_id => [data_points]}` for the given device IDs. Currently a + stub returning an empty map for every device — wired through the + callers so a future ping-based implementation can replace it without + N+1 queries (one call per site vs one call per device). + """ + @spec get_latency_data_for_devices([term()], keyword()) :: %{term() => list()} + def get_latency_data_for_devices(device_ids, _opts) when is_list(device_ids) do + Map.new(device_ids, &{&1, []}) + end end diff --git a/lib/towerops/profiles/yaml_profiles.ex b/lib/towerops/profiles/yaml_profiles.ex index 7c76dfb9..7bf07577 100644 --- a/lib/towerops/profiles/yaml_profiles.ex +++ b/lib/towerops/profiles/yaml_profiles.ex @@ -784,8 +784,22 @@ defmodule Towerops.Profiles.YamlProfiles do end end + # Dot-boundary match: `1.3.6.1.4.1.9` must match `1.3.6.1.4.1.9` and + # `1.3.6.1.4.1.9.X` but NOT `1.3.6.1.4.1.99`. Plain `String.contains?` is + # unsafe for OID prefix checks because "9" is a substring of "99". + # Some profile YAMLs end the prefix with a trailing dot ("1.3.6.1.4.1.25506.") + # — normalize that off before comparing. + defp oid_prefix_match?(sys_object_id, block_oid) when is_binary(sys_object_id) and is_binary(block_oid) do + prefix = String.trim_trailing(block_oid, ".") + + sys_object_id == prefix or + String.starts_with?(sys_object_id, prefix <> ".") + end + + defp oid_prefix_match?(_, _), do: false + defp block_matches?(block, sys_object_id, sys_descr, match_conditional, client_opts) do - oid_match = block.oid && String.contains?(sys_object_id, block.oid) + oid_match = block.oid && oid_prefix_match?(sys_object_id, block.oid) condition_match = block.has_condition == match_conditional descr_match = block_matches_descr?(block, sys_descr) snmpget_match = block_matches_snmpget?(block, match_conditional, client_opts) diff --git a/lib/towerops/workers/cn_maestro_sync_worker.ex b/lib/towerops/workers/cn_maestro_sync_worker.ex index 5d7d6496..577414ec 100644 --- a/lib/towerops/workers/cn_maestro_sync_worker.ex +++ b/lib/towerops/workers/cn_maestro_sync_worker.ex @@ -9,6 +9,7 @@ defmodule Towerops.Workers.CnMaestroSyncWorker do alias Towerops.CnMaestro.Sync alias Towerops.Integrations alias Towerops.Workers.PollingOffset + alias Towerops.Workers.SyncErrors require Logger @@ -50,19 +51,27 @@ defmodule Towerops.Workers.CnMaestroSyncWorker do def perform(%Oban.Job{args: %{"integration_id" => id}}) do case Integrations.get_integration_by_id(id) do {:ok, integration} -> - case Sync.sync_organization(integration) do - {:ok, result} -> - Logger.info("cnMaestro sync completed: #{inspect(result)}") - :ok - - {:error, reason} -> - Logger.error("cnMaestro sync failed: #{inspect(reason)}") - :ok - end + sync_integration(integration) {:error, :not_found} -> Logger.warning("cnMaestro sync: integration #{id} not found") :ok end end + + defp sync_integration(integration) do + case Sync.sync_organization(integration) do + {:ok, result} -> + Logger.info("cnMaestro sync completed: #{inspect(result)}") + :ok + + {:error, reason} -> + Logger.error("cnMaestro sync failed: #{inspect(reason)}") + # Distinguish permanent failures (auth, bad config) from transient + # ones (network, rate limits, 5xx) so Oban retries the latter — + # stale monitoring data is worse than a redundant retry on a + # permanent failure. + if SyncErrors.transient?(reason), do: {:error, reason}, else: :ok + end + end end diff --git a/lib/towerops/workers/preseem_sync_worker.ex b/lib/towerops/workers/preseem_sync_worker.ex index ec3d7039..62209e47 100644 --- a/lib/towerops/workers/preseem_sync_worker.ex +++ b/lib/towerops/workers/preseem_sync_worker.ex @@ -15,6 +15,7 @@ defmodule Towerops.Workers.PreseemSyncWorker do alias Towerops.Integrations alias Towerops.Preseem.Sync alias Towerops.Workers.PollingOffset + alias Towerops.Workers.SyncErrors require Logger @@ -71,7 +72,11 @@ defmodule Towerops.Workers.PreseemSyncWorker do {:error, reason} -> Logger.error("Preseem sync failed for org #{integration.organization_id}: #{inspect(reason)}") - :ok + # Distinguish permanent failures (auth, bad config) from transient + # ones (network, rate limits, 5xx) so Oban retries the latter — + # stale monitoring data is worse than a redundant retry on a + # permanent failure. + if SyncErrors.transient?(reason), do: {:error, reason}, else: :ok end end diff --git a/lib/towerops/workers/sync_errors.ex b/lib/towerops/workers/sync_errors.ex new file mode 100644 index 00000000..a0c2321c --- /dev/null +++ b/lib/towerops/workers/sync_errors.ex @@ -0,0 +1,28 @@ +defmodule Towerops.Workers.SyncErrors do + @moduledoc """ + Classifier for vendor-sync error reasons. + + Sync workers (UISP, Preseem, cnMaestro, etc.) call third-party APIs that + can fail for permanent reasons (bad credentials, missing resources) or + transient ones (network blips, rate limits, 5xx). Permanent failures + should not be retried — they will keep failing until config is fixed. + Transient failures should retry so data doesn't go stale waiting on the + next cron cycle. + """ + + @doc """ + Returns `true` if the error reason is likely transient and worth retrying. + + Unknown error shapes default to transient: stale monitoring data is the + bigger risk than an extra retry on a permanent failure. + """ + @spec transient?(term()) :: boolean() + def transient?(:unauthorized), do: false + def transient?(:forbidden), do: false + def transient?(:not_found), do: false + def transient?(:invalid_credentials), do: false + def transient?(:bad_request), do: false + def transient?({:http_error, status, _}) when status in 400..499, do: false + def transient?({:http_error, status}) when status in 400..499, do: false + def transient?(_), do: true +end diff --git a/lib/towerops/workers/uisp_sync_worker.ex b/lib/towerops/workers/uisp_sync_worker.ex index 735eaf03..8c35faf0 100644 --- a/lib/towerops/workers/uisp_sync_worker.ex +++ b/lib/towerops/workers/uisp_sync_worker.ex @@ -15,6 +15,7 @@ defmodule Towerops.Workers.UispSyncWorker do alias Towerops.Integrations alias Towerops.Uisp.Sync alias Towerops.Workers.PollingOffset + alias Towerops.Workers.SyncErrors require Logger @@ -69,7 +70,10 @@ defmodule Towerops.Workers.UispSyncWorker do {:error, reason} -> Logger.error("UISP sync failed for org #{integration.organization_id}: #{inspect(reason)}") - :ok + # Distinguish permanent failures (auth, bad config) from transient + # ones (network, rate limits, 5xx) so Oban retries the latter and + # data doesn't stay stale until the next cron tick. + if SyncErrors.transient?(reason), do: {:error, reason}, else: :ok end end diff --git a/lib/towerops_web/live/site_live/show.ex b/lib/towerops_web/live/site_live/show.ex index 74262d4a..50bd0362 100644 --- a/lib/towerops_web/live/site_live/show.ex +++ b/lib/towerops_web/live/site_live/show.ex @@ -103,13 +103,17 @@ defmodule ToweropsWeb.SiteLive.Show do nil else twenty_four_hours_ago = DateTime.add(DateTime.utc_now(), -24, :hour) + device_ids = Enum.map(devices, & &1.id) + + latency_by_device = + Monitoring.get_latency_data_for_devices(device_ids, since: twenty_four_hours_ago, limit: 1000) datasets = devices |> Enum.map(fn device -> checks = - device.id - |> Monitoring.get_latency_data(since: twenty_four_hours_ago, limit: 1000) + latency_by_device + |> Map.get(device.id, []) |> Enum.reverse() %{