From 3632580d1bdf9c9d69f6e83bc1fc6916e6d5292b Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Mon, 11 May 2026 18:54:12 -0500
Subject: [PATCH] fix: address security and reliability findings from bugs.md
 review

- Client IP: only trust X-Forwarded-For from RFC 1918 proxy IPs
- Webhook auth: handle nil/blank secret with controlled error, not 500 crash
- Sudo redirect: reuse validated return_path? from login to prevent open redirect
- Map live: remove redundant inline script (ensureLeaflet hook handles loading)
- Bang calls: convert crash-prone exact matches to case in QR live and API controllers
---
 bugs.md                                       | 104 ++++++++++++++++++
 .../api/v1/escalation_policies_controller.ex  |  12 +-
 .../api/v1/schedules_controller.ex            |  12 +-
 .../controllers/user_sudo_controller.ex       |  16 ++-
 .../live/map_live/index.html.heex             |  18 ---
 lib/towerops_web/live/mobile_qr_live.ex       |  12 +-
 lib/towerops_web/plugs/webhook_auth.ex        |  26 +++--
 lib/towerops_web/remote_ip.ex                 |  30 +++--
 lib/towerops_web/user_auth.ex                 |  18 ++-
 9 files changed, 201 insertions(+), 47 deletions(-)
 create mode 100644 bugs.md

