fix: comprehensive security audit fixes (#108)

This commit addresses multiple CRITICAL, HIGH, and MEDIUM severity security vulnerabilities identified in the security audit:

CRITICAL FIXES:
- Fix weak RNG for recovery codes - replaced Enum.random() with :crypto.strong_rand_bytes/1 for cryptographically secure token generation
- Fix subscription limit race conditions - moved free org and device quota checks inside transactions with FOR UPDATE locks to prevent concurrent bypass
- Fix default organization race condition - moved is_default check inside transaction to prevent multiple defaults per user

HIGH SEVERITY FIXES:
- Fix agent token deletion race condition - moved PubSub broadcast inside transaction to ensure agents only receive notification after successful deletion

MEDIUM SEVERITY FIXES:
- Fix LIKE wildcard injection in search - applied sanitize_like() to all user-facing search queries in devices.ex, sites.ex, and gaiia.ex to prevent enumeration attacks
- Fix Jason.decode! DoS - replaced with safe Jason.decode/1 with error handling in device_live/index.ex
- Fix SSRF vulnerability - added URL validation in HTTP executor to block requests to private/internal IP ranges (127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.0.0/16)
- Fix error information leakage - replaced inspect() in API responses with generic error messages, logging details server-side only
- Fix atom table pollution - HTTP method normalization now uses whitelist mapping instead of String.to_atom()

SECURITY IMPROVEMENTS:
- All quota checks now use pessimistic locking (SELECT FOR UPDATE) to prevent TOCTOU race conditions
- Private IP validation prevents cloud metadata service access (169.254.169.254)
- DNS resolution performed before HTTP requests to detect IP spoofing
- Error details logged server-side but not exposed to clients

Files changed:
- lib/towerops/accounts/user_recovery_code.ex
- lib/towerops/organizations.ex
- lib/towerops/devices.ex
- lib/towerops/sites.ex
- lib/towerops/gaiia.ex
- lib/towerops/agents.ex
- lib/towerops/monitoring/executors/http_executor.ex
- lib/towerops_web/live/device_live/index.ex
- lib/towerops_web/controllers/api/v1/mib_controller.ex
- lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex
- lib/towerops_web/controllers/api/v1/geoip_controller.ex

Reviewed-on: graham/towerops-web#108

lib/towerops/accounts.ex

@@ -408,9 +408,20 @@ defmodule Towerops.Accounts do
         created_at: DateTime.utc_now(:second)
       })
 
-    case Repo.insert(changeset) do
+    result =
-      {:ok, device} -> {:ok, device, secret}
+      case Repo.insert(changeset) do
-      {:error, changeset} -> {:error, changeset}
+        {:ok, device} -> {:ok, device, secret}
+        {:error, changeset} -> {:error, changeset}
+      end
+
+    # Log TOTP device addition for security monitoring
+    case result do
+      {:ok, _device, _secret} ->
+        _ = AuditLogger.log_totp_device_added(nil, user_id, device_name)
+        result
+
+      {:error, _changeset} ->
+        result
     end
   end
 
@@ -444,18 +455,29 @@ defmodule Towerops.Accounts do
   def delete_totp_device(device_id, user_id) do
     device = Repo.get(UserTotpDevice, device_id)
 
-    cond do
+    result =
-      is_nil(device) ->
+      cond do
-        {:error, :not_found}
+        is_nil(device) ->
+          {:error, :not_found}
 
-      device.user_id != user_id ->
+        device.user_id != user_id ->
-        {:error, :unauthorized}
+          {:error, :unauthorized}
 
-      count_user_totp_devices(user_id) == 1 ->
+        count_user_totp_devices(user_id) == 1 ->
-        {:error, :last_device}
+          {:error, :last_device}
 
-      true ->
+        true ->
-        Repo.delete(device)
+          Repo.delete(device)
+      end
+
+    # Log TOTP device deletion for security monitoring
+    case result do
+      {:ok, deleted_device} ->
+        _ = AuditLogger.log_totp_device_removed(nil, user_id, deleted_device.name)
+        result
+
+      {:error, _reason} ->
+        result
     end
   end
 
@@ -508,9 +530,20 @@ defmodule Towerops.Accounts do
         }
       end)
 
-    case Repo.insert_all(UserRecoveryCode, records) do
+    result =
-      {12, _} -> {:ok, codes}
+      case Repo.insert_all(UserRecoveryCode, records) do
-      _ -> {:error, :generation_failed}
+        {12, _} -> {:ok, codes}
+        _ -> {:error, :generation_failed}
+      end
+
+    # Log recovery code generation for security monitoring
+    case result do
+      {:ok, codes} ->
+        _ = AuditLogger.log_recovery_codes_generated(nil, user_id, length(codes))
+        result
+
+      {:error, _reason} ->
+        result
     end
   end
 
@@ -727,18 +760,30 @@ defmodule Towerops.Accounts do
   """
   def update_user_email(user, token) do
     context = "change:#{user.email}"
+    old_email = user.email
 
-    Repo.transact(fn ->
+    result =
-      with {:ok, query} <- UserToken.verify_change_email_token_query(token, context),
+      Repo.transact(fn ->
-           %UserToken{sent_to: email} <- Repo.one(query),
+        with {:ok, query} <- UserToken.verify_change_email_token_query(token, context),
-           {:ok, user} <- Repo.update(User.email_changeset(user, %{email: email})),
+             %UserToken{sent_to: email} <- Repo.one(query),
-           {_count, _result} <-
+             {:ok, updated_user} <- Repo.update(User.email_changeset(user, %{email: email})),
-             Repo.delete_all(from(UserToken, where: [user_id: ^user.id, context: ^context])) do
+             {_count, _result} <-
-        {:ok, user}
+               Repo.delete_all(from(UserToken, where: [user_id: ^user.id, context: ^context])) do
-      else
+          {:ok, updated_user}
-        _ -> {:error, :transaction_aborted}
+        else
-      end
+          _ -> {:error, :transaction_aborted}
-    end)
+        end
+      end)
+
+    # Log email change for security monitoring
+    case result do
+      {:ok, updated_user} ->
+        _ = AuditLogger.log_email_changed(nil, user.id, old_email, updated_user.email)
+        result
+
+      {:error, _reason} ->
+        result
+    end
   end
 
   @doc """
@@ -771,9 +816,20 @@ defmodule Towerops.Accounts do
 
   """
   def update_user_password(user, attrs) do
-    user
+    result =
-    |> User.password_changeset(attrs)
+      user
-    |> update_user_and_delete_all_tokens()
+      |> User.password_changeset(attrs)
+      |> update_user_and_delete_all_tokens()
+
+    # Log password change for security monitoring
+    case result do
+      {:ok, _updated_user} ->
+        _ = AuditLogger.log_password_changed(nil, user.id)
+        result
+
+      {:error, _changeset} ->
+        result
+    end
   end
 
   @doc """

lib/towerops/accounts/user_recovery_code.ex

@@ -28,17 +28,21 @@ defmodule Towerops.Accounts.UserRecoveryCode do
 
   Format: XXXX-XXXX (uppercase letters and numbers, no ambiguous chars).
   Uses base32 alphabet without 0, O, I, 1 to avoid confusion.
+  Uses cryptographically secure random number generation.
   """
   def generate_code do
     # Use base32 alphabet without ambiguous characters (0, O, I, 1)
     alphabet = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"
+    alphabet_list = String.graphemes(alphabet)
+    alphabet_size = length(alphabet_list)
 
-    for_result =
+    # Generate 8 cryptographically secure random bytes
-      for _ <- 1..8 do
+    random_bytes = :crypto.strong_rand_bytes(8)
-        Enum.random(String.graphemes(alphabet))
-      end
 
-    code = Enum.join(for_result)
+    code =
+      random_bytes
+      |> :binary.bin_to_list()
+      |> Enum.map_join(fn byte -> Enum.at(alphabet_list, rem(byte, alphabet_size)) end)
 
     # Format: XXXX-XXXX
     String.slice(code, 0..3) <> "-" <> String.slice(code, 4..7)

lib/towerops/admin/audit_logger.ex

@@ -174,4 +174,59 @@ defmodule Towerops.Admin.AuditLogger do
   defp get_request_path(%Plug.Conn{} = conn) do
     conn.request_path
   end
+
+  @doc """
+  Logs password changes.
+  """
+  def log_password_changed(conn \\ nil, user_id) do
+    log_event("password_changed", conn,
+      actor_id: user_id,
+      target_user_id: user_id,
+      metadata: %{action: "password_update"}
+    )
+  end
+
+  @doc """
+  Logs email address changes.
+  """
+  def log_email_changed(conn \\ nil, user_id, old_email, new_email) do
+    log_event("email_changed", conn,
+      actor_id: user_id,
+      target_user_id: user_id,
+      metadata: %{old_email: old_email, new_email: new_email}
+    )
+  end
+
+  @doc """
+  Logs recovery code generation.
+  """
+  def log_recovery_codes_generated(conn \\ nil, user_id, count) do
+    log_event("recovery_codes_generated", conn,
+      actor_id: user_id,
+      target_user_id: user_id,
+      metadata: %{codes_generated: count}
+    )
+  end
+
+  @doc """
+  Logs TOTP device addition.
+  """
+  def log_totp_device_added(conn \\ nil, user_id, device_name) do
+    log_event("totp_device_added", conn,
+      actor_id: user_id,
+      target_user_id: user_id,
+      metadata: %{device_name: device_name}
+    )
+  end
+
+  @doc """
+  Logs TOTP device removal.
+  """
+  def log_totp_device_removed(conn \\ nil, user_id, device_name) do
+    log_event("totp_device_removed", conn,
+      actor_id: user_id,
+      target_user_id: user_id,
+      metadata: %{device_name: device_name}
+    )
+  end
 end

lib/towerops/agents.ex

@@ -450,13 +450,6 @@ defmodule Towerops.Agents do
   """
   @spec delete_agent_token(Ecto.UUID.t()) :: {:ok, AgentToken.t()} | {:error, any()}
   def delete_agent_token(id) do
