# Security Audit: API & External Integrations

**Date:** 2026-03-14
**Scope:** REST API, GraphQL API, webhook handlers, integration clients, background workers
**Auditor:** Automated code review

---

## Summary

| Severity | Count |
|----------|-------|
| High     | 2     |
| Medium   | 5     |
| Low      | 3     |

Overall the codebase demonstrates strong security practices: webhook signature verification uses `Plug.Crypto.secure_compare`, API tokens are hashed with SHA-256, credentials are encrypted at rest (Cloak AES-256-GCM), rate limiting is applied to auth and API endpoints, and GraphQL has complexity/depth limits. The findings below are areas for improvement.

---

## HIGH Severity

### H1: SSRF via User-Controlled Integration URLs (Sonar, Splynx)

**Files:**
- `lib/towerops/sonar/client.ex` — lines 141-142
- `lib/towerops/splynx/client.ex` — lines 13-15 (via `authenticate/3`), lines 95-96 (via `get/4`)
- `lib/towerops/sonar/sync.ex` — line 21
- `lib/towerops/splynx/sync.ex` — line 20

**Description:**
Sonar and Splynx integrations accept a user-supplied `instance_url` stored in `integration.credentials["instance_url"]`. This URL is directly concatenated into HTTP requests without any validation:

```elixir
# sonar/client.ex:142
url = String.trim_trailing(instance_url, "/") <> "/api/graphql"

# splynx/client.ex — authenticate/3 and get/4
url = String.trim_trailing(instance_url, "/") <> "/api/2.0/admin/auth/tokens"
```

An attacker with org-admin access could set `instance_url` to `http://169.254.169.254` (cloud metadata), `http://localhost:5432`, or any internal service, causing the server to make requests to internal network resources. The integration sync workers then execute these requests periodically in the background.

**Impact:** Server-Side Request Forgery (SSRF). Could access cloud metadata endpoints (AWS/GCP instance credentials), internal services, or scan internal networks.

**Suggested Fix:**
Add URL validation in `Towerops.Integrations` when creating/updating integrations:

```elixir
defp validate_instance_url(changeset) do
  validate_change(changeset, :credentials, fn :credentials, creds ->
    case creds["instance_url"] do
      nil -> []
      url ->
        uri = URI.parse(url)
        cond do
          uri.scheme not in ["https", "http"] ->
            [credentials: "instance_url must use http or https"]
          is_private_ip?(uri.host) ->
            [credentials: "instance_url cannot point to a private IP address"]
          uri.host in ["localhost", "127.0.0.1", "::1", "0.0.0.0"] ->
            [credentials: "instance_url cannot point to localhost"]
          true -> []
        end
    end
  end)
end
```

Also consider resolving the hostname and checking the IP before making requests.

---

### H2: Admin API Routes Missing Rate Limiting

**File:** `lib/towerops_web/router.ex` — lines 172-180

**Description:**
The admin API scope (`/admin/api`) only pipes through `:api_v1` but does **not** include `:rate_limit_api` or any rate limiting pipeline:

```elixir
scope "/admin/api", V1 do
  pipe_through :api_v1          # <-- no rate_limit_api

  get "/mibs", MibController, :index
  post "/mibs", MibController, :upload
  delete "/mibs/:vendor", MibController, :delete
  post "/geoip/import", GeoipController, :import_database
end
```

While these endpoints do check for superuser status in the controller, a compromised superuser API token could be used to flood the server with MIB uploads or GeoIP imports without any rate limiting, potentially causing DoS.

**Impact:** Denial of service via resource exhaustion (disk space from MIB uploads, database from GeoIP imports).

**Suggested Fix:**
Add rate limiting to the admin API pipeline:

```elixir
scope "/admin/api", V1 do
  pipe_through [:api_v1, :rate_limit_api]  # or a stricter :admin_rate_limit
  ...
end
```

---

## MEDIUM Severity

### M1: GraphQL Complexity/Depth Limits Disabled in Non-Production Environments

**File:** `lib/towerops_web/graphql/context.ex` — lines 15-20

**Description:**
The GraphQL context plug conditionally applies complexity analysis only in production:

```elixir
opts =
  if Application.get_env(:towerops, :env) == :prod do
    [context: context, analyze_complexity: true, max_complexity: 500]
  else
    [context: context]
  end
```

Note: The router (`router.ex:139-141`) does set `analyze_complexity: true, max_complexity: 500, max_depth: 10` at the Absinthe.Plug level. However, the Context plug **overwrites** these options via `Absinthe.Plug.put_options/2`, which replaces the entire options map. In non-production environments, this effectively removes the complexity and depth limits set in the router.

**Impact:** In staging/dev environments (which may be publicly accessible), deeply nested or highly complex queries could cause CPU/memory exhaustion.

**Suggested Fix:**
Apply complexity limits in all environments:

```elixir
def call(conn, _opts) do
  context = build_context(conn)
  Absinthe.Plug.put_options(conn,
    context: context,
    analyze_complexity: true,
    max_complexity: 500
  )
end
```

