# Security Audit Report — TowerOps Web Application

**Date:** 2026-03-14  
**Scope:** Authentication, authorization, and session security  
**Auditor:** Automated code review (Skippy)

---

## Summary

The TowerOps application demonstrates a well-structured security posture overall. Authentication uses Argon2 password hashing, TOTP-based MFA, session token management with expiry, and CSRF protection. Organization-scoped data access is consistently enforced through `ScopedResource` and `AccessControl` helpers.

The findings below are verified issues found in the source code.

---

## Findings

### 1. API Device Create Allows `organization_id` Override

**Severity:** HIGH  
**File:** `lib/towerops_web/controllers/api/v1/devices_controller.ex`, lines 245–249  
**Issue:** The `default_organization_id/2` function only sets `organization_id` when the param is `nil` or empty. If a client provides a *different* `organization_id` in the request body, it is passed through to `Devices.create_device/2` unchanged. This allows an API token holder for Org A to create devices in Org B.

```elixir
defp default_organization_id(device_params, organization_id) do
  case Map.get(device_params, "organization_id") do
    nil -> Map.put(device_params, "organization_id", organization_id)
    "" -> Map.put(device_params, "organization_id", organization_id)
    _provided_id -> device_params  # ← attacker-controlled org_id accepted
  end
end
```

**Suggested Fix:** Always force the authenticated organization_id:
```elixir
defp default_organization_id(device_params, organization_id) do
  Map.put(device_params, "organization_id", organization_id)
end
```

---

### 2. Brute Force Protection Disabled in Production

**Severity:** HIGH  
**File:** `lib/towerops_web/plugs/brute_force_protection.ex`, line 38  
**Issue:** The plug's `call/2` function unconditionally returns `conn` without any processing. The comment says "TEMPORARILY DISABLED." While Cloudflare provides edge-level protection, if Cloudflare is bypassed or misconfigured, the application has no server-side brute force protection beyond rate limiting.

```elixir
def call(conn, _opts) do
  # TEMPORARILY DISABLED - brute force protection bypassed
  conn
end
```

**Suggested Fix:** Re-enable the brute force protection or remove the plug entirely to avoid a false sense of security. At minimum, document the Cloudflare dependency as a security requirement.

---

### 3. Admin API MIB/GeoIP Routes Use Application-Level Superuser Check, Not Pipeline

**Severity:** MEDIUM  
**File:** `lib/towerops_web/router.ex`, lines 168–176  
**Files affected:** `lib/towerops_web/controllers/api/v1/mib_controller.ex`, `lib/towerops_web/controllers/api/v1/geoip_controller.ex`  
**Issue:** The admin API scope (`/admin/api`) uses only the `:api_v1` pipeline (Bearer token auth) without the `:require_superuser` pipeline plug. Superuser checks are done inside each controller action via `require_superuser/1`. This is defense-in-depth weakness — if a developer adds a new action and forgets the check, it would be accessible to any API token holder.

```elixir
# Router - no superuser pipeline plug
scope "/admin/api", V1 do
  pipe_through :api_v1
  # ...
end
```

The controllers do implement their own `require_superuser` check, which works, but the `halt()` return from within `with :ok <- require_superuser(conn)` is not propagated correctly because `halt()` is called inside the private function but `with` still proceeds. Let me verify...

Actually, reviewing more carefully: the `require_superuser` function calls `halt()` on the conn and returns the halted conn. The `with :ok <-` pattern means if it doesn't return `:ok`, the `with` block falls through. But the `require_superuser/1` function calls `json` + `halt` and returns the conn, not `:ok`. So the `with` clause correctly handles this — if `require_superuser` returns the halted conn instead of `:ok`, the `with` doesn't match and the function returns without executing the body. **However**, the halted conn is not actually returned to the caller — it's discarded. The controller action returns normally without the halted conn.

**Wait** — re-reading the code: the `with :ok <- require_superuser(conn)` pattern means if `require_superuser` returns anything other than `:ok`, the `with` block uses its else clause or falls through. Since there's no explicit else, the non-matching return value (the halted conn) is returned as the function's return value. Phoenix will see that the conn is halted and stop processing. This is correct.

**Revised assessment:** The controller-level checks work correctly. The concern remains that this is a defense-in-depth gap — a pipeline-level check would be more robust.