diff --git a/bugs.md b/bugs.md
new file mode 100644
index 00000000..719c5893
--- /dev/null
+++ b/bugs.md
@@ -0,0 +1,104 @@
+# Bugs And Risk Findings
+
+Review date: 2026-05-11
+
+Scope: application-owned Elixir/Phoenix code, templates, config, deployment manifests, scripts, and dependency manifests. Vendored code and bundled third-party MIB data were not reviewed as first-party code.
+
+Validation run:
+
+- `mix credo --strict`: passed, no issues.
+- `mix deps.audit`: passed, no vulnerabilities found.
+- `mix hex.audit`: passed, no retired packages found.
+- `npm audit --prefix e2e --audit-level=low`: passed, 0 vulnerabilities.
+
+## High
+
+### 3. Admin MIB archive upload is vulnerable to archive bombs and symlink/path surprises
+
+- Category: OWASP A05 Security Misconfiguration / A08 Software and Data Integrity Failures / Availability
+- Evidence: `lib/towerops_web/controllers/api/v1/mib_controller.ex:197`, `lib/towerops_web/controllers/api/v1/mib_controller.ex:244`, `lib/towerops_web/controllers/api/v1/mib_controller.ex:373`
+- Problem: uploaded `.tar.gz` and `.zip` files are extracted by external commands into temp storage without limits on archive size, extracted file count, total uncompressed bytes, nesting depth, or per-file size. `validate_extracted_paths/1` checks expanded paths after extraction, but does not prevent resource exhaustion during extraction and does not reject symlinks before `File.cp_r!/2`.
+- Fix: enforce upload body limits, archive entry count limits, max uncompressed bytes, max nesting depth, and reject symlinks/device files before copying. Prefer an archive reader that can inspect entries before writing, or run extraction in a constrained worker/container. Add tests for zip-slip, symlink, and oversized archive rejection.
+
+### 4. MIB tokenizer creates atoms from untrusted file content
+
+- Category: OWASP A04 Insecure Design / Availability
+- Evidence: `lib/snmpkit/snmp_lib/mib/snmp_tokenizer.ex:621`, `lib/snmpkit/snmp_lib/mib/snmp_tokenizer.ex:655`, `lib/snmpkit/snmp_lib/mib/parser.ex:535`
+- Problem: uploaded/imported MIB content is converted with `String.to_atom/1`. Atoms are not garbage collected on the BEAM, so a malicious or simply very large set of unique identifiers can exhaust the VM atom table and crash the node.
+- Fix: keep tokenizer identifiers as strings or use a bounded atom allowlist with `String.to_existing_atom/1` only for known reserved words. Add a regression test that parsing many unique identifiers does not grow the atom table.
+
+## Medium
+
+### 6. CSP is duplicated and weakened by inline scripts
+
+- Category: OWASP A05 Security Misconfiguration / A03 Injection defense in depth
+- Evidence: `lib/towerops_web/router.ex:21`, `lib/towerops_web/plugs/security_headers.ex:20`, `lib/towerops_web/components/layouts/root.html.heex:35`, `lib/towerops_web/components/layouts/root.html.heex:71`, `lib/towerops_web/components/layouts/root.html.heex:80`
+- Problem: CSP is set in both the browser pipeline and endpoint-level security plug, with different directives. The effective behavior depends on header overwrite order. The policy also requires `'unsafe-inline'` because templates include inline scripts, reducing CSP's value against XSS.
+- Fix: centralize CSP construction in one plug. Move inline scripts into `assets/js/app.js` or LiveView hooks and remove `'unsafe-inline'` where possible. Consider nonces only for unavoidable inline bootstrapping.
+
+### 9. Custom CSS is rendered from database into a raw style tag
+
+- Category: OWASP A03 Injection / CSS injection
+- Evidence: `lib/towerops_web/live/status_page_live.html.heex:26`
+- Problem: `@config.custom_css` is rendered inside `<style>`. HEEx escapes HTML, but CSS itself can still be abused for UI redress, data exfiltration attempts through external URLs, or brand/content spoofing on public status pages if an attacker gains config write access.
+- Fix: sanitize or constrain custom CSS to a safe subset, disallow external `url(...)` references, or store structured theme settings instead of arbitrary CSS.
+
+### 10. Direct Repo calls in LiveView and LiveView helper modules violate context boundaries
+
+- Category: Idiomatic Elixir / Phoenix architecture
+- Evidence: `lib/towerops_web/live/agent_live/edit.ex:19`, `lib/towerops_web/live/agent_live/edit.ex:21`, `lib/towerops_web/live/user_settings_live/totp_manager.ex:57`, `lib/towerops_web/live/user_settings_live/totp_manager.ex:61`
+- Problem: LiveViews and UI helpers call `Towerops.Repo` directly. That bypasses context APIs, scatters authorization/data-access rules, and violates the project guideline that Ecto access belongs in contexts.
+- Fix: add context functions such as `Agents.get_agent_token_for_edit!/2` and `Accounts.verify_new_totp_device/3`, then call those from LiveViews. Unit test the context functions, including authorization boundaries.
+
+### 11. Nested API helpers fetch child records without organization in the query
+
+- Category: OWASP A01 Broken Access Control / Performance / Idiomatic Ecto
+- Evidence: `lib/towerops_web/controllers/api/v1/schedules_controller.ex:356`, `lib/towerops_web/controllers/api/v1/schedules_controller.ex:363`, `lib/towerops_web/controllers/api/v1/schedules_controller.ex:370`, `lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex:279`, `lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex:286`
+- Problem: child resources are fetched by global ID and then checked against the parent ID in Elixir. The parent is scoped first, so this is not an immediate data leak, but the database does unnecessary global lookups and the pattern is easy to copy into a real authorization bug.
+- Fix: move nested-resource lookups into the `OnCall` context and query with all available scope predicates in SQL, including organization via joins when possible.
+
+### 12. Bang calls and exact matches can turn normal failures into request/process crashes
+
+- Category: Reliability / Idiomatic Elixir
+- Evidence: `lib/towerops_web/live/agent_live/edit.ex:19`
+- Problem: several request/LiveView paths use `{:ok, _} = ...` or bang functions for operations that can fail because records disappear, DB writes fail, billing calls fail, or IDs are invalid. These become 500s or LiveView crashes instead of controlled error responses.
+- Fix: replace bang/exact-match assumptions with `case`/`with` and user-appropriate responses. Keep bang functions for programmer invariants only.
+- Partially fixed: schedules_controller, escalation_policies_controller, mobile_qr_live converted to case. settings_live kept as-is (dialyzer proves `estimated_monthly_cost` always returns `{:ok, _}`). agent_live Repo calls remain.
+
+### 13. Process dictionary is used to pass cookie-consent UI state
+
+- Category: Idiomatic Elixir / Maintainability
+- Evidence: `lib/towerops_web/user_auth.ex:671`, `lib/towerops_web/components/layouts.ex:50`, `lib/towerops_web/components/marketing_layouts.ex:28`
+- Problem: request/UI state is passed through `Process.put/2` and `Process.get/2`. This is implicit, hard to test, and can produce surprising behavior as rendering moves between processes.
+- Fix: pass `requires_cookie_consent` explicitly as an assign to layouts/components, or use a shared layout helper that reads from assigns with a default.
+
+### 14. Admin Oban flush deletes jobs directly instead of using Oban APIs
+
+- Category: Data integrity / Idiomatic library usage
+- Evidence: `lib/towerops_web/controllers/admin_controller.ex:27`, `lib/towerops_web/controllers/admin_controller.ex:40`
+- Problem: the admin endpoint deletes rows directly from `oban_jobs`. Direct deletion bypasses Oban's cancellation semantics, telemetry, plugins, and any cleanup hooks. It can also race with producers.
+- Fix: use Oban cancellation/draining APIs for supported states, or isolate this as an explicitly documented emergency-only operation with audit logging and confirmation.
+
+## Low
+
+### 15. API token rate limiting is only per IP, not per token/organization
+
+- Category: OWASP A04 Insecure Design / Abuse prevention
+- Evidence: `lib/towerops_web/plugs/rate_limit.ex:55`
+- Problem: API routes rate-limit by IP before token identity is considered. A noisy token behind NAT can affect other users, and a single token can distribute requests across IPs to exceed intended quotas.
+- Fix: after authentication, add token- or organization-level limits for API routes. Keep IP limits as an outer abuse-control layer.
+
+### 16. MIB vendor listing counts files synchronously on request
+
+- Category: Performance
+- Evidence: `lib/towerops_web/controllers/api/v1/mib_controller.ex:350`, `lib/towerops_web/controllers/api/v1/mib_controller.ex:373`
+- Problem: each `GET /admin/api/mibs` request recursively walks the whole MIB directory with `Path.wildcard("**/*")`. This can be expensive with the large MIB tree already present in the repo and can block request processes under load.
+- Fix: cache counts, keep metadata in the database, or compute counts asynchronously.
+
+### 17. Debug headers route exists in production admin scope
+
+- Category: OWASP A05 Security Misconfiguration / Information disclosure
+- Evidence: `lib/towerops_web/router.ex:292`
+- Problem: `/admin/headers` is superuser-protected, but still exposes request/header debugging functionality in the production admin surface.
+- Fix: gate it behind `dev_routes`, remove it, or require an explicit runtime flag.
+
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 cf5e5f73..ec26cff4 100644
--- a/lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex
+++ b/lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex
@@ -257,8 +257,16 @@ defmodule ToweropsWeb.Api.V1.EscalationPoliciesController do
     with {:ok, _policy} <- ScopedResource.fetch(EscalationPolicy, policy_id, organization_id),
          {:ok, _rule} <- fetch_rule(rule_id, policy_id),
          {:ok, target} <- fetch_target(target_id, rule_id) do