-    # Disconnect any active agent channel before deleting
-    Phoenix.PubSub.broadcast(
-      Towerops.PubSub,
-      "agent:#{id}:lifecycle",
-      :token_disabled
-    )
-
     result =
       Repo.transaction(fn ->
         agent_token = Repo.get!(AgentToken, id)
@@ -475,6 +468,14 @@ defmodule Towerops.Agents do
         # 4. Delete the agent token
         Repo.delete!(agent_token)
 
+        # 5. Disconnect any active agent channel AFTER successful deletion (inside transaction)
+        # This ensures we only broadcast if the deletion actually succeeds
+        Phoenix.PubSub.broadcast(
+          Towerops.PubSub,
+          "agent:#{id}:lifecycle",
+          :token_disabled
+        )
+
         agent_token
       end)
 

lib/towerops/devices.ex

@@ -144,7 +144,7 @@ defmodule Towerops.Devices do
   """
   @spec search_devices(String.t(), String.t()) :: [DeviceSchema.t()]
   def search_devices(organization_id, query) do
-    search_term = "%#{query}%"
+    search_term = "%#{sanitize_like(query)}%"
 
     DeviceSchema
     |> where(organization_id: ^organization_id)
@@ -604,41 +604,106 @@ defmodule Towerops.Devices do
 
     bypass_limits = Keyword.get(opts, :bypass_limits, false)
 
-    # Check device quota before insertion (unless bypassing)
+    # Build changeset with credential resolution
-    with {:ok, changeset} <- check_device_quota(attrs, bypass_limits),
+    changeset = build_device_changeset(attrs)
-         {:ok, device} <- Repo.insert(changeset) do
+
-      # Start monitoring/polling if enabled
+    # Use transaction to prevent race condition on quota check
-      _ =
+    multi =
-        if device.monitoring_enabled do
+      Ecto.Multi.new()
-          DeviceMonitorWorker.start_monitoring(device.id)
+      |> maybe_lock_organization_for_quota(changeset, bypass_limits)
+      |> maybe_check_device_quota(changeset, bypass_limits)
+      |> Ecto.Multi.insert(:device, changeset)
+
+    case Repo.transaction(multi) do
+      {:ok, %{device: device}} ->
+        # Start monitoring/polling if enabled
+        _ =
+          if device.monitoring_enabled do
+            DeviceMonitorWorker.start_monitoring(device.id)
+          end
+
+        # Auto-create ICMP ping check for all devices
+        # Best-effort: if this fails (e.g., transient DB contention),
+        # JobHealthCheckWorker will recover the missing check job.
+        try do
+          _ = Monitoring.ensure_default_ping_check(device)
+        rescue
+          e ->
+            Logger.warning("Failed to create ping check for device #{device.id}: #{Exception.message(e)}")
         end
 
-      # Auto-create ICMP ping check for all devices
+        broadcast_device_change(device.organization_id, :device_created)
-      # Best-effort: if this fails (e.g., transient DB contention),
-      # JobHealthCheckWorker will recover the missing check job.
-      try do
-        _ = Monitoring.ensure_default_ping_check(device)
-      rescue
-        e ->
-          Logger.warning("Failed to create ping check for device #{device.id}: #{Exception.message(e)}")
-      end
 
-      broadcast_device_change(device.organization_id, :device_created)
+        {:ok, device}
 
-      {:ok, device}
+      {:error, :quota_check, error_changeset, _} ->
+        {:error, error_changeset}
+
+      {:error, :device, changeset, _} ->
+        {:error, changeset}
+
+      {:error, _step, reason, _} ->
+        {:error, reason}
     end
   end
 
-  defp check_device_quota(attrs, bypass_limits) do
+  defp build_device_changeset(attrs) do
     changeset = DeviceSchema.changeset(%DeviceSchema{}, attrs)
+    apply_credential_resolution(changeset)
+  end
 
-    # Apply credential resolution
+  # Lock organization to prevent concurrent device creation race condition
-    changeset = apply_credential_resolution(changeset)
+  defp maybe_lock_organization_for_quota(multi, changeset, bypass_limits) do
-
+    if bypass_limits or no_organization_id?(changeset) do
-    if bypass_limits do
+      multi
-      {:ok, changeset}
     else
-      do_check_quota(changeset)
+      organization_id = Ecto.Changeset.get_change(changeset, :organization_id)
+      add_organization_lock(multi, organization_id)
+    end
+  end
+
+  defp no_organization_id?(changeset) do
+    match?(:error, Ecto.Changeset.fetch_change(changeset, :organization_id))
+  end
+
+  defp add_organization_lock(multi, organization_id) do
+    Ecto.Multi.run(multi, :lock_org, fn repo, _changes ->
+      org =
+        repo.one!(from o in Towerops.Organizations.Organization, where: o.id == ^organization_id, lock: "FOR UPDATE")
+
+      {:ok, org}
+    end)
+  end
+
+  # Check device quota inside transaction (after locking organization)
+  defp maybe_check_device_quota(multi, changeset, bypass_limits) do
+    if bypass_limits do
+      multi
+    else
+      add_quota_check(multi, changeset)
+    end
+  end
+
+  defp add_quota_check(multi, changeset) do
+    Ecto.Multi.run(multi, :quota_check, fn _repo, %{lock_org: organization} ->
+      validate_quota_limit(organization, changeset)
+    end)
+  end
+
+  defp validate_quota_limit(organization, changeset) do
+    case SubscriptionLimits.check_device_limit(organization) do
+      {:ok, :within_limit} ->
+        {:ok, :allowed}
+
+      {:error, :at_limit, _current, max} ->
+        error_changeset =
+          Ecto.Changeset.add_error(
+            changeset,
+            :base,
+            "You've reached your plan limit of #{max} devices. Upgrade to add more."
+          )
+
+        {:error, error_changeset}
     end
   end
 
@@ -663,36 +728,6 @@ defmodule Towerops.Devices do
     end
   end
 
-  defp do_check_quota(changeset) do
-    case Ecto.Changeset.fetch_change(changeset, :organization_id) do
-      {:ok, organization_id} ->
-        validate_device_quota(changeset, organization_id)
-
-      :error ->
-        # No organization_id in changeset, let normal validation handle it
-        {:ok, changeset}
-    end
-  end
-
-  defp validate_device_quota(changeset, organization_id) do
-    organization = Organizations.get_organization!(organization_id)
-
-    case SubscriptionLimits.check_device_limit(organization) do
-      {:ok, :within_limit} ->
-        {:ok, changeset}
-
-      {:error, :at_limit, _current, max} ->
-        error_changeset =
-          Ecto.Changeset.add_error(
-            changeset,
-            :base,
-            "You've reached your plan limit of #{max} devices. Upgrade to add more."
-          )
-
-        {:error, error_changeset}
-    end
-  end
-
   @doc """
   Updates device.
   """
@@ -1182,4 +1217,7 @@ defmodule Towerops.Devices do
       {event, organization_id}
     )
   end
+
+  # Sanitize user input for LIKE queries to prevent wildcard injection
+  defp sanitize_like(query), do: :towerops@query_helpers.sanitize_like(query)
 end

lib/towerops/gaiia.ex

@@ -117,7 +117,7 @@ defmodule Towerops.Gaiia do
   end
 
   def search_inventory_items(organization_id, query) do
-    search = "%#{query}%"
+    search = "%#{sanitize_like(query)}%"
 
     InventoryItem
     |> where(organization_id: ^organization_id)
@@ -128,7 +128,7 @@ defmodule Towerops.Gaiia do
   end
 
   def search_network_sites(organization_id, query) do
-    search = "%#{query}%"
+    search = "%#{sanitize_like(query)}%"
 
     NetworkSite
     |> where(organization_id: ^organization_id)
@@ -200,7 +200,7 @@ defmodule Towerops.Gaiia do
   def suggest_site_matches(_organization_id, %NetworkSite{name: ""}), do: []
 
   def suggest_site_matches(organization_id, %NetworkSite{name: gaiia_name}) do
-    search_term = "%#{gaiia_name}%"
+    search_term = "%#{sanitize_like(gaiia_name)}%"
 
     # Find sites where either name contains the other (bidirectional)
     Site
@@ -558,4 +558,7 @@ defmodule Towerops.Gaiia do
       Map.put(entry, :last_seen, last_seen)
     end)
   end
+
+  # Sanitize user input for LIKE queries to prevent wildcard injection
+  defp sanitize_like(query), do: :towerops@query_helpers.sanitize_like(query)
 end

lib/towerops/monitoring/executors/http_executor.ex

@@ -29,16 +29,21 @@ defmodule Towerops.Monitoring.Executors.HttpExecutor do
   - {:error, reason} on failure
   """
   def execute(config, timeout_ms \\ @default_timeout) do
