Graham McIntire
6fa0b791f2
- M3: Replace blacklist-based valid_return_path? with whitelist of known app route prefixes, plus URI decoding to prevent %2f encoding bypasses - M6: Replace full-org device/link load in get_node_detail with targeted join query filtering by discovered node fields - M7: Add partial unique index on (organization_id, email) for pending invitations (WHERE accepted_at IS NULL) - M12: Add on_mount superuser verification to all 7 admin LiveViews for defense-in-depth
326 lines
12 KiB
Markdown
326 lines
12 KiB
Markdown
# 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.
|
|
|
|
---
|
|
|
|
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
## 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)`.
|
|
|
|
---
|
|
|
|
|
|
### 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}"`.
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
### 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`.
|
|
|
|
---
|
|
|
|
## 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.
|
|
|
|
---
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
### 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
|