diff --git a/bugs.md b/bugs.md index 218c9b32..0e486509 100644 --- a/bugs.md +++ b/bugs.md @@ -6,18 +6,6 @@ ## HIGH -### H6. N+1 Interface Capacity Queries - -**File:** `lib/towerops/capacity.ex:53-83` - -**Severity:** HIGH — 20+ individual queries per site capacity view - -**Description:** `get_site_capacity_summary` loops over interfaces calling `get_utilization` which does `InterfaceStat |> where(interface_id: ^id) |> Repo.all()` per interface. - -**Fix:** Batch-load all interface stats in a single query. - ---- - ### H7. Unauthenticated Webhook Endpoints **File:** `lib/towerops/router.ex:232-239` @@ -30,19 +18,6 @@ --- -### H8. Missing Session Expiry Check in Mobile/GraphQL Auth - -**File:** `lib/towerops_web/plugs/mobile_auth.ex:48-53` -**File:** `lib/towerops_web/plugs/graphql_auth.ex:58-63` - -**Severity:** HIGH — Expired/revoked sessions remain usable indefinitely - -**Description:** `validate_session` checks the record exists but does NOT check `expires_at`. If the query doesn't filter expired rows, revoked sessions stay active. - -**Fix:** Add `DateTime.compare(expires_at, DateTime.utc_now())` check. - ---- - ### H9. CSP `img-src https:` Wildcard **File:** `lib/towerops_web/plugs/security_headers.ex:38` @@ -93,30 +68,6 @@ --- -### H17. Reports LiveView — Missing Org Scope on Individual Operations - -**File:** `lib/towerops/reports_live.ex:73-101` - -**Severity:** HIGH — IDOR on report toggle, delete, run_now - -**Description:** `toggle`, `delete`, `run_now` fetch reports by ID without verifying org membership. A user who guesses another org's report ID can manipulate it. - -**Fix:** Add org scope verification to all individual operations. - ---- - -### H18. Mass Assignment Risk Across All CRUD Endpoints - -**Files:** Multiple v1 controllers - -**Severity:** HIGH — Unchecked params passed to context functions - -**Description:** All create/update actions pass raw user params to context functions. Only protection is changeset `cast/3`. If any changeset doesn't restrict fields properly, arbitrary columns can be set. - -**Fix:** Add explicit field allowlist filtering at controller level (like `IntegrationsController.to_atom_keys/1`). - ---- - ### H19. QR Token Verify Leaks User Email **File:** `lib/towerops_web/controllers/api/mobile_auth_controller.ex:43-45` diff --git a/lib/towerops/capacity.ex b/lib/towerops/capacity.ex index 6017dd66..43f757cd 100644 --- a/lib/towerops/capacity.ex +++ b/lib/towerops/capacity.ex @@ -53,9 +53,14 @@ defmodule Towerops.Capacity do def get_site_capacity_summary(site_id) do interfaces = list_capacity_interfaces_for_site(site_id) + interface_ids = Enum.map(interfaces, fn {iface, _name} -> iface.id end) + since = DateTime.add(DateTime.utc_now(), -900, :second) + throughput_by_id = batch_calculate_throughput(interface_ids, since) + interface_summaries = Enum.map(interfaces, fn {iface, device_name} -> - utilization = get_utilization(iface) + throughput = Map.get(throughput_by_id, iface.id, zero_throughput()) + utilization_pct = utilization_pct(throughput.max_bps, iface.configured_capacity_bps) %{ id: iface.id, @@ -63,8 +68,8 @@ defmodule Towerops.Capacity do device_name: device_name, capacity_bps: iface.configured_capacity_bps, capacity_source: iface.capacity_source, - throughput: if(utilization, do: utilization.throughput, else: zero_throughput()), - utilization_pct: if(utilization, do: utilization.utilization_pct, else: 0.0) + throughput: throughput, + utilization_pct: utilization_pct } end) @@ -120,6 +125,27 @@ defmodule Towerops.Capacity do end) end + # Load every stat for `interface_ids` in a single query, then compute + # throughput per interface — replaces a per-interface SELECT in the + # site capacity loop (was N+1 over the interface count). + defp batch_calculate_throughput([], _since), do: %{} + + defp batch_calculate_throughput(interface_ids, since) do + until = DateTime.utc_now() + + InterfaceStat + |> where([s], s.interface_id in ^interface_ids) + |> where([s], s.checked_at >= ^since and s.checked_at <= ^until) + |> order_by([s], asc: s.interface_id, asc: s.checked_at) + |> Repo.all() + |> Enum.group_by(& &1.interface_id) + |> Map.new(fn {id, stats} -> {id, calculate_throughput_from_stats(stats)} end) + end + + defp utilization_pct(_throughput_bps, nil), do: 0.0 + defp utilization_pct(_throughput_bps, cap) when cap <= 0, do: 0.0 + defp utilization_pct(throughput_bps, cap), do: throughput_bps / cap * 100 + defp list_capacity_interfaces_for_site(site_id) do Interface |> join(:inner, [i], sd in Towerops.Snmp.Device, on: i.snmp_device_id == sd.id) diff --git a/lib/towerops/reports.ex b/lib/towerops/reports.ex index 425bb8ef..68a1f013 100644 --- a/lib/towerops/reports.ex +++ b/lib/towerops/reports.ex @@ -34,6 +34,15 @@ defmodule Towerops.Reports do end end + @doc """ + Fetches a report scoped to an organization. Returns `nil` if the report + doesn't exist or belongs to a different organization. Use this for any + user-driven operation (toggle, delete, run) to prevent IDOR. + """ + def get_organization_report(id, organization_id) when is_binary(organization_id) do + Repo.get_by(Report, id: id, organization_id: organization_id) + end + def create_report(attrs) do %Report{} |> Report.changeset(attrs) diff --git a/lib/towerops_web/controllers/api/param_filter.ex b/lib/towerops_web/controllers/api/param_filter.ex new file mode 100644 index 00000000..2ba3d401 --- /dev/null +++ b/lib/towerops_web/controllers/api/param_filter.ex @@ -0,0 +1,35 @@ +defmodule ToweropsWeb.Api.ParamFilter do + @moduledoc """ + Defense-in-depth helpers for stripping sensitive fields from user-supplied + API params before they reach a changeset. + + Changesets already restrict fields via `cast/3`, but if a cast list ever + accidentally includes identity fields like `organization_id`, an attacker + could mass-assign their way into another tenant's data. Filtering at the + controller boundary gives us a second layer of protection. + """ + + @sensitive_fields ~w( + id + organization_id + user_id + created_by_id + inserted_at + updated_at + ) + + @doc """ + Removes identity/ownership fields that should never be set from API input. + + ## Example + + iex> ToweropsWeb.Api.ParamFilter.strip_sensitive(%{"name" => "Foo", "organization_id" => "other-org"}) + %{"name" => "Foo"} + """ + @spec strip_sensitive(map()) :: map() + def strip_sensitive(params) when is_map(params) do + Map.drop(params, @sensitive_fields) + end + + def strip_sensitive(other), do: other +end diff --git a/lib/towerops_web/controllers/api/v1/checks_controller.ex b/lib/towerops_web/controllers/api/v1/checks_controller.ex index 202c9772..cfb59883 100644 --- a/lib/towerops_web/controllers/api/v1/checks_controller.ex +++ b/lib/towerops_web/controllers/api/v1/checks_controller.ex @@ -12,6 +12,7 @@ defmodule ToweropsWeb.Api.V1.ChecksController do alias Towerops.Monitoring alias Towerops.Monitoring.Check + alias ToweropsWeb.Api.ParamFilter alias ToweropsWeb.ScopedResource @service_check_types ~w(http tcp dns ping) @@ -50,7 +51,11 @@ defmodule ToweropsWeb.Api.V1.ChecksController do check_type = Map.get(check_params, "check_type", "") if check_type in @service_check_types do - attrs = Map.put(check_params, "organization_id", organization_id) + attrs = + check_params + |> ParamFilter.strip_sensitive() + |> Map.put("organization_id", organization_id) + create_and_respond(conn, attrs) else conn @@ -115,7 +120,7 @@ defmodule ToweropsWeb.Api.V1.ChecksController do case ScopedResource.fetch(Check, id, organization_id) do {:ok, check} -> - case Monitoring.update_check(check, check_params) do + case Monitoring.update_check(check, ParamFilter.strip_sensitive(check_params)) do {:ok, updated_check} -> json(conn, format_check(updated_check)) diff --git a/lib/towerops_web/controllers/api/v1/devices_controller.ex b/lib/towerops_web/controllers/api/v1/devices_controller.ex index 2c3a8605..e1263d84 100644 --- a/lib/towerops_web/controllers/api/v1/devices_controller.ex +++ b/lib/towerops_web/controllers/api/v1/devices_controller.ex @@ -13,6 +13,7 @@ defmodule ToweropsWeb.Api.V1.DevicesController do alias Towerops.Devices.Device alias Towerops.Sites.Site alias Towerops.Workers.DiscoveryWorker + alias ToweropsWeb.Api.ParamFilter alias ToweropsWeb.ScopedResource @doc """ @@ -94,7 +95,10 @@ defmodule ToweropsWeb.Api.V1.DevicesController do organization_id = conn.assigns.current_organization_id current_user = conn.assigns[:current_user] - device_params = default_organization_id(device_params, organization_id) + device_params = + device_params + |> ParamFilter.strip_sensitive() + |> default_organization_id(organization_id) case verify_site_access(device_params["site_id"], organization_id) do :ok -> @@ -178,7 +182,7 @@ defmodule ToweropsWeb.Api.V1.DevicesController do case ScopedResource.fetch(Device, id, organization_id) do {:ok, device} -> - case Devices.update_device(device, device_params) do + case Devices.update_device(device, ParamFilter.strip_sensitive(device_params)) do {:ok, updated_device} -> json(conn, format_device_details(updated_device)) diff --git a/lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex b/lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex index ec26cff4..bf5d7ff5 100644 --- a/lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex +++ b/lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex @@ -14,6 +14,7 @@ defmodule ToweropsWeb.Api.V1.EscalationPoliciesController do alias Towerops.OnCall.EscalationRule alias Towerops.OnCall.EscalationTarget alias Towerops.Repo + alias ToweropsWeb.Api.ParamFilter alias ToweropsWeb.ScopedResource # --- Policy CRUD --- @@ -33,7 +34,11 @@ defmodule ToweropsWeb.Api.V1.EscalationPoliciesController do @doc "POST /api/v1/escalation_policies" def create(conn, %{"escalation_policy" => policy_params}) do organization_id = conn.assigns.current_organization_id - attrs = Map.put(policy_params, "organization_id", organization_id) + + attrs = + policy_params + |> ParamFilter.strip_sensitive() + |> Map.put("organization_id", organization_id) case OnCall.create_escalation_policy(attrs) do {:ok, policy} -> @@ -80,7 +85,7 @@ defmodule ToweropsWeb.Api.V1.EscalationPoliciesController do case ScopedResource.fetch(EscalationPolicy, id, organization_id) do {:ok, policy} -> - case OnCall.update_escalation_policy(policy, policy_params) do + case OnCall.update_escalation_policy(policy, ParamFilter.strip_sensitive(policy_params)) do {:ok, updated} -> json(conn, format_policy(updated)) diff --git a/lib/towerops_web/controllers/api/v1/sites_controller.ex b/lib/towerops_web/controllers/api/v1/sites_controller.ex index 87d3f747..f2036e56 100644 --- a/lib/towerops_web/controllers/api/v1/sites_controller.ex +++ b/lib/towerops_web/controllers/api/v1/sites_controller.ex @@ -11,6 +11,7 @@ defmodule ToweropsWeb.Api.V1.SitesController do alias Towerops.Sites alias Towerops.Sites.Site + alias ToweropsWeb.Api.ParamFilter alias ToweropsWeb.ScopedResource @doc """ @@ -49,7 +50,11 @@ defmodule ToweropsWeb.Api.V1.SitesController do """ def create(conn, %{"site" => site_params}) do organization_id = conn.assigns.current_organization_id - attrs = Map.put(site_params, "organization_id", organization_id) + + attrs = + site_params + |> ParamFilter.strip_sensitive() + |> Map.put("organization_id", organization_id) case Sites.create_site(attrs) do {:ok, site} -> @@ -104,7 +109,7 @@ defmodule ToweropsWeb.Api.V1.SitesController do case ScopedResource.fetch(Site, id, organization_id) do {:ok, site} -> - case Sites.update_site(site, site_params) do + case Sites.update_site(site, ParamFilter.strip_sensitive(site_params)) do {:ok, updated_site} -> json(conn, format_site(updated_site)) diff --git a/lib/towerops_web/live/reports_live.ex b/lib/towerops_web/live/reports_live.ex index 074c86ad..b12b7425 100644 --- a/lib/towerops_web/live/reports_live.ex +++ b/lib/towerops_web/live/reports_live.ex @@ -71,49 +71,54 @@ defmodule ToweropsWeb.ReportsLive do @impl true def handle_event("toggle", %{"id" => id}, socket) do - report = Reports.get_report!(id) - - case Reports.toggle_report(report) do - {:ok, _} -> - reports = Reports.list_reports(socket.assigns.organization_id) - {:noreply, assign(socket, reports: reports)} - - {:error, _} -> - {:noreply, put_flash(socket, :error, "Failed to toggle report")} + with %_{} = report <- fetch_org_report(socket, id), + {:ok, _} <- Reports.toggle_report(report) do + reports = Reports.list_reports(socket.assigns.organization_id) + {:noreply, assign(socket, reports: reports)} + else + nil -> {:noreply, put_flash(socket, :error, "Report not found")} + {:error, _} -> {:noreply, put_flash(socket, :error, "Failed to toggle report")} end end @impl true def handle_event("delete", %{"id" => id}, socket) do - report = Reports.get_report!(id) + with %_{} = report <- fetch_org_report(socket, id), + {:ok, _} <- Reports.delete_report(report) do + reports = Reports.list_reports(socket.assigns.organization_id) - case Reports.delete_report(report) do - {:ok, _} -> - reports = Reports.list_reports(socket.assigns.organization_id) - - {:noreply, - socket - |> put_flash(:info, "Report deleted") - |> assign(:reports, reports)} - - {:error, _} -> - {:noreply, put_flash(socket, :error, "Failed to delete report")} + {:noreply, + socket + |> put_flash(:info, "Report deleted") + |> assign(:reports, reports)} + else + nil -> {:noreply, put_flash(socket, :error, "Report not found")} + {:error, _} -> {:noreply, put_flash(socket, :error, "Failed to delete report")} end end @impl true def handle_event("run_now", %{"id" => id}, socket) do - case %{"report_id" => id} - |> ReportWorker.new() - |> Oban.insert() do - {:ok, _} -> - {:noreply, put_flash(socket, :info, "Report queued for delivery")} + case fetch_org_report(socket, id) do + nil -> + {:noreply, put_flash(socket, :error, "Report not found")} - {:error, _} -> - {:noreply, put_flash(socket, :error, "Failed to queue report")} + report -> + case %{"report_id" => report.id} + |> ReportWorker.new() + |> Oban.insert() do + {:ok, _} -> {:noreply, put_flash(socket, :info, "Report queued for delivery")} + {:error, _} -> {:noreply, put_flash(socket, :error, "Failed to queue report")} + end end end + # Scoped fetch that returns nil if the report belongs to a different + # organization, preventing IDOR on toggle/delete/run_now. + defp fetch_org_report(socket, id) do + Reports.get_organization_report(id, socket.assigns.organization_id) + end + # -- Helpers -- @doc false diff --git a/lib/towerops_web/plugs/graphql_auth.ex b/lib/towerops_web/plugs/graphql_auth.ex index e4e86590..ece84679 100644 --- a/lib/towerops_web/plugs/graphql_auth.ex +++ b/lib/towerops_web/plugs/graphql_auth.ex @@ -57,11 +57,27 @@ defmodule ToweropsWeb.Plugs.GraphQLAuth do defp validate_mobile_session(token) do case MobileSessions.get_session_by_token(token) do - nil -> {:error, :invalid_token} - session -> {:ok, session} + nil -> + {:error, :invalid_token} + + session -> + # Defense in depth: the query already filters expired rows, but + # explicitly verifying here means a refactor that drops the filter + # can't quietly re-enable revoked sessions. + if session_expired?(session) do + {:error, :invalid_token} + else + {:ok, session} + end end end + defp session_expired?(%{expires_at: nil}), do: true + + defp session_expired?(%{expires_at: expires_at}) do + DateTime.compare(expires_at, DateTime.utc_now()) != :gt + end + defp get_user(user_id) do case Accounts.get_user(user_id) do nil -> {:error, :user_not_found} diff --git a/lib/towerops_web/plugs/mobile_auth.ex b/lib/towerops_web/plugs/mobile_auth.ex index 2637db73..2595c021 100644 --- a/lib/towerops_web/plugs/mobile_auth.ex +++ b/lib/towerops_web/plugs/mobile_auth.ex @@ -47,11 +47,27 @@ defmodule ToweropsWeb.Plugs.MobileAuth do defp validate_session(token) do case MobileSessions.get_session_by_token(token) do - nil -> {:error, :invalid_token} - session -> {:ok, session} + nil -> + {:error, :invalid_token} + + session -> + # Defense in depth: the query already filters expired rows, but + # explicitly verifying here means a refactor that drops the filter + # can't quietly re-enable revoked sessions. + if session_expired?(session) do + {:error, :invalid_token} + else + {:ok, session} + end end end + defp session_expired?(%{expires_at: nil}), do: true + + defp session_expired?(%{expires_at: expires_at}) do + DateTime.compare(expires_at, DateTime.utc_now()) != :gt + end + defp get_user(user_id) do case Accounts.get_user(user_id) do nil -> {:error, :user_not_found}