Graham McIntire
095c5d3236
Use Repo.get_by(schema, id: id, organization_id: organization_id) instead of Repo.get + pattern match so that resources from wrong orgs return :not_found instead of :forbidden, preventing org membership discovery.
199 lines
5.8 KiB
Markdown
199 lines
5.8 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
|
|
|
|
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
|
|
### 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.
|
|
|
|
---
|
|
|
|
|
|
|
|
|
|
### 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.
|