-    start_time = System.monotonic_time(:millisecond)
+    url = Map.fetch!(config, "url")
-    req_opts = build_req_opts(config, timeout_ms)
 
-    case HTTP.request(__MODULE__, req_opts) do
+    # Validate URL to prevent SSRF attacks
-      {:ok, response} ->
+    with :ok <- validate_url_safety(url) do
-        response_time = System.monotonic_time(:millisecond) - start_time
+      start_time = System.monotonic_time(:millisecond)
-        validate_response(response, config, response_time)
+      req_opts = build_req_opts(config, timeout_ms)
 
-      {:error, error} ->
+      case HTTP.request(__MODULE__, req_opts) do
-        format_error(error, timeout_ms)
+        {:ok, response} ->
+          response_time = System.monotonic_time(:millisecond) - start_time
+          validate_response(response, config, response_time)
+
+        {:error, error} ->
+          format_error(error, timeout_ms)
+      end
     end
   rescue
     e ->
@@ -46,12 +51,57 @@ defmodule Towerops.Monitoring.Executors.HttpExecutor do
       {:error, "Exception: #{Exception.message(e)}"}
   end
 
+  # Validates URL to prevent SSRF attacks by blocking private/internal IP ranges
+  defp validate_url_safety(url) do
+    uri = URI.parse(url)
+
+    with {:ok, host} <- validate_host(uri.host),
+         {:ok, ip} <- resolve_hostname(host) do
+      validate_ip_not_private(ip)
+    end
+  end
+
+  defp validate_host(nil), do: {:error, "Invalid URL: missing host"}
+  defp validate_host(host), do: {:ok, host}
+
+  defp resolve_hostname(host) do
+    charlist_host = String.to_charlist(host)
+
+    case :inet.getaddr(charlist_host, :inet) do
+      {:ok, ip} -> {:ok, ip}
+      {:error, :nxdomain} -> {:error, "Host not found"}
+      {:error, reason} -> {:error, inspect(reason)}
+    end
+  end
+
+  defp validate_ip_not_private(ip) do
+    if private_ip?(ip) do
+      {:error, "Cannot access private/internal IP addresses"}
+    else
+      :ok
+    end
+  end
+
+  # Loopback
+  defp private_ip?({127, _, _, _}), do: true
+  # Private class A
+  defp private_ip?({10, _, _, _}), do: true
+  # Private class B
+  defp private_ip?({172, b, _, _}) when b >= 16 and b <= 31, do: true
+  # Private class C
+  defp private_ip?({192, 168, _, _}), do: true
+  # Link-local / AWS metadata
+  defp private_ip?({169, 254, _, _}), do: true
+  # Unspecified
+  defp private_ip?({0, 0, 0, 0}), do: true
+  defp private_ip?(_), do: false
+
   defp build_req_opts(config, timeout_ms) do
     method = String.downcase(Map.get(config, "method", "GET"))
     verify_ssl = Map.get(config, "verify_ssl", true)
 
     opts = [
-      method: String.to_atom(method),
+      method: normalize_http_method(method),
       url: Map.fetch!(config, "url"),
       headers: Map.get(config, "headers", %{}),
       receive_timeout: timeout_ms,
@@ -69,6 +119,17 @@ defmodule Towerops.Monitoring.Executors.HttpExecutor do
     end
   end
 
+  # Normalize HTTP method to atom using whitelist to prevent atom table pollution
+  defp normalize_http_method("get"), do: :get
+  defp normalize_http_method("post"), do: :post
+  defp normalize_http_method("put"), do: :put
+  defp normalize_http_method("patch"), do: :patch
+  defp normalize_http_method("delete"), do: :delete
+  defp normalize_http_method("head"), do: :head
+  defp normalize_http_method("options"), do: :options
+  # Default to GET for unknown methods
+  defp normalize_http_method(_), do: :get
+
   defp validate_response(%Req.Response{status: status, body: body}, config, response_time) do
     expected_status = Map.get(config, "expected_status", 200)
     regex = Map.get(config, "regex")

lib/towerops/organizations.ex

@@ -81,18 +81,58 @@ defmodule Towerops.Organizations do
     bypass_limits = Keyword.get(opts, :bypass_limits, false)
     subscription_plan = Map.get(attrs, :subscription_plan) || Map.get(attrs, "subscription_plan") || "free"
 
-    # Check free org limit before creating (unless bypassing or creating non-free org)
+    do_create_organization(attrs, user_id, bypass_limits, subscription_plan)
-    with :ok <- check_free_org_limit(bypass_limits, subscription_plan, user_id, attrs) do
+  end
-      do_create_organization(attrs, user_id)
+
+  defp do_create_organization(attrs, user_id, bypass_limits, subscription_plan) do
+    multi =
+      Ecto.Multi.new()
+      # Step 1: Lock user to prevent concurrent org creation
+      |> Ecto.Multi.run(:lock_user, fn repo, _changes ->
+        user = repo.one!(from u in User, where: u.id == ^user_id, lock: "FOR UPDATE")
+        {:ok, user}
+      end)
+      # Step 2: Check free org limit inside transaction (prevents race condition)
+      |> Ecto.Multi.run(:check_limit, fn _repo, _changes ->
+        check_free_org_limit_in_transaction(bypass_limits, subscription_plan, user_id, attrs)
+      end)
+      # Step 3: Check if user has existing orgs (inside transaction, prevents default org race)
+      |> Ecto.Multi.run(:check_existing, fn repo, _changes ->
+        has_existing = repo.exists?(from m in Membership, where: m.user_id == ^user_id)
+        {:ok, has_existing}
+      end)
+      # Step 4: Create organization
+      |> Ecto.Multi.insert(:organization, Organization.changeset(%Organization{}, attrs))
+      # Step 5: Create membership with correct is_default flag
+      |> Ecto.Multi.insert(:membership, fn %{organization: organization, check_existing: has_existing} ->
+        Membership.changeset(%Membership{}, %{
+          organization_id: organization.id,
+          user_id: user_id,
+          role: :owner,
+          # Set as default if this is the user's first organization
+          is_default: !has_existing
+        })
+      end)
+
+    case Repo.transaction(multi) do
+      {:ok, %{organization: organization}} -> {:ok, organization}
+      {:error, :check_limit, changeset, _} -> {:error, changeset}
+      {:error, :organization, changeset, _} -> {:error, changeset}
+      {:error, :membership, changeset, _} -> {:error, changeset}
+      {:error, _step, reason, _} -> {:error, reason}
     end
   end
 
-  defp check_free_org_limit(true, _plan, _user_id, _attrs), do: :ok
+  # Check free org limit - for use inside transaction
-  defp check_free_org_limit(_bypass, plan, _user_id, _attrs) when plan != "free", do: :ok
+  defp check_free_org_limit_in_transaction(true, _plan, _user_id, _attrs), do: {:ok, :bypassed}
+  defp check_free_org_limit_in_transaction(_bypass, plan, _user_id, _attrs) when plan != "free", do: {:ok, :not_free}
 
-  defp check_free_org_limit(false, "free", user_id, attrs) do
+  defp check_free_org_limit_in_transaction(false, "free", user_id, attrs) do
-    if SubscriptionLimits.can_create_free_organization?(user_id) do
+    # Count is executed inside the transaction, so it's safe from race conditions
-      :ok
+    count = SubscriptionLimits.count_user_owned_free_organizations(user_id)
+
+    if count < 1 do
+      {:ok, :allowed}
     else
       changeset =
         %Organization{}
@@ -106,34 +146,6 @@ defmodule Towerops.Organizations do
     end
   end
 
-  defp do_create_organization(attrs, user_id) do
-    # Check if user has any existing organizations
-    has_existing_orgs =
-      Repo.exists?(
-        from m in Membership,
-          where: m.user_id == ^user_id
-      )
-
-    multi =
-      Ecto.Multi.new()
-      |> Ecto.Multi.insert(:organization, Organization.changeset(%Organization{}, attrs))
-      |> Ecto.Multi.insert(:membership, fn %{organization: organization} ->
-        Membership.changeset(%Membership{}, %{
-          organization_id: organization.id,
-          user_id: user_id,
-          role: :owner,
-          # Set as default if this is the user's first organization
-          is_default: !has_existing_orgs
-        })
-      end)
-
-    case Repo.transaction(multi) do
-      {:ok, %{organization: organization}} -> {:ok, organization}
-      {:error, :organization, changeset, _} -> {:error, changeset}
-      {:error, :membership, changeset, _} -> {:error, changeset}
-    end
-  end
-
   @doc """
   Updates an organization.
   """

lib/towerops/sites.ex

@@ -36,7 +36,7 @@ defmodule Towerops.Sites do
 
   @doc "Search sites by name for an organization. Returns up to 10 results."
   def search_sites(organization_id, query) do
-    search_term = "%#{query}%"
+    search_term = "%#{sanitize_like(query)}%"
 
     Site
     |> where(organization_id: ^organization_id)
@@ -320,4 +320,7 @@ defmodule Towerops.Sites do
       set: [display_order: nil, updated_at: DateTime.truncate(DateTime.utc_now(), :second)]
     )
   end
