Graham McIntire cec8eabcd8 fix: M13, M14, M15, M18 — download safety, queue blocking, PubSub chunking, protobuf validation

- M18: Add validate_snmp_device/1 with 255-byte limits on community,
  v3_auth_password, and v3_priv_password to prevent memory exhaustion
- M13: Add client-side mime_type allowlist to phx:download handler
- M14: Replace Process.sleep loop in WeatherSyncWorker with Task.async_stream
  to avoid blocking the Oban queue slot
- M15: Chunk device list in CloudLatencyProbeWorker PubSub broadcasts
  (100 devices per message) to prevent megabyte-sized messages

2026-05-12 12:54:48 -05:00

7.7 KiB

Raw Blame History

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}".

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.

7.7 KiB Raw Blame History