From ca7bb75472d7097779ea9f08bef8434a77d90d7d Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 10:20:52 -0500 Subject: [PATCH] fix: 11 critical security/correctness bugs from code audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - C1: Replace innerHTML template literals with DOM textContent in sites_map.ts - C2: Derive user_id from current_scope instead of client params in policy consent - C3: Fix broken halt() in GDPR data export (orphaned _ = ... halt()) - C4: Add owner/admin authorization check to MembersController update/delete - C5: Require HMAC signature on agent release webhook (was optional) - C6: Fix Repo.all_by/2 (not a real Ecto function) → Repo.all with query - C7: Move auth exemption to before_send callback in BruteForceProtection - C8: Block /`, `>'check_id' = ANY(?)", ^check_ids)` in a single query. + +--- + +### H3. N+1 Alert Resolution + +**File:** `lib/towerops/alerts.ex:381-388` + +**Severity:** HIGH — One DB write per alert when resolving + +**Description:** Loads all alerts then `Enum.each` calls `resolve_alert_silent()` per alert. For 1000 alerts in a storm: 1000 individual `Repo.update` calls + 1000 PubSub broadcasts. + +**Fix:** Use `Repo.update_all` with a single query. + +--- + +### H4. Race Condition in `create_check` (Auto-Discovery) + +**File:** `lib/towerops/monitoring.ex:152-184` + +**Severity:** HIGH — Concurrent discoveries crash the job with Ecto.ConstraintError + +**Description:** Two concurrent discoveries can both get `nil` from `get_by` and both attempt `insert`. The unique index prevents duplicates but one crashes with unhandled `Ecto.ConstraintError`. + +**Fix:** Use `Repo.insert(..., on_conflict: :nothing)` or wrap in transaction with `FOR UPDATE`. + +--- + +### H5. Missing Transaction in `apply_agent_to_all_equipment` + +**File:** `lib/towerops/sites.ex:223-252` + +**Severity:** HIGH — Partial data loss on failure + +**Description:** `delete_all` runs before `insert_all`. If insert fails, assignments are lost without rollback. + +**Fix:** Wrap in `Repo.transaction`. + +--- + +### H6. N+1 Interface Capacity Queries + +**File:** `lib/towerops/capacity.ex:53-83` + +**Severity:** HIGH — 20+ individual queries per site capacity view + +**Description:** `get_site_capacity_summary` loops over interfaces calling `get_utilization` which does `InterfaceStat |> where(interface_id: ^id) |> Repo.all()` per interface. + +**Fix:** Batch-load all interface stats in a single query. + +--- + +### 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. + +--- + +### H8. Missing Session Expiry Check in Mobile/GraphQL Auth + +**File:** `lib/towerops_web/plugs/mobile_auth.ex:48-53` +**File:** `lib/towerops_web/plugs/graphql_auth.ex:58-63` + +**Severity:** HIGH — Expired/revoked sessions remain usable indefinitely + +**Description:** `validate_session` checks the record exists but does NOT check `expires_at`. If the query doesn't filter expired rows, revoked sessions stay active. + +**Fix:** Add `DateTime.compare(expires_at, DateTime.utc_now())` check. + +--- + +### 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. + +--- + +### H10. Missing HSTS Header + +**File:** `lib/towerops_web/plugs/security_headers.ex:22-29` + +**Severity:** HIGH — SSL stripping attacks possible + +**Description:** Security headers plug sets CSP, X-Frame-Options, X-Content-Type-Options, Referrer-Policy, Permissions-Policy — but NOT `Strict-Transport-Security`. + +**Fix:** Add `strict-transport-security: max-age=63072000; includeSubDomains; preload`. + +--- + +### 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. + +--- + +### H14. `String.to_integer/1` on Unvalidated SNMP OID Components + +**Files:** +- `lib/towerops/snmp/neighbor_discovery.ex:202,299,129` +- `lib/towerops/snmp/discovery.ex:1368` +- `lib/towerops/snmp/profiles/base.ex:1332` + +**Severity:** HIGH — Malicious SNMP responses crash GenServers (DoS) + +**Description:** OID suffixes extracted from SNMP responses are passed directly to `String.to_integer/1`. An attacker-controlled device can send non-numeric OID fragments, crashing the poller process. + +**Fix:** Replace with `Integer.parse/1` + pattern match. Never trust OID suffixes from network data. + +--- + +### H15. `String.to_existing_atom/1` on User Input in ActivityFeed + +**File:** `lib/towerops/activity_feed_live.ex:55` + +**Severity:** HIGH — Any user can crash the activity feed LiveView (DoS) + +**Description:** `String.to_existing_atom(type_str)` where `type_str` comes from client event params. If the atom doesn't exist, `ArgumentError` kills the LiveView process. + +**Fix:** Use a whitelist of allowed types. + +--- + +### H16. `String.to_integer/1` on User-Controlled Page Param + +**Files:** +- `lib/towerops_web/live/user_settings_live.ex:63` +- `lib/towerops_web/live/admin/dashboard_live.ex:19` + +**Severity:** HIGH — Non-integer page param crashes LiveView (DoS) + +**Description:** `params |> Map.get("page", "1") |> String.to_integer()` — passing `?page=abc` raises an error killing the process. + +**Fix:** Use `Integer.parse` with fallback to 1. + +--- + +### H17. Reports LiveView — Missing Org Scope on Individual Operations + +**File:** `lib/towerops/reports_live.ex:73-101` + +**Severity:** HIGH — IDOR on report toggle, delete, run_now + +**Description:** `toggle`, `delete`, `run_now` fetch reports by ID without verifying org membership. A user who guesses another org's report ID can manipulate it. + +**Fix:** Add org scope verification to all individual operations. + +--- + +### H18. Mass Assignment Risk Across All CRUD Endpoints + +**Files:** Multiple v1 controllers + +**Severity:** HIGH — Unchecked params passed to context functions + +**Description:** All create/update actions pass raw user params to context functions. Only protection is changeset `cast/3`. If any changeset doesn't restrict fields properly, arbitrary columns can be set. + +**Fix:** Add explicit field allowlist filtering at controller level (like `IntegrationsController.to_atom_keys/1`). + +--- + +### 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. + +--- + +### H23. Sync Workers Silently Swallow Errors + +**Files:** All sync workers (uisp, preseem, cn_maestro, sonar, splynx, visp, netbox, gaiia) + +**Severity:** HIGH — Transient failures NEVER retried; data silently stale + +**Description:** All sync workers log errors but return `:ok`. Network timeouts, API rate limits, DNS failures are all silently accepted. Data goes stale until next cron cycle. + +**Fix:** Distinguish permanent (return `:ok`/`:discard`) from transient (return `{:error, reason}` for Oban retry). + +--- + +### 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`. + +--- + +### H26. N+1 Latency Queries per Site Device + +**File:** `lib/towerops_web/live/site_live/show.ex:101-128` + +**Severity:** HIGH — 50+ queries for large sites + +**Description:** Loops through devices calling `Monitoring.get_latency_data` per device. For 50+ devices at a site, 50+ individual queries. + +**Fix:** Batch the latency query or add pagination/limit. + +--- + +### H27. Backslash-Wildcard Injection in Search + +**File:** `lib/towerops/search.ex:90-94` + +**Severity:** HIGH — Search bypass via backslash injection leaks wildcard matching + +**Description:** `sanitize/1` escapes `%` and `_` but NOT `\` first. A search for `\%%` becomes `\\%\\%` — PG interprets `\\` as literal `\` followed by `%` wildcard, leaking wildcard matching. + +**Fix:** Escape `\` first, or reuse `Towerops.QueryHelpers.sanitize_like/1`. + +--- + +### H28. Nominatim API Response Injected via innerHTML + +**File:** `assets/js/hooks/coverage_hooks.ts:635-645` + +**Severity:** HIGH — Third-party API response flows into innerHTML + +**Description:** Nominatim search API `display_name` is passed directly to `marker.bindPopup()` (innerHTML). If Nominatim is compromised or DNS poisoned, arbitrary JS executes. + +**Fix:** Sanitize `display_name` with `escapeHtml()` before `bindPopup()`. + +--- + +### H29. OID Injection via Substring Match in Profiles + +**File:** `lib/towerops/profiles/yaml_profiles.ex:788` + +**Severity:** HIGH — Incorrect profile matching due to partial OID match + +**Description:** `String.contains?(sys_object_id, block.oid)` matches partial octets. OID `1.3.6.1.4.1.9` would incorrectly match `1.3.6.1.4.1.99` because "9" is a substring of "99". + +**Fix:** Use `String.starts_with?(sys_object_id <> ".", block.oid <> ".")` for dot-boundary matching. + +--- + +### H30. JobCleanupTask Cancels Executing Jobs + +**File:** `lib/towerops/workers/job_cleanup_task.ex:47-63` + +**Severity:** HIGH — Rolling deployments kill currently executing poll jobs + +**Description:** `cancel_all_jobs` filters on `["available", "scheduled", "executing", "retryable"]` states. In-progress polling/monitoring is discarded. + +**Fix:** Exclude `"executing"` state so active jobs complete naturally. + +--- + +## 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 diff --git a/config/dev.exs b/config/dev.exs index f5188f0b..817a84a1 100644 --- a/config/dev.exs +++ b/config/dev.exs @@ -239,8 +239,7 @@ config :towerops, dev_routes: true # Stripe configuration for development config :towerops, - stripe_secret_key: - "sk_test_51T7zcQS77kvnTfgyu0DCQU2xVKzeaneVCueHaXV3jsIw6HAwGWWllEL3J8jGZOybHAtPyu6oYIiuvJHhReFtTycE00VrJYJ8Ao", + stripe_secret_key: System.get_env("STRIPE_SECRET_KEY", "sk_test_use_your_own_key"), stripe_webhook_secret: "whsec_dev_fake_secret", stripe_price_id: "price_1T81XBS77kvnTfgyPlw1jF8N", stripe_meter_id: "mtr_test_61UHPU067veEZ7bhl41S77kvnTfgyODY" diff --git a/k8s/secrets.example.yaml b/k8s/secrets.example.yaml index 60cbe8bd..f067281e 100644 --- a/k8s/secrets.example.yaml +++ b/k8s/secrets.example.yaml @@ -1,5 +1,5 @@ # Unified template for every Kubernetes Secret the towerops Deployment -# references. Bootstrap your local copy: +# references. Copy to k8s/secrets.yaml and fill in real values: # # cp k8s/secrets.example.yaml k8s/secrets.yaml # # edit k8s/secrets.yaml — fill in real values @@ -26,12 +26,12 @@ metadata: type: Opaque stringData: # Generate with: openssl rand -base64 32 - RELEASE_COOKIE: "" + RELEASE_COOKIE: "CHANGE_ME_openssl_rand_-base64_32" # Generate with: mix phx.gen.secret (or openssl rand -base64 64) - SECRET_KEY_BASE: "" + SECRET_KEY_BASE: "CHANGE_ME_mix_phx_gen_secret" # Generate with: openssl rand -base64 32 # CRITICAL: losing this makes encrypted columns unrecoverable. - CLOAK_KEY: "" + CLOAK_KEY: "CHANGE_ME_openssl_rand_-base64_32" --- apiVersion: v1 kind: Secret @@ -41,14 +41,12 @@ metadata: type: Opaque stringData: # Full ecto:// URL — what runtime.exs reads. - DATABASE_URL: "ecto://USER:PASSWORD@HOST:5432/DATABASE" - # The individual fields are convenience for ops — not required by the - # app but kept here for parity with the existing prod secret. - POSTGRES_HOST: "" + DATABASE_URL: "postgresql://towerops:CHANGE_ME@postgres-host:5432/towerops" + POSTGRES_HOST: "CHANGE_ME_postgres_host" POSTGRES_PORT: "5432" - POSTGRES_DB: "" - POSTGRES_USER: "" - POSTGRES_PASSWORD: "" + POSTGRES_DB: "towerops" + POSTGRES_USER: "towerops" + POSTGRES_PASSWORD: "CHANGE_ME" --- apiVersion: v1 kind: Secret @@ -57,9 +55,9 @@ metadata: namespace: towerops type: Opaque stringData: - AWS_ACCESS_KEY_ID: "" - AWS_SECRET_ACCESS_KEY: "" - AWS_REGION: "" + AWS_ACCESS_KEY_ID: "CHANGE_ME" + AWS_SECRET_ACCESS_KEY: "CHANGE_ME" + AWS_REGION: "us-east-1" --- apiVersion: v1 kind: Secret @@ -68,11 +66,9 @@ metadata: namespace: towerops type: Opaque stringData: - # K8s pattern — runtime.exs builds the connection from these - # individual fields. Dokku alternative is REDIS_URL. - REDIS_HOST: "" + REDIS_HOST: "CHANGE_ME_redis_host" REDIS_PORT: "6379" - REDIS_PASSWORD: "" + REDIS_PASSWORD: "CHANGE_ME" --- apiVersion: v1 kind: Secret @@ -81,11 +77,10 @@ metadata: namespace: towerops type: Opaque stringData: - # Stripe — every key is optional; the deployment marks them optional: true. - STRIPE_SECRET_KEY: "" - STRIPE_WEBHOOK_SECRET: "" - STRIPE_PRICE_ID: "" - STRIPE_METER_ID: "" + STRIPE_SECRET_KEY: "CHANGE_ME_sk_live_..." + STRIPE_WEBHOOK_SECRET: "CHANGE_ME_whsec_..." + STRIPE_PRICE_ID: "CHANGE_ME_price_..." + STRIPE_METER_ID: "CHANGE_ME_mtr_..." --- apiVersion: v1 kind: Secret @@ -94,8 +89,5 @@ metadata: namespace: towerops type: Opaque stringData: - # DeepSeek API key — get one at https://platform.deepseek.com/ - # Without this, insight LLM enrichment stays no-op (insights still render). - DEEPSEEK_API_KEY: "" - # Optional: override the default model (default: deepseek-v4-pro). + DEEPSEEK_API_KEY: "CHANGE_ME_sk_..." DEEPSEEK_MODEL: "" diff --git a/lib/towerops/accounts.ex b/lib/towerops/accounts.ex index fa2fe826..45405f6e 100644 --- a/lib/towerops/accounts.ex +++ b/lib/towerops/accounts.ex @@ -383,7 +383,7 @@ defmodule Towerops.Accounts do fn -> case Repo.update(changeset) do {:ok, user} -> - tokens_to_expire = Repo.all_by(UserToken, user_id: user.id) + tokens_to_expire = Repo.all(from t in UserToken, where: t.user_id == ^user.id) Repo.delete_all(from(t in UserToken, where: t.id in ^Enum.map(tokens_to_expire, & &1.id))) {:ok, {user, tokens_to_expire}} diff --git a/lib/towerops/status_pages/status_page_config.ex b/lib/towerops/status_pages/status_page_config.ex index 961733ab..fd952f28 100644 --- a/lib/towerops/status_pages/status_page_config.ex +++ b/lib/towerops/status_pages/status_page_config.ex @@ -53,10 +53,23 @@ defmodule Towerops.StatusPages.StatusPageConfig do changeset css when is_binary(css) -> - if String.contains?(String.downcase(css), "url(") do - add_error(changeset, :custom_css, "url() references are not allowed in custom CSS") - else - changeset + downcased = String.downcase(css) + + cond do + String.contains?(downcased, "url(") -> + add_error(changeset, :custom_css, "url() references are not allowed in custom CSS") + + String.contains?(downcased, " + add_error(changeset, :custom_css, "HTML tags are not allowed in custom CSS") + + String.contains?(downcased, " + add_error(changeset, :custom_css, "HTML tags are not allowed in custom CSS") + + String.contains?(downcased, " + add_error(changeset, :custom_css, "HTML tags are not allowed in custom CSS") + + true -> + changeset end _ -> diff --git a/lib/towerops/workers/mikrotik_backup_worker.ex b/lib/towerops/workers/mikrotik_backup_worker.ex index 8f0c8aaa..b516e5a5 100644 --- a/lib/towerops/workers/mikrotik_backup_worker.ex +++ b/lib/towerops/workers/mikrotik_backup_worker.ex @@ -14,9 +14,6 @@ defmodule Towerops.Workers.MikrotikBackupWorker do use Oban.Worker, queue: :maintenance, max_attempts: 3 - alias Towerops.Agent.AgentJob - alias Towerops.Agent.MikrotikCommand - alias Towerops.Agent.MikrotikDevice alias Towerops.Devices alias Towerops.Devices.BackupRequests @@ -65,68 +62,16 @@ defmodule Towerops.Workers.MikrotikBackupWorker do # Build backup job job_id = "backup:#{device.id}:#{DateTime.to_unix(DateTime.utc_now())}" - mikrotik_config = Devices.get_mikrotik_config(device) - - # Generate unique filename for this backup (without .rsc extension, RouterOS adds it) - backup_filename = "towerops-backup-#{DateTime.to_unix(DateTime.utc_now())}" - - # Build chunked read commands - # Max chunk size is 32KB (32768 bytes), we'll read 10 chunks to cover 320KB - # Most MikroTik configs are under this size - chunk_size = 32_768 - num_chunks = 10 - - read_commands = - for i <- 0..(num_chunks - 1) do - %MikrotikCommand{ - command: "/file/read", - args: %{ - "file" => "#{backup_filename}.rsc", - "offset" => to_string(i * chunk_size), - "chunk-size" => to_string(chunk_size) - } - } - end - - job = %AgentJob{ - job_id: job_id, - job_type: :MIKROTIK, - device_id: device.id, - mikrotik_device: %MikrotikDevice{ - ip: device.ip_address, - username: mikrotik_config.username, - password: mikrotik_config.password, - port: mikrotik_config.port, - ssh_port: mikrotik_config.ssh_port, - use_ssl: mikrotik_config.use_ssl - }, - mikrotik_commands: - [ - # Step 1: Export config to file on router (compact format, no defaults) - %MikrotikCommand{ - command: "/export", - args: %{"file" => backup_filename, "compact" => ""} - } - ] ++ - read_commands ++ - [ - # Final step: Delete the temporary file - %MikrotikCommand{ - command: "/file/remove", - args: %{"numbers" => "#{backup_filename}.rsc"} - } - ] - } - # Create tracking request case BackupRequests.create_request(device.id, job_id) do {:ok, _request} -> - # Broadcast to agent via PubSub + # Broadcast device id to agent channel via PubSub — the channel + # resolves credentials itself to avoid exposing them on PubSub. _ = Phoenix.PubSub.broadcast( Towerops.PubSub, "agent:#{agent_token_id}:backup", - {:backup_requested, job} + {:backup_requested, device.id, job_id} ) Logger.info("Backup job sent to agent", diff --git a/lib/towerops_web/channels/agent_channel.ex b/lib/towerops_web/channels/agent_channel.ex index c72c9dad..ef74f609 100644 --- a/lib/towerops_web/channels/agent_channel.ex +++ b/lib/towerops_web/channels/agent_channel.ex @@ -316,17 +316,27 @@ defmodule ToweropsWeb.AgentChannel do end # Handle PubSub broadcast when backup is requested for a device - def handle_info({:backup_requested, job}, socket) do - Logger.info("Backup requested for device, sending backup job to agent", + def handle_info({:backup_requested, device_id, job_id}, socket) do + Logger.info("Backup requested for device, assembling and sending backup job to agent", agent_token_id: socket.assigns.agent_token_id, - device_id: job.device_id, - job_id: job.job_id + device_id: device_id, + job_id: job_id ) - job_list = %AgentJobList{jobs: [job]} - binary = AgentJobList.encode(job_list) + case Devices.get_device_with_details(device_id) do + nil -> + Logger.warning("Backup requested but device not found", + device_id: device_id, + job_id: job_id + ) + + device -> + job = build_backup_job(device, job_id) + job_list = %AgentJobList{jobs: [job]} + binary = AgentJobList.encode(job_list) + push(socket, "backup_job", %{binary: Base.encode64(binary)}) + end - push(socket, "backup_job", %{binary: Base.encode64(binary)}) {:noreply, socket} end @@ -452,6 +462,53 @@ defmodule ToweropsWeb.AgentChannel do {:noreply, socket} end + defp build_backup_job(device, job_id) do + mikrotik_config = Devices.get_mikrotik_config(device) + backup_filename = "towerops-backup-#{DateTime.to_unix(DateTime.utc_now())}" + chunk_size = 32_768 + num_chunks = 10 + + read_commands = + for i <- 0..(num_chunks - 1) do + %MikrotikCommand{ + command: "/file/read", + args: %{ + "file" => "#{backup_filename}.rsc", + "offset" => to_string(i * chunk_size), + "chunk-size" => to_string(chunk_size) + } + } + end + + %AgentJob{ + job_id: job_id, + job_type: :MIKROTIK, + device_id: device.id, + mikrotik_device: %MikrotikDevice{ + ip: device.ip_address || "", + username: mikrotik_config.username || "", + password: mikrotik_config.password || "", + port: mikrotik_config.port || 8729, + ssh_port: mikrotik_config.ssh_port || 22, + use_ssl: mikrotik_config.use_ssl || false + }, + mikrotik_commands: + [ + %MikrotikCommand{ + command: "/export", + args: %{"file" => backup_filename, "compact" => "true"} + } + ] ++ + read_commands ++ + [ + %MikrotikCommand{ + command: "/file/remove", + args: %{"numbers" => "#{backup_filename}.rsc"} + } + ] + } + end + @impl true @spec handle_in(String.t(), map(), socket()) :: {:noreply, socket()} def handle_in("result", %{"binary" => binary_b64}, socket) when is_binary(binary_b64) do diff --git a/lib/towerops_web/controllers/api/account_data_controller.ex b/lib/towerops_web/controllers/api/account_data_controller.ex index d08bffe6..d94661f4 100644 --- a/lib/towerops_web/controllers/api/account_data_controller.ex +++ b/lib/towerops_web/controllers/api/account_data_controller.ex @@ -17,38 +17,37 @@ defmodule ToweropsWeb.Api.AccountDataController do user = conn.assigns.current_scope.user # Verify user account is confirmed before allowing data export - _ = - if !user.confirmed_at do - conn - |> put_status(:forbidden) - |> json(%{error: "Email address must be confirmed before exporting account data"}) - |> halt() - end + if user.confirmed_at do + # Log the data export for audit trail + AuditLogger.log_user_data_exported(conn, user.id) - # Log the data export for audit trail - AuditLogger.log_user_data_exported(conn, user.id) - - # Gather all user data - data = %{ - profile: build_profile_data(user), - organizations: build_organizations_data(user), - devices: build_devices_data(user), - alerts: build_alerts_data(user), - audit_logs: build_audit_logs_data(user), - export_info: %{ - exported_at: DateTime.utc_now(), - format: "JSON", - gdpr_article: "Article 15 - Right to Access" + # Gather all user data + data = %{ + profile: build_profile_data(user), + organizations: build_organizations_data(user), + devices: build_devices_data(user), + alerts: build_alerts_data(user), + audit_logs: build_audit_logs_data(user), + export_info: %{ + exported_at: DateTime.utc_now(), + format: "JSON", + gdpr_article: "Article 15 - Right to Access" + } } - } - conn - |> put_resp_content_type("application/json") - |> put_resp_header( - "content-disposition", - "attachment; filename=\"towerops-data-#{user.id}-#{DateTime.to_unix(DateTime.utc_now())}.json\"" - ) - |> json(data) + conn + |> put_resp_content_type("application/json") + |> put_resp_header( + "content-disposition", + "attachment; filename=\"towerops-data-#{user.id}-#{DateTime.to_unix(DateTime.utc_now())}.json\"" + ) + |> json(data) + else + conn + |> put_status(:forbidden) + |> json(%{error: "Email address must be confirmed before exporting account data"}) + |> halt() + end end # Build profile data diff --git a/lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex b/lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex index 8120b595..baca54d8 100644 --- a/lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex +++ b/lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex @@ -14,7 +14,7 @@ defmodule ToweropsWeb.Api.V1.AgentReleaseWebhookController do @max_age_seconds 300 def create(conn, _params) do - case verify_optional_signature(conn) do + case verify_signature(conn) do :ok -> case Oban.insert(AgentReleaseWebhookWorker.new(%{})) do {:ok, _job} -> @@ -34,17 +34,17 @@ defmodule ToweropsWeb.Api.V1.AgentReleaseWebhookController do end end - defp verify_optional_signature(conn) do + defp verify_signature(conn) do sig_headers = Plug.Conn.get_req_header(conn, "x-agent-webhook-signature") case sig_headers do - [] -> - :ok - [header] -> secret = Application.get_env(:towerops, :agent_webhook_secret) raw_body = conn.private[:raw_body] || "" verify_hmac_signature(header, raw_body, secret) + + [] -> + {:error, :missing_signature} end end diff --git a/lib/towerops_web/controllers/api/v1/members_controller.ex b/lib/towerops_web/controllers/api/v1/members_controller.ex index efadf32a..c2994924 100644 --- a/lib/towerops_web/controllers/api/v1/members_controller.ex +++ b/lib/towerops_web/controllers/api/v1/members_controller.ex @@ -20,19 +20,25 @@ defmodule ToweropsWeb.Api.V1.MembersController do def update(conn, %{"id" => user_id, "role" => role}) do organization_id = conn.assigns.current_organization_id - case Organizations.update_member_role(organization_id, user_id, role) do - {:ok, membership} -> - membership = Towerops.Repo.preload(membership, :user) - json(conn, %{data: format_member(membership)}) + case authorize_member_management(conn, organization_id) do + {:ok, conn} -> + case Organizations.update_member_role(organization_id, user_id, role) do + {:ok, membership} -> + membership = Towerops.Repo.preload(membership, :user) + json(conn, %{data: format_member(membership)}) - {:error, :cannot_change_owner_role} -> - conn |> put_status(:forbidden) |> json(%{error: "Cannot change owner role"}) + {:error, :cannot_change_owner_role} -> + conn |> put_status(:forbidden) |> json(%{error: "Cannot change owner role"}) - {:error, :not_found} -> - conn |> put_status(:not_found) |> json(%{error: "Member not found"}) + {:error, :not_found} -> + conn |> put_status(:not_found) |> json(%{error: "Member not found"}) - {:error, changeset} -> - conn |> put_status(:unprocessable_entity) |> json(%{errors: translate_errors(changeset)}) + {:error, changeset} -> + conn |> put_status(:unprocessable_entity) |> json(%{errors: translate_errors(changeset)}) + end + + {:halt, conn} -> + conn end end @@ -43,10 +49,44 @@ defmodule ToweropsWeb.Api.V1.MembersController do def delete(conn, %{"id" => user_id}) do organization_id = conn.assigns.current_organization_id - case Organizations.remove_member(organization_id, user_id) do - {:ok, _} -> conn |> put_status(:no_content) |> send_resp(204, "") - {:error, :cannot_remove_owner} -> conn |> put_status(:forbidden) |> json(%{error: "Cannot remove owner"}) - {:error, :not_found} -> conn |> put_status(:not_found) |> json(%{error: "Member not found"}) + case authorize_member_management(conn, organization_id) do + {:ok, conn} -> + case Organizations.remove_member(organization_id, user_id) do + {:ok, _} -> conn |> put_status(:no_content) |> send_resp(204, "") + {:error, :cannot_remove_owner} -> conn |> put_status(:forbidden) |> json(%{error: "Cannot remove owner"}) + {:error, :not_found} -> conn |> put_status(:not_found) |> json(%{error: "Member not found"}) + end + + {:halt, conn} -> + conn + end + end + + defp authorize_member_management(conn, organization_id) do + user = conn.assigns[:current_user] + + if user do + membership = Organizations.get_membership(organization_id, user.id) + + if membership && membership.role in [:owner, :admin] do + {:ok, conn} + else + conn = + conn + |> put_status(:forbidden) + |> json(%{error: "Only owners and admins can manage members"}) + |> halt() + + {:halt, conn} + end + else + conn = + conn + |> put_status(:unauthorized) + |> json(%{error: "Authentication required"}) + |> halt() + + {:halt, conn} end end diff --git a/lib/towerops_web/live/device_live/show.ex b/lib/towerops_web/live/device_live/show.ex index ea27363b..854359fc 100644 --- a/lib/towerops_web/live/device_live/show.ex +++ b/lib/towerops_web/live/device_live/show.ex @@ -932,71 +932,19 @@ defmodule ToweropsWeb.DeviceLive.Show do end defp trigger_manual_backup(socket, device, agent_token_id) do - alias Towerops.Agent.AgentJob - alias Towerops.Agent.MikrotikCommand - alias Towerops.Agent.MikrotikDevice alias Towerops.Devices.BackupRequests job_id = "backup:#{device.id}:#{DateTime.to_unix(DateTime.utc_now())}" - mikrotik_config = Devices.get_mikrotik_config(device) - - # Generate unique filename for this backup (without .rsc extension, RouterOS adds it) - backup_filename = "towerops-backup-#{DateTime.to_unix(DateTime.utc_now())}" - - # Build chunked read commands - # Max chunk size is 32KB (32768 bytes), we'll read 10 chunks to cover 320KB - chunk_size = 32_768 - num_chunks = 10 - - read_commands = - for i <- 0..(num_chunks - 1) do - %MikrotikCommand{ - command: "/file/read", - args: %{ - "file" => "#{backup_filename}.rsc", - "offset" => to_string(i * chunk_size), - "chunk-size" => to_string(chunk_size) - } - } - end - - job = %AgentJob{ - job_id: job_id, - job_type: :MIKROTIK, - device_id: device.id, - mikrotik_device: %MikrotikDevice{ - ip: device.ip_address, - username: mikrotik_config.username, - password: mikrotik_config.password, - port: mikrotik_config.port, - ssh_port: mikrotik_config.ssh_port, - use_ssl: mikrotik_config.use_ssl - }, - mikrotik_commands: - [ - # Step 1: Export config to file on router (compact format, no defaults) - %MikrotikCommand{ - command: "/export", - args: %{"file" => backup_filename, "compact" => ""} - } - ] ++ - read_commands ++ - [ - # Final step: Delete the temporary file - %MikrotikCommand{ - command: "/file/remove", - args: %{"numbers" => "#{backup_filename}.rsc"} - } - ] - } case BackupRequests.create_request(device.id, job_id, "manual") do {:ok, _request} -> + # Send device id + job id over PubSub — agent channel resolves + # credentials itself to avoid exposing them on PubSub. _ = Phoenix.PubSub.broadcast( Towerops.PubSub, "agent:#{agent_token_id}:backup", - {:backup_requested, job} + {:backup_requested, device.id, job_id} ) {:noreply, diff --git a/lib/towerops_web/plugs/brute_force_protection.ex b/lib/towerops_web/plugs/brute_force_protection.ex index 932187b0..8a8389ff 100644 --- a/lib/towerops_web/plugs/brute_force_protection.ex +++ b/lib/towerops_web/plugs/brute_force_protection.ex @@ -15,7 +15,7 @@ defmodule ToweropsWeb.Plugs.BruteForceProtection do ## Exemptions - - Authenticated users (conn.assigns[:current_user] present) + - Authenticated users (checked in before_send callback, since auth runs after this plug) - Agent WebSocket connections (/socket/agent - authenticated at channel join) - Whitelisted IPs (checked via BruteForce context) @@ -43,10 +43,6 @@ defmodule ToweropsWeb.Plugs.BruteForceProtection do ip_address = RemoteIp.from_conn(conn) cond do - # Skip for authenticated users - conn.assigns[:current_user] != nil -> - conn - # Skip for agent WebSocket connections (authenticated at channel join) String.starts_with?(conn.request_path, "/socket/agent") -> conn @@ -69,7 +65,12 @@ defmodule ToweropsWeb.Plugs.BruteForceProtection do defp register_404_tracker(conn, ip_address) do register_before_send(conn, fn response_conn -> - track_404_if_needed(response_conn, ip_address, conn.request_path) + # Skip tracking for authenticated users (auth runs after this plug, + # but before_send runs after the full pipeline including auth). + if !response_conn.assigns[:current_user] do + track_404_if_needed(response_conn, ip_address, conn.request_path) + end + response_conn end) end diff --git a/lib/towerops_web/user_auth.ex b/lib/towerops_web/user_auth.ex index c79abda0..db606f53 100644 --- a/lib/towerops_web/user_auth.ex +++ b/lib/towerops_web/user_auth.ex @@ -690,9 +690,10 @@ defmodule ToweropsWeb.UserAuth do # Attach LiveView event handler for accepting updated policies socket = LiveView.attach_hook(socket, :policy_consent_handler, :handle_event, fn - "accept_updated_policies", %{"user-id" => user_id}, socket -> + "accept_updated_policies", _params, socket -> # Grant consent for all policies that need re-consent policies_needing_consent = Map.get(socket.assigns, :policies_needing_consent, []) + user_id = socket.assigns.current_scope.user.id Enum.each(policies_needing_consent, fn policy_type -> Accounts.grant_consent(user_id, policy_type) diff --git a/setup_stripe_meter.exs b/setup_stripe_meter.exs index 5a753903..0d147750 100644 --- a/setup_stripe_meter.exs +++ b/setup_stripe_meter.exs @@ -2,7 +2,7 @@ # Setup script for Stripe billing meter -stripe_key = "sk_test_51T7zcQS77kvnTfgyu0DCQU2xVKzeaneVCueHaXV3jsIw6HAwGWWllEL3J8jGZOybHAtPyu6oYIiuvJHhReFtTycE00VrJYJ8Ao" +stripe_key = System.get_env("STRIPE_SECRET_KEY", "sk_test_use_your_own_key") product_id = "prod_U6DpFdl21ftiDz" IO.puts("Creating Stripe billing meter...") diff --git a/test/towerops/workers/mikrotik_backup_worker_test.exs b/test/towerops/workers/mikrotik_backup_worker_test.exs index 8c1c3e60..f339c8b1 100644 --- a/test/towerops/workers/mikrotik_backup_worker_test.exs +++ b/test/towerops/workers/mikrotik_backup_worker_test.exs @@ -93,22 +93,13 @@ defmodule Towerops.Workers.MikrotikBackupWorkerTest do assert :ok = perform_job(MikrotikBackupWorker, %{}) end) - assert_receive {:backup_requested, job}, 1_000 - assert job.device_id == device.id - assert job.mikrotik_device.username == "admin" - assert job.mikrotik_device.port == 8729 - assert job.mikrotik_device.ssh_port == 22 - assert job.mikrotik_device.use_ssl == true - assert job.job_type == :MIKROTIK - # 1 export + 10 read chunks + 1 remove = 12 commands - assert length(job.mikrotik_commands) == 12 - [first | _] = job.mikrotik_commands - assert first.command == "/export" - last = List.last(job.mikrotik_commands) - assert last.command == "/file/remove" + assert_receive {:backup_requested, received_device_id, job_id}, 1_000 + assert received_device_id == device.id + assert is_binary(job_id) + assert String.starts_with?(job_id, "backup:#{device.id}:") # Backup request was created in DB - assert BackupRequests.get_request_by_job_id(job.job_id) + assert BackupRequests.get_request_by_job_id(job_id) end test "returns {:error, _} when at least one backup request fails to create" do diff --git a/test/towerops_web/channels/agent_channel_processing_test.exs b/test/towerops_web/channels/agent_channel_processing_test.exs index 69896445..fce6164d 100644 --- a/test/towerops_web/channels/agent_channel_processing_test.exs +++ b/test/towerops_web/channels/agent_channel_processing_test.exs @@ -10,7 +10,6 @@ defmodule ToweropsWeb.AgentChannelProcessingTest do alias Towerops.AccountsFixtures alias Towerops.Agent.AgentHeartbeat - alias Towerops.Agent.AgentJob alias Towerops.Agent.AgentJobList alias Towerops.Agent.MikrotikResult alias Towerops.Agent.MikrotikSentence @@ -288,23 +287,19 @@ defmodule ToweropsWeb.AgentChannelProcessingTest do agent_token: agent_token, device: device } do - job = %AgentJob{ - job_id: "backup:#{device.id}", - job_type: :MIKROTIK, - device_id: device.id - } + job_id = "backup:#{device.id}" Phoenix.PubSub.broadcast( Towerops.PubSub, "agent:#{agent_token.id}:backup", - {:backup_requested, job} + {:backup_requested, device.id, job_id} ) assert_push "backup_job", %{binary: jobs_binary}, 200 {:ok, decoded} = Base.decode64(jobs_binary) {:ok, job_list} = AgentJobList.decode(decoded) - assert hd(job_list.jobs).job_id == "backup:#{device.id}" + assert hd(job_list.jobs).job_id == job_id end end diff --git a/test/towerops_web/channels/agent_channel_test.exs b/test/towerops_web/channels/agent_channel_test.exs index 6f1e1aa6..ca63eb4f 100644 --- a/test/towerops_web/channels/agent_channel_test.exs +++ b/test/towerops_web/channels/agent_channel_test.exs @@ -6,7 +6,6 @@ defmodule ToweropsWeb.AgentChannelTest do alias Towerops.AccountsFixtures alias Towerops.Agent.AgentError alias Towerops.Agent.AgentHeartbeat - alias Towerops.Agent.AgentJob alias Towerops.Agent.AgentJobList alias Towerops.Agent.CheckResult, as: CheckResultProto alias Towerops.Agent.CredentialTestResult @@ -771,17 +770,11 @@ defmodule ToweropsWeb.AgentChannelTest do end end - describe "handle_info {:backup_requested, job}" do + describe "handle_info {:backup_requested, device_id, job_id}" do test "pushes backup job to agent", %{socket: socket, device: device} do - job = %AgentJob{ - job_id: "backup:#{device.id}", - job_type: :MIKROTIK, - device_id: device.id, - mikrotik_device: nil, - mikrotik_commands: [] - } + job_id = "backup:#{device.id}" - send(socket.channel_pid, {:backup_requested, job}) + send(socket.channel_pid, {:backup_requested, device.id, job_id}) assert_push "backup_job", %{binary: jobs_binary} @@ -789,7 +782,7 @@ defmodule ToweropsWeb.AgentChannelTest do {:ok, job_list} = AgentJobList.decode(decoded_binary) backup_job = hd(job_list.jobs) - assert backup_job.job_id == "backup:#{device.id}" + assert backup_job.job_id == job_id assert backup_job.device_id == device.id end end @@ -2170,17 +2163,12 @@ defmodule ToweropsWeb.AgentChannelTest do agent_token: agent_token, device: device } do - # Channel should be subscribed to "agent:#{agent_token.id}:backup" - job = %AgentJob{ - job_id: "backup:#{device.id}", - job_type: :MIKROTIK, - device_id: device.id - } + job_id = "backup:#{device.id}" Phoenix.PubSub.broadcast( Towerops.PubSub, "agent:#{agent_token.id}:backup", - {:backup_requested, job} + {:backup_requested, device.id, job_id} ) assert_push "backup_job", %{binary: jobs_binary}, 200 diff --git a/test/towerops_web/controllers/api/v1/agent_release_webhook_controller_test.exs b/test/towerops_web/controllers/api/v1/agent_release_webhook_controller_test.exs index 5951799f..f3a06888 100644 --- a/test/towerops_web/controllers/api/v1/agent_release_webhook_controller_test.exs +++ b/test/towerops_web/controllers/api/v1/agent_release_webhook_controller_test.exs @@ -36,11 +36,29 @@ defmodule ToweropsWeb.Api.V1.AgentReleaseWebhookControllerTest do describe "create/2 response" do test "enqueues job and returns 200", %{conn: conn} do - conn = post(conn, ~p"/api/v1/webhooks/agent-release") + ts = :second |> System.system_time() |> to_string() + secret = Application.get_env(:towerops, :agent_webhook_secret) + raw_body = "" + + sig = + :hmac + |> :crypto.mac(:sha256, secret, ts <> "." <> raw_body) + |> Base.encode16(case: :lower) + + conn = + conn + |> put_req_header("x-agent-webhook-signature", "t=#{ts},v1=#{sig}") + |> post(~p"/api/v1/webhooks/agent-release") assert json_response(conn, 200) == %{"status" => "ok"} assert_enqueued(worker: AgentReleaseWebhookWorker) end + + test "returns 401 when signature header is missing", %{conn: conn} do + conn = post(conn, ~p"/api/v1/webhooks/agent-release") + + assert json_response(conn, 401)["error"] == "Signature verification failed" + end end describe "check_timestamp/1" do diff --git a/test/towerops_web/plugs/brute_force_protection_test.exs b/test/towerops_web/plugs/brute_force_protection_test.exs index 4a3c5232..a0b9b9b8 100644 --- a/test/towerops_web/plugs/brute_force_protection_test.exs +++ b/test/towerops_web/plugs/brute_force_protection_test.exs @@ -132,9 +132,10 @@ defmodule ToweropsWeb.Plugs.BruteForceProtectionTest do |> BruteForceProtection.call([]) refute conn.halted - # No before_send callback registered for authenticated users — they - # bypass the 404 tracker entirely. - assert conn.private[:before_send] in [nil, []] + # before_send callback is registered but will skip tracking for + # authenticated users (auth check moved into the callback since + # this plug runs before authentication). + assert is_list(conn.private[:before_send]) end test "passes through immediately for agent socket paths" do