diff --git a/bugs.md b/bugs.md
index aed7ac11..79979a65 100644
--- a/bugs.md
+++ b/bugs.md
@@ -1,23 +1,36 @@
# Bugs And Risk Findings
-Review date: 2026-05-11
+Review date: 2026-05-12 (all issues fixed 2026-05-12)
Scope: application-owned Elixir/Phoenix code, templates, config, deployment manifests, scripts, and dependency manifests. Vendored code and bundled third-party MIB data were not reviewed as first-party code.
Validation run:
-- `mix credo --strict`: passed, no issues.
-- `mix deps.audit`: passed, no vulnerabilities found.
-- `mix hex.audit`: passed, no retired packages found.
-- `npm audit --prefix e2e --audit-level=low`: passed, 0 vulnerabilities.
+- `mix test`: passed, 12,668 tests, 0 failures, 59 skipped, 235 excluded.
-## Medium
+## Fixed
-### 6. CSP is duplicated and weakened by inline scripts
+### 1. Users can mint API tokens for organizations they do not belong to (HIGH)
-- Category: OWASP A05 Security Misconfiguration / A03 Injection defense in depth
-- Evidence: `lib/towerops_web/router.ex:21`, `lib/towerops_web/plugs/security_headers.ex:20`, `lib/towerops_web/components/layouts/root.html.heex:35`, `lib/towerops_web/components/layouts/root.html.heex:71`, `lib/towerops_web/components/layouts/root.html.heex:80`
-- Problem: CSP is set in both the browser pipeline and endpoint-level security plug, with different directives. The effective behavior depends on header overwrite order. The policy also requires `'unsafe-inline'` because templates include inline scripts, reducing CSP's value against XSS.
-- Fix: centralize CSP construction in one plug. Move inline scripts into `assets/js/app.js` or LiveView hooks and remove `'unsafe-inline'` where possible. Consider nonces only for unavoidable inline bootstrapping.
-- Deferred: requires careful browser-compatibility testing across all LiveView pages.
+- Fix: Added `validate_membership/2` in `ApiTokens.create_api_token/1` that checks the user is a member of the organization before creating a token. Added test for unauthorized token creation rejection.
+- Files: `lib/towerops/api_tokens.ex`, `test/towerops/api_tokens_test.exs`, `test/towerops_web/live/user_settings_live/api_token_manager_test.exs`
+### 2. Admin security allowlist form crashes because it reads `current_user` (MEDIUM)
+
+- Fix: Changed `socket.assigns.current_user` to `socket.assigns.current_scope.user` in the `add_whitelist` event handler.
+- File: `lib/towerops_web/live/admin/security_live/index.ex`
+
+### 3. Hidden archive entries bypass MIB upload validation (MEDIUM)
+
+- Fix: Replaced `Path.wildcard` (which excludes dotfiles) with recursive `File.ls/1` via `list_all_entries/1`. Updated `check_no_symlinks/1` to use `File.lstat/1` instead of `File.read_link/1`.
+- File: `lib/towerops_web/controllers/api/v1/mib_controller.ex`
+
+### 4. ZIP MIB uploads are extracted before checking upload size (MEDIUM)
+
+- Fix: Added `check_upload_size/1` call before extraction in `extract_zip/3`, matching the tarball path.
+- File: `lib/towerops_web/controllers/api/v1/mib_controller.ex`
+
+### 5. CSP is duplicated and weakened by inline scripts (MEDIUM)
+
+- Fix: Removed CSP from router's `put_secure_browser_headers` (centralized in `SecurityHeaders` plug). Replaced `'unsafe-inline'` with per-request nonces on all inline scripts. Added nonce attributes to inline scripts in root layout and API docs template.
+- Files: `lib/towerops_web/router.ex`, `lib/towerops_web/plugs/security_headers.ex`, `lib/towerops_web/components/layouts/root.html.heex`, `lib/towerops_web/controllers/api_docs_html/index.html.heex`, `test/towerops_web/plugs/security_headers_test.exs`
diff --git a/lib/towerops/api_tokens.ex b/lib/towerops/api_tokens.ex
index 58453f7e..58517842 100644
--- a/lib/towerops/api_tokens.ex
+++ b/lib/towerops/api_tokens.ex
@@ -34,14 +34,27 @@ defmodule Towerops.ApiTokens do
attrs = Map.put(attrs, :token_hash, token_hash)
- case %ApiToken{}
- |> ApiToken.changeset(attrs)
- |> Repo.insert() do
- {:ok, api_token} ->
- {:ok, {api_token, raw_token}}
+ with :ok <- validate_membership(attrs[:organization_id], attrs[:user_id]) do
+ case %ApiToken{}
+ |> ApiToken.changeset(attrs)
+ |> Repo.insert() do
+ {:ok, api_token} ->
+ {:ok, {api_token, raw_token}}
- {:error, changeset} ->
- {:error, changeset}
+ {:error, changeset} ->
+ {:error, changeset}
+ end
+ end
+ end
+
+ defp validate_membership(_org_id, nil), do: :ok
+ defp validate_membership(nil, _user_id), do: {:error, "organization_id is required"}
+
+ defp validate_membership(org_id, user_id) do
+ if Towerops.Organizations.get_membership(org_id, user_id) do
+ :ok
+ else
+ {:error, "user is not a member of this organization"}
end
end
diff --git a/lib/towerops_web/components/layouts/root.html.heex b/lib/towerops_web/components/layouts/root.html.heex
index 309222ff..d8dbc78f 100644
--- a/lib/towerops_web/components/layouts/root.html.heex
+++ b/lib/towerops_web/components/layouts/root.html.heex
@@ -32,7 +32,7 @@
-
-
-
diff --git a/lib/towerops_web/controllers/api/v1/mib_controller.ex b/lib/towerops_web/controllers/api/v1/mib_controller.ex
index ee7e1105..184740b8 100644
--- a/lib/towerops_web/controllers/api/v1/mib_controller.ex
+++ b/lib/towerops_web/controllers/api/v1/mib_controller.ex
@@ -251,50 +251,52 @@ defmodule ToweropsWeb.Api.V1.MibController do
end
defp extract_zip(conn, upload, vendor_dir, vendor) do
- # Extract to temporary directory first to validate contents
- random_suffix = 16 |> :crypto.strong_rand_bytes() |> Base.encode16(case: :lower)
- temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{random_suffix}")
- File.mkdir_p!(temp_dir)
+ case check_upload_size(upload) do
+ :ok ->
+ random_suffix = 16 |> :crypto.strong_rand_bytes() |> Base.encode16(case: :lower)
+ temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{random_suffix}")
+ File.mkdir_p!(temp_dir)
- try do
- case System.cmd("unzip", ["-o", upload.path, "-d", temp_dir]) do
- {_output, 0} ->
- # Validate all extracted paths are safe (no directory traversal)
- case validate_extracted_paths(temp_dir) do
- :ok ->
- # Safe to copy to vendor directory - vendor_dir constructed from validated vendor name
- # and all paths in temp_dir have been validated by validate_extracted_paths/1
- _ = File.cp_r!(temp_dir, vendor_dir)
- files_count = count_files(vendor_dir)
- Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
- invalidate_mib_cache()
+ try do
+ case System.cmd("unzip", ["-o", upload.path, "-d", temp_dir]) do
+ {_output, 0} ->
+ case validate_extracted_paths(temp_dir) do
+ :ok ->
+ _ = File.cp_r!(temp_dir, vendor_dir)
+ files_count = count_files(vendor_dir)
+ Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
+ invalidate_mib_cache()
- conn
- |> put_status(:created)
- |> json(%{
- status: "ok",
- message: "Successfully extracted MIB archive",
- vendor: vendor,
- files_count: files_count
- })
+ conn
+ |> put_status(:created)
+ |> json(%{
+ status: "ok",
+ message: "Successfully extracted MIB archive",
+ vendor: vendor,
+ files_count: files_count
+ })
- {:error, reason} ->
- Logger.warning("Archive validation failed for vendor #{vendor}: #{reason}")
+ {:error, reason} ->
+ Logger.warning("Archive validation failed for vendor #{vendor}: #{reason}")
+
+ conn
+ |> put_status(:bad_request)
+ |> json(%{error: reason})
+ end
+
+ {error, exit_code} ->
+ Logger.error("Failed to extract zip: #{error}")
conn
|> put_status(:bad_request)
- |> json(%{error: reason})
+ |> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
end
+ after
+ File.rm_rf!(temp_dir)
+ end
- {error, exit_code} ->
- Logger.error("Failed to extract zip: #{error}")
-
- conn
- |> put_status(:bad_request)
- |> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
- end
- after
- File.rm_rf!(temp_dir)
+ {:error, reason} ->
+ conn |> put_status(:bad_request) |> json(%{error: reason})
end
end
@@ -409,17 +411,34 @@ defmodule ToweropsWeb.Api.V1.MibController do
end
defp count_files(dir) do
+ dir |> list_all_entries() |> Enum.count(&File.regular?/1)
+ end
+
+ # Recursively list all entries including hidden files and directories.
+ defp list_all_entries(dir) do
dir
- |> Path.join("**/*")
- |> Path.wildcard()
- |> Enum.count(&File.regular?/1)
+ |> list_dir_entries()
+ |> Enum.flat_map(fn path ->
+ case File.lstat(path) do
+ {:ok, stat} when stat.type == :directory -> [path | list_all_entries(path)]
+ _ -> [path]
+ end
+ end)
+ end
+
+ defp list_dir_entries(dir) do
+ case File.ls(dir) do
+ {:ok, entries} -> Enum.map(entries, &Path.join(dir, &1))
+ {:error, _} -> []
+ end
end
# Validate that extracted archive contents don't contain path traversal attacks,
# symlinks, or excessive file counts/sizes.
+ # Uses list_all_entries/1 which includes hidden files/directories (unlike Path.wildcard).
defp validate_extracted_paths(extract_dir) do
canonical_extract_dir = Path.expand(extract_dir)
- all_paths = extract_dir |> Path.join("**/*") |> Path.wildcard()
+ all_paths = list_all_entries(extract_dir)
with :ok <- check_no_symlinks(all_paths),
:ok <- check_no_traversal(all_paths, canonical_extract_dir) do
@@ -432,13 +451,17 @@ defmodule ToweropsWeb.Api.V1.MibController do
end
defp check_no_symlinks(paths) do
- if Enum.any?(paths, &match?({:ok, _}, File.read_link(&1))) do
+ if Enum.any?(paths, &symlink?/1) do
{:error, "Archive contains symlinks, which are not allowed"}
else
:ok
end
end
+ defp symlink?(path) do
+ match?({:ok, %{type: :symlink}}, File.lstat(path))
+ end
+
defp check_no_traversal(paths, canonical_base) do
if Enum.all?(paths, fn path ->
path |> Path.expand() |> String.starts_with?(canonical_base)
diff --git a/lib/towerops_web/controllers/api_docs_html/index.html.heex b/lib/towerops_web/controllers/api_docs_html/index.html.heex
index c09086e3..81c52fb1 100644
--- a/lib/towerops_web/controllers/api_docs_html/index.html.heex
+++ b/lib/towerops_web/controllers/api_docs_html/index.html.heex
@@ -4752,7 +4752,7 @@ terraform destroy
-