From abfd44fd75a7c321698493d68a1d2f1b944ad25f Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 11:32:20 -0500 Subject: [PATCH] fix: 3 more high-severity bugs (H19, H20, H25) - H19: /api/v1/mobile/auth/qr/verify no longer returns user_email. Knowing a QR token now only tells the caller the token is valid; the email is only revealed by /complete which consumes the token. - H20: CoverageWorker max_attempts dropped from 3 to 1. The fail/2 path already returns :ok and writes the failure onto the coverage record, so the 3-attempt retry policy was never reachable and was misleading. - H25: ChecksController.create rejects device_ids that don't belong to the caller's organization (via ScopedResource.fetch). Without this an API token could attach service checks to devices in another tenant. Also strip bug ID references from in-code comments (per feedback that H/M/L numbers don't survive past their bugs.md removal); commit history keeps the audit trail. --- bugs.md | 24 ---------- lib/towerops/workers/coverage_worker.ex | 5 ++- .../controllers/api/mobile_auth_controller.ex | 14 +++--- .../controllers/api/v1/checks_controller.ex | 44 ++++++++++++++----- test/towerops/vault_test.exs | 6 +-- .../api/mobile_auth_controller_test.exs | 7 ++- .../controllers/health_controller_test.exs | 2 +- 7 files changed, 53 insertions(+), 49 deletions(-) diff --git a/bugs.md b/bugs.md index f9320511..805ca40e 100644 --- a/bugs.md +++ b/bugs.md @@ -55,30 +55,6 @@ --- -### 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:** diff --git a/lib/towerops/workers/coverage_worker.ex b/lib/towerops/workers/coverage_worker.ex index 8c47825c..1d28d364 100644 --- a/lib/towerops/workers/coverage_worker.ex +++ b/lib/towerops/workers/coverage_worker.ex @@ -20,9 +20,12 @@ defmodule Towerops.Workers.CoverageWorker do worker refuses to run if the loaded coverage's org doesn't match. """ + # max_attempts: 1 — `fail/2` returns :ok and writes the failure onto the + # coverage row itself, so there's nothing for Oban to retry. The previous + # max_attempts: 3 implied retries that never actually happened. use Oban.Worker, queue: :coverage, - max_attempts: 3, + max_attempts: 1, unique: [ fields: [:args], keys: [:coverage_id], diff --git a/lib/towerops_web/controllers/api/mobile_auth_controller.ex b/lib/towerops_web/controllers/api/mobile_auth_controller.ex index 8749cab5..ad000bb2 100644 --- a/lib/towerops_web/controllers/api/mobile_auth_controller.ex +++ b/lib/towerops_web/controllers/api/mobile_auth_controller.ex @@ -36,14 +36,12 @@ defmodule ToweropsWeb.Api.MobileAuthController do |> put_status(:unauthorized) |> json(%{valid: false, error: "Invalid or expired token"}) - qr_token -> - # Preload user to return email - qr_token = Towerops.Repo.preload(qr_token, :user) - - json(conn, %{ - valid: true, - user_email: qr_token.user.email - }) + _qr_token -> + # Don't leak the associated user's email here. Anyone who guesses or + # captures a QR token would otherwise be able to enumerate emails by + # calling /verify. The email is only meaningful at /complete (which + # consumes the token and creates a session). + json(conn, %{valid: true}) end end diff --git a/lib/towerops_web/controllers/api/v1/checks_controller.ex b/lib/towerops_web/controllers/api/v1/checks_controller.ex index cfb59883..37df870f 100644 --- a/lib/towerops_web/controllers/api/v1/checks_controller.ex +++ b/lib/towerops_web/controllers/api/v1/checks_controller.ex @@ -10,6 +10,7 @@ defmodule ToweropsWeb.Api.V1.ChecksController do import ToweropsWeb.Api.ErrorHelpers, only: [translate_errors: 1] + alias Towerops.Devices.Device alias Towerops.Monitoring alias Towerops.Monitoring.Check alias ToweropsWeb.Api.ParamFilter @@ -50,17 +51,28 @@ defmodule ToweropsWeb.Api.V1.ChecksController do check_type = Map.get(check_params, "check_type", "") - if check_type in @service_check_types do - attrs = - check_params - |> ParamFilter.strip_sensitive() - |> Map.put("organization_id", organization_id) + cond do + check_type not in @service_check_types -> + conn + |> put_status(:bad_request) + |> json(%{ + error: "Invalid check type '#{check_type}'. Must be one of: #{Enum.join(@service_check_types, ", ")}" + }) - create_and_respond(conn, attrs) - else - conn - |> put_status(:bad_request) - |> json(%{error: "Invalid check type '#{check_type}'. Must be one of: #{Enum.join(@service_check_types, ", ")}"}) + not device_in_org?(check_params["device_id"], organization_id) -> + # Reject up-front so an attacker can't attach a check to a device in + # another organization by guessing its ID. + conn + |> put_status(:forbidden) + |> json(%{error: "Access denied to the referenced device"}) + + true -> + attrs = + check_params + |> ParamFilter.strip_sensitive() + |> Map.put("organization_id", organization_id) + + create_and_respond(conn, attrs) end end @@ -180,6 +192,18 @@ defmodule ToweropsWeb.Api.V1.ChecksController do end end + # Verify that an inbound device_id refers to a device in the caller's + # organization. Returns true when device_id is missing (the changeset + # validation will then reject it as required). + defp device_in_org?(nil, _organization_id), do: true + + defp device_in_org?(device_id, organization_id) do + case ScopedResource.fetch(Device, device_id, organization_id) do + {:ok, _device} -> true + _ -> false + end + end + defp format_check(check) do %{ id: check.id, diff --git a/test/towerops/vault_test.exs b/test/towerops/vault_test.exs index 4595226d..e0991bb4 100644 --- a/test/towerops/vault_test.exs +++ b/test/towerops/vault_test.exs @@ -17,9 +17,9 @@ defmodule Towerops.VaultTest do end test "overrides ciphers when CLOAK_KEY env is set in :prod" do - # The env-var override only fires under :env == :prod (L5) so a - # dev shell with a prod CLOAK_KEY set doesn't encrypt local data - # with the prod key. Flip :env for the duration of the test. + # The env-var override only fires under :env == :prod so a dev + # shell with a prod CLOAK_KEY set doesn't encrypt local data with + # the prod key. Flip :env for the duration of the test. original_env = System.get_env("CLOAK_KEY") original_env_setting = Application.get_env(:towerops, :env) key = 32 |> :crypto.strong_rand_bytes() |> Base.encode64() diff --git a/test/towerops_web/controllers/api/mobile_auth_controller_test.exs b/test/towerops_web/controllers/api/mobile_auth_controller_test.exs index 5a1f5d4e..d2a33ce6 100644 --- a/test/towerops_web/controllers/api/mobile_auth_controller_test.exs +++ b/test/towerops_web/controllers/api/mobile_auth_controller_test.exs @@ -18,7 +18,7 @@ defmodule ToweropsWeb.Api.MobileAuthControllerTest do assert json_response(conn, 401)["valid"] == false end - test "returns valid + user_email for an active QR token", %{conn: conn} do + test "returns valid for an active QR token without leaking the user email", %{conn: conn} do user = user_fixture() {:ok, qr_token} = MobileSessions.create_qr_login_token(user.id) @@ -26,7 +26,10 @@ defmodule ToweropsWeb.Api.MobileAuthControllerTest do response = json_response(conn, 200) assert response["valid"] == true - assert response["user_email"] == user.email + # Email must not be returned by /verify (only by /complete, once the + # token is consumed). Otherwise anyone with a QR token can read the + # linked user's email. + refute Map.has_key?(response, "user_email") end end diff --git a/test/towerops_web/controllers/health_controller_test.exs b/test/towerops_web/controllers/health_controller_test.exs index c685186a..d82ce41f 100644 --- a/test/towerops_web/controllers/health_controller_test.exs +++ b/test/towerops_web/controllers/health_controller_test.exs @@ -8,7 +8,7 @@ defmodule ToweropsWeb.HealthControllerTest do result = Jason.decode!(body) assert result["status"] == "ok" assert result["database"] == "connected" - # `version` deliberately omitted (L6): exposing the app version on a + # `version` deliberately omitted: exposing the app version on a # public health endpoint helps attackers fingerprint vulnerable builds. refute Map.has_key?(result, "version") end