Remove incorrect H1 finding from findings.md
The :env config key IS properly set in all environments: - config/test.exs:112 sets :test - config/dev.exs:216 sets :dev - config/runtime.exs:461 sets :prod Application.get_env(:towerops, :env) works correctly at runtime.
This commit is contained in:
parent
4421727e61
commit
b07d42bcba
1 changed files with 303 additions and 0 deletions
303
findings.md
Normal file
303
findings.md
Normal file
|
|
@ -0,0 +1,303 @@
|
|||
# Codebase Findings
|
||||
|
||||
## HIGH
|
||||
|
||||
### H1. Hardcoded email bypasses security validation
|
||||
|
||||
**File**: `lib/towerops_web/live/device_live/form.ex:779`
|
||||
|
||||
```elixir
|
||||
assigns.current_scope.user.email == "graham@mcintire.me"
|
||||
```
|
||||
|
||||
A personal email address is hardcoded to bypass the non-routable IP check for cloud pollers. If anyone registers with this email, they bypass IP validation. Should use a proper configuration flag or user attribute (e.g., `developer?` boolean).
|
||||
|
||||
---
|
||||
|
||||
### H2. `String.to_atom/1` on user-supplied parameter keys
|
||||
|
||||
**File**: `lib/towerops_web/controllers/api/v1/integrations_controller.ex:127`
|
||||
|
||||
```elixir
|
||||
{String.to_atom(k), v}
|
||||
```
|
||||
|
||||
Converts user-supplied parameter keys directly to atoms. This is an atom table DoS vector — an attacker can send requests with unique parameter names and exhaust the VM atom table. Use `String.to_existing_atom/1` or a whitelist-based approach.
|
||||
|
||||
---
|
||||
|
||||
### H3. Missing `@impl true` on callback implementations
|
||||
|
||||
Multiple `handle_event` and `handle_info` callbacks are missing `@impl true`. The project compiles with `--warnings-as-errors`, so this will cause compilation failures.
|
||||
|
||||
**`lib/towerops_web/live/alert_live/index.ex`** — missing `@impl true` on:
|
||||
- Line 65: `handle_event("resolve", ...)`
|
||||
- Line 89: `handle_event("toggle_alert", ...)`
|
||||
- Line 94: `handle_event("toggle_select", ...)`
|
||||
- Line 105: `handle_event("select_all", ...)`
|
||||
- Line 110: `handle_event("select_none", ...)`
|
||||
- Line 114: `handle_event("bulk_acknowledge", ...)`
|
||||
- Line 135: `handle_event("bulk_resolve", ...)`
|
||||
- Line 155: `handle_event("toggle_site", ...)`
|
||||
- Line 194: `handle_info(_msg, socket)` (catch-all)
|
||||
|
||||
**`lib/towerops_web/live/dashboard_live.ex:87`** — missing `@impl true` on:
|
||||
- Line 87: `handle_info({:alert_changed, _org_id}, socket)`
|
||||
|
||||
---
|
||||
|
||||
## MEDIUM
|
||||
|
||||
### M1. Direct `changeset.changes` access instead of `Ecto.Changeset.get_field/2`
|
||||
|
||||
**File**: `lib/towerops_web/live/device_live/form.ex:321,352,383`
|
||||
|
||||
The project guidelines explicitly require using `Ecto.Changeset.get_field/2` to access changeset fields, but `changeset.changes` is accessed directly:
|
||||
- Line 321: `build_device_snmpv3_creds(form_data, changeset.changes)`
|
||||
- Line 352: `Map.get(changes, :snmpv3_auth_password)`
|
||||
- Line 383: `changes = changeset.changes`
|
||||
|
||||
Also in `lib/towerops/admin.ex:241`: `changes: changeset.changes`
|
||||
|
||||
---
|
||||
|
||||
### M2. N+1 query: 4 separate count queries for alert counts
|
||||
|
||||
**File**: `lib/towerops_web/live/alert_live/index.ex:207-211`
|
||||
|
||||
```elixir
|
||||
counts = %{
|
||||
all: Alerts.count_organization_alerts(organization_id),
|
||||
critical: Alerts.count_organization_alerts(organization_id, "critical"),
|
||||
unresolved: Alerts.count_organization_alerts(organization_id, "unresolved"),
|
||||
resolved: Alerts.count_organization_alerts(organization_id, "resolved")
|
||||
}
|
||||
```
|
||||
|
||||
Each call executes a separate `SELECT COUNT(*)`. A single query with `COUNT(*) FILTER(WHERE ...)` or `CASE WHEN` aggregation could return all counts in one roundtrip.
|
||||
|
||||
---
|
||||
|
||||
### M3. N+1 in bulk alert operations
|
||||
|
||||
**File**: `lib/towerops_web/live/alert_live/index.ex:118-126` (bulk_acknowledge), `139-146` (bulk_resolve)
|
||||
|
||||
```elixir
|
||||
Enum.reduce(socket.assigns.selected_alerts, 0, fn id, acc ->
|
||||
with {:ok, alert} <- AccessControl.verify_alert_access(id, organization.id),
|
||||
{:ok, _} <- Alerts.acknowledge_alert(alert, user_id) do
|
||||
acc + 1
|
||||
end
|
||||
end)
|
||||
```
|
||||
|
||||
If a user selects 100 alerts, this makes 100 `verify_alert_access` queries + 100 `acknowledge_alert` calls. A single `WHERE id IN (^ids)` query would be far more efficient.
|
||||
|
||||
---
|
||||
|
||||
### M4. No URL parameter validation in `DashboardLive.handle_params`
|
||||
|
||||
**File**: `lib/towerops_web/live/dashboard_live.ex:37`
|
||||
|
||||
```elixir
|
||||
insight_source = params["insight_source"]
|
||||
```
|
||||
|
||||
The `insight_source` param is assigned directly without validation against allowed values (`preseem`, `snmp`, `gaiia`, `system`). While downstream code likely handles invalid values gracefully, this is inconsistent with the project's URL-sync guidelines.
|
||||
|
||||
---
|
||||
|
||||
### M5. `Ecto.UUID.dump/1` pattern match can crash
|
||||
|
||||
**File**: `lib/towerops_web/live/device_live/show.ex:938-939`
|
||||
|
||||
```elixir
|
||||
{:ok, iid} = Ecto.UUID.dump(interface_id)
|
||||
{:ok, oid} = Ecto.UUID.dump(org_id)
|
||||
```
|
||||
|
||||
If `interface_id` or `org_id` is not a valid UUID (e.g., race condition where device was deleted between access check and this call), `dump/1` returns `:error`, causing a `MatchError` crash.
|
||||
|
||||
---
|
||||
|
||||
### M6. `Repo.query!` in LiveView context
|
||||
|
||||
**File**: `lib/towerops_web/live/device_live/show.ex:942`
|
||||
|
||||
```elixir
|
||||
result = Towerops.Repo.query!(...)
|
||||
```
|
||||
|
||||
The `!` variant raises on any database error, crashing the LiveView process. A non-bang variant with proper error handling would be more resilient.
|
||||
|
||||
---
|
||||
|
||||
### M7. Smoke tests use wrong query parameter names
|
||||
|
||||
**File**: `test/towerops_web/live/smoke_test.exs:174,187`
|
||||
|
||||
```elixir
|
||||
test "GET /alerts with query filter", %{conn: conn} do
|
||||
assert {:ok, _view, _html} = live(conn, ~p"/alerts?severity=critical")
|
||||
end
|
||||
```
|
||||
|
||||
The test uses `severity=critical` but `AlertLive.Index.handle_params/3` reads `filter`, not `severity`. Similarly, line 187 uses `?status=resolved` but the handler reads `filter=resolved`. Tests pass because `live/2` just mounts, but the params are silently ignored.
|
||||
|
||||
---
|
||||
|
||||
### M8. Double query in token deletion
|
||||
|
||||
**File**: `lib/towerops/accounts.ex:389-390`
|
||||
|
||||
```elixir
|
||||
tokens_to_expire = Repo.all(from t in UserToken, where: t.user_id == ^user.id)
|
||||
Repo.delete_all(from(t in UserToken, where: t.id in ^Enum.map(tokens_to_expire, & &1.id)))
|
||||
```
|
||||
|
||||
This SELECTs all tokens just to extract IDs, then DELETEs by those IDs. A single `Repo.delete_all` with the user_id condition suffices.
|
||||
|
||||
---
|
||||
|
||||
### M9. N+1 risk: `membership.user` without preload
|
||||
|
||||
**File**: `lib/towerops/billing/billing_notifier.ex:52`
|
||||
|
||||
```elixir
|
||||
memberships |> Enum.map(fn m -> m.user.email end)
|
||||
```
|
||||
|
||||
If `membership` records are not preloaded with `:user`, each access triggers a separate query.
|
||||
|
||||
Also in `lib/towerops_web/controllers/api/v1/members_controller.ex:96` and `lib/towerops_web/graphql/types/member.ex:15`.
|
||||
|
||||
---
|
||||
|
||||
### M10. Process dictionary for cross-component state
|
||||
|
||||
**Files**:
|
||||
- `lib/towerops_web/user_auth.ex:611,1085` — `Process.put(:unresolved_alert_count, ...)`
|
||||
- `lib/towerops_web/components/layouts.ex:124` — `Process.get(:unresolved_alert_count, 0)`
|
||||
|
||||
The `:unresolved_alert_count` is stored in the process dictionary and read from a layout component. The project guidelines state "Using the process dictionary is typically a sign of unidiomatic code." A socket assign would be cleaner and less fragile.
|
||||
|
||||
---
|
||||
|
||||
### M11. Many `list_*` functions lack limits
|
||||
|
||||
At least 18 `list_*` functions across context modules return `Repo.all` results without any limit, risking unbounded result sets:
|
||||
|
||||
- `lib/towerops/reports.ex:19` — `list_reports/1`
|
||||
- `lib/towerops/sites.ex` — `list_sites_with_coordinates/0`, `list_organization_sites/1`, `list_root_sites/1`
|
||||
- `lib/towerops/devices.ex` — `list_site_devices/1`, `list_monitored_devices/0`, `list_snmp_enabled_devices/0`, `list_mikrotik_devices_with_api/0`
|
||||
- `lib/towerops/agents.ex` — `list_all_agent_tokens/0`, `list_cloud_pollers/0`, `list_updatable_agents/0`, `list_online_cloud_pollers/0`, `list_cloud_polled_devices/0`
|
||||
- `lib/towerops/organizations.ex` — `list_organization_ids/0`, `list_user_organizations/1`, `list_organizations_with_active_subscriptions/0`
|
||||
- `lib/towerops/monitoring.ex` — `list_checks/2`, `list_checks_for_agent/2`
|
||||
|
||||
---
|
||||
|
||||
### M12. `DashboardLive` never unsubscribes from PubSub
|
||||
|
||||
**File**: `lib/towerops_web/live/dashboard_live.ex:18-22`
|
||||
|
||||
`DashboardLive` subscribes to PubSub topics in `mount` but has no cleanup. Phoenix auto-cleans subscriptions on process termination, but if the LiveView is reused across navigations within the same process (LiveView's default behavior), subscriptions accumulate.
|
||||
|
||||
---
|
||||
|
||||
### M13. Missing `@doc false` or unnecessary `@doc false` on public functions
|
||||
|
||||
**File**: `lib/towerops_web/live/alert_live/index.ex`
|
||||
|
||||
Functions like `filter_alerts/2`, `sort_alerts/3`, `severity_weight/1`, `severity_color/1`, `age_severity_color/1`, `format_age_minutes/1`, `format_number/1`, `duration_text/1`, `format_duration_minutes/1`, `get_subscriber_count/2`, `get_site_subscriber_count/1` are marked `@doc false` but are used only within the module. These should be `defp` instead of `def`.
|
||||
|
||||
---
|
||||
|
||||
## LOW
|
||||
|
||||
### L1. `then/2` used for socket rebind instead of explicit assignment
|
||||
|
||||
**File**: `lib/towerops_web/live/schedule_live/index.ex:188`
|
||||
|
||||
```elixir
|
||||
|> then(&{:noreply, &1})
|
||||
```
|
||||
|
||||
This is a style issue — explicit assignment would be clearer.
|
||||
|
||||
---
|
||||
|
||||
### L2. `Process.sleep(2000)` in application startup
|
||||
|
||||
**File**: `lib/towerops/application.ex:165`
|
||||
|
||||
A 2-second `Process.sleep` in `post_startup` before running `JobCleanupTask` delays application startup for every environment, including production restarts.
|
||||
|
||||
---
|
||||
|
||||
### L3. `Process.sleep` in device deletion
|
||||
|
||||
**File**: `lib/towerops/devices.ex:878`
|
||||
|
||||
```elixir
|
||||
Process.sleep(deletion_delay_ms)
|
||||
```
|
||||
|
||||
Sleeping during a device deletion blocks the calling process. If this is called from a LiveView or web request, it blocks the process for the duration of the sleep.
|
||||
|
||||
---
|
||||
|
||||
### L4. `Scope.for_user(nil)` returns `nil`
|
||||
|
||||
**File**: `lib/towerops/accounts/scope.ex:39`
|
||||
|
||||
```elixir
|
||||
def for_user(nil), do: nil
|
||||
```
|
||||
|
||||
This means `socket.assigns.current_scope` can be `nil` for unauthenticated users. Most callers handle this with `&&` guards, but it's a fragile pattern — one nil access on `.user` and the LiveView crashes.
|
||||
|
||||
---
|
||||
|
||||
|
||||
|
||||
### L6. `favicon_status/2` in DashboardLive unused argument
|
||||
|
||||
**File**: `lib/towerops_web/live/dashboard_live.ex:267`
|
||||
|
||||
```elixir
|
||||
defp favicon_status(devices_down, _alert_count) when devices_down > 0, do: "red"
|
||||
```
|
||||
|
||||
The second argument `_alert_count` is unused in all but the middle clause, suggesting the function could be simplified.
|
||||
|
||||
---
|
||||
|
||||
## INFO
|
||||
|
||||
### I1. 55 Oban worker modules — consider organizing
|
||||
|
||||
**Pattern**: The workers directory has 55 modules. Some have grown large (`discovery_worker.ex` at 522 lines). Consider splitting into subdirectories by domain (monitoring, sync, maintenance, etc.).
|
||||
|
||||
### I2. Heavy dashboard data load on every navigation
|
||||
|
||||
**File**: `lib/towerops_web/live/dashboard_live.ex:109-187`
|
||||
|
||||
`load_dashboard_data/2` fetches alerts, devices, sites, subscribers, impact data, incidents, activity feed, insights, config changes, and a setup checklist on every `handle_params`. For heavily used routes, consider caching or lazy loading some sections.
|
||||
|
||||
### I3. SNMP discovery module is very large
|
||||
|
||||
**File**: `lib/towerops/snmp/discovery.ex` — 1745+ lines
|
||||
|
||||
This single module is the largest in the codebase. Per the project guidelines on module complexity, it should be broken into focused sub-modules.
|
||||
|
||||
### I4. `device_live/show.ex` is very large
|
||||
|
||||
The device show LiveView is likely the largest LiveView in the codebase. Consider extracting helper modules for chart builders, data loaders, and formatters (some already exist in `live/device_live/helpers/`).
|
||||
|
||||
### I5. `.dialyzer_ignore.exs` is clean
|
||||
|
||||
Only dependency and vendored-code suppressions — no app-level warnings are suppressed. Good practice.
|
||||
|
||||
### I6. No bare `rescue _ ->` catch-all blocks
|
||||
|
||||
The codebase properly avoids silent error swallowing. Good practice.
|
||||
Loading…
Add table
Reference in a new issue