14 KiB
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.
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.
H21. Unbounded Sequential Processing in Sync Workers
Files:
gaiia_sync_worker.ex:20sonar_sync_worker.ex:16splynx_sync_worker.ex:16visp_sync_worker.ex:16netbox_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.
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).
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").
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.
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
- C1 — Stored XSS in Sites Map popup
- C2 — IDOR in policy consent handler (client-controlled user_id)
- C3 — Broken
halt()in GDPR data export - C6 —
Repo.all_by/2runtime crash on password/email changes - C9 — Remove
k8s/secrets.yamlwith live production credentials - C10 — Remove hardcoded Stripe test key from source
- C8 — Sanitize
custom_cssin status page to prevent HTML injection - C11 — Enable
force_sslin production - H7 — Add auth plug to webhook pipeline
- H17 — Fix missing org scope on individual report operations