diff --git a/bugs.md b/bugs.md index 8853366e..36d26aec 100644 --- a/bugs.md +++ b/bugs.md @@ -71,17 +71,6 @@ --- -### M3. Captcha/Verification Token Pattern Matches - -**File:** `lib/towerops_web/user_auth.ex:70-86` (valid_return_path?) - -**Severity:** MEDIUM — Open redirect via URL encoding bypasses - -**Description:** Blacklist-based `valid_return_path?` can be bypassed with URL encoding: `/%2f%2fevil.com`, tab injection, double encoding. - -**Fix:** Use whitelist approach matching known route prefixes. - ---- ### M4. Stripe Webhook Signature Not Enforced at Plug Level @@ -107,41 +96,8 @@ --- -### M6. `get_node_detail` Loads Entire Organization -**File:** `lib/towerops/topology.ex:612-663` -**Severity:** MEDIUM — Massive unnecessary data transfer for single node lookup - -**Description:** Loads ALL device IDs and ALL unlinked DeviceLink records for the entire organization just to find one node. - -**Fix:** Find the link directly with a join query based on the discovered node ID. - ---- - -### M7. No Unique Constraint on `(organization_id, email)` for Invitations - -**File:** Migration `20251221192513_create_organization_invitations.exs` - -**Severity:** MEDIUM — Duplicate pending invitations possible; XSS-like confusion - -**Description:** Individual indexes on `token`, `organization_id`, `email` but no composite unique for pending invitations. - -**Fix:** Add `create unique_index(:organization_invitations, [:organization_id, :email], where: "accepted_at IS NULL")`. - ---- - -### M12. No Superuser Verification in Admin LiveViews - -**Files:** All `admin/*.ex` LiveViews - -**Severity:** MEDIUM — No defense-in-depth if route misconfiguration exposes admin views - -**Description:** Router applies `:require_superuser` but admin LiveViews don't perform their own `current_scope.superuser` check. - -**Fix:** Add `on_mount` or `mount` superuser verification to admin LiveViews. - ---- ### M13. CSV/Download Content-Type Validation Missing diff --git a/lib/towerops/topology.ex b/lib/towerops/topology.ex index 91801c96..cae68492 100644 --- a/lib/towerops/topology.ex +++ b/lib/towerops/topology.ex @@ -609,31 +609,8 @@ defmodule Towerops.Topology do For discovered nodes (id starts with "discovered_"): returns info from link metadata. Returns nil if node not found or not in organization. """ - def get_node_detail("discovered_" <> _suffix = node_id, organization_id) do - # Find the link that created this discovered node - device_ids = - Repo.all( - from(d in Device, - where: d.organization_id == ^organization_id, - select: d.id - ) - ) - - links = - Repo.all( - from(l in DeviceLink, - where: l.source_device_id in ^device_ids and is_nil(l.target_device_id), - preload: [:source_device, :source_interface, :target_interface] - ) - ) - - # Find the link matching this discovered node ID - link = - Enum.find(links, fn l -> - discovered_node_id(l) == node_id - end) - - case link do + def get_node_detail("discovered_" <> suffix = node_id, organization_id) do + case find_discovered_link(suffix, organization_id) do nil -> nil @@ -712,6 +689,44 @@ defmodule Towerops.Topology do end end + defp find_discovered_link(suffix, organization_id) do + link = + Repo.one( + from l in DeviceLink, + join: d in Device, + on: l.source_device_id == d.id, + where: d.organization_id == ^organization_id, + where: is_nil(l.target_device_id), + where: + l.discovered_remote_mac == ^suffix or + l.discovered_remote_ip == ^suffix or + l.discovered_remote_name == ^suffix, + preload: [:source_device, :source_interface, :target_interface] + ) + + link || find_discovered_link_by_anonymous_id(suffix, organization_id) + end + + defp find_discovered_link_by_anonymous_id("anonymous_" <> link_id, organization_id) do + case Ecto.UUID.cast(link_id) do + {:ok, uuid} -> + Repo.one( + from l in DeviceLink, + join: d in Device, + on: l.source_device_id == d.id, + where: d.organization_id == ^organization_id, + where: is_nil(l.target_device_id), + where: l.id == ^uuid, + preload: [:source_device, :source_interface, :target_interface] + ) + + :error -> + nil + end + end + + defp find_discovered_link_by_anonymous_id(_suffix, _organization_id), do: nil + defp link_to_connection(link, device_id) do {connected_device, interface} = if link.source_device_id == device_id do diff --git a/lib/towerops_web/live/admin/agent_live/index.ex b/lib/towerops_web/live/admin/agent_live/index.ex index f98c9a2c..35cd5550 100644 --- a/lib/towerops_web/live/admin/agent_live/index.ex +++ b/lib/towerops_web/live/admin/agent_live/index.ex @@ -6,6 +6,8 @@ defmodule ToweropsWeb.Admin.AgentLive.Index do alias Towerops.Agents + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @impl true def mount(_params, _session, socket) do timer_ref = diff --git a/lib/towerops_web/live/admin/audit_live/index.ex b/lib/towerops_web/live/admin/audit_live/index.ex index fb25102a..73d8b2ba 100644 --- a/lib/towerops_web/live/admin/audit_live/index.ex +++ b/lib/towerops_web/live/admin/audit_live/index.ex @@ -7,6 +7,8 @@ defmodule ToweropsWeb.Admin.AuditLive.Index do alias Towerops.Admin + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @impl true def mount(_params, _session, socket) do {:ok, diff --git a/lib/towerops_web/live/admin/dashboard_live.ex b/lib/towerops_web/live/admin/dashboard_live.ex index afc270f0..e2b84543 100644 --- a/lib/towerops_web/live/admin/dashboard_live.ex +++ b/lib/towerops_web/live/admin/dashboard_live.ex @@ -6,6 +6,8 @@ defmodule ToweropsWeb.Admin.DashboardLive do alias Towerops.Admin + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @impl true def mount(_params, _session, socket) do {:ok, diff --git a/lib/towerops_web/live/admin/monitoring_live.ex b/lib/towerops_web/live/admin/monitoring_live.ex index dc2d960a..f994fb83 100644 --- a/lib/towerops_web/live/admin/monitoring_live.ex +++ b/lib/towerops_web/live/admin/monitoring_live.ex @@ -14,6 +14,8 @@ defmodule ToweropsWeb.Admin.MonitoringLive do alias Towerops.JobMonitoring alias Towerops.JobMonitoring.Metrics + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @topic "job:lifecycle" @impl Phoenix.LiveView diff --git a/lib/towerops_web/live/admin/org_live/index.ex b/lib/towerops_web/live/admin/org_live/index.ex index 2e86e527..d2ceed44 100644 --- a/lib/towerops_web/live/admin/org_live/index.ex +++ b/lib/towerops_web/live/admin/org_live/index.ex @@ -10,6 +10,8 @@ defmodule ToweropsWeb.Admin.OrgLive.Index do alias Towerops.Billing alias Towerops.Organizations.Organization + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @impl true def mount(_params, _session, socket) do orgs = Admin.list_all_organizations() diff --git a/lib/towerops_web/live/admin/security_live/index.ex b/lib/towerops_web/live/admin/security_live/index.ex index cb4094c2..9165abcf 100644 --- a/lib/towerops_web/live/admin/security_live/index.ex +++ b/lib/towerops_web/live/admin/security_live/index.ex @@ -10,6 +10,8 @@ defmodule ToweropsWeb.Admin.SecurityLive.Index do alias Towerops.Security.BruteForce + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @impl true def mount(_params, _session, socket) do _ = diff --git a/lib/towerops_web/live/admin/user_live/index.ex b/lib/towerops_web/live/admin/user_live/index.ex index 15ba18f5..fcb1adae 100644 --- a/lib/towerops_web/live/admin/user_live/index.ex +++ b/lib/towerops_web/live/admin/user_live/index.ex @@ -7,6 +7,8 @@ defmodule ToweropsWeb.Admin.UserLive.Index do alias Towerops.Admin alias Towerops.Admin.AuditLogger + on_mount {ToweropsWeb.UserAuth, :require_superuser} + @impl true def mount(_params, _session, socket) do users = Admin.list_all_users() diff --git a/lib/towerops_web/user_auth.ex b/lib/towerops_web/user_auth.ex index 3767bf51..ddadb8da 100644 --- a/lib/towerops_web/user_auth.ex +++ b/lib/towerops_web/user_auth.ex @@ -65,30 +65,69 @@ defmodule ToweropsWeb.UserAuth do @doc """ Validates whether a return-to path is a safe application-local destination. - Rejects external URLs, auth pages, double-slashes, and backslash variants. + Decodes URL-encoded characters first to prevent encoding-based bypasses + (e.g. %2f%2fevil.com → //evil.com), then checks against a whitelist of + known application route prefixes. """ def valid_return_path?(nil), do: false def valid_return_path?(path) when is_binary(path) do - invalid_prefixes = [ - "/users/log-in", - "/users/register", - "/users/reset-password", - "/users/confirm", - "/dev/", - "/assets/", - "/health" + valid_prefixes = [ + "/orgs/", + "/orgs", + "/devices/", + "/devices", + "/sites/", + "/sites", + "/alerts/", + "/alerts", + "/reports", + "/monitoring", + "/dashboard", + "/network-map", + "/rf-links", + "/weathermap", + "/maintenance/", + "/maintenance", + "/schedules/", + "/schedules", + "/insights", + "/coverage/", + "/coverage", + "/coverages/", + "/data", + "/help", + "/changelog", + "/organization", + "/organizations/", + "/organizations", + "/settings/", + "/settings", + "/activity" ] - path != "/" and - String.starts_with?(path, "/") and - not String.contains?(path, "//") and - not String.contains?(path, "\\") and - not Enum.any?(invalid_prefixes, &String.starts_with?(path, &1)) + with {:ok, decoded} <- safe_decode(path) do + String.starts_with?(decoded, "/") and + decoded != "/" and + not String.contains?(decoded, "//") and + not String.contains?(decoded, "\\") and + not contains_control_chars?(decoded) and + Enum.any?(valid_prefixes, &String.starts_with?(decoded, &1)) + end end def valid_return_path?(_), do: false + defp safe_decode(path) do + {:ok, URI.decode(path)} + rescue + _ -> :error + end + + defp contains_control_chars?(str) do + String.contains?(str, ["\t", "\n", "\r", "\0"]) + end + @doc """ Logs the user out. diff --git a/priv/repo/migrations/20260512173653_add_unique_constraint_on_organization_invitations.exs b/priv/repo/migrations/20260512173653_add_unique_constraint_on_organization_invitations.exs new file mode 100644 index 00000000..2056ae03 --- /dev/null +++ b/priv/repo/migrations/20260512173653_add_unique_constraint_on_organization_invitations.exs @@ -0,0 +1,10 @@ +defmodule Towerops.Repo.Migrations.AddUniqueConstraintOnOrganizationInvitations do + use Ecto.Migration + + def change do + create unique_index(:organization_invitations, [:organization_id, :email], + where: "accepted_at IS NULL", + name: :unique_pending_invitation_per_org + ) + end +end diff --git a/test/towerops_web/controllers/user_session_controller_test.exs b/test/towerops_web/controllers/user_session_controller_test.exs index 127053fa..d1655db4 100644 --- a/test/towerops_web/controllers/user_session_controller_test.exs +++ b/test/towerops_web/controllers/user_session_controller_test.exs @@ -93,7 +93,7 @@ defmodule ToweropsWeb.UserSessionControllerTest do test "logs the user in with return to", %{conn: conn, user: user} do conn = conn - |> init_test_session(user_return_to: "/foo/bar") + |> init_test_session(user_return_to: "/devices") |> post(~p"/users/log-in", %{ "user" => %{ "email" => user.email, @@ -101,7 +101,7 @@ defmodule ToweropsWeb.UserSessionControllerTest do } }) - assert redirected_to(conn) == "/foo/bar" + assert redirected_to(conn) == "/devices" end test "emits error message with invalid credentials", %{conn: conn, user: user} do diff --git a/test/towerops_web/user_auth_test.exs b/test/towerops_web/user_auth_test.exs index 79b40096..fc60ed7d 100644 --- a/test/towerops_web/user_auth_test.exs +++ b/test/towerops_web/user_auth_test.exs @@ -60,8 +60,8 @@ defmodule ToweropsWeb.UserAuthTest do end test "redirects to the configured path", %{conn: conn, user: user} do - conn = conn |> put_session(:user_return_to, "/hello") |> UserAuth.log_in_user(user) - assert redirected_to(conn) == "/hello" + conn = conn |> put_session(:user_return_to, "/dashboard") |> UserAuth.log_in_user(user) + assert redirected_to(conn) == "/dashboard" end test "writes a cookie if remember_me is configured", %{conn: conn, user: user} do