graham/towerops
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: 5 more high-severity bugs (H6, H8, H17, H18)

Browse source 
- H6: batch interface stats query in get_site_capacity_summary, replacing
  per-interface SELECT with a single grouped query
- H8: explicit expires_at check in MobileAuth and GraphQLAuth plugs as
  defense in depth — the query already filters expired rows, but a future
  refactor dropping the filter can't quietly re-enable revoked sessions
- H17: Reports LiveView (toggle/delete/run_now) now uses
  Reports.get_organization_report/2 to scope by organization_id, fixing
  IDOR where a user could manipulate other tenants' reports by ID
- H18: ToweropsWeb.Api.ParamFilter strips identity fields (id,
  organization_id, user_id, created_by_id, inserted_at, updated_at) from
  user-supplied API params; applied to devices, sites, checks, and
  escalation_policies create/update paths to prevent mass-assignment of
  tenant ownership fields if a changeset cast list ever drifts
This commit is contained in:
Graham McIntire 2026-05-12 11:04:31 -05:00
parent 7e6ec7098c
commit b252cb81b9
11 changed files with 169 additions and 92 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

49
bugs.md
View file

@ -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`

32
lib/towerops/capacity.ex
View file

@ -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)

9
lib/towerops/reports.ex
View file

@ -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)

35
lib/towerops_web/controllers/api/param_filter.ex Normal file
View file

@ -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

9
lib/towerops_web/controllers/api/v1/checks_controller.ex
View file

@ -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))

8
lib/towerops_web/controllers/api/v1/devices_controller.ex
View file

@ -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))

9
lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex
View file

@ -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))

9
lib/towerops_web/controllers/api/v1/sites_controller.ex
View file

@ -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))

61
lib/towerops_web/live/reports_live.ex
View file

@ -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

20
lib/towerops_web/plugs/graphql_auth.ex
View file

@ -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}

20
lib/towerops_web/plugs/mobile_auth.ex
View file

@ -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}