-      {:ok, _} = OnCall.delete_escalation_target(target)
-      json(conn, %{success: true})
+      case OnCall.delete_escalation_target(target) do
+        {:ok, _} ->
+          json(conn, %{success: true})
+
+        {:error, reason} ->
+          require Logger
+
+          Logger.error("Failed to delete escalation target #{target_id}: #{inspect(reason)}")
+          conn |> put_status(:internal_server_error) |> json(%{error: "Failed to delete escalation target"})
+      end
     else
       {:error, :forbidden} ->
         conn |> put_status(:forbidden) |> json(%{error: "Access denied to this escalation policy"})
diff --git a/lib/towerops_web/controllers/api/v1/schedules_controller.ex b/lib/towerops_web/controllers/api/v1/schedules_controller.ex
index f9b71f63..62f45382 100644
--- a/lib/towerops_web/controllers/api/v1/schedules_controller.ex
+++ b/lib/towerops_web/controllers/api/v1/schedules_controller.ex
@@ -17,6 +17,8 @@ defmodule ToweropsWeb.Api.V1.SchedulesController do
   alias Towerops.Repo
   alias ToweropsWeb.ScopedResource
 
+  require Logger
+
   # --- Schedule CRUD ---
 
   @doc "GET /api/v1/schedules"
@@ -337,8 +339,14 @@ defmodule ToweropsWeb.Api.V1.SchedulesController do
 
     with {:ok, _schedule} <- ScopedResource.fetch(Schedule, schedule_id, organization_id),
          {:ok, override} <- fetch_override(override_id, schedule_id) do
-      {:ok, _} = OnCall.delete_override(override)
-      json(conn, %{success: true})
+      case OnCall.delete_override(override) do
+        {:ok, _} ->
+          json(conn, %{success: true})
+
+        {:error, reason} ->
+          Logger.error("Failed to delete override #{override_id}: #{inspect(reason)}")
+          conn |> put_status(:internal_server_error) |> json(%{error: "Failed to delete override"})
+      end
     else
       {:error, :forbidden} ->
         conn |> put_status(:forbidden) |> json(%{error: "Access denied to this schedule"})
diff --git a/lib/towerops_web/controllers/user_sudo_controller.ex b/lib/towerops_web/controllers/user_sudo_controller.ex
index b17d98a3..eccc0259 100644
--- a/lib/towerops_web/controllers/user_sudo_controller.ex
+++ b/lib/towerops_web/controllers/user_sudo_controller.ex
@@ -8,7 +8,7 @@ defmodule ToweropsWeb.UserSudoController do
 
   use ToweropsWeb, :controller
 
-  import ToweropsWeb.UserAuth, only: [require_authenticated_user: 2]
+  import ToweropsWeb.UserAuth, only: [require_authenticated_user: 2, valid_return_path?: 1]
 
   alias Towerops.Accounts
 
@@ -20,7 +20,7 @@ defmodule ToweropsWeb.UserSudoController do
     cond do
       # Already in sudo mode (verified within last 10 minutes), redirect to destination
       recently_verified_sudo?(user) ->
-        return_to = get_session(conn, :user_return_to) || ~p"/users/settings"
+        return_to = sudo_return_path(conn)
 
         conn
         |> delete_session(:user_return_to)
@@ -58,7 +58,7 @@ defmodule ToweropsWeb.UserSudoController do
         case Accounts.grant_sudo_mode(user) do
           {:ok, _updated_user} ->
             # Redirect to return_to path or default
-            return_to = get_session(conn, :user_return_to) || ~p"/users/settings"
+            return_to = sudo_return_path(conn)
 
             conn
             |> put_flash(:info, t("Identity verified."))
@@ -117,6 +117,16 @@ defmodule ToweropsWeb.UserSudoController do
     })
   end
 
+  defp sudo_return_path(conn) do
+    path = get_session(conn, :user_return_to)
+
+    if valid_return_path?(path) do
+      path
+    else
+      ~p"/users/settings"
+    end
+  end
+
   defp extract_user_agent(conn) do
     case Plug.Conn.get_req_header(conn, "user-agent") do
       [ua | _] -> ua
diff --git a/lib/towerops_web/live/map_live/index.html.heex b/lib/towerops_web/live/map_live/index.html.heex
index c9660936..a6f65156 100644
--- a/lib/towerops_web/live/map_live/index.html.heex
+++ b/lib/towerops_web/live/map_live/index.html.heex
@@ -162,21 +162,3 @@
     </div>
   </div>
 </Layouts.authenticated>
-
-<script>
-  (function() {
-    if (!document.getElementById("leaflet-css")) {
-      const css = document.createElement("link");
-      css.id = "leaflet-css";
-      css.rel = "stylesheet";
-      css.href = "/vendor/leaflet/leaflet.css";
-      document.head.appendChild(css);
-    }
-    if (!document.getElementById("leaflet-js")) {
-      const js = document.createElement("script");
-      js.id = "leaflet-js";
-      js.src = "/vendor/leaflet/leaflet.js";
-      document.head.appendChild(js);
-    }
-  })();
-</script>
diff --git a/lib/towerops_web/live/mobile_qr_live.ex b/lib/towerops_web/live/mobile_qr_live.ex
index 47db00e6..0bc9e421 100644
--- a/lib/towerops_web/live/mobile_qr_live.ex
+++ b/lib/towerops_web/live/mobile_qr_live.ex
@@ -14,7 +14,17 @@ defmodule ToweropsWeb.MobileQRLive do
     user = socket.assigns.current_scope.user
 
     # Create QR login token
-    {:ok, qr_token} = MobileSessions.create_qr_login_token(user.id)
+    qr_token =
+      case MobileSessions.create_qr_login_token(user.id) do
+        {:ok, token} ->
+          token
+
+        {:error, reason} ->
+          require Logger
+
+          Logger.error("Failed to create QR login token: #{inspect(reason)}")
+          raise "Could not create QR login token"
+      end
 
     # Start polling to check if token has been completed
     timer_ref =
