diff --git a/bugs.md b/bugs.md index 9322ac1c..89a9f904 100644 --- a/bugs.md +++ b/bugs.md @@ -4,194 +4,6 @@ --- -## CRITICAL - -### C1. ✓ FIXED — Stored XSS via Sites Map Popup (innerHTML) - -**Files:** -- `assets/js/hooks/sites_map.ts:75-101` — popup construction with template literals -- `map_live/index.html.heex:51` — `data-sites={Jason.encode!(@sites)}` - -**Severity:** CRITICAL — Stored XSS affecting all users viewing the map - -**Description:** Site name, description, and address from the database are interpolated into template literals and injected via `marker.bindPopup()` (which uses `innerHTML`). HEEx escapes the JSON attribute in the template, but `JSON.parse` in the hook restores the original unescaped strings, then template literals inject them directly into HTML without sanitization. Any user who can create/edit a site can execute arbitrary JS for every map viewer. - -```typescript -// vulnerable code -let popupContent = `
-

${site.name}

` -``` - -**Fix:** Use `textContent`-based popup rendering or sanitize via DOMPurify. Create a DOM element and set `textContent` for user data: - -```typescript -const popupEl = document.createElement('div') -popupEl.className = 'p-2' -const h3 = popupEl.appendChild(document.createElement('h3')) -h3.className = 'font-semibold text-lg mb-1' -h3.textContent = site.name -marker.bindPopup(popupEl) -``` - ---- - -### C2. ✓ FIXED — IDOR in Policy Consent Handler — Client-Controlled User ID - -**File:** `lib/towerops_web/user_auth.ex:693` - -**Severity:** CRITICAL — Any authenticated user can grant/revoke consent for any other user - -**Description:** The `accept_updated_policies` on_mount handler reads `user-id` directly from LiveView event params with zero validation: - -```elixir -"accept_updated_policies", %{"user-id" => user_id}, socket -> - ... - Accounts.grant_consent(user_id, policy_type) -``` - -Attached to ALL authenticated `live_session` scopes. An attacker can send arbitrary `user_id` values. - -**Fix:** Derive user ID from `socket.assigns.current_scope.user.id` instead of client params. - ---- - -### C3. ✓ FIXED — Broken `halt()` in GDPR Data Export - -**File:** `lib/towerops_web/controllers/api/account_data_controller.ex:20-26` - -**Severity:** CRITICAL — Unconfirmed users can download full GDPR data export - -**Description:** The halting check for unconfirmed users discards the result: - -```elixir -_ = if !user.confirmed_at do - conn |> put_status(:forbidden) |> json(...) |> halt() -end -``` - -`halt()` returns a modified conn, but it's bound to `_`. The original `conn` is used, so the function proceeds to gather ALL user data. - -**Fix:** Restructure to early-return pattern, not orphan `halt()`. - ---- - -### C4. ✓ FIXED — Missing Authorization on Member Role Changes - -**File:** `lib/towerops_web/controllers/api/v1/members_controller.ex:20-51` - -**Severity:** CRITICAL — Any API token holder can change roles or remove any member - -**Description:** `update` and `delete` actions call into `Organizations` context functions that do NOT check whether the requesting user has permission. The context functions look up membership by organization_id + user_id and perform the action — no role/authority check for the caller. - -**Fix:** Add authorization at the controller level — verify `current_user` is owner/admin before allowing role changes or removals. - ---- - -### C5. ✓ FIXED — Agent Release Webhook — Optional Signature Means No Auth - -**File:** `lib/towerops_web/controllers/api/v1/agent_release_webhook_controller.ex:37-42` - -**Severity:** CRITICAL — Anyone who discovers the endpoint can trigger mass agent updates - -**Description:** If signature header is absent, the webhook accepts the request unconditionally: - -```elixir -defp verify_optional_signature(conn) do - sig_headers = Plug.Conn.get_req_header(conn, "x-agent-webhook-signature") - case sig_headers do - [] -> :ok # passes without any auth - ... -``` - -**Fix:** Remove the "optional" behavior. Require HMAC signature on every request. - ---- - -### C6. ✓ FIXED — Repo.all_by/2 Does Not Exist — Runtime Crash - -**File:** `lib/towerops/accounts.ex:386-387` - -**Severity:** CRITICAL — Runtime FunctionClauseError when updating passwords/changing email - -**Description:** `Repo.all_by/2` is NOT a standard Ecto function. It will raise `FunctionClauseError` at runtime when `update_user_and_delete_all_tokens/1` is called (password change, email change). The DB transaction crashes with a generic error. - -```elixir -tokens_to_expire = Repo.all_by(UserToken, user_id: user.id) -``` - -**Fix:** Replace with `Repo.all(from t in UserToken, where: t.user_id == ^user.id)`. - ---- - -### C7. ✓ FIXED — Brute Force Protection Runs Before Authentication - -**File:** `lib/towerops_web/endpoint.ex:68` → plug runs here -**File:** `lib/towerops_web/plugs/brute_force_protection.ex:47` — dead auth exemption check - -**Severity:** CRITICAL — Authenticated user exemption path is unreachable; every authenticated 404 gets tracked - -**Description:** `BruteForceProtection` runs in the endpoint pipeline LONG BEFORE auth happens. The check `conn.assigns[:current_user] != nil` will ALWAYS be `nil`. The authenticated-user exemption path is completely dead code. - -**Fix:** Move auth exemption check to `register_before_send` callback, or restructure pipeline ordering. - ---- - -### C8. ✓ FIXED — Status Page `custom_css` Injection - -**File:** `lib/towerops_web/live/status_page_live.html.heex:26-30` - -**Severity:** CRITICAL — Any org admin with status page access can inject arbitrary HTML/JS - -**Description:** `custom_css` is inserted raw into a `` injection: - -```heex -<%= if @config.custom_css do %> - -<% end %> -``` - -**Fix:** Sanitize to remove ``, ``, ` put_req_header("accept", "application/json") File.rm_rf!(@mib_dir) - on_exit(fn -> File.rm_rf!(@mib_dir) end) + invalidate_mib_cache() + + on_exit(fn -> + File.rm_rf!(@mib_dir) + invalidate_mib_cache() + end) %{conn: conn, user: user, organization: organization} end + defp invalidate_mib_cache do + :persistent_term.erase(:towerops_mib_vendor_cache) + rescue + ArgumentError -> :ok + end + defp promote(user) do user |> Ecto.Changeset.change(%{is_superuser: true}) |> Towerops.Repo.update!() end