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