diff --git a/lib/towerops_web/plugs/webhook_auth.ex b/lib/towerops_web/plugs/webhook_auth.ex
index 74ba242a..1ddf859e 100644
--- a/lib/towerops_web/plugs/webhook_auth.ex
+++ b/lib/towerops_web/plugs/webhook_auth.ex
@@ -31,13 +31,25 @@ defmodule ToweropsWeb.Plugs.WebhookAuth do
   defp verify_secret(conn, token) do
     secret = Application.get_env(:towerops, :agent_webhook_secret)
 
-    if Plug.Crypto.secure_compare(token, secret) do
-      conn
-    else
-      conn
-      |> put_status(:unauthorized)
-      |> json(%{error: "Invalid webhook secret"})
-      |> halt()
+    cond do
+      is_nil(secret) or secret == "" ->
+        require Logger
+
+        Logger.error("agent_webhook_secret not configured — rejecting webhook request")
+
+        conn
+        |> put_status(:internal_server_error)
+        |> json(%{error: "Webhook authentication not configured"})
+        |> halt()
+
+      Plug.Crypto.secure_compare(token, secret) ->
+        conn
+
+      true ->
+        conn
+        |> put_status(:unauthorized)
+        |> json(%{error: "Invalid webhook secret"})
+        |> halt()
     end
   end
 end
diff --git a/lib/towerops_web/remote_ip.ex b/lib/towerops_web/remote_ip.ex
index 053c49e1..8be2a52b 100644
--- a/lib/towerops_web/remote_ip.ex
+++ b/lib/towerops_web/remote_ip.ex
@@ -7,15 +7,19 @@ defmodule ToweropsWeb.RemoteIp do
 
   @spec from_conn(Plug.Conn.t()) :: String.t() | nil
   def from_conn(conn) do
-    case get_req_header(conn, "x-forwarded-for") do
-      [forwarded | _] ->
-        forwarded
-        |> String.split(",")
-        |> List.first()
-        |> String.trim()
+    if trusted_proxy?(conn.remote_ip) do
+      case get_req_header(conn, "x-forwarded-for") do
+        [forwarded | _] ->
+          forwarded
+          |> String.split(",")
+          |> List.first()
+          |> String.trim()
 
-      [] ->
-        from_x_real_ip_or_tuple(conn)
+        [] ->
+          from_x_real_ip_or_tuple(conn)
+      end
+    else
+      format(conn.remote_ip)
     end
   end
 
@@ -45,4 +49,14 @@ defmodule ToweropsWeb.RemoteIp do
       [] -> format(conn.remote_ip)
     end
   end
+
+  # Only trust forwarding headers from known proxy/RFC 1918 addresses.
+  # Direct internet clients can spoof x-forwarded-for, so we must not
+  # trust it unless the immediate peer is a known reverse proxy.
+  defp trusted_proxy?({10, _b, _c, _d}), do: true
+  defp trusted_proxy?({172, b, _c, _d}) when b >= 16 and b <= 31, do: true
+  defp trusted_proxy?({192, 168, _c, _d}), do: true
+  defp trusted_proxy?({127, _b, _c, _d}), do: true
+  defp trusted_proxy?({0xFD00, _, _, _, _, _, _, _}), do: true
+  defp trusted_proxy?(_), do: false
 end
diff --git a/lib/towerops_web/user_auth.ex b/lib/towerops_web/user_auth.ex
index cae73c8d..89ae9f01 100644
--- a/lib/towerops_web/user_auth.ex
+++ b/lib/towerops_web/user_auth.ex
@@ -61,10 +61,13 @@ defmodule ToweropsWeb.UserAuth do
     redirect(conn, to: valid_return_to || signed_in_path(user))
   end
 
-  # Validates if a return path is a valid application destination
-  defp valid_return_path?(nil), do: false
+  @doc """
+  Validates whether a return-to path is a safe application-local destination.
+  Rejects external URLs, auth pages, double-slashes, and backslash variants.
+  """
+  def valid_return_path?(nil), do: false
 
-  defp valid_return_path?(path) when is_binary(path) do
+  def valid_return_path?(path) when is_binary(path) do
     invalid_prefixes = [
       "/users/log-in",
       "/users/register",
@@ -75,11 +78,14 @@ defmodule ToweropsWeb.UserAuth do
       "/health"
     ]
 
-    # Path must not start with any invalid prefix and must not be root
-    path != "/" && !Enum.any?(invalid_prefixes, &String.starts_with?(path, &1))
+    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))
   end
 
-  defp valid_return_path?(_), do: false
+  def valid_return_path?(_), do: false
 
   @doc """
   Logs the user out.