towerops/docs/SECURITY_AUDIT_INPUT_VALIDATION.md

Security Audit: Input Validation, Injection & Data Handling

Date: 2026-03-14 Auditor: Automated Security Review (Skippy) Scope: lib/towerops/, lib/towerops_web/live/, lib/towerops_web/controllers/, lib/towerops_web/components/

---

Summary

Severity	Count
🔴 High	3
🟠 Medium	5
🟡 Low	4

---

🔴 HIGH Severity

H1: IDOR — API Device Creation Allows Cross-Organization Assignment

File: lib/towerops_web/controllers/api/v1/devices_controller.ex, lines 245–250 File: lib/towerops/devices/device.ex, lines 133–140 (organization_id in cast list)

Issue: The default_organization_id/2 function only sets organization_id when it's nil or "". If the caller provides a different organization_id, it passes through unmodified. Since organization_id is in the Device changeset's cast list, an authenticated API user can create devices in other organizations by supplying a foreign organization_id in the request body.

# devices_controller.ex:245-250
defp default_organization_id(device_params, organization_id) do
  case Map.get(device_params, "organization_id") do
    nil -> Map.put(device_params, "organization_id", organization_id)
    "" -> Map.put(device_params, "organization_id", organization_id)
    _provided_id -> device_params  # ← User-supplied org_id passes through!
  end
end

Impact: An authenticated user with a valid API token can create devices (and trigger SNMP discovery) in any organization they know the ID of.

Fix: Always force the authenticated organization's ID:

defp default_organization_id(device_params, organization_id) do
  Map.put(device_params, "organization_id", organization_id)
end

---

H2: IDOR — Agent Token Deletion Without Organization Scoping

File: lib/towerops_web/live/agent_live/index.ex, line 163 File: lib/towerops/agents.ex, line 451

Issue: The "delete_agent" event handler passes the user-supplied id directly to Agents.delete_agent_token(id) without verifying the agent token belongs to the current user's organization. delete_agent_token/1 fetches by raw ID with Repo.get!, no org check.

# agent_live/index.ex:163
def handle_event("delete_agent", %{"id" => id}, socket) do
  case Agents.delete_agent_token(id) do  # ← No org scope check!

Impact: A user could delete agent tokens belonging to other organizations by crafting a WebSocket event with a known agent token ID.

Fix: Verify organization ownership before deletion:

def handle_event("delete_agent", %{"id" => id}, socket) do
  organization = socket.assigns.current_scope.organization
  case Agents.get_organization_agent_token(organization.id, id) do
    nil -> {:noreply, put_flash(socket, :error, "Agent not found")}
    agent_token -> 
      case Agents.delete_agent_token(agent_token.id) do
        # ...
      end
  end
end

---

H3: IDOR — Schedule/Escalation Policy Access Without Organization Check

Files:

• lib/towerops_web/live/schedule_live/show.ex, line 19
• lib/towerops_web/live/schedule_live/form.ex, line 27
• lib/towerops_web/live/escalation_policy_live/show.ex, line 10
• lib/towerops_web/live/escalation_policy_live/form.ex, line 27

Issue: These LiveViews fetch schedules and escalation policies by raw ID (OnCall.get_schedule!(id), OnCall.get_escalation_policy!(id)) without verifying the resource belongs to the user's current organization. Any authenticated user who knows a schedule/policy UUID can view, edit, and delete it.

# schedule_live/show.ex:19
schedule = OnCall.get_schedule!(id)  # ← No org check
# escalation_policy_live/show.ex:10
policy = OnCall.get_escalation_policy!(id)  # ← No org check

Impact: Cross-organization read/write/delete access to on-call schedules and escalation policies.

Fix: Add organization scoping to the queries:

def get_schedule!(id, organization_id) do
  Schedule
  |> where(id: ^id, organization_id: ^organization_id)
  |> Repo.one!()
  |> Repo.preload(overrides: :user, layers: [members: :user])
end

Then use OnCall.get_schedule!(id, organization.id) in the LiveViews.

---

🟠 MEDIUM Severity

M1: LIKE Wildcard Injection in Multiple Search Functions

Files:

• lib/towerops/devices.ex, line 148 (search_devices/2)
• lib/towerops/sites.ex, line 40 (search_sites/2)
• lib/towerops/gaiia.ex, lines 122, 133, 208 (search_inventory_items, search_network_sites, suggest_site_matches)