---

### M2: GraphQL Introspection Block is Bypassable

**File:** `lib/towerops_web/plugs/graphql_introspection.ex` — lines 18-24

**Description:**
The introspection check uses simple string matching:

```elixir
String.contains?(query, "__schema") or String.contains?(query, "__type")
```

This can be bypassed using:
1. **GraphQL aliases:** `query { myAlias: __schema { types { name } } }` — Actually still contains `__schema`, so this specific bypass won't work.
2. **Batch requests:** If the `query` parameter is sent as part of a JSON array (batch queries), `conn.body_params` won't match the `%{"query" => query}` pattern since it would be a list, not a map.
3. **The `operationName` + `variables` pattern:** The check only examines the `query` field, but if the GraphQL request uses persisted queries or extensions, the query text may not be in `body_params["query"]`.

More critically, the check only runs on `body_params["query"]`. If the query is sent via GET request with query parameters (which Absinthe supports by default), the body params would be empty and the check is bypassed entirely.

**Impact:** Schema exposure in production, revealing the full API surface to attackers.

**Suggested Fix:**
Also check query params for GET requests, and consider using Absinthe's built-in introspection control:

```elixir
# In schema.ex - more reliable approach
def context(ctx) do
  if Application.get_env(:towerops, :env) == :prod do
    Map.put(ctx, :introspection, false)
  else
    ctx
  end
end
```

Or disable introspection at the Absinthe level by adding a phase or middleware.

---

### M3: Webhook Endpoints (Gaiia, PagerDuty) Lack Dedicated Rate Limiting

**File:** `lib/towerops_web/router.ex` — lines 163-169

**Description:**
The Gaiia and PagerDuty webhook routes share the general `:rate_limit_api` (1000 req/min per IP):

```elixir
scope "/api/v1/webhooks", V1 do
  pipe_through [:api, :rate_limit_api]

  post "/gaiia/:organization_id", GaiiaWebhookController, :create
  post "/pagerduty/:organization_id", PagerdutyWebhookController, :create
end
```

These endpoints are unauthenticated (signature verification happens in the controller after the request is processed). At 1000 requests/minute, an attacker can:
- Force the server to perform 1000 database lookups per minute (to find the integration and its webhook secret)
- Force 1000 HMAC computations per minute
- The `organization_id` in the URL can be enumerated to discover valid organizations

**Impact:** Organization enumeration and moderate DoS potential through resource consumption.

**Suggested Fix:**
Apply stricter rate limiting (e.g., 60 req/min) to webhook endpoints:

```elixir
pipeline :rate_limit_webhook do
  plug RateLimit, type: :webhook  # Add a :webhook type with lower limits
end
```

---

### M4: Stripe Webhook Raw Body May Be Empty

**File:** `lib/towerops_web/controllers/api/v1/stripe_webhook_controller.ex` — line 14

**Description:**
The Stripe webhook controller reads the raw body with a fallback to empty string:

```elixir
raw_body = conn.private[:raw_body] || ""
```

If `raw_body` is empty (e.g., due to a middleware issue or the body being consumed), `verify_signature/2` would still attempt verification against an empty payload. The `parse_signature_header/1` would crash on `nil` input if the `stripe-signature` header is also missing (since `List.first([])` returns `nil`, and `String.split(nil, ",")` raises).

**Impact:** Unhandled exception could leak stack traces in non-production error responses, and the empty body fallback could mask misconfiguration issues.

**Suggested Fix:**
Add explicit nil/empty checks:

```elixir
def create(conn, _params) do
  with signature when is_binary(signature) <- get_req_header(conn, "stripe-signature") |> List.first(),
       raw_body when is_binary(raw_body) and raw_body != "" <- conn.private[:raw_body] do
    # ... process
  else
    nil -> conn |> put_status(400) |> json(%{error: "Missing signature header"})
    "" -> conn |> put_status(400) |> json(%{error: "Empty request body"})
  end
end
```

---

### M5: `to_atom_keys` in IntegrationsController Could Crash on Invalid Keys

**File:** `lib/towerops_web/controllers/api/v1/integrations_controller.ex` — lines 89-93

**Description:**
The `to_atom_keys/1` helper converts user-supplied string keys to atoms using `String.to_existing_atom/1`:

```elixir
defp to_atom_keys(map) when is_map(map) do
  Map.new(map, fn {k, v} -> {String.to_existing_atom(k), v} end)
rescue
  ArgumentError -> map
end
```

