fix: 6 medium-severity bugs (M2, M8, M9, M10, M11, M16)

- M2: get_user_by_email/1 uses lower(email) = lower(?) so User@Example.com
  and user@example.com resolve to the same account; closes a lookalike-
  registration / password-reset confusion risk.
- M8: webhook auth plug no longer echoes "Webhook authentication not
  configured" — the misconfiguration is logged server-side and the caller
  gets a generic Internal server error so endpoints can't be probed.
- M9: coverages controller logs the underlying KMZ build error and
  returns a generic "Failed to build KMZ" string — filesystem paths and
  zip internals no longer leak.
- M10: account-data controller no longer crashes with MatchError when an
  org has zero or multiple memberships; defaults to "member" and treats
  any owner membership as owner.
- M11: ActivityController switches to a hard whitelist
  (Atom.to_string/1 comparison) instead of String.to_existing_atom/1;
  unknown filter types are dropped silently and the atom table can't be
  grown from API input.
- M16: title-tracking MutationObserver is stored on `this` so destroyed()
  actually disconnects it — fixes a memory leak per LV navigation.
This commit is contained in:
Graham McIntire 2026-05-12 11:38:13 -05:00
parent abfd44fd75
commit ea91dae0e6
7 changed files with 40 additions and 87 deletions

View file

@ -280,8 +280,10 @@ const StatusTitle = {
// Store the initial base title (without any existing emoji)
this.baseTitle = document.title.replace(/^(?:(?:🔴|🟡|🟢)\s)+/u, "")
// Update base title whenever page title changes (from LiveView navigation)
const observer = new MutationObserver(() => {
// Update base title whenever page title changes (from LiveView navigation).
// Stored on `this` so destroyed() can disconnect it — a plain `const`
// would leave the observer alive forever.
this.observer = new MutationObserver(() => {
const currentTitle = document.title
// Only update baseTitle if it's not an emoji update (i.e., it's a real navigation)
if (currentTitle && !currentTitle.startsWith('🔴') && !currentTitle.startsWith('🟡') && !currentTitle.startsWith('🟢')) {
@ -289,7 +291,7 @@ const StatusTitle = {
}
})
observer.observe(document.querySelector('title')!, {
this.observer.observe(document.querySelector('title')!, {
childList: true,
characterData: true,
subtree: true

72
bugs.md
View file

@ -122,18 +122,6 @@
---
### 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?)
@ -194,54 +182,6 @@
---
### 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
@ -290,18 +230,6 @@
---
### 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

View file

@ -37,7 +37,10 @@ defmodule Towerops.Accounts do
"""
@spec get_user_by_email(String.t()) :: User.t() | nil
def get_user_by_email(email) when is_binary(email) do
Repo.get_by(User, email: email)
# Case-insensitive match so `User@Example.com` and `user@example.com`
# resolve to the same account — prevents lookalike registrations and
# password-reset confusion.
Repo.one(from u in User, where: fragment("lower(?) = lower(?)", u.email, ^email))
end
@doc """

View file

@ -70,14 +70,22 @@ defmodule ToweropsWeb.Api.AccountDataController do
user.id
|> Organizations.list_user_organizations()
|> Enum.map(fn org ->
# Organization has the user's membership preloaded (only their membership)
[membership] = org.memberships
# Organization has the user's membership preloaded. Handle the
# zero-or-more-than-one cases defensively — historically this was a
# plain `[membership] = ...` pattern match that crashed on any
# unexpected count and produced a 500 from this user-facing endpoint.
role =
case org.memberships do
[%{role: :owner} | _] -> "owner"
[_first | _] -> "member"
_ -> "member"
end
%{
id: org.id,
name: org.name,
slug: org.slug,
role: if(membership.role == :owner, do: "owner", else: "member"),
role: role,
inserted_at: org.inserted_at,
updated_at: org.updated_at
}

View file

@ -21,23 +21,27 @@ defmodule ToweropsWeb.Api.V1.ActivityController do
defp maybe_add_types(opts, nil), do: opts
# Allowlist of activity types the API accepts. Anything else is dropped
# silently. Using a hard list avoids `String.to_existing_atom/1` which
# would convert any string that happens to match an internal atom
# somewhere in the BEAM — that produces unpredictable filter behavior
# and grows the atom table from user input.
@allowed_types ~w(config_change alert_fired alert_resolved device_event sync device_added)a
defp maybe_add_types(opts, types) when is_binary(types) do
type_atoms =
types
|> String.split(",")
|> Enum.map(&String.trim/1)
|> Enum.map(&safe_to_atom/1)
|> Enum.reject(&is_nil/1)
|> Enum.flat_map(&allowed_type/1)
Keyword.put(opts, :types, type_atoms)
end
defp maybe_add_types(opts, _), do: opts
defp safe_to_atom(str) do
String.to_existing_atom(str)
rescue
ArgumentError -> nil
defp allowed_type(str) do
Enum.filter(@allowed_types, fn t -> Atom.to_string(t) == str end)
end
defp parse_int(nil, default), do: default

View file

@ -199,9 +199,15 @@ defmodule ToweropsWeb.Api.V1.CoveragesController do
|> json(%{error: "Coverage is not ready yet"})
{:error, reason} ->
require Logger
# Log the underlying reason server-side so we can debug, but don't
# leak filesystem paths / zip errors / IO details to the caller.
Logger.error("Failed to build KMZ for coverage: #{inspect(reason)}")
conn
|> put_status(:internal_server_error)
|> json(%{error: "Failed to build KMZ: #{inspect(reason)}"})
|> json(%{error: "Failed to build KMZ"})
end
end

View file

@ -37,9 +37,11 @@ defmodule ToweropsWeb.Plugs.WebhookAuth do
Logger.error("agent_webhook_secret not configured — rejecting webhook request")
# Log the real reason server-side; don't echo it to the caller —
# otherwise an attacker can probe for unconfigured endpoints.
conn
|> put_status(:internal_server_error)
|> json(%{error: "Webhook authentication not configured"})
|> json(%{error: "Internal server error"})
|> halt()
Plug.Crypto.secure_compare(token, secret) ->