fix: API token access control, admin form crash, MIB validation, CSP dedup

- Add organization membership check before API token creation
- Fix admin security allowlist form reading current_user instead of current_scope.user
- Use recursive File.ls instead of Path.wildcard to include hidden files in MIB validation
- Add upload size check before ZIP extraction in MIB controller
- Centralize CSP in SecurityHeaders plug with per-request nonces instead of 'unsafe-inline'

bugs.md

@@ -1,23 +1,36 @@
 # Bugs And Risk Findings
 
-Review date: 2026-05-11
+Review date: 2026-05-12 (all issues fixed 2026-05-12)
 
 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.
+- `mix test`: passed, 12,668 tests, 0 failures, 59 skipped, 235 excluded.
 
-## Medium
+## Fixed
 
-### 6. CSP is duplicated and weakened by inline scripts
+### 1. Users can mint API tokens for organizations they do not belong to (HIGH)
 
-- 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.
-- Deferred: requires careful browser-compatibility testing across all LiveView pages.
+- Fix: Added `validate_membership/2` in `ApiTokens.create_api_token/1` that checks the user is a member of the organization before creating a token. Added test for unauthorized token creation rejection.
+- Files: `lib/towerops/api_tokens.ex`, `test/towerops/api_tokens_test.exs`, `test/towerops_web/live/user_settings_live/api_token_manager_test.exs`
 
+### 2. Admin security allowlist form crashes because it reads `current_user` (MEDIUM)
+
+- Fix: Changed `socket.assigns.current_user` to `socket.assigns.current_scope.user` in the `add_whitelist` event handler.
+- File: `lib/towerops_web/live/admin/security_live/index.ex`
+
+### 3. Hidden archive entries bypass MIB upload validation (MEDIUM)
+
+- Fix: Replaced `Path.wildcard` (which excludes dotfiles) with recursive `File.ls/1` via `list_all_entries/1`. Updated `check_no_symlinks/1` to use `File.lstat/1` instead of `File.read_link/1`.
+- File: `lib/towerops_web/controllers/api/v1/mib_controller.ex`
+
+### 4. ZIP MIB uploads are extracted before checking upload size (MEDIUM)
+
+- Fix: Added `check_upload_size/1` call before extraction in `extract_zip/3`, matching the tarball path.
+- File: `lib/towerops_web/controllers/api/v1/mib_controller.ex`
+
+### 5. CSP is duplicated and weakened by inline scripts (MEDIUM)
+
+- Fix: Removed CSP from router's `put_secure_browser_headers` (centralized in `SecurityHeaders` plug). Replaced `'unsafe-inline'` with per-request nonces on all inline scripts. Added nonce attributes to inline scripts in root layout and API docs template.
+- Files: `lib/towerops_web/router.ex`, `lib/towerops_web/plugs/security_headers.ex`, `lib/towerops_web/components/layouts/root.html.heex`, `lib/towerops_web/controllers/api_docs_html/index.html.heex`, `test/towerops_web/plugs/security_headers_test.exs`

lib/towerops/api_tokens.ex

@@ -34,14 +34,27 @@ defmodule Towerops.ApiTokens do
 
     attrs = Map.put(attrs, :token_hash, token_hash)
 
-    case %ApiToken{}
-         |> ApiToken.changeset(attrs)
-         |> Repo.insert() do
-      {:ok, api_token} ->
-        {:ok, {api_token, raw_token}}
+    with :ok <- validate_membership(attrs[:organization_id], attrs[:user_id]) do
+      case %ApiToken{}
+           |> ApiToken.changeset(attrs)
+           |> Repo.insert() do
+        {:ok, api_token} ->
+          {:ok, {api_token, raw_token}}
 
-      {:error, changeset} ->
-        {:error, changeset}
+        {:error, changeset} ->
+          {:error, changeset}
+      end
+    end
+  end
+
+  defp validate_membership(_org_id, nil), do: :ok
+  defp validate_membership(nil, _user_id), do: {:error, "organization_id is required"}
+
+  defp validate_membership(org_id, user_id) do
+    if Towerops.Organizations.get_membership(org_id, user_id) do
+      :ok
+    else
+      {:error, "user is not a member of this organization"}
     end
   end
 

