fix: remaining bugs.md findings — Repo calls, process dict, CSS, rate limits, Oban, debug route

- Extract Repo calls from LiveViews into Agents.get_agent_token_for_org/2
  and Accounts.verify_new_totp_device/2 context functions
- Remove Process dictionary usage for cookie consent; use explicit assigns
- Reject url() references in status page custom CSS changeset
- Add per-organization rate limit check alongside per-IP
- Document Oban flush as emergency-only with audit logging
- Gate /admin/headers behind dev_routes (debug endpoint)

bugs.md

@@ -36,20 +36,6 @@ Validation run:
 - 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
@@ -57,37 +43,8 @@ Validation run:
 - 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
@@ -95,10 +52,3 @@ Validation run:
 - 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.
-

lib/towerops/accounts.ex

@@ -179,6 +179,7 @@ defmodule Towerops.Accounts do
   defdelegate count_user_totp_devices(user_id), to: TOTP, as: :count_user_devices
   defdelegate create_totp_device(user_id, name), to: TOTP, as: :create_device
   defdelegate create_totp_device(user_id, name, secret), to: TOTP, as: :create_device
+  defdelegate verify_new_totp_device(device_id, user_id), to: TOTP, as: :verify_new_device
   defdelegate verify_user_totp_any_device(user, code), to: TOTP, as: :verify_user_any_device
   defdelegate delete_totp_device(device_id, user_id), to: TOTP, as: :delete_device
   defdelegate rename_totp_device(device_id, user_id, new_name), to: TOTP, as: :rename_device

lib/towerops/accounts/totp.ex

@@ -195,6 +195,19 @@ defmodule Towerops.Accounts.TOTP do
     end
   end
 
+  @doc """
+  Marks a newly-created TOTP device as verified by touching its timestamp.
+
+  Returns `{:ok, device}` or `{:error, reason}`.
+  """
+  def verify_new_device(device_id, user_id) do
+    case Repo.get(UserTotpDevice, device_id) do
+      nil -> {:error, :not_found}
+      device when device.user_id != user_id -> {:error, :unauthorized}
+      device -> update_device_timestamp(device, user_id)
+    end
+  end
+
   @doc "Delete a device, refusing if it would leave the user with zero devices."
   def delete_device(device_id, user_id) do
     device = Repo.get(UserTotpDevice, device_id)

lib/towerops/agents.ex

@@ -191,6 +191,16 @@ defmodule Towerops.Agents do
   @spec get_agent_token!(Ecto.UUID.t()) :: AgentToken.t()
   def get_agent_token!(id), do: Repo.get!(AgentToken, id)
 
+  @doc """
+  Gets an agent token scoped to an organization.
+
+  Returns the agent token if it belongs to the given organization, nil otherwise.
+  """
+  @spec get_agent_token_for_org(Ecto.UUID.t(), Ecto.UUID.t()) :: AgentToken.t() | nil
+  def get_agent_token_for_org(id, organization_id) do
+    Repo.get_by(AgentToken, id: id, organization_id: organization_id)
+  end
+
   @doc """
   Updates the heartbeat timestamp and metadata for an agent token.
 

lib/towerops/status_pages/status_page_config.ex

@@ -41,8 +41,26 @@ defmodule Towerops.StatusPages.StatusPageConfig do
     |> validate_required([:slug, :organization_id])
     |> validate_format(:slug, ~r/^[a-z0-9][a-z0-9-]*[a-z0-9]$/, message: "must be lowercase alphanumeric with hyphens")
     |> validate_length(:slug, min: 3, max: 63)
+    |> validate_no_css_urls()
     |> unique_constraint(:slug)
     |> unique_constraint(:organization_id)
     |> foreign_key_constraint(:organization_id)
   end
+
+  defp validate_no_css_urls(changeset) do
+    case get_change(changeset, :custom_css) do
+      nil ->
+        changeset
+
+      css when is_binary(css) ->
+        if String.contains?(String.downcase(css), "url(") do
+          add_error(changeset, :custom_css, "url() references are not allowed in custom CSS")
+        else
+          changeset
+        end
+
+      _ ->
+        changeset
+    end
+  end
 end

lib/towerops_web/components/layouts.ex

@@ -47,9 +47,7 @@ defmodule ToweropsWeb.Layouts do
   slot :inner_block, required: true
 
   def app(assigns) do
-    # Get cookie consent from process dictionary (set in plug for non-authenticated pages)
-    requires_cookie_consent = Process.get(:requires_cookie_consent, false)
-    assigns = Map.put(assigns, :requires_cookie_consent, requires_cookie_consent)
+    assigns = Map.put_new(assigns, :requires_cookie_consent, false)
 
     ~H"""
     <div class="flex min-h-screen flex-col bg-gray-50 dark:bg-gray-950">