**Suggested Fix:** Add a `require_superuser_api` plug to the admin API pipeline:
```elixir
scope "/admin/api", V1 do
  pipe_through [:api_v1, :require_superuser_api]
end
```

---

### 4. Honeybadger API Key Hardcoded in Source

**Severity:** MEDIUM  
**File:** `config/config.exs`, line 29  
**Issue:** The Honeybadger API key (`hbp_xe5xMnpLZ2XJsXQJujoEkgCmiqCfwa0uYA3Y`) is hardcoded in version-controlled source. While Honeybadger is an error-tracking service (not end-user-facing), a leaked API key could allow attackers to send fake error reports, pollute error data, or potentially access error data containing sensitive information (stack traces, request params).

```elixir
config :honeybadger,
  api_key: "hbp_xe5xMnpLZ2XJsXQJujoEkgCmiqCfwa0uYA3Y",
```

**Suggested Fix:** Move to an environment variable:
```elixir
config :honeybadger,
  api_key: System.get_env("HONEYBADGER_API_KEY"),
```

---

### 5. Org Settings LiveView Missing Role-Based Authorization on Sensitive Operations

**Severity:** MEDIUM  
**File:** `lib/towerops_web/live/org/settings_live.ex`, lines 103–225  
**Issue:** The `handle_event` callbacks for `"save"` (update org settings), `"send_invitation"`, `"remove_member"`, `"change_role"`, `"apply_snmp_to_all"`, and `"apply_agent_to_all"` do not check the current user's role/permissions. Any authenticated user who is a member of the organization can access the settings page (it's in the `require_authenticated_user_and_organization` live_session) and perform these operations, regardless of whether they are an owner, admin, or technician.

The `Permissions` module exists but is not imported or used in this LiveView. Only the backup delete event in `DeviceLive.Show` uses `owner?()`.

**Suggested Fix:** Add role checks to sensitive operations:
```elixir
def handle_event("save", %{"organization" => org_params}, socket) do
  import ToweropsWeb.Permissions
  unless admin?(socket), do: {:noreply, put_flash(socket, :error, "Permission denied")}
  # ... existing logic
end

def handle_event("send_invitation", params, socket) do
  import ToweropsWeb.Permissions
  unless admin?(socket), do: {:noreply, put_flash(socket, :error, "Permission denied")}
  # ... existing logic
end
```

---

### 6. Session Cookie Missing `secure: true` Flag

**Severity:** MEDIUM  
**File:** `lib/towerops_web/endpoint.ex`, lines 9–14  
**Issue:** The session cookie options don't set `secure: true`. While the comment in `prod.exs` says "SSL/TLS is handled by Cloudflared proxy," the cookie `secure` flag is independent of TLS termination — it tells the browser to only send the cookie over HTTPS connections. Without it, if a user somehow accesses the app over HTTP (e.g., direct IP access bypassing Cloudflare), the session cookie would be transmitted in cleartext.

```elixir
@session_options [
  store: :cookie,
  key: "_towerops_key",
  signing_salt: "hrDZxLhd",
  encryption_salt: "vK3p8mNx",
  same_site: "Lax"
  # missing: secure: true
]
```

**Suggested Fix:** Add `secure: true` for production:
```elixir
@session_options [
  store: :cookie,
  key: "_towerops_key",
  signing_salt: "hrDZxLhd",
  encryption_salt: "vK3p8mNx",
  same_site: "Lax",
  secure: Application.compile_env(:towerops, :env) == :prod
]
```

Or configure via `config/prod.exs`.

---

### 7. Remember-Me Cookie Missing `http_only: true` Flag

**Severity:** LOW  
**File:** `lib/towerops_web/user_auth.ex`, lines 19–23  
**Issue:** The `@remember_me_options` for the remember-me cookie don't explicitly set `http_only: true`. While Phoenix's `put_resp_cookie` defaults to `http_only: true`, and `secure` defaults to the conn's scheme, it's best practice to be explicit. The `secure` flag is also not set, same concern as finding #6.

```elixir
@remember_me_options [
  sign: true,
  max_age: @max_cookie_age_in_days * 24 * 60 * 60,
  same_site: "Lax"
  # missing explicit: http_only: true, secure: true
]
```

**Suggested Fix:** Be explicit:
```elixir
@remember_me_options [
  sign: true,
  max_age: @max_cookie_age_in_days * 24 * 60 * 60,
  same_site: "Lax",
  http_only: true,
  secure: true
]
```

