# Code Review Findings

> Full codebase audit conducted 2026-05-12. Covers OWASP Top 10, performance, reliability, and correctness issues across the entire TowerOps Web application.

---

## CRITICAL

### C1. ✓ FIXED — Stored XSS via Sites Map Popup (innerHTML)

**Files:**
- `assets/js/hooks/sites_map.ts:75-101` — popup construction with template literals
- `map_live/index.html.heex:51` — `data-sites={Jason.encode!(@sites)}`

**Severity:** CRITICAL — Stored XSS affecting all users viewing the map

**Description:** Site name, description, and address from the database are interpolated into template literals and injected via `marker.bindPopup()` (which uses `innerHTML`). HEEx escapes the JSON attribute in the template, but `JSON.parse` in the hook restores the original unescaped strings, then template literals inject them directly into HTML without sanitization. Any user who can create/edit a site can execute arbitrary JS for every map viewer.

```typescript
// vulnerable code
let popupContent = `<div class="p-2">
  <h3 class="font-semibold text-lg mb-1">${site.name}</h3>`
```

**Fix:** Use `textContent`-based popup rendering or sanitize via DOMPurify. Create a DOM element and set `textContent` for user data:

```typescript
const popupEl = document.createElement('div')
popupEl.className = 'p-2'
const h3 = popupEl.appendChild(document.createElement('h3'))
h3.className = 'font-semibold text-lg mb-1'
h3.textContent = site.name
marker.bindPopup(popupEl)
```

---

### C2. ✓ FIXED — IDOR in Policy Consent Handler — Client-Controlled User ID

**File:** `lib/towerops_web/user_auth.ex:693`

**Severity:** CRITICAL — Any authenticated user can grant/revoke consent for any other user

**Description:** The `accept_updated_policies` on_mount handler reads `user-id` directly from LiveView event params with zero validation:

```elixir
"accept_updated_policies", %{"user-id" => user_id}, socket ->
  ...
  Accounts.grant_consent(user_id, policy_type)
```

Attached to ALL authenticated `live_session` scopes. An attacker can send arbitrary `user_id` values.

**Fix:** Derive user ID from `socket.assigns.current_scope.user.id` instead of client params.

---

### C3. ✓ FIXED — Broken `halt()` in GDPR Data Export

**File:** `lib/towerops_web/controllers/api/account_data_controller.ex:20-26`

**Severity:** CRITICAL — Unconfirmed users can download full GDPR data export

**Description:** The halting check for unconfirmed users discards the result:

```elixir
_ = if !user.confirmed_at do
  conn |> put_status(:forbidden) |> json(...) |> halt()
end
```

`halt()` returns a modified conn, but it's bound to `_`. The original `conn` is used, so the function proceeds to gather ALL user data.

**Fix:** Restructure to early-return pattern, not orphan `halt()`.

---

### C4. ✓ FIXED — Missing Authorization on Member Role Changes

**File:** `lib/towerops_web/controllers/api/v1/members_controller.ex:20-51`

**Severity:** CRITICAL — Any API token holder can change roles or remove any member

**Description:** `update` and `delete` actions call into `Organizations` context functions that do NOT check whether the requesting user has permission. The context functions look up membership by organization_id + user_id and perform the action — no role/authority check for the caller.

**Fix:** Add authorization at the controller level — verify `current_user` is owner/admin before allowing role changes or removals.

---

### C5. ✓ FIXED — Agent Release Webhook — Optional Signature Means No Auth

**File:** `lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex:37-42`

**Severity:** CRITICAL — Anyone who discovers the endpoint can trigger mass agent updates

**Description:** If signature header is absent, the webhook accepts the request unconditionally:

```elixir
defp verify_optional_signature(conn) do
  sig_headers = Plug.Conn.get_req_header(conn, "x-agent-webhook-signature")
  case sig_headers do
    [] -> :ok   # passes without any auth
    ...
```

**Fix:** Remove the "optional" behavior. Require HMAC signature on every request.

---

### C6. ✓ FIXED — Repo.all_by/2 Does Not Exist — Runtime Crash

**File:** `lib/towerops/accounts.ex:386-387`

**Severity:** CRITICAL — Runtime FunctionClauseError when updating passwords/changing email

**Description:** `Repo.all_by/2` is NOT a standard Ecto function. It will raise `FunctionClauseError` at runtime when `update_user_and_delete_all_tokens/1` is called (password change, email change). The DB transaction crashes with a generic error.

```elixir
tokens_to_expire = Repo.all_by(UserToken, user_id: user.id)
```

**Fix:** Replace with `Repo.all(from t in UserToken, where: t.user_id == ^user.id)`.

---

### C7. ✓ FIXED — Brute Force Protection Runs Before Authentication

**File:** `lib/towerops_web/endpoint.ex:68` → plug runs here
**File:** `lib/towerops_web/plugs/brute_force_protection.ex:47` — dead auth exemption check

**Severity:** CRITICAL — Authenticated user exemption path is unreachable; every authenticated 404 gets tracked

**Description:** `BruteForceProtection` runs in the endpoint pipeline LONG BEFORE auth happens. The check `conn.assigns[:current_user] != nil` will ALWAYS be `nil`. The authenticated-user exemption path is completely dead code.

**Fix:** Move auth exemption check to `register_before_send` callback, or restructure pipeline ordering.

---

### C8. ✓ FIXED — Status Page `custom_css` Injection

**File:** `lib/towerops_web/live/status_page_live.html.heex:26-30`

**Severity:** CRITICAL — Any org admin with status page access can inject arbitrary HTML/JS

**Description:** `custom_css` is inserted raw into a `<style>` tag. The server-side validator blocks `url()` but NOT `</style>` injection:

```heex
<%= if @config.custom_css do %>
  <style>
    {@config.custom_css}
  </style>
<% end %>
```

**Fix:** Sanitize to remove `</style>`, `</script>`, `<script` strings. Add server-side validation.

---

### C9. ✓ FIXED — Live Production Credentials in Plaintext on Disk

**File:** `k8s/secrets.yaml` (exists on disk, in `.gitignore`)

**Severity:** CRITICAL — Full production credential set on local filesystem

**Description:** Contains real production credentials: Stripe live key, AWS access keys, DeepSeek API key, PostgreSQL superuser credentials, Cloak encryption key, secret_key_base, Redis password, release cookie. A single accidental `git add --force` or CI misconfiguration leaks everything.

**Fix:** Remove the file; use `k8s/secrets.example.yaml` with placeholders. Load real secrets from 1Password or Kubernetes secrets at runtime.

---

### C10. ✓ FIXED — Hardcoded Stripe Test Key in Git

**Files:** `config/dev.exs:243`, `setup_stripe_meter.exs:5`

**Severity:** CRITICAL — Real Stripe Test API key in version control

**Description:** `sk_test_51T7zcQS77kvnTfgyu0DCQU2xVKzeaneVCueHaXV3jsIw6HAwGWWllEL3J8jGZOybHAtPyu6oYIiuvJHhReFtTycE00VrJYJ8Ao` is hardcoded in two files committed to git. Every developer has Stripe test environment access.

**Fix:** Load from `STRIPE_SECRET_KEY` env var consistently across all environments.

---

### C11. BY DESIGN — No `force_ssl` in Production

**File:** `config/prod.exs:41` — relies on Cloudflared proxy

**Severity:** CRITICAL — HTTP downgrade / cleartext traffic risk

**Description:** If Cloudflared is misconfigured, traffic arrives over plain HTTP with no redirect. No HSTS headers. Session cookies / API tokens sent in cleartext.

**Fix:** Enable `force_ssl: [hsts: true]` in production endpoint config.

---

### C12. FALSE POSITIVE — Race Condition in Device Quota Check

**Files:** `lib/towerops/devices.ex:539-578, 586-598`

**Severity:** CRITICAL — Concurrent device creation bypasses subscription limits

**Description:** `create_device` locks org row with `FOR UPDATE` but this doesn't serialize inserts into the `devices` table. Two concurrent calls both pass quota check (same count), then both insert.

**Fix:** Use `pg_advisory_xact_lock` keyed on `organization_id`, or move device count into the organization row.

---

### C13. ✓ FIXED — MikroTik Credentials Broadcast Over PubSub

**File:** `lib/towerops/workers/mikrotik_backup_worker.ex:95-98`

**Severity:** CRITICAL — Plaintext credentials exposed to all PubSub subscribers

**Description:** MikroTik usernames and passwords are placed in the `MikrotikDevice` struct and broadcast over PubSub on `"agent:#{agent_token_id}:backup"` channel. Any internal process or LiveView subscribed receives credentials.

**Fix:** Use a job ID that agents resolve independently, or encrypt credentials with Vault before broadcasting.

---

## HIGH

### H1. N+1 Queries in MobileController

**File:** `lib/towerops_web/controllers/api/mobile_controller.ex:38-46,78-84`

**Severity:** HIGH — 3N + 2M extra queries per request

**Description:** For each organization: 3 count queries (sites, devices, active alerts). For each site: 2 count queries (total devices, down devices). 1 + 3N + 2M DB round-trips.

**Fix:** Use batch count queries as already partially implemented in `devices.ex:98`.

---

### H2. N+1 Oban `cancel_all_jobs` per Check

**File:** `lib/towerops/monitoring.ex:463-473`

**Severity:** HIGH — One DB round-trip per check when stopping device checks

**Description:** Loops through check_ids calling `Oban.cancel_all_jobs` individually. A device with 50 checks = 50 queries. Should batch all check_ids into a single query using `ANY(?)`.

**Fix:** Use `fragment("args->>'check_id' = ANY(?)", ^check_ids)` in a single query.