+
+  # Sanitize user input for LIKE queries to prevent wildcard injection
+  defp sanitize_like(query), do: :towerops@query_helpers.sanitize_like(query)
 end

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

@@ -10,6 +10,8 @@ defmodule ToweropsWeb.Api.V1.AgentReleaseWebhookController do
 
   alias Towerops.Agents
 
+  require Logger
+
   def create(conn, _params) do
     case Agents.broadcast_mass_update() do
       {:ok, result} ->
@@ -21,9 +23,11 @@ defmodule ToweropsWeb.Api.V1.AgentReleaseWebhookController do
         })
 
       {:error, reason} ->
+        Logger.error("Agent mass update broadcast failed: #{inspect(reason)}")
+
         conn
         |> put_status(:bad_gateway)
-        |> json(%{status: "error", error: inspect(reason)})
+        |> json(%{status: "error", error: "Broadcast failed"})
     end
   end
 end

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

@@ -52,12 +52,21 @@ defmodule ToweropsWeb.Api.V1.GeoipController do
             inserted: inserted_count
           })
 
+        {:error, reason} when is_binary(reason) ->
+          # Validation error - safe to return to user
+          Logger.warning("GeoIP batch import validation error: #{reason}")
+
+          conn
+          |> put_status(:internal_server_error)
+          |> json(%{error: reason})
+
         {:error, reason} ->