---

### 8. No `force_ssl` Configured for Production

**Severity:** LOW  
**File:** `config/prod.exs`, line 41  
**Issue:** The comment acknowledges that SSL is handled by Cloudflared proxy, so `force_ssl` is not configured. However, without `force_ssl`, the application won't set HSTS headers. HSTS ensures browsers always use HTTPS even if a user types `http://`. This is a defense-in-depth measure.

**Suggested Fix:** Enable `force_ssl` with HSTS even behind Cloudflare:
```elixir
config :towerops, ToweropsWeb.Endpoint,
  force_ssl: [hsts: true, rewrite_on: [:x_forwarded_proto]]
```

The `rewrite_on: [:x_forwarded_proto]` ensures it works correctly behind a proxy.

---

### 9. Alert Show Leaks Existence of Alerts Across Organizations via Error Type

**Severity:** LOW  
**File:** `lib/towerops_web/controllers/api/v1/alerts_controller.ex`, lines 32–44  
**Issue:** The `show` action uses `Alerts.get_alert!(id)` which raises `Ecto.NoResultsError` for non-existent alerts, but returns a 404 with "Alert not found" for alerts that exist in other organizations. An attacker can distinguish between "alert doesn't exist" and "alert exists but belongs to another org" by observing whether the error is caught by the rescue clause or the if-else branch. In practice, the response is the same 404 message, but the code path differs (potential timing difference).

This is a very minor information leak. The same pattern exists in `acknowledge` and `resolve`.

**Suggested Fix:** Use a non-raising `get_alert/1` function that returns nil:
```elixir
def show(conn, %{"id" => id}) do
  organization_id = conn.assigns.current_organization_id

  case Alerts.get_alert(id) do
    nil ->
      conn |> put_status(:not_found) |> json(%{error: "Alert not found"})
    alert ->
      if alert.device && alert.device.organization_id == organization_id do
        json(conn, %{data: format_alert(alert)})
      else
        conn |> put_status(:not_found) |> json(%{error: "Alert not found"})
      end
  end
end
```

---

## Positive Observations

These are **not findings** — they are security strengths worth noting:

1. **Password hashing:** Uses Argon2 with timing-safe comparison and `no_user_verify()` to prevent user enumeration.
2. **TOTP implementation:** Properly separates TOTP from recovery codes for sudo mode. Recovery codes cannot bypass sudo verification.
3. **Session management:** Tokens expire after 14 days, sessions are reissued after 7 days, and there's a 30-minute inactivity timeout.
4. **CSRF protection:** `protect_from_forgery` plug in browser pipeline, session renewal on login to prevent fixation.
5. **Organization scoping:** `ScopedResource` pattern consistently enforces org-level isolation in API controllers.
6. **LiveView auth:** All live_sessions use `on_mount` hooks for authentication/authorization checks.
7. **Impersonation audit trail:** Impersonation start/stop creates audit log entries with IP addresses.
8. **API token hashing:** Tokens are SHA-256 hashed before storage; raw tokens shown only once.
9. **Rate limiting:** Auth endpoints limited to 10 req/min, API to 1000 req/min per IP.
10. **GraphQL hardening:** Introspection disabled in production, complexity and depth limits enforced.
11. **Webhook signature verification:** Uses `Plug.Crypto.secure_compare` for timing-safe comparison.
12. **Content Security Policy:** Properly configured with `frame-ancestors 'none'` to prevent clickjacking.
13. **MIB upload path traversal protection:** Vendor names and filenames validated; archive contents checked for path traversal.
14. **Mobile API authorization:** Each endpoint verifies the user has access to the requested organization.

---

## Risk Summary

| Severity | Count | Key Issues |
|----------|-------|-----------|
| CRITICAL | 0 | — |
| HIGH | 2 | API org_id override, brute force protection disabled |
| MEDIUM | 3 | Admin API pipeline gap, hardcoded API key, missing role checks in org settings |
| LOW | 3 | Cookie flags, no HSTS, minor info leak |

**Overall Assessment:** No critical vulnerabilities found. The two HIGH findings should be addressed promptly — the API `organization_id` override is the most actionable. The brute force protection gap is mitigated by Cloudflare but represents a defense-in-depth weakness.