towerops/bugs.md

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

---

## HIGH

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

---

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

---

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

---

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

---

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

---

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