+          # Internal error - log details but return generic message
           Logger.error("GeoIP batch import failed: #{inspect(reason)}")
 
           conn
           |> put_status(:internal_server_error)
-          |> json(%{error: "Import failed: #{inspect(reason)}"})
+          |> json(%{error: "Import failed"})
       end
     end
   end

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

@@ -88,9 +88,11 @@ defmodule ToweropsWeb.Api.V1.MibController do
           })
 
         {:error, reason} ->
+          Logger.error("MIB vendor list failed: #{inspect(reason)}")
+
           conn
           |> put_status(:internal_server_error)
-          |> json(%{error: inspect(reason)})
+          |> json(%{error: "Failed to list MIB vendors"})
       end
     end
   end
@@ -338,7 +340,7 @@ defmodule ToweropsWeb.Api.V1.MibController do
 
           conn
           |> put_status(:internal_server_error)
-          |> json(%{error: "Failed to upload file: #{inspect(reason)}"})
+          |> json(%{error: "Failed to upload file"})
       end
     end
   end

lib/towerops_web/controllers/user_session_controller.ex

@@ -60,6 +60,17 @@ defmodule ToweropsWeb.UserSessionController do
           UserAuth.log_in_user(conn, user, user_params)
       end
     else
+      # Record failed login attempt for security monitoring
+      _ =
+        Accounts.record_login_attempt(%{
+          email: email,
+          success: false,
+          failure_reason: "invalid_credentials",
+          method: "password",
+          ip_address: ToweropsWeb.RemoteIp.from_conn(conn),
+          user_agent: extract_user_agent(conn)
+        })
+
       form = Phoenix.Component.to_form(user_params, as: "user")
 
       # In order to prevent user enumeration attacks, don't disclose whether the email is registered.