@@ -122,9 +120,7 @@ defmodule ToweropsWeb.Layouts do
   slot :inner_block, required: true
 
   def authenticated(assigns) do
-    # Get cookie consent from process dictionary (set in on_mount hook)
-    requires_cookie_consent = Process.get(:requires_cookie_consent, false)
-    assigns = Map.put(assigns, :requires_cookie_consent, requires_cookie_consent)
+    assigns = Map.put_new(assigns, :requires_cookie_consent, false)
 
     # Get unresolved alert count from process dictionary (set/updated in on_mount hook)
     unresolved_alert_count = Process.get(:unresolved_alert_count, 0)
@@ -881,9 +877,7 @@ defmodule ToweropsWeb.Layouts do
   slot :inner_block, required: true
 
   def admin(assigns) do
-    # Get cookie consent from process dictionary (set in on_mount hook)
-    requires_cookie_consent = Process.get(:requires_cookie_consent, false)
-    assigns = Map.put(assigns, :requires_cookie_consent, requires_cookie_consent)
+    assigns = Map.put_new(assigns, :requires_cookie_consent, false)
 
     ~H"""
     <div class="flex min-h-screen flex-col bg-gray-50 dark:bg-gray-950">

lib/towerops_web/components/marketing_layouts.ex

@@ -25,9 +25,7 @@ defmodule ToweropsWeb.MarketingLayouts do
   slot :inner_block, required: true
 
   def marketing(assigns) do
-    # Get cookie consent from process dictionary (set in plug)
-    requires_cookie_consent = Process.get(:requires_cookie_consent, false)
-    assigns = Map.put(assigns, :requires_cookie_consent, requires_cookie_consent)
+    assigns = Map.put_new(assigns, :requires_cookie_consent, false)
 
     ~H"""
     <div class="bg-white dark:bg-slate-950">

lib/towerops_web/controllers/admin_controller.ex

@@ -21,12 +21,19 @@ defmodule ToweropsWeb.AdminController do
   end
 
   @doc """
-  Flush Oban jobs by state. Cancels all jobs in the given state(s).
+  Emergency-only flush of Oban jobs by state.
+
+  Directly deletes jobs in the given state(s). This bypasses Oban's
+  cancellation hooks and telemetry — only use when a queue is poisoned
+  and normal draining fails. All flushes are audit logged.
+
   POST /admin/oban/flush?states=scheduled,retryable
   """
   def flush_oban(conn, %{"states" => states_param}) do
     import Ecto.Query
 
+    require Logger
+
     valid_states = ~w(scheduled retryable available)
     requested = states_param |> String.split(",") |> Enum.map(&String.trim/1)
     states = Enum.filter(requested, &(&1 in valid_states))
@@ -39,6 +46,10 @@ defmodule ToweropsWeb.AdminController do
         |> where([j], j.state in ^states)
         |> Towerops.Repo.delete_all()
 
+      user = conn.assigns.current_scope.user
+
+      Logger.warning("OBAN_FLUSH: user=#{user.email} (#{user.id}) deleted #{count} jobs in states #{inspect(states)}")
+
       json(conn, %{ok: true, deleted: count, states: states})
     end
   end

lib/towerops_web/live/agent_live/edit.ex

@@ -16,9 +16,10 @@ defmodule ToweropsWeb.AgentLive.Edit do
     # Superadmins can edit any agent; regular users only their org's agents
     agent_token =
       if current_user.is_superuser do