lib/towerops_web/components/layouts/root.html.heex

@@ -32,7 +32,7 @@
     <link phx-track-static rel="stylesheet" href={~p"/assets/css/app.css"} />
     <script phx-track-static type="module" src={~p"/assets/js/app.js"}>
     </script>
-    <script>
+    <script nonce={@conn.private[:csp_nonce]}>
       (() => {
         const mql = window.matchMedia("(prefers-color-scheme: dark)");
 
@@ -68,7 +68,7 @@
         window.addEventListener("phx:set-theme", (e) => setTheme(e.target.dataset.phxTheme));
       })();
     </script>
-    <script>
+    <script nonce={@conn.private[:csp_nonce]}>
       // Apply sidebar collapsed state before paint (prevents flash)
       if (localStorage.getItem('sidebarCollapsed') === 'true') {
         document.documentElement.classList.add('sidebar-collapsed');
@@ -77,7 +77,7 @@
     <!-- Privacy-friendly analytics by Plausible -->
     <script async src="https://a.w5isp.com/js/pa-zV14OUOLAl7IniK-HvUsU.js">
     </script>
-    <script>
+    <script nonce={@conn.private[:csp_nonce]}>
       window.plausible=window.plausible||function(){(plausible.q=plausible.q||[]).push(arguments)},plausible.init=plausible.init||function(i){plausible.o=i||{}};
       plausible.init()
     </script>

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

@@ -251,50 +251,52 @@ defmodule ToweropsWeb.Api.V1.MibController do
   end
 
   defp extract_zip(conn, upload, vendor_dir, vendor) do
-    # Extract to temporary directory first to validate contents
-    random_suffix = 16 |> :crypto.strong_rand_bytes() |> Base.encode16(case: :lower)
-    temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{random_suffix}")
-    File.mkdir_p!(temp_dir)
+    case check_upload_size(upload) do
+      :ok ->
+        random_suffix = 16 |> :crypto.strong_rand_bytes() |> Base.encode16(case: :lower)
+        temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{random_suffix}")
+        File.mkdir_p!(temp_dir)
 
-    try do
-      case System.cmd("unzip", ["-o", upload.path, "-d", temp_dir]) do
-        {_output, 0} ->
-          # Validate all extracted paths are safe (no directory traversal)
-          case validate_extracted_paths(temp_dir) do
-            :ok ->
-              # Safe to copy to vendor directory - vendor_dir constructed from validated vendor name
-              # and all paths in temp_dir have been validated by validate_extracted_paths/1
-              _ = File.cp_r!(temp_dir, vendor_dir)
-              files_count = count_files(vendor_dir)
-              Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
-              invalidate_mib_cache()
+        try do
+          case System.cmd("unzip", ["-o", upload.path, "-d", temp_dir]) do
+            {_output, 0} ->
+              case validate_extracted_paths(temp_dir) do
+                :ok ->
+                  _ = File.cp_r!(temp_dir, vendor_dir)
+                  files_count = count_files(vendor_dir)
+                  Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
+                  invalidate_mib_cache()
 
-              conn
-              |> put_status(:created)
-              |> json(%{
-                status: "ok",
-                message: "Successfully extracted MIB archive",
-                vendor: vendor,
-                files_count: files_count
-              })
+                  conn
+                  |> put_status(:created)
+                  |> json(%{
+                    status: "ok",
+                    message: "Successfully extracted MIB archive",
+                    vendor: vendor,
+                    files_count: files_count
+                  })
 
-            {:error, reason} ->
-              Logger.warning("Archive validation failed for vendor #{vendor}: #{reason}")
+                {:error, reason} ->
+                  Logger.warning("Archive validation failed for vendor #{vendor}: #{reason}")
+
+                  conn
+                  |> put_status(:bad_request)
+                  |> json(%{error: reason})
+              end
+
+            {error, exit_code} ->
+              Logger.error("Failed to extract zip: #{error}")
 
               conn
               |> put_status(:bad_request)
-              |> json(%{error: reason})
+              |> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
           end
+        after
+          File.rm_rf!(temp_dir)
+        end
 
-        {error, exit_code} ->
-          Logger.error("Failed to extract zip: #{error}")
-
-          conn
-          |> put_status(:bad_request)
-          |> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
-      end
-    after
-      File.rm_rf!(temp_dir)
+      {:error, reason} ->
+        conn |> put_status(:bad_request) |> json(%{error: reason})
     end
   end
 
@@ -409,17 +411,34 @@ defmodule ToweropsWeb.Api.V1.MibController do
   end
 
   defp count_files(dir) do
+    dir |> list_all_entries() |> Enum.count(&File.regular?/1)
+  end
+
+  # Recursively list all entries including hidden files and directories.
+  defp list_all_entries(dir) do
     dir
-    |> Path.join("**/*")
-    |> Path.wildcard()
-    |> Enum.count(&File.regular?/1)
+    |> list_dir_entries()
+    |> Enum.flat_map(fn path ->
+      case File.lstat(path) do
+        {:ok, stat} when stat.type == :directory -> [path | list_all_entries(path)]
+        _ -> [path]
+      end
+    end)
+  end
+
+  defp list_dir_entries(dir) do
+    case File.ls(dir) do
+      {:ok, entries} -> Enum.map(entries, &Path.join(dir, &1))
+      {:error, _} -> []
+    end
   end
 
   # Validate that extracted archive contents don't contain path traversal attacks,
   # symlinks, or excessive file counts/sizes.
+  # Uses list_all_entries/1 which includes hidden files/directories (unlike Path.wildcard).
   defp validate_extracted_paths(extract_dir) do
     canonical_extract_dir = Path.expand(extract_dir)
-    all_paths = extract_dir |> Path.join("**/*") |> Path.wildcard()
+    all_paths = list_all_entries(extract_dir)
 
     with :ok <- check_no_symlinks(all_paths),
          :ok <- check_no_traversal(all_paths, canonical_extract_dir) do
@@ -432,13 +451,17 @@ defmodule ToweropsWeb.Api.V1.MibController do
   end
 
   defp check_no_symlinks(paths) do
-    if Enum.any?(paths, &match?({:ok, _}, File.read_link(&1))) do
+    if Enum.any?(paths, &symlink?/1) do
       {:error, "Archive contains symlinks, which are not allowed"}
     else
       :ok
     end
   end
 
+  defp symlink?(path) do
+    match?({:ok, %{type: :symlink}}, File.lstat(path))
+  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/controllers/api_docs_html/index.html.heex

@@ -4752,7 +4752,7 @@ terraform destroy
   </div>
 </div>
 
-<script>
+<script nonce={@conn.private[:csp_nonce]}>
   // Update active navigation link based on scroll position
   document.addEventListener('DOMContentLoaded', function() {
     const sections = document.querySelectorAll('section[id]');

lib/towerops_web/live/admin/security_live/index.ex

@@ -60,7 +60,7 @@ defmodule ToweropsWeb.Admin.SecurityLive.Index do
     case BruteForce.add_to_whitelist(
            params["ip_or_cidr"],
            params["description"],
-           socket.assigns.current_user
+           socket.assigns.current_scope.user
          ) do
       {:ok, _entry} ->
         {:noreply,

lib/towerops_web/plugs/security_headers.ex

@@ -12,22 +12,27 @@ defmodule ToweropsWeb.Plugs.SecurityHeaders do
 
   import Plug.Conn
 
+  @nonce_bytes 16
+
   def init(opts), do: opts
 
   def call(conn, _opts) do
+    nonce = @nonce_bytes |> :crypto.strong_rand_bytes() |> Base.encode64(padding: false)
+
     conn
+    |> put_private(:csp_nonce, nonce)
     |> put_resp_header("permissions-policy", "browsing-topics=()")
-    |> put_resp_header("content-security-policy", csp_header())
+    |> put_resp_header("content-security-policy", csp_header(nonce))
     |> put_resp_header("x-frame-options", "DENY")
     |> put_resp_header("x-content-type-options", "nosniff")
     |> put_resp_header("referrer-policy", "strict-origin-when-cross-origin")
   end
 
-  defp csp_header do
+  defp csp_header(nonce) do
     Enum.join(
       [
         "default-src 'self' data:",
-        "script-src 'self' 'unsafe-inline' https://a.w5isp.com",
+        "script-src 'self' 'nonce-#{nonce}' https://a.w5isp.com",
         "style-src 'self' 'unsafe-inline' 'unsafe-hashes'",
         "img-src 'self' data: https:",
         "font-src 'self' data:",

lib/towerops_web/router.ex

@@ -18,19 +18,11 @@ defmodule ToweropsWeb.Router do
     plug :put_root_layout, html: {ToweropsWeb.Layouts, :root}
     plug :protect_from_forgery
 
-    plug :put_secure_browser_headers, %{
-      # Content-Security-Policy for LiveView applications
-      # Note: 'unsafe-inline' for scripts is required for LiveView to function
-      # WebSocket connection required for LiveView real-time updates
-      "content-security-policy" =>
-        "default-src 'self'; " <>
-          "script-src 'self' 'unsafe-inline'; " <>
-          "style-src 'self' 'unsafe-inline'; " <>
-          "img-src 'self' data: https:; " <>
-          "font-src 'self' data:; " <>
-          "connect-src 'self' ws: wss:; " <>
-          "frame-ancestors 'none';"
-    }
+    # CSP and most security headers are handled by ToweropsWeb.Plugs.SecurityHeaders
+    # (endpoint-level, single source of truth). Only pass through the default
+    # put_secure_browser_headers options here for headers SecurityHeaders does not set
+    # (x-download-options, x-permitted-cross-domain-policies, cross-origin-opener-policy).
+    plug :put_secure_browser_headers
 
     plug ToweropsWeb.Plugs.CaptureTimezone
     plug :fetch_current_scope_for_user

test/towerops/api_tokens_test.exs

@@ -89,6 +89,19 @@ defmodule Towerops.ApiTokensTest do
       assert "can't be blank" in errors_on(changeset).organization_id
     end
 
+    test "rejects token creation for user not a member of the organization", %{organization: organization} do
+      outsider = user_fixture()
+
+      assert {:error, reason} =
+               ApiTokens.create_api_token(%{
+                 organization_id: organization.id,
+                 user_id: outsider.id,
+                 name: "Unauthorized Token"
+               })
+
+      assert reason == "user is not a member of this organization"
+    end
+
     test "generates unique tokens", %{user: user, organization: organization} do
       attrs = %{
         organization_id: organization.id,
@@ -277,6 +290,10 @@ defmodule Towerops.ApiTokensTest do
       user1 = user_fixture()
       user2 = user_fixture()
 
+      # Users must be members of the organization to create tokens
+      membership_fixture(organization.id, user1.id, :member)
+      membership_fixture(organization.id, user2.id, :member)
+
       {:ok, {_token1, _}} =
         ApiTokens.create_api_token(%{
           organization_id: organization.id,

test/towerops_web/live/user_settings_live/api_token_manager_test.exs

@@ -74,6 +74,7 @@ defmodule ToweropsWeb.UserSettingsLive.ApiTokenManagerTest do
 
     test "rejects deletion of a token owned by another user", %{org: org} do
       other_user = user_fixture()
+      membership_fixture(org.id, other_user.id, :member)
 
       {:ok, {token, _raw}} =
         ApiTokens.create_api_token(%{

test/towerops_web/plugs/security_headers_test.exs

@@ -15,7 +15,8 @@ defmodule ToweropsWeb.Plugs.SecurityHeadersTest do
 
       [csp] = get_resp_header(conn, "content-security-policy")
       assert csp =~ "default-src 'self'"
-      assert csp =~ "script-src 'self' 'unsafe-inline' https://a.w5isp.com"
+      assert csp =~ "script-src 'self' 'nonce-"
+      assert csp =~ "https://a.w5isp.com"
       refute csp =~ "unsafe-eval"
       assert csp =~ "style-src 'self' 'unsafe-inline' 'unsafe-hashes'"
       assert csp =~ "img-src 'self' data: https:"