@@ -181,7 +192,19 @@ defmodule ToweropsWeb.UserSessionController do
         )
         |> complete_totp_login(user)
 
-      {:error, _reason} ->
+      {:error, reason} ->
+        # Record failed TOTP verification for security monitoring
+        _ =
+          Accounts.record_login_attempt(%{
+            email: user.email,
+            success: false,
+            failure_reason: "invalid_totp",
+            method: "totp",
+            ip_address: ToweropsWeb.RemoteIp.from_conn(conn),
+            user_agent: extract_user_agent(conn),
+            metadata: %{reason: inspect(reason)}
+          })
+
         render_totp_error(conn, code)
     end
   end
@@ -209,4 +232,11 @@ defmodule ToweropsWeb.UserSessionController do
     |> put_flash(:error, t("Invalid authentication code. Please try again."))
     |> render(:totp, form: form)
   end
+
+  defp extract_user_agent(conn) do
+    case Plug.Conn.get_req_header(conn, "user-agent") do
+      [ua | _] -> ua
+      [] -> nil
+    end
+  end
 end

lib/towerops_web/controllers/user_sudo_controller.ex

@@ -75,6 +75,9 @@ defmodule ToweropsWeb.UserSudoController do
         end
 
       {:error, :recovery_code_not_allowed} ->
