remove all fixed critical items from bugs.md, fix flaky MIB cache test
This commit is contained in:
parent
e96d077fdb
commit
810dafbf5e
2 changed files with 12 additions and 189 deletions
188
bugs.md
188
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 = `<div class="p-2">
|
||||
<h3 class="font-semibold text-lg mb-1">${site.name}</h3>`
|
||||
```
|
||||
|
||||
**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 `<style>` tag. The server-side validator blocks `url()` but NOT `</style>` injection:
|
||||
|
||||
```heex
|
||||
<%= if @config.custom_css do %>
|
||||
<style>
|
||||
{@config.custom_css}
|
||||
</style>
|
||||
<% end %>
|
||||
```
|
||||
|
||||
**Fix:** Sanitize to remove `</style>`, `</script>`, `<script` strings. Add server-side validation.
|
||||
|
||||
---
|
||||
|
||||
### C9. ✓ FIXED — Live Production Credentials in Plaintext on Disk
|
||||
|
||||
**File:** `k8s/secrets.yaml` (exists on disk, in `.gitignore`)
|
||||
|
||||
**Severity:** CRITICAL — Full production credential set on local filesystem
|
||||
|
||||
**Description:** Contains real production credentials: Stripe live key, AWS access keys, DeepSeek API key, PostgreSQL superuser credentials, Cloak encryption key, secret_key_base, Redis password, release cookie. A single accidental `git add --force` or CI misconfiguration leaks everything.
|
||||
|
||||
**Fix:** Remove the file; use `k8s/secrets.example.yaml` with placeholders. Load real secrets from 1Password or Kubernetes secrets at runtime.
|
||||
|
||||
---
|
||||
|
||||
### C10. ✓ FIXED — Hardcoded Stripe Test Key in Git
|
||||
|
||||
**Files:** `config/dev.exs:243`, `setup_stripe_meter.exs:5`
|
||||
|
||||
**Severity:** CRITICAL — Real Stripe Test API key in version control
|
||||
|
||||
**Description:** `sk_test_51T7zcQS77kvnTfgyu0DCQU2xVKzeaneVCueHaXV3jsIw6HAwGWWllEL3J8jGZOybHAtPyu6oYIiuvJHhReFtTycE00VrJYJ8Ao` is hardcoded in two files committed to git. Every developer has Stripe test environment access.
|
||||
|
||||
**Fix:** Load from `STRIPE_SECRET_KEY` env var consistently across all environments.
|
||||
|
||||
---
|
||||
|
||||
### C13. ✓ FIXED — MikroTik Credentials Broadcast Over PubSub
|
||||
|
||||
**File:** `lib/towerops/workers/mikrotik_backup_worker.ex:95-98`
|
||||
|
||||
**Severity:** CRITICAL — Plaintext credentials exposed to all PubSub subscribers
|
||||
|
||||
**Description:** MikroTik usernames and passwords are placed in the `MikrotikDevice` struct and broadcast over PubSub on `"agent:#{agent_token_id}:backup"` channel. Any internal process or LiveView subscribed receives credentials.
|
||||
|
||||
**Fix:** Use a job ID that agents resolve independently, or encrypt credentials with Vault before broadcasting.
|
||||
|
||||
---
|
||||
|
||||
## HIGH
|
||||
|
||||
### H1. N+1 Queries in MobileController
|
||||
|
|
|
|||
|
|
@ -25,11 +25,22 @@ defmodule ToweropsWeb.Api.V1.MibControllerTest do
|
|||
|> 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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue