From ea91dae0e60038d1bc63eeac9f39e8de9b5bf542 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 11:38:13 -0500 Subject: [PATCH] fix: 6 medium-severity bugs (M2, M8, M9, M10, M11, M16) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- assets/js/app.ts | 8 ++- bugs.md | 72 ------------------- lib/towerops/accounts.ex | 5 +- .../api/account_data_controller.ex | 14 +++- .../controllers/api/v1/activity_controller.ex | 16 +++-- .../api/v1/coverages_controller.ex | 8 ++- lib/towerops_web/plugs/webhook_auth.ex | 4 +- 7 files changed, 40 insertions(+), 87 deletions(-) diff --git a/assets/js/app.ts b/assets/js/app.ts index f37540fb..7b75cc0e 100644 --- a/assets/js/app.ts +++ b/assets/js/app.ts @@ -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 diff --git a/bugs.md b/bugs.md index 805ca40e..58bf62ea 100644 --- a/bugs.md +++ b/bugs.md @@ -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 diff --git a/lib/towerops/accounts.ex b/lib/towerops/accounts.ex index 45405f6e..ae2c493e 100644 --- a/lib/towerops/accounts.ex +++ b/lib/towerops/accounts.ex @@ -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 """ diff --git a/lib/towerops_web/controllers/api/account_data_controller.ex b/lib/towerops_web/controllers/api/account_data_controller.ex index d94661f4..7df5a274 100644 --- a/lib/towerops_web/controllers/api/account_data_controller.ex +++ b/lib/towerops_web/controllers/api/account_data_controller.ex @@ -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 } diff --git a/lib/towerops_web/controllers/api/v1/activity_controller.ex b/lib/towerops_web/controllers/api/v1/activity_controller.ex index c5e33507..e1f5957e 100644 --- a/lib/towerops_web/controllers/api/v1/activity_controller.ex +++ b/lib/towerops_web/controllers/api/v1/activity_controller.ex @@ -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 diff --git a/lib/towerops_web/controllers/api/v1/coverages_controller.ex b/lib/towerops_web/controllers/api/v1/coverages_controller.ex index ca008f7c..8c786edb 100644 --- a/lib/towerops_web/controllers/api/v1/coverages_controller.ex +++ b/lib/towerops_web/controllers/api/v1/coverages_controller.ex @@ -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 diff --git a/lib/towerops_web/plugs/webhook_auth.ex b/lib/towerops_web/plugs/webhook_auth.ex index 1ddf859e..9bdd7a3d 100644 --- a/lib/towerops_web/plugs/webhook_auth.ex +++ b/lib/towerops_web/plugs/webhook_auth.ex @@ -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) ->