---

### H3. N+1 Alert Resolution

**File:** `lib/towerops/alerts.ex:381-388`

**Severity:** HIGH — One DB write per alert when resolving

**Description:** Loads all alerts then `Enum.each` calls `resolve_alert_silent()` per alert. For 1000 alerts in a storm: 1000 individual `Repo.update` calls + 1000 PubSub broadcasts.

**Fix:** Use `Repo.update_all` with a single query.

---

### H4. Race Condition in `create_check` (Auto-Discovery)

**File:** `lib/towerops/monitoring.ex:152-184`

**Severity:** HIGH — Concurrent discoveries crash the job with Ecto.ConstraintError

**Description:** Two concurrent discoveries can both get `nil` from `get_by` and both attempt `insert`. The unique index prevents duplicates but one crashes with unhandled `Ecto.ConstraintError`.

**Fix:** Use `Repo.insert(..., on_conflict: :nothing)` or wrap in transaction with `FOR UPDATE`.

---

### H5. Missing Transaction in `apply_agent_to_all_equipment`

**File:** `lib/towerops/sites.ex:223-252`

**Severity:** HIGH — Partial data loss on failure

**Description:** `delete_all` runs before `insert_all`. If insert fails, assignments are lost without rollback.

**Fix:** Wrap in `Repo.transaction`.

---

### H6. N+1 Interface Capacity Queries

**File:** `lib/towerops/capacity.ex:53-83`

**Severity:** HIGH — 20+ individual queries per site capacity view

**Description:** `get_site_capacity_summary` loops over interfaces calling `get_utilization` which does `InterfaceStat |> where(interface_id: ^id) |> Repo.all()` per interface.

**Fix:** Batch-load all interface stats in a single query.

---

### H7. Unauthenticated Webhook Endpoints

**File:** `lib/towerops/router.ex:232-239`

**Severity:** HIGH — Webhook endpoints protected only by per-org secret, no router-level auth

**Description:** Gaiia, PagerDuty, and MikroTik webhooks use the `:api` pipeline which only has `plug :accepts, ["json"]` — no authentication. Auth is delegated entirely to each controller.

**Fix:** Add webhook auth plug at pipeline level.

---

### H8. Missing Session Expiry Check in Mobile/GraphQL Auth

**File:** `lib/towerops_web/plugs/mobile_auth.ex:48-53`
**File:** `lib/towerops_web/plugs/graphql_auth.ex:58-63`

**Severity:** HIGH — Expired/revoked sessions remain usable indefinitely

**Description:** `validate_session` checks the record exists but does NOT check `expires_at`. If the query doesn't filter expired rows, revoked sessions stay active.

**Fix:** Add `DateTime.compare(expires_at, DateTime.utc_now())` check.

---

### H9. CSP `img-src https:` Wildcard

**File:** `lib/towerops_web/plugs/security_headers.ex:38`

**Severity:** HIGH — Broad data exfiltration vector via any XSS

**Description:** `https:` for img-src allows images from any HTTPS origin. Combined with any XSS, an attacker can exfiltrate data via image request URLs.

**Fix:** Restrict to specific domains.

---

### H10. Missing HSTS Header

**File:** `lib/towerops_web/plugs/security_headers.ex:22-29`

**Severity:** HIGH — SSL stripping attacks possible

**Description:** Security headers plug sets CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy — but NOT `Strict-Transport-Security`.

**Fix:** Add `strict-transport-security: max-age=63072000; includeSubDomains; preload`.

---

### H11. Hardcoded Session Salts (8 bytes)

**File:** `lib/towerops_web/endpoint.ex:11-12`
**File:** `config/config.exs:135`

**Severity:** HIGH — Weak entropy for cookie signing; identical across all environments

**Description:** `signing_salt: "hrDZxLhd"` and `encryption_salt: "vK3p8mNx"` are hardcoded 8-byte salts. Phoenix recommends 16+ bytes. Also baked into source code — can't rotate without deploy.

**Fix:** Load from env vars, increase to 16+ bytes.

---

### H12. Missing `Secure` and `HttpOnly` on Session Cookie

**File:** `lib/towerops_web/endpoint.ex:8-14`
**File:** `lib/towerops/user_auth.ex:29-33` (remember-me cookie)

**Severity:** HIGH — Session cookie sent over HTTP; accessible via JS

**Description:** `@session_options` has `same_site: "Lax"` but no `secure: true` or `http_only: true`. Session cookie transmitted in cleartext over HTTP and accessible via `document.cookie`.

**Fix:** Add `secure: true, http_only: true` to both session and remember-me cookie configs.

---

### H13. PromEx Metrics Server Unauthenticated

**File:** `config/config.exs:106-117`

**Severity:** HIGH — Internal metrics accessible without any auth

**Description:** `auth_strategy: :none` on metrics server. Exposes DB query performance, memory info, business logic execution times.

**Fix:** Add at minimum basic token auth, or document network isolation explicitly.

---

### H14. `String.to_integer/1` on Unvalidated SNMP OID Components

**Files:**
- `lib/towerops/snmp/neighbor_discovery.ex:202,299,129`
- `lib/towerops/snmp/discovery.ex:1368`
- `lib/towerops/snmp/profiles/base.ex:1332`

**Severity:** HIGH — Malicious SNMP responses crash GenServers (DoS)

**Description:** OID suffixes extracted from SNMP responses are passed directly to `String.to_integer/1`. An attacker-controlled device can send non-numeric OID fragments, crashing the poller process.

**Fix:** Replace with `Integer.parse/1` + pattern match. Never trust OID suffixes from network data.

---

### H15. `String.to_existing_atom/1` on User Input in ActivityFeed

**File:** `lib/towerops/activity_feed_live.ex:55`

**Severity:** HIGH — Any user can crash the activity feed LiveView (DoS)

**Description:** `String.to_existing_atom(type_str)` where `type_str` comes from client event params. If the atom doesn't exist, `ArgumentError` kills the LiveView process.

**Fix:** Use a whitelist of allowed types.

---

### H16. `String.to_integer/1` on User-Controlled Page Param

**Files:**
- `lib/towerops_web/live/user_settings_live.ex:63`
- `lib/towerops_web/live/admin/dashboard_live.ex:19`

**Severity:** HIGH — Non-integer page param crashes LiveView (DoS)

**Description:** `params |> Map.get("page", "1") |> String.to_integer()` — passing `?page=abc` raises an error killing the process.

**Fix:** Use `Integer.parse` with fallback to 1.

---

### H17. Reports LiveView — Missing Org Scope on Individual Operations

**File:** `lib/towerops/reports_live.ex:73-101`

**Severity:** HIGH — IDOR on report toggle, delete, run_now

**Description:** `toggle`, `delete`, `run_now` fetch reports by ID without verifying org membership. A user who guesses another org's report ID can manipulate it.

**Fix:** Add org scope verification to all individual operations.

---

### H18. Mass Assignment Risk Across All CRUD Endpoints

**Files:** Multiple v1 controllers

**Severity:** HIGH — Unchecked params passed to context functions

**Description:** All create/update actions pass raw user params to context functions. Only protection is changeset `cast/3`. If any changeset doesn't restrict fields properly, arbitrary columns can be set.

**Fix:** Add explicit field allowlist filtering at controller level (like `IntegrationsController.to_atom_keys/1`).

---

### H19. QR Token Verify Leaks User Email

**File:** `lib/towerops_web/controllers/api/mobile_auth_controller.ex:43-45`

**Severity:** HIGH — Token validity + user email disclosed to anyone with a QR token

**Description:** `json(conn, %{valid: true, user_email: qr_token.user.email})` — reveals the associated email on token verify.

**Fix:** Only return `valid: true/false`. Don't disclose email until token is consumed.

---

### H20. CoverageWorker Contradictory Retry Settings

**File:** `lib/towerops/workers/coverage_worker.ex:23-25, 485-486`

**Severity:** HIGH — `max_attempts: 3` but `fail/2` returns `:ok`

**Description:** Configured for retries, but the `fail` path returns `:ok`, preventing Oban from ever retrying. Misleading configuration.

**Fix:** Remove `max_attempts` (use default 1) or return `{:error, reason}` for transient failures.

---

### H21. Unbounded Sequential Processing in Sync Workers

**Files:**
- `gaiia_sync_worker.ex:20`
- `sonar_sync_worker.ex:16`
- `splynx_sync_worker.ex:16`
- `visp_sync_worker.ex:16`
- `netbox_sync_worker.ex:20`

**Severity:** HIGH — Single job processes ALL integrations sequentially; 10+ minute blocks

**Description:** `Enum.map(integrations, &sync_integration/1)` — sequentially syncs every enabled integration in a single Oban job. Blocks the queue slot for extended periods.

**Fix:** Follow `UispSyncWorker` pattern — staggered per-integration jobs with unique constraints.

---

### H22. DataRetentionWorker Unbounded DELETEs

**File:** `lib/towerops/workers/data_retention_worker.ex:61-173`

**Severity:** HIGH — Massive DELETE statements with no batching

**Description:** Each table purged with a single DELETE. On millions of rows: WAL amplification, autovacuum storms, long row locks, blocked concurrent reads.

**Fix:** Use batched deletes (`LIMIT 10000` in a loop) or partition-based truncation.

---

### H23. Sync Workers Silently Swallow Errors

**Files:** All sync workers (uisp, preseem, cn_maestro, sonar, splynx, visp, netbox, gaiia)

**Severity:** HIGH — Transient failures NEVER retried; data silently stale

**Description:** All sync workers log errors but return `:ok`. Network timeouts, API rate limits, DNS failures are all silently accepted. Data goes stale until next cron cycle.

**Fix:** Distinguish permanent (return `:ok`/`:discard`) from transient (return `{:error, reason}` for Oban retry).

---

### H24. WeatherSyncWorker Never Starts

**File:** `lib/towerops/workers/weather_sync_worker.ex`

**Severity:** HIGH — Weather data never fetched (dead code path)

**Description:** Worker self-schedules at end of each run via `schedule_next/0`, but no initial trigger exists in any cron config. First run never occurs.

**Fix:** Add to Oban cron schedule or trigger at application startup.

---

### H25. No Device/Site Organization Validation on Check Creation

**File:** `lib/towerops_web/controllers/api/v1/checks_controller.ex:47-60`

**Severity:** HIGH — Check can reference devices from other organizations

**Description:** Only `check_type` is validated. The `device_id` is not verified to belong to the authenticated org.

**Fix:** Add device access verification using `ScopedResource.fetch/3`.

---

### H26. N+1 Latency Queries per Site Device

**File:** `lib/towerops_web/live/site_live/show.ex:101-128`

**Severity:** HIGH — 50+ queries for large sites

**Description:** Loops through devices calling `Monitoring.get_latency_data` per device. For 50+ devices at a site, 50+ individual queries.

**Fix:** Batch the latency query or add pagination/limit.

---

### H27. Backslash-Wildcard Injection in Search

**File:** `lib/towerops/search.ex:90-94`

**Severity:** HIGH — Search bypass via backslash injection leaks wildcard matching