-        Towerops.Repo.get!(AgentToken, id)
+        Agents.get_agent_token!(id)
       else
-        Towerops.Repo.get_by!(AgentToken, id: id, organization_id: organization.id)
+        Agents.get_agent_token_for_org(id, organization.id) ||
+          raise Ecto.NoResultsError, queryable: AgentToken, id: id, organization_id: organization.id
       end
 
     changeset = AgentToken.update_changeset(agent_token, %{})

lib/towerops_web/live/user_settings_live/totp_manager.ex

@@ -7,7 +7,6 @@ defmodule ToweropsWeb.UserSettingsLive.TotpManager do
   import ToweropsWeb.GettextHelpers
 
   alias Towerops.Accounts
-  alias Towerops.Accounts.UserTotpDevice
 
   @doc """
   Shows the modal for adding a new TOTP device.
@@ -51,22 +50,22 @@ defmodule ToweropsWeb.UserSettingsLive.TotpManager do
   def verify_new_device(socket, code) do
     secret = socket.assigns.new_device_secret
     device_id = socket.assigns.new_device_id
+    user = socket.assigns.current_scope.user
 
     if Accounts.verify_totp(secret, code) do
-      # Device already created, just mark as verified by touching it
-      device = Towerops.Repo.get!(UserTotpDevice, device_id)
+      case Accounts.verify_new_totp_device(device_id, user.id) do
+        {:ok, _device} ->
+          socket
+          |> Phoenix.LiveView.put_flash(:info, t_auth("Device added successfully!"))
+          |> Phoenix.Component.assign(:show_device_qr_modal, false)
+          |> Phoenix.Component.assign(:new_device_id, nil)
+          |> Phoenix.Component.assign(:new_device_secret, nil)
+          |> Phoenix.Component.assign(:new_device_qr_code, nil)
+          |> assign_totp_devices()
 
-      device
-      |> UserTotpDevice.touch_changeset()
-      |> Towerops.Repo.update!()
-
-      socket
-      |> Phoenix.LiveView.put_flash(:info, t_auth("Device added successfully!"))
-      |> Phoenix.Component.assign(:show_device_qr_modal, false)
-      |> Phoenix.Component.assign(:new_device_id, nil)
-      |> Phoenix.Component.assign(:new_device_secret, nil)
-      |> Phoenix.Component.assign(:new_device_qr_code, nil)
-      |> assign_totp_devices()
+        {:error, _reason} ->
+          Phoenix.LiveView.put_flash(socket, :error, t_auth("Failed to verify device. Please try again."))
+      end
     else
       Phoenix.LiveView.put_flash(socket, :error, t_auth("Invalid code. Please try again."))
     end

lib/towerops_web/plugs/detect_eu_user.ex

@@ -33,9 +33,6 @@ defmodule ToweropsWeb.Plugs.DetectEUUser do
     conn = fetch_cookies(conn)
     requires_consent = detect_eu_user(conn)
 
-    # Store in process dictionary so layouts can access it
-    Process.put(:requires_cookie_consent, requires_consent)
-
     conn
     |> put_session(:requires_cookie_consent, requires_consent)
     |> assign(:requires_cookie_consent, requires_consent)

lib/towerops_web/plugs/rate_limit.ex

@@ -53,25 +53,42 @@ defmodule ToweropsWeb.Plugs.RateLimit do
 
   defp check_rate_limit(conn, type) do
     remote_ip = RemoteIp.from_conn(conn)
-    bucket_key = "#{type}:#{remote_ip}"
-
     {limit, period} = limits_for(type)
+    ip_key = "#{type}:#{remote_ip}"
 
-    case Towerops.RateLimit.hit(bucket_key, period, limit) do
+    case Towerops.RateLimit.hit(ip_key, period, limit) do
       {:allow, _count} ->
-        conn
+        check_org_rate_limit(conn, type, period, limit)
 
       {:deny, retry_after} ->
-        Logger.warning("Rate limit exceeded for #{bucket_key}, retry_after: #{retry_after}ms")
-
-        conn
-        |> put_resp_header("retry-after", Integer.to_string(div(retry_after, 1000)))
-        |> put_status(:too_many_requests)
-        |> json(%{error: "Too many requests. Please try again later."})
-        |> halt()
+        Logger.warning("Rate limit exceeded for #{ip_key}, retry_after: #{retry_after}ms")
+        halt_rate_limit(conn, retry_after)
     end
   end
 
+  defp check_org_rate_limit(conn, type, period, limit) do
+    org_id = Map.get(conn.assigns, :current_organization_id)
+
+    if org_id do
+      org_key = "#{type}:org:#{org_id}"
+
+      case Towerops.RateLimit.hit(org_key, period, limit) do
+        {:allow, _} -> conn
+        {:deny, retry_after} -> halt_rate_limit(conn, retry_after)
+      end
+    else
+      conn
+    end
+  end
+
+  defp halt_rate_limit(conn, retry_after) do
+    conn
+    |> put_resp_header("retry-after", Integer.to_string(div(retry_after, 1000)))
+    |> put_status(:too_many_requests)
+    |> json(%{error: "Too many requests. Please try again later."})
+    |> halt()
+  end
+
   defp limits_for(:auth), do: {@auth_limit, @auth_period}
   defp limits_for(:api), do: {@api_limit, @api_period}
   defp limits_for(:admin), do: {@admin_limit, @admin_period}

lib/towerops_web/router.ex

@@ -280,11 +280,15 @@ defmodule ToweropsWeb.Router do
 
     oban_dashboard("/oban")
     error_tracker_dashboard("/errors")
-    get "/headers", ToweropsWeb.DebugController, :headers
   end
 
-  # Enable Swoosh mailbox preview in development
+  # Enable Swoosh mailbox preview and debug endpoints in development only
   if Application.compile_env(:towerops, :dev_routes) do
+    scope "/admin" do
+      pipe_through [:browser, :require_authenticated_user, :require_superuser]
+      get "/headers", ToweropsWeb.DebugController, :headers
+    end
+
     scope "/dev" do
       pipe_through :browser
 

lib/towerops_web/user_auth.ex

@@ -674,9 +674,6 @@ defmodule ToweropsWeb.UserAuth do
   def on_mount(:load_cookie_consent, _params, session, socket) do
     requires_consent = Map.get(session, "requires_cookie_consent", false)
 
-    # Store in process dictionary so layouts can access without explicit component attribute
-    Process.put(:requires_cookie_consent, requires_consent)
-
     socket =
       socket
       |> Phoenix.Component.assign(:requires_cookie_consent, requires_consent)

test/towerops_web/controllers/debug_controller_test.exs

@@ -5,7 +5,7 @@ defmodule ToweropsWeb.DebugControllerTest do
   import Towerops.OrganizationsFixtures
 
   describe "GET /admin/headers" do
-    test "returns headers JSON for superuser", %{conn: conn} do
+    test "returns 404 in non-dev environments (gated behind dev_routes)", %{conn: conn} do
       superuser =
         user_fixture()
         |> Ecto.Changeset.change(%{is_superuser: true})
@@ -18,35 +18,7 @@ defmodule ToweropsWeb.DebugControllerTest do
         |> log_in_user(superuser)
         |> get(~p"/admin/headers")
 
-      response = json_response(conn, 200)
-
-      assert Map.has_key?(response, "headers")
-      assert Map.has_key?(response, "remote_ip")
-      assert Map.has_key?(response, "request_path")
-      assert Map.has_key?(response, "method")
-
-      assert is_map(response["headers"])
-      assert is_binary(response["remote_ip"])
-      assert response["request_path"] == "/admin/headers"
-      assert response["method"] == "GET"
-    end
-
-    test "non-superuser is redirected", %{conn: conn} do
-      user = user_fixture()
-      _org = organization_fixture(user.id)
-
-      conn =
-        conn
-        |> log_in_user(user)
-        |> get(~p"/admin/headers")
-
-      assert redirected_to(conn) == ~p"/orgs"
-    end
-
-    test "unauthenticated user is redirected to login", %{conn: conn} do
-      conn = get(conn, ~p"/admin/headers")
-
-      assert redirected_to(conn) == ~p"/users/log-in"
+      assert conn.status == 404
     end
   end
 end