While `String.to_existing_atom/1` is safe (won't create new atoms), the `rescue` clause catches `ArgumentError` and returns the original map with string keys. This means:
1. If ANY key is not an existing atom, ALL keys remain as strings (including valid ones)
2. The changeset then receives a mix of expected atom keys and unexpected string keys, potentially silently dropping valid fields

This is a correctness bug more than a security issue, but it could lead to integration updates silently failing to apply credential changes.

**Suggested Fix:**
Convert keys individually, falling back per-key:

```elixir
defp to_atom_keys(map) when is_map(map) do
  Map.new(map, fn {k, v} ->
    key = try do
      String.to_existing_atom(k)
    rescue
      ArgumentError -> k
    end
    {key, v}
  end)
end
```

---

## LOW Severity

### L1: API Token Verification Uses Database Lookup Instead of Constant-Time Comparison

**File:** `lib/towerops/api_tokens.ex` — lines 79-97

**Description:**
API token verification hashes the input token and does a database lookup:

```elixir
def verify_token(raw_token) do
  token_hash = hash_token(raw_token)
  case Repo.get_by(ApiToken, token_hash: token_hash) do
    nil -> {:error, :invalid_token}
    token -> ...
  end
end
```

This is the standard and correct approach for hashed tokens — database lookup timing is dominated by query execution time, making timing attacks impractical. The hash comparison happens in PostgreSQL which uses constant-time comparison for indexed lookups.

However, the SHA-256 hash is not salted. While this doesn't enable practical attacks (tokens are 32 random bytes with ~256 bits of entropy), salted hashing (e.g., bcrypt) would provide defense-in-depth if the database were compromised.

**Impact:** Theoretical — if the database is breached, unsalted SHA-256 hashes of tokens are exposed. Given the high entropy of the tokens, brute-forcing is infeasible.

**No immediate action required.** Note for future consideration.

---

### L2: Mobile Session Token Lookup Timing

**File:** `lib/towerops/mobile_sessions.ex` — lines 38-47

**Description:**
Mobile session tokens are hashed and looked up via database query:

```elixir
def get_session_by_token(token) when is_binary(token) do
  hashed = MobileSession.hash_token(token)
  MobileSession
  |> where([s], s.token == ^hashed)
  |> where([s], s.expires_at > ^now)
  |> Repo.one()
end
```

Same pattern as API tokens — hash-then-lookup. This is secure for the same reasons as L1.

**No action required.**

---

### L3: Gaiia Webhook Signature Mismatch Logging Leaks Partial Signatures

**File:** `lib/towerops_web/controllers/api/v1/gaiia_webhook_controller.ex` — lines 96-103

**Description:**
On signature mismatch, the controller logs partial signature values:

```elixir
Logger.warning(
  "Gaiia webhook signature mismatch — " <>
    "ts=#{timestamp} body_len=#{byte_size(raw_body)} " <>
    "secret_len=#{byte_size(secret)} " <>
    "expected=#{String.slice(expected, 0, 12)}… " <>
    "received=#{String.slice(v1_signature, 0, 12)}…"
)
```

This logs:
- The length of the webhook secret (`secret_len`)
- The first 12 characters of the expected HMAC signature

While the HMAC output is not the secret itself, logging the secret length reveals information that could help an attacker. The first 12 hex chars of the expected signature provide limited information (6 bytes of a 32-byte HMAC).

**Impact:** Minor information leakage in logs. Not directly exploitable but violates principle of least information.

**Suggested Fix:**
Remove `secret_len` and `expected` from the log message:

```elixir
Logger.warning(
  "Gaiia webhook signature mismatch — " <>
    "ts=#{timestamp} body_len=#{byte_size(raw_body)}"
)
```

---

## Positive Findings (No Issues)

These areas were reviewed and found to be properly secured:

1. **Webhook signature verification:** All three webhook handlers (Stripe, Gaiia, PagerDuty) use `Plug.Crypto.secure_compare/2` for constant-time signature comparison. ✅

2. **Agent webhook auth:** Uses `Plug.Crypto.secure_compare/2` against a shared secret. ✅

3. **Rate limiting:** Applied to auth endpoints (10/min), API endpoints (1000/min), and admin browser routes (100/min). ✅

4. **GraphQL depth/complexity limits:** Router configures `max_depth: 10` and `max_complexity: 500` with `analyze_complexity: true`. ✅ (But see M1 about context plug overwriting in non-prod.)

5. **Credentials at rest:** Integration credentials use `Encrypted.Map` (Cloak AES-256-GCM). ✅

6. **CSRF protection:** Browser pipeline includes `:protect_from_forgery`. ✅

7. **Security headers:** CSP, frame-ancestors, and secure browser headers configured. ✅

8. **MIB upload path traversal protection:** Vendor names validated with regex, extracted archive paths validated against extraction directory. ✅

9. **Oban worker error handling:** Workers use `max_attempts` configuration and return `:ok` on non-retryable errors, `{:error, reason}` on retryable errors. No infinite retry loops found. ✅

10. **Organization scoping:** API controllers consistently scope queries to `conn.assigns.current_organization_id`. ✅

11. **Integration client error handling:** All HTTP clients (VISP, Sonar, Splynx, Gaiia, Preseem) handle error responses gracefully with pattern matching and don't leak credentials in error messages. ✅

12. **Token generation:** Uses `:crypto.strong_rand_bytes(32)` for API tokens — cryptographically secure. ✅