+        # Log failed sudo attempt (recovery code used)
+        _ = log_failed_sudo_attempt(conn, user, "recovery_code_not_allowed")
+
         form = Phoenix.Component.to_form(%{"totp_code" => totp_code}, as: "user")
 
         conn
@@ -85,6 +88,9 @@ defmodule ToweropsWeb.UserSudoController do
         |> render(:verify, form: form)
 
       {:error, :invalid_code} ->
+        # Log failed sudo attempt (invalid code)
+        _ = log_failed_sudo_attempt(conn, user, "invalid_code")
+
         form = Phoenix.Component.to_form(%{"totp_code" => totp_code}, as: "user")
 
         conn
@@ -98,4 +104,23 @@ defmodule ToweropsWeb.UserSudoController do
     form = Phoenix.Component.to_form(%{}, as: "user")
     render(conn, :verify, form: form)
   end
+
+  defp log_failed_sudo_attempt(conn, user, reason) do
+    Accounts.record_login_attempt(%{
+      email: user.email,
+      success: false,
+      failure_reason: "sudo_verification_failed",
+      method: "sudo_totp",
+      ip_address: ToweropsWeb.RemoteIp.from_conn(conn),
+      user_agent: extract_user_agent(conn),
+      metadata: %{sudo_failure_reason: reason}
+    })
+  end
+
+  defp extract_user_agent(conn) do
+    case Plug.Conn.get_req_header(conn, "user-agent") do
+      [ua | _] -> ua
+      [] -> nil
+    end
+  end
 end

lib/towerops_web/live/device_live/index.ex

@@ -193,16 +193,21 @@ defmodule ToweropsWeb.DeviceLive.Index do
   end
 
   def handle_event("add_discovered_device", params, socket) do
-    identifier = Jason.decode!(params["identifier"])
+    case Jason.decode(params["identifier"]) do
-    discovered = Enum.find(socket.assigns.discovered_devices, &(&1.identifier == identifier))
+      {:ok, identifier} ->
+        discovered = Enum.find(socket.assigns.discovered_devices, &(&1.identifier == identifier))
 
-    prefill_params = %{
+        prefill_params = %{
-      "name" => discovered.hostname || "",
+          "name" => discovered.hostname || "",
-      "ip_address" => List.first(discovered.ip_addresses) || "",
+          "ip_address" => List.first(discovered.ip_addresses) || "",
-      "snmp_enabled" => "true"
+          "snmp_enabled" => "true"
-    }
+        }
 
-    {:noreply, push_navigate(socket, to: ~p"/devices/new?#{prefill_params}")}
+        {:noreply, push_navigate(socket, to: ~p"/devices/new?#{prefill_params}")}
+
+      {:error, _reason} ->
+        {:noreply, put_flash(socket, :error, t_equipment("Invalid device identifier"))}
+    end
   end
 
   defp perform_site_reorder(socket, site_id, new_position, organization_id) do

lib/towerops_web/user_auth.ex

@@ -19,6 +19,7 @@ defmodule ToweropsWeb.UserAuth do
   alias Phoenix.LiveView
   alias Towerops.Accounts
   alias Towerops.Accounts.Scope
+  alias Towerops.Admin.AuditLogger
   alias ToweropsWeb.Helpers.StatusHelpers
 
   # Make the remember me cookie valid for 14 days. This should match
@@ -443,6 +444,15 @@ defmodule ToweropsWeb.UserAuth do
         |> assign(:can_view_financials, Policy.can_view_financials?(membership))
         |> put_session(:current_organization_id, organization.id)
       else
+        # Log failed authorization attempt
+        _ =
+          AuditLogger.log_failed_access_attempt(
+            conn,
+            scope.user.id,
+            "organization:#{organization.id}",
+            "not_member"
+          )
+
         conn
         |> put_flash(:error, t_auth("You don't have access to this organization."))
         |> redirect(to: ~p"/orgs")