Issue: These functions build ILIKE patterns as "%#{query}%" without escaping % and _ wildcards. A user can submit % as a search query to match all records, or use _ for single-character wildcards.

Compare with lib/towerops/search.ex (lines 89–93) and lib/towerops/trace.ex (lines 456–461) which correctly call sanitize/sanitize_like.

# devices.ex:148 — NO sanitization
search_term = "%#{query}%"

# search.ex:89 — CORRECT
defp sanitize(query) do
  query
  |> String.replace("%", "\\%")
  |> String.replace("_", "\\_")
end

Impact: Information disclosure — users can craft patterns to enumerate all records or perform wildcard-based probing. Low direct risk since queries are org-scoped, but violates defense-in-depth.

Fix: Add sanitize_like/1 to all search functions that build ILIKE patterns, or extract a shared helper.

---

M2: Jason.decode! on Untrusted Input (Crash on Malformed JSON)

File: lib/towerops_web/live/device_live/index.ex, line 196

Issue: Jason.decode!(params["identifier"]) will raise a Jason.DecodeError if the client sends malformed JSON. While Phoenix LiveView catches this and the process restarts, it causes a brief disruption for the user and generates noisy error logs.

def handle_event("add_discovered_device", params, socket) do
  identifier = Jason.decode!(params["identifier"])  # ← Crashes on bad input

Impact: Denial of service for the user's LiveView session. The process restarts, but data in assigns is lost.

Fix: Use Jason.decode/1 with error handling:

case Jason.decode(params["identifier"]) do
  {:ok, identifier} -> # proceed
  {:error, _} -> {:noreply, put_flash(socket, :error, "Invalid identifier")}
end

---

M3: Role Change Without Caller Permission Check

File: lib/towerops_web/live/org/settings_live.ex, lines 208–223

Issue: The "change_role" event handler calls Organizations.update_member_role/3 without checking whether the current user has permission to change roles. Any authenticated member of the organization can change anyone else's role (except the owner's). A member or viewer role user could promote themselves to admin.

The update_member_role/3 function only prevents changing the owner's role, but doesn't verify the caller's role:

def update_member_role(organization_id, user_id, new_role) do
  case get_membership(organization_id, user_id) do
    %Membership{role: :owner} -> {:error, :cannot_change_owner_role}
    %Membership{} = membership -> update_membership(membership, %{role: new_role})
    # ← No check that the CALLER is an owner/admin
  end
end

Impact: Privilege escalation — any org member can promote themselves to admin role.

Fix: Check that the current user is an owner or admin before allowing role changes:

def handle_event("change_role", %{"user-id" => user_id, "role" => role}, socket) do
  organization = socket.assigns.organization
  current_user = socket.assigns.current_scope.user
  
  # Verify caller has permission
  caller_membership = Organizations.get_membership(organization.id, current_user.id)
  unless caller_membership.role in [:owner, :admin] do
    {:noreply, put_flash(socket, :error, "You don't have permission to change roles")}
  else
    # proceed with role change
  end
end

---

M4: SSRF via HTTP Monitoring Checks

File: lib/towerops/monitoring/executors/http_executor.ex File: lib/towerops_web/live/check_live/form_component.ex, lines 282–303

Issue: Users can create HTTP monitoring checks with arbitrary URLs. The executor makes server-side HTTP requests to these URLs without restricting the target. This could be used to probe internal services (metadata endpoints, localhost services, internal APIs).

