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.

```elixir
# 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:
```elixir
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.

```elixir
# 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:
```elixir
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.

```elixir
# 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:
```elixir
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`.

```elixir
# 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.

```elixir
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:
```elixir
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:

```elixir
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:
```elixir
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).

```elixir
# 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.

```elixir
# 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:
```elixir
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:
```elixir
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.

```heex
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:
```heex
<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.

```elixir
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:
```elixir
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.