**Description:** `sanitize/1` escapes `%` and `_` but NOT `\` first. A search for `\%%` becomes `\\%\\%` — PG interprets `\\` as literal `\` followed by `%` wildcard, leaking wildcard matching.

**Fix:** Escape `\` first, or reuse `Towerops.QueryHelpers.sanitize_like/1`.

---

### H28. Nominatim API Response Injected via innerHTML

**File:** `assets/js/hooks/coverage_hooks.ts:635-645`

**Severity:** HIGH — Third-party API response flows into innerHTML

**Description:** Nominatim search API `display_name` is passed directly to `marker.bindPopup()` (innerHTML). If Nominatim is compromised or DNS poisoned, arbitrary JS executes.

**Fix:** Sanitize `display_name` with `escapeHtml()` before `bindPopup()`.

---

### H29. OID Injection via Substring Match in Profiles

**File:** `lib/towerops/profiles/yaml_profiles.ex:788`

**Severity:** HIGH — Incorrect profile matching due to partial OID match

**Description:** `String.contains?(sys_object_id, block.oid)` matches partial octets. OID `1.3.6.1.4.1.9` would incorrectly match `1.3.6.1.4.1.99` because "9" is a substring of "99".

**Fix:** Use `String.starts_with?(sys_object_id <> ".", block.oid <> ".")` for dot-boundary matching.

---

### H30. JobCleanupTask Cancels Executing Jobs

**File:** `lib/towerops/workers/job_cleanup_task.ex:47-63`

**Severity:** HIGH — Rolling deployments kill currently executing poll jobs

**Description:** `cancel_all_jobs` filters on `["available", "scheduled", "executing", "retryable"]` states. In-progress polling/monitoring is discarded.

**Fix:** Exclude `"executing"` state so active jobs complete naturally.

---

## MEDIUM

### M1. Resource Scoping Reveals Org Membership

**File:** `lib/towerops_web/scoped_resource.ex:15-20`

**Severity:** MEDIUM — Cross-org resource existence probing

**Description:** Returns `:forbidden` for wrong-org resources vs `:not_found` for nonexistent. Organization A can probe if a specific resource ID belongs to Organization B.

**Fix:** Return `:not_found` in both cases — use `Repo.get_by(schema, id: id, organization_id: organization_id)`.

---

### M2. Email Lookups Are Case-Sensitive

**File:** `lib/towerops/accounts.ex:39-41`

**Severity:** MEDIUM — Lookalike account registration possible

**Description:** `Repo.get_by(User, email: email)` — `User@Example.com` and `user@example.com` are separate accounts.

**Fix:** Add `citext` column or use `fragment("LOWER(email) = LOWER(?)", ^email)`.

---

### M3. Captcha/Verification Token Pattern Matches

**File:** `lib/towerops_web/user_auth.ex:70-86` (valid_return_path?)

**Severity:** MEDIUM — Open redirect via URL encoding bypasses

**Description:** Blacklist-based `valid_return_path?` can be bypassed with URL encoding: `/%2f%2fevil.com`, tab injection, double encoding.

**Fix:** Use whitelist approach matching known route prefixes.

---

### M4. Stripe Webhook Signature Not Enforced at Plug Level

**File:** `lib/towerops_web/router.ex:144-148`

**Severity:** MEDIUM — Any future controller could forget to verify

**Description:** Stripe webhook verification is deferred entirely to the controller with no router-level enforcement.

**Fix:** Create a dedicated Stripe webhook auth plug.

---

### M5. Rate Limiting Keyed Only by IP

**File:** `lib/towerops_web/plugs/rate_limit.ex:57`

**Severity:** MEDIUM — NAT'd users share budget; IP-rotation bypasses limits

**Description:** Rate limit key is `"#{type}:#{remote_ip}"` with no user/token component. Users behind NAT share a single budget. Attackers with large IP pools bypass limits entirely.

**Fix:** Include `current_user.id` or token ID in the key: `"#{type}:#{user_id}:#{remote_ip}"`.

---

### M6. `get_node_detail` Loads Entire Organization

**File:** `lib/towerops/topology.ex:612-663`

**Severity:** MEDIUM — Massive unnecessary data transfer for single node lookup

**Description:** Loads ALL device IDs and ALL unlinked DeviceLink records for the entire organization just to find one node.

**Fix:** Find the link directly with a join query based on the discovered node ID.

---

### M7. No Unique Constraint on `(organization_id, email)` for Invitations

**File:** Migration `20251221192513_create_organization_invitations.exs`

**Severity:** MEDIUM — Duplicate pending invitations possible; XSS-like confusion

**Description:** Individual indexes on `token`, `organization_id`, `email` but no composite unique for pending invitations.

**Fix:** Add `create unique_index(:organization_invitations, [:organization_id, :email], where: "accepted_at IS NULL")`.

---

### M8. Webhook Secret Misconfiguration Disclosed

**File:** `lib/towerops_web/plugs/webhook_auth.ex:36-43`

**Severity:** MEDIUM — Attacker can determine webhook endpoint has zero auth

**Description:** Returns `"Webhook authentication not configured"` in the HTTP response body.

**Fix:** Return generic `%{error: "Internal server error"}`.

---

### M9. Information Disclosure via KMZ Error

**File:** `lib/towerops_web/controllers/api/v1/coverages_controller.ex:203-204`

**Severity:** MEDIUM — Internal paths and system errors leaked to client

**Description:** `inspect(reason)` is sent in error response. Could include system paths from `File.read`, `:zip.create`, or file system errors.

**Fix:** Log error server-side, return generic message.

---

### M10. `[membership]` Pattern Match Crash

**File:** `lib/towerops_web/controllers/api/account_data_controller.ex:75`

**Severity:** MEDIUM — 500 error for edge-case membership counts

**Description:** `[membership] = org.memberships` crashes with MatchError if 0 or 2+ memberships.

**Fix:** Use `case org.memberships do [membership] -> ... end`.

---

### M11. ActivityController `String.to_existing_atom` on User Input

**File:** `lib/towerops_web/controllers/api/v1/activity_controller.ex:37-41`

**Severity:** MEDIUM — Unpredictable atom matching from user-supplied strings

**Description:** User-supplied filter types are converted to atoms via `to_existing_atom`. If a string happens to match an internal atom, query behavior could be altered.

**Fix:** Whitelist allowed atom values.

---

### M12. No Superuser Verification in Admin LiveViews

**Files:** All `admin/*.ex` LiveViews

**Severity:** MEDIUM — No defense-in-depth if route misconfiguration exposes admin views

**Description:** Router applies `:require_superuser` but admin LiveViews don't perform their own `current_scope.superuser` check.

**Fix:** Add `on_mount` or `mount` superuser verification to admin LiveViews.

---

### M13. CSV/Download Content-Type Validation Missing

**File:** `assets/js/app.ts:374-393`

**Severity:** MEDIUM — Downloaded HTML file could execute scripts

**Description:** The `phx:download` handler accepts any `mime_type` from server. If server ever allows user content to influence mime_type, an HTML download with scripts could be created.

**Fix:** Add client-side `mime_type` allowlist.

---

### M14. WeatherSyncWorker Blocks Queue with `Process.sleep`

**File:** `lib/towerops/workers/weather_sync_worker.ex:46-53`

**Severity:** MEDIUM — Worker holds queue slot for 110s+ for 100 sites

**Description:** `Process.sleep(1_100)` between each site fetch blocks the Oban queue slot.

**Fix:** Use `Task.async_stream` with `max_concurrency: 1` and `timeout`, or dispatch per-site jobs.

---

### M15. CloudLatencyProbeWorker Unbounded PubSub Message

**File:** `lib/towerops/workers/cloud_latency_probe_worker.ex:39-43`

**Severity:** MEDIUM — Megabyte-sized PubSub messages with many devices

**Description:** Full device list broadcast as single PubSub message. With thousands of devices, this can overwhelm PubSub.

**Fix:** Chunk the device list or send IDs that agents resolve independently.

---

### M16. MutationObserver Not Stored for Cleanup

**File:** `assets/js/app.ts:284-310`

**Severity:** MEDIUM — Memory leak on every page navigation mounting this hook

**Description:** `const observer = new MutationObserver(...)` — stored locally, not as `this.observer`. The `destroyed()` method checks `this.observer` which is never set.

**Fix:** Change `const observer` to `this.observer = ...`.

---

### M17. SNMPv3 Fallback to MD5 Auth Protocol

**Files:** Multiple executor files

**Severity:** MEDIUM — Cryptographically broken auth protocol

**Description:** Default SNMPv3 auth protocol is `"MD5"`. MD5 is cryptographically broken for authentication.

**Fix:** Change default to `"SHA-256"` for new devices.

---

### M18. No String Length Validation for SNMP Credentials in Protobuf

**File:** `lib/towerops/proto/decode.ex:1509-1593`

**Severity:** MEDIUM — Memory exhaustion via oversized credential strings

**Description:** `decode_snmp_device` does NOT call `validate_string` on `community`, `v3_auth_password`, `v3_priv_password`. An attacker could send multi-megabyte credential strings.

**Fix:** Add `validate_string` with reasonable max lengths (255 bytes).

---

### M19. Missing Validation on Multiple Protobuf Decode Paths

**File:** `lib/towerops/proto/decode.ex` (multiple decode functions)

**Severity:** MEDIUM — No validation on `decode_mikrotik_device`, `decode_mikrotik_command`, `decode_check`, `decode_snmp_query`, `decode_sensor`, `decode_interface`

**Description:** Several decode functions have zero validation (no `validate_string` calls, no field-length checks).

**Fix:** Add validation consistent with existing validated decode paths.

---

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

---

### M23. Permission Checks Silent Failure on Unpreloaded Associations

**File:** `lib/towerops_web/permissions.ex:140-146`

**Severity:** MEDIUM — Hard-to-debug silent denial of access

**Description:** When `org.memberships` is not preloaded, `Ecto.assoc_loaded?` returns false, the `if` block evaluates to `nil`, and `do_can?` returns `false` silently.

**Fix:** Log a warning when permissions are checked without preloaded data.

---

## LOW

### L1. Cloudflare API Token Potentially Leaked in Logs

**File:** `lib/towerops/security/cloudflare_client.ex:47,74-76`

**Severity:** LOW — Debug logging could expose bearer token

**Description:** Cloudflare API token is passed as Bearer token. If HTTP client logs headers in debug mode, token leaks to logs.

**Fix:** Ensure HTTP client strips authorization headers from logs.

---

### L2. 404 Tracker EXPIRE Without NX Flag

**File:** `lib/towerops/security/four_oh_four_tracker.ex:32`

**Severity:** LOW — Attacker can extend tracking window indefinitely

**Description:** `EXPIRE` called on every 404 regardless of whether key already exists. NO `NX` option means each hit refreshes the TTL.

**Fix:** Use `EXPIRE key 60 NX` to only set expiry on first creation.

---

### L3. IPv4-Mapped IPv6 Not Handled in Whitelist

**File:** `lib/towerops/security/brute_force.ex:36-49`

**Severity:** LOW — `::ffff:192.168.1.1` doesn't match whitelist entry `192.168.1.1`

**Description:** Exact string matching means IPv4-mapped IPv6 addresses bypass whitelist matching.

**Fix:** Normalize IPs via `:inet.parse_address/1` before comparing.

---

### L4. No Auth on PromEx Metrics Port (Documentation Gap)

**File:** `config/config.exs:106-117`

**Severity:** LOW — Document that network isolation is the only protection

**Description:** `auth_strategy: :none` is acceptable if the port is firewalled, but this should be explicitly documented.

**Fix:** Add comment documenting expected network security.

---

### L5. Vault Initialization Reads Prod Key in Dev

**File:** `lib/towerops/vault.ex:29`

**Severity:** LOW — If `CLOAK_KEY` env var is set, dev uses production encryption key

**Description:** `if System.get_env("CLOAK_KEY") do` — no environment guard. A developer with prod env set will use the production key in dev.

**Fix:** Only read `CLOAK_KEY` in production, or use a separate `CLOAK_KEY_DEV`.

---

### L6. Health Endpoint Leaks App Version

**File:** `lib/towerops_web/controllers/health_controller.ex:29`

**Severity:** LOW — Helps attackers identify vulnerable versions

**Description:** `Application.spec(:towerops, :vsn)` returned publicly.

**Fix:** Remove version from public health endpoint.

---

### L7. Device Capacity Operations Missing Org Scoping

**File:** `lib/towerops_web/live/device_live/show.ex:728-757`

**Severity:** LOW — `interface_id` from params not verified to belong to user's org

**Description:** `set_capacity` handler uses the `interface_id` from client params with no org membership verification.

**Fix:** Verify interface belongs to a device in the user's org.

---

### L8. Insights Dismiss Missing Org Scope

**File:** `lib/towerops_web/live/insights_live/index.ex:71-82`

**Severity:** LOW — IDOR on dismiss, but low impact (read-only dismissal)

**Description:** `Preseem.dismiss_insight(id)` with no org scope check.

**Fix:** Verify org ownership before dismissing.

---

### L9. Duplicate Pattern in Sync Workers

**Files:** `gaiia_sync_worker.ex`, `sonar_sync_worker.ex`, `splynx_sync_worker.ex`, `visp_sync_worker.ex`, `netbox_sync_worker.ex`

**Severity:** LOW — Code duplication invites inconsistency

**Description:** Five workers have identical `should_sync?/1` logic duplicated verbatim.

**Fix:** Extract to `Integrations` context module.

---

### L10. SidebarCollapse Event Listener Not Cleaned Up

**File:** `assets/js/app.ts:316-324`

**Severity:** LOW — Anonymous arrow function can't be removed; listener accumulates

**Description:** `mounted()` adds event listener but no `destroyed()` method to remove it.

**Fix:** Add `destroyed()` with stored handler reference.

---

### L11. WebMCP Navigate Path Not Validated

**File:** `assets/js/app.ts:478-480`

**Severity:** LOW — `window.location.href = path` could accept `javascript:` URLs

**Description:** If a compromised WebMCP agent sends a malicious path, user could be navigated to phishing URL.

**Fix:** Validate path starts with `/`.

---

### L12. `window.liveSocket` Exposed Globally

**File:** `assets/js/app.ts:423`

**Severity:** LOW — CSRF token accessible via `liveSocket.socket.params._csrf_token`

**Description:** Exposing LiveSocket globally makes CSRF token trivially accessible to any XSS.

**Fix:** Guard behind `NODE_ENV !== "production"`.

---

### L13. Overly Broad Changeset in Alert Updates

**File:** `lib/towerops/alerts.ex:391-405`

**Severity:** LOW — `Alert.changeset/2` casts 17 fields for a partial update

**Description:** Using the full changeset for simple field updates (`resolved_at`) is fragile. Extra keys could overwrite system fields.

**Fix:** Use a dedicated changeset or `Ecto.Changeset.change/2`.

---

### 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

**File:** `lib/towerops/weather/client.ex:13,65`

**Severity:** LOW — API key in URL may be logged by Req in debug/trace

**Fix:** Use header-based auth or ensure Req strips query params from logs.

---

### L16. COW/Vault ETS Table Name Collision Risk

**File:** `lib/towerops/vault.ex` (ETS table naming)

**Severity:** LOW — If two nodes share a node name, ETS table creation could conflict

**Fix:** Use unique ETS names incorporating node identifier.

---

## SUMMARY

### By Severity

| Severity | Count | Key Areas |
|----------|-------|-----------|
| CRITICAL | 13 | XSS (maps, status page), IDOR (consent, members, reports), auth bypass (GDPR halt, webhook), credential exposure (k8s/secrets.yaml, Stripe in git, MikroTik over PubSub), runtime crash (Repo.all_by), dead code (brute force), SSL (no force_ssl), race condition (device quota) |
| HIGH | 30 | N+1 queries (mobile, Oban, alerts, capacity, latency), race conditions (create_check, apply_agent), injection (Search wildcard, OID, user input → atom/integer), IDOR (reports, checks), auth gaps (webhooks, sessions), CSP/HSTS, hardcoded salts, cookie flags, missing validation (protobuf, check creation), worker reliability (sync errors swallowed, weather never starts, unbounded processing) |
| MEDIUM | 23 | IDOR (resource scoping), email case-sensitivity, open redirect, Stripe webhook not enforced, rate limiting key, overfetching, missing unique constraint, error disclosure, pattern match crashes, admin defense-in-depth, download content-type, queue blocking, PubSub unbounded, memory leaks (MutationObserver), MD5 default, incomplete archive validation, public ETS, impersonation expiry, silent permission failures |
| LOW | 16 | Log leaks, TTL extension, IP normalization, config documentation, vault dev/prod bleed, version disclosure, device capacity scoping, insights IDOR, code duplication, listener cleanup, path validation, liveSocket global, broad changesets, pagination, API key in URL, ETS naming |

**Total: 82 issues** (13 Critical, 30 High, 23 Medium, 16 Low)

### Top 10 Priority Fixes

1. **C1** — Stored XSS in Sites Map popup
2. **C2** — IDOR in policy consent handler (client-controlled user_id)
3. **C3** — Broken `halt()` in GDPR data export
4. **C6** — `Repo.all_by/2` runtime crash on password/email changes
5. **C9** — Remove `k8s/secrets.yaml` with live production credentials
6. **C10** — Remove hardcoded Stripe test key from source
7. **C8** — Sanitize `custom_css` in status page to prevent HTML injection
8. **C11** — Enable `force_ssl` in production
9. **H7** — Add auth plug to webhook pipeline
10. **H17** — Fix missing org scope on individual report operations