# http_executor.ex — No URL validation/restriction
opts = [
  method: String.to_atom(method),
  url: Map.fetch!(config, "url"),  # ← Any URL, including internal

Impact: Server-side request forgery (SSRF). Can probe internal network, cloud metadata services (169.254.169.254), and localhost services.

Fix: Validate URLs against an allowlist or block internal/private IP ranges before making the request. At minimum, block requests to 127.0.0.0/8, 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16, 169.254.169.254, and ::1.

---

M5: inspect() Leaking Internal Details in API Error Responses

Files:

• lib/towerops_web/controllers/api/v1/mib_controller.ex, line 93
• lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex, line 26

Issue: These endpoints return inspect(reason) in JSON error responses, which can leak internal Elixir data structures, module names, and system paths to API clients.

# mib_controller.ex:93
|> json(%{error: inspect(reason)})  # ← Leaks internal error details

# agent_release_webhook_controller.ex:26
|> json(%{status: "error", error: inspect(reason)})

Impact: Information disclosure — internal error details aid attackers in understanding the application structure.

Fix: Return generic error messages to clients and log the detailed error server-side:

Logger.error("MIB listing failed: #{inspect(reason)}")
json(conn, %{error: "Internal server error"})

---

🟡 LOW Severity

L1: String.to_integer/1 on URL Params Without Validation

Files:

• lib/towerops_web/live/device_live/index.ex, line 67: String.to_integer()
• lib/towerops_web/live/device_live/show.ex, line 76: String.to_integer()

Issue: String.to_integer/1 raises ArgumentError on non-numeric input. If a user crafts a URL with ?page=abc, the LiveView process crashes.

Impact: Minor DoS — LiveView process restarts. Low severity because LiveView handles this gracefully with reconnection.

Fix: Use Integer.parse/1 with a default fallback:

page = case Integer.parse(Map.get(params, "page", "1")) do
  {n, ""} when n > 0 -> n
  _ -> 1
end

---

L2: Unverified TOTP Device Deletion Race

File: lib/towerops_web/live/user_settings_live/totp_manager.ex, line 57

Issue: Repo.get!(UserTotpDevice, device_id) fetches the TOTP device by ID from socket.assigns.new_device_id without verifying it still belongs to the current user. While new_device_id is set server-side, a race condition exists if the user opens the verification flow in multiple tabs.

Impact: Minimal — the device ID comes from server-side assigns, not user input. Theoretical only.

Fix: Scope the query: Repo.get_by!(UserTotpDevice, id: device_id, user_id: user.id)

---

L3: raw() Usage in Registration Template with t_auth() Translations

File: lib/towerops_web/controllers/user_registration_html/new.html.heex, lines 164, 187

Issue: The raw() call wraps the output of t_auth() translation, which includes interpolated values. If translation strings are ever sourced from user-editable content (CMS, admin panel), this becomes an XSS vector. Currently safe because translations are compile-time .po files.

raw(~s(<a href="/privacy" ...>#{t_auth("Privacy Policy")}</a>))

Impact: Currently no risk (compile-time translations). Becomes XSS if translation source changes.

Fix: Use HEEx components instead of raw() for link construction:

<a href="/privacy" target="_blank" class="..."><%= t_auth("Privacy Policy") %></a>

---

L4: HTTP Executor String.to_atom/1 on User Input

File: lib/towerops/monitoring/executors/http_executor.ex, line 57

Issue: String.to_atom(method) converts the HTTP method from config to an atom. While the method comes from check config (not direct user request params), atoms are not garbage collected. Repeated creation of unique method strings could leak atoms, though in practice the valid HTTP methods are a small fixed set.

method: String.to_atom(method),  # ← Atom table pollution risk

Impact: Theoretical atom table exhaustion. Low because check config is validated and limited.

Fix: Use String.to_existing_atom/1 or a whitelist:

defp method_to_atom("get"), do: :get
defp method_to_atom("post"), do: :post
defp method_to_atom("put"), do: :put
defp method_to_atom("delete"), do: :delete
defp method_to_atom("head"), do: :head
defp method_to_atom("patch"), do: :patch
defp method_to_atom(_), do: :get

---

✅ Positive Findings (Things Done Well)

1. AccessControl module (lib/towerops_web/live/helpers/access_control.ex) — Devices, sites, and alerts are properly scoped to organization. Good pattern.

2. LIKE sanitization in Search, Trace, and Snmp modules — Properly escapes % and _ wildcards.

3. MIB Controller path traversal protection — Vendor names are validated against ^[a-zA-Z0-9_-]+$, extracted archive paths are validated against the extraction directory, and filenames use Path.basename().

4. Stripe webhook signature verification — Uses Plug.Crypto.secure_compare/2 for timing-safe comparison and validates timestamp tolerance.

5. Error JSON handler — ErrorJSON returns generic 500 messages without leaking internals.

6. CSRF protection — Phoenix's built-in CSRF tokens are active for all forms.

7. HEEx auto-escaping — All templates use {@variable} syntax which auto-escapes by default. No XSS from dynamic user content in templates.

8. Ecto parameterized queries — All fragment() calls use ^variable bindings, preventing SQL injection.

9. Encrypted sensitive fields — SNMP passwords and MikroTik credentials use Encrypted.Binary type.

10. Backup deletion — Properly verifies device organization access via AccessControl.verify_device_access/2 before allowing deletion.