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

@@ -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:**

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],

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 ->
+      _qr_token ->
-        # Preload user to return email
+        # Don't leak the associated user's email here. Anyone who guesses or
-        qr_token = Towerops.Repo.preload(qr_token, :user)
+        # captures a QR token would otherwise be able to enumerate emails by
-
+        # calling /verify. The email is only meaningful at /complete (which
-        json(conn, %{
+        # consumes the token and creates a session).
-          valid: true,
+        json(conn, %{valid: true})
-          user_email: qr_token.user.email
-        })
     end
   end
 

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
+    cond do
-      attrs =
+      check_type not in @service_check_types ->
-        check_params
+        conn
-        |> ParamFilter.strip_sensitive()
+        |> put_status(:bad_request)
-        |> Map.put("organization_id", organization_id)
+        |> json(%{
+          error: "Invalid check type '#{check_type}'. Must be one of: #{Enum.join(@service_check_types, ", ")}"
+        })
 
-      create_and_respond(conn, attrs)
+      not device_in_org?(check_params["device_id"], organization_id) ->
-    else
+        # Reject up-front so an attacker can't attach a check to a device in
-      conn
+        # another organization by guessing its ID.
-      |> put_status(:bad_request)
+        conn
-      |> json(%{error: "Invalid check type '#{check_type}'. Must be one of: #{Enum.join(@service_check_types, ", ")}"})
+        |> 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,

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
+      # The env-var override only fires under :env == :prod so a dev
-      # dev shell with a prod CLOAK_KEY set doesn't encrypt local data
+      # shell with a prod CLOAK_KEY set doesn't encrypt local data with
-      # with the prod key. Flip :env for the duration of the test.
+      # 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()

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
 

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