fix: M20, M21, M22, L14 — archive safety, ETS protection, impersonation expiry, check pagination

- M20: Add check_no_hardlinks and check_no_special_files to MIB upload
  archive extraction to prevent hard link, device node, and FIFO attacks
- M21: Change RateLimit ETS table from :public to :protected; route hit/get/reset
  through GenServer.call instead of direct ETS access
- M22: Add 8-hour impersonation timeout — store impersonated_at in session and
  auto-revoke impersonation when expired
- L14: Add default limit (500) and optional offset to list_checks for pagination

bugs.md

@@ -92,41 +92,8 @@
 
 ---
 
-### M20. MIB Upload — Hard Links Not Checked in Archive Extraction
 
-**File:** `lib/towerops_web/controllers/api/v1/mib_controller.ex:453-458`
 
-**Severity:** MEDIUM — Hard-linked files in archive could overwrite system files
-
-**Description:** `check_no_symlinks` only checks symlinks. Hard links, device nodes, and fifos are not inspected.
-
-**Fix:** Add `check_no_hardlinks(paths)` and `check_no_special_files(paths)` using `File.stat`.
-
----
-
-### M21. ETS Rate Limit Table Is Public
-
-**File:** `lib/towerops/rate_limit.ex:156-164`
-
-**Severity:** MEDIUM — Any Erlang/Elixir process can manipulate rate limit counters
-
-**Description:** `:ets.new(table, [:named_table, :set, :public, ...])` — `:public` allows any process to read/write counters.
-
-**Fix:** Use `:protected` and have GenServer expose `hit/3` via `call`.
-
----
-
-### M22. Impersonation Session Has No Expiry
-
-**File:** `lib/towerops_web/user_auth.ex:879-910`
-
-**Severity:** MEDIUM — Superuser impersonation persists indefinitely
-
-**Description:** If a superuser starts impersonating and walks away, the session continues impersonating until manually stopped.
-
-**Fix:** Tie impersonation to sudo mode or add dedicated `expires_at`.
-
----
 
 ## LOW
 
@@ -158,15 +125,6 @@
 
 
 
-### L14. `list_checks` No Pagination
-
-**File:** `lib/towerops/monitoring.ex:24-32`
-
-**Severity:** LOW — No limit on check listing for orgs with thousands of checks
-
-**Fix:** Add default limit and pagination support.
-
----
 
 ### L15. OpenWeatherMap API Key in URL Query Param
 

lib/towerops/monitoring.ex

@@ -20,6 +20,13 @@ defmodule Towerops.Monitoring do
 
   @doc """
   Returns the list of checks for an organization.
+
+  Options:
+    - `:device_id` — filter by device
+    - `:check_type` — filter by check type
+    - `:enabled` — filter by enabled status
+    - `:limit` — max results to return (default: 500)
+    - `:offset` — offset for pagination
   """
   def list_checks(organization_id, opts \\ []) do
     organization_id
@@ -28,9 +35,17 @@ defmodule Towerops.Monitoring do
     |> maybe_filter_by_device(opts[:device_id])
     |> maybe_filter_by_check_type(opts[:check_type])
     |> maybe_filter_by_enabled(opts[:enabled])
+    |> maybe_limit(opts[:limit])
+    |> maybe_offset(opts[:offset])
     |> Repo.all()
   end
 
+  defp maybe_limit(query, nil), do: limit(query, ^500)
+  defp maybe_limit(query, n) when is_integer(n) and n > 0, do: limit(query, ^n)
+
+  defp maybe_offset(query, nil), do: query
+  defp maybe_offset(query, n) when is_integer(n) and n >= 0, do: offset(query, ^n)
+
   defp maybe_filter_by_device(query, nil), do: query
   defp maybe_filter_by_device(query, device_id), do: CheckQuery.for_device(query, device_id)
 

lib/towerops/rate_limit.ex

@@ -106,18 +106,7 @@ defmodule Towerops.RateLimit do
   def hit(table, key, scale, limit, increment)
       when is_atom(table) and is_integer(scale) and scale > 0 and is_integer(limit) and limit > 0 and
              is_integer(increment) and increment >= 0 do
-    now = now_ms()
-    window = div(now, scale)
-    expires_at = (window + 1) * scale
-    full_key = {key, window}
-
-    count = :ets.update_counter(table, full_key, increment, {full_key, 0, expires_at})
-
-    if count <= limit do
-      {:allow, count}
-    else
-      {:deny, expires_at - now}
-    end
+    GenServer.call(table, {:hit, key, scale, limit, increment})
   end
 
   @doc """
@@ -126,12 +115,7 @@ defmodule Towerops.RateLimit do
   """
   @spec get(atom(), key(), scale_ms()) :: count()
   def get(table \\ @default_table, key, scale) when is_integer(scale) and scale > 0 do
-    window = div(now_ms(), scale)
-
-    case :ets.lookup(table, {key, window}) do
-      [{_full_key, count, _expires_at}] -> count
-      [] -> 0
-    end
+    GenServer.call(table, {:get, key, scale})
   end
 
   @doc """
@@ -139,9 +123,7 @@ defmodule Towerops.RateLimit do
   """
   @spec reset(atom(), key(), scale_ms()) :: :ok
   def reset(table \\ @default_table, key, scale) when is_integer(scale) and scale > 0 do
-    window = div(now_ms(), scale)
-    :ets.delete(table, {key, window})
-    :ok
+    GenServer.call(table, {:reset, key, scale})
   end
 
   # ---------------------------------------------------------------------------
@@ -157,7 +139,7 @@ defmodule Towerops.RateLimit do
       :ets.new(table, [
         :named_table,
         :set,
-        :public,
+        :protected,
         {:read_concurrency, true},
         {:write_concurrency, true},
         {:decentralized_counters, true}
@@ -167,6 +149,46 @@ defmodule Towerops.RateLimit do
     {:ok, %{table: table, clean_period: clean_period}}
   end
 
+  @impl GenServer
+  def handle_call({:hit, key, scale, limit, increment}, _from, state) do
+    now = now_ms()
+    window = div(now, scale)
+    expires_at = (window + 1) * scale
+    full_key = {key, window}
+
+    count =
+      :ets.update_counter(state.table, full_key, increment, {full_key, 0, expires_at})
+
+    result =
+      if count <= limit do
+        {:allow, count}
+      else
+        {:deny, expires_at - now}
+      end
+
+    {:reply, result, state}
+  end
+
+  @impl GenServer
+  def handle_call({:get, key, scale}, _from, state) do
+    window = div(now_ms(), scale)
+
+    count =
+      case :ets.lookup(state.table, {key, window}) do
+        [{_full_key, c, _expires_at}] -> c
+        [] -> 0
+      end
+
+    {:reply, count, state}
+  end
+
+  @impl GenServer
+  def handle_call({:reset, key, scale}, _from, state) do
+    window = div(now_ms(), scale)
+    :ets.delete(state.table, {key, window})
+    {:reply, :ok, state}
+  end
+
   @impl GenServer
   def handle_info(:clean, state) do
     sweep(state.table)

lib/towerops_web/controllers/api/v1/mib_controller.ex

@@ -441,6 +441,8 @@ defmodule ToweropsWeb.Api.V1.MibController do
     all_paths = list_all_entries(extract_dir)
 
     with :ok <- check_no_symlinks(all_paths),
+         :ok <- check_no_hardlinks(all_paths),
+         :ok <- check_no_special_files(all_paths),
          :ok <- check_no_traversal(all_paths, canonical_extract_dir) do
       check_file_limits(all_paths)
     end
@@ -462,6 +464,36 @@ defmodule ToweropsWeb.Api.V1.MibController do
     match?({:ok, %{type: :symlink}}, File.lstat(path))
   end
 
+  defp check_no_hardlinks(paths) do
+    if Enum.any?(paths, &hardlink?/1) do
+      {:error, "Archive contains hard links, which are not allowed"}
+    else
+      :ok
+    end
+  end
+
+  defp hardlink?(path) do
+    case File.lstat(path) do
+      {:ok, %{nlink: nlink}} -> nlink > 1
+      _ -> false
+    end
+  end
+
+  defp check_no_special_files(paths) do
+    if Enum.any?(paths, &special_file?/1) do
+      {:error, "Archive contains device nodes, FIFOs, or other special files"}
+    else
+      :ok
+    end
+  end
+
+  defp special_file?(path) do
+    case File.lstat(path) do
+      {:ok, stat} -> stat.type not in [:regular, :directory, :symlink]
+      _ -> false
+    end
+  end
+
   defp check_no_traversal(paths, canonical_base) do
     if Enum.all?(paths, fn path ->
          path |> Path.expand() |> String.starts_with?(canonical_base)

lib/towerops_web/user_auth.ex

@@ -42,6 +42,7 @@ defmodule ToweropsWeb.UserAuth do
   # token. This can be set to a value greater than `@max_cookie_age_in_days` to disable
   # the reissuing of tokens completely.
   @session_reissue_age_in_days 7
+  @impersonation_timeout_hours 8
 
   @doc """
   Logs the user in.
@@ -881,14 +882,21 @@ defmodule ToweropsWeb.UserAuth do
   defp build_impersonation_scope(session) do
     superuser_id = session["superuser_id"]
     target_user_id = session["target_user_id"]
+    impersonated_at = session["impersonated_at"]
 
-    if superuser_id && target_user_id do
+    if superuser_id && target_user_id && impersonation_active?(impersonated_at) do
       fetch_impersonation_users(superuser_id, target_user_id)
     else
       Scope.for_user(nil)
     end
   end
 
+  defp impersonation_active?(nil), do: false
+
+  defp impersonation_active?(impersonated_at) do
+    DateTime.diff(DateTime.utc_now(), impersonated_at, :hour) < @impersonation_timeout_hours
+  end
+
   defp fetch_impersonation_users(superuser_id, target_user_id) do
     with superuser when not is_nil(superuser) <- Accounts.get_user(superuser_id),
          target_user when not is_nil(target_user) <- Accounts.get_user(target_user_id) do
@@ -945,6 +953,7 @@ defmodule ToweropsWeb.UserAuth do
       |> put_session(:superuser_id, superuser.id)
       |> put_session(:target_user_id, target_user.id)
       |> put_session(:impersonating, true)
+      |> put_session(:impersonated_at, DateTime.utc_now())
       |> assign(:current_scope, Scope.for_impersonation(superuser, target_user))
       |> maybe_set_default_organization(target_user)
       |> put_flash(:info, t_admin("Now impersonating %{email}", email: target_user.email))

test/towerops/rate_limit_test.exs

@@ -13,7 +13,7 @@ defmodule Towerops.RateLimitTest do
       start_supervised!(
         {RateLimit,
          [
-           name: :"rl_proc_#{System.unique_integer([:positive])}",
+           name: table,
            table: table,
            clean_period: 60_000
          ]}
@@ -164,7 +164,7 @@ defmodule Towerops.RateLimitTest do
         start_supervised!(
           {RateLimit,
            [
-             name: :"rl_auto_proc_#{System.unique_integer([:positive])}",
+             name: table,
              table: table,
              clean_period: 30
            ]},
@@ -213,7 +213,7 @@ defmodule Towerops.RateLimitTest do
       # Defensive: the public API does not validate, but callers pass positive
       # ints. We document this contract by exercising a positive-only call.
       table = :"rl_validate_#{System.unique_integer([:positive])}"
-      _pid = start_supervised!({RateLimit, [name: :rl_validate_proc, table: table]}, id: :validate)
+      _pid = start_supervised!({RateLimit, [name: table, table: table]}, id: :validate)
 
       assert {:allow, 1} = RateLimit.hit(table, "ok", 1, 1)
     end

test/towerops_web/user_auth_test.exs

@@ -1258,7 +1258,8 @@ defmodule ToweropsWeb.UserAuthTest do
           %{
             "impersonating" => true,
             "superuser_id" => superuser.id,
-            "target_user_id" => target_user.id
+            "target_user_id" => target_user.id,
+            "impersonated_at" => DateTime.utc_now()
           },
           socket
         )