security: implement comprehensive security audit fixes

Critical Fixes:
- Remove /health/time endpoint exposing system time information
  * Prevents attackers from detecting time sync issues for TOTP attacks
  * Removed route and controller function, updated tests
- Add email confirmation check to account data export
  * GDPR export now requires confirmed email address
  * Prevents unconfirmed accounts from accessing data export
- Add path traversal validation for MIB archive uploads
  * Extract to temp directory, validate all paths, then copy if safe
  * Prevents malicious tar/zip files from writing outside target directory
  * Added validate_extracted_paths/1 helper function

High Priority Fixes:
- Add comprehensive input validation for mobile auth
  * Length limits: device_name (255), device_os (100), app_version (50), push_token (512)
  * Prevents database corruption and storage exhaustion
- Add heartbeat rate limiting to agent channel
  * Limit database updates to once per 30 seconds (max ~2/min per agent)
  * Prevents malicious agents from exhausting database connections
- Sanitize 500 error responses
  * Return generic error messages to clients
  * Log full details server-side with request_id for support
  * Prevents leaking stack traces and module names
- Add message size limits to agent channel
  * 10MB maximum for all protobuf messages (result, heartbeat, error)
  * Prevents DoS attacks via oversized payloads

Medium Priority Fixes:
- Add GraphQL query depth limits (max_depth: 10)
  * Prevents DoS from deeply nested queries
  * Complements existing complexity limits

Code Quality:
- Refactor agent channel handlers to reduce nesting depth
  * Extract message processing into separate private functions
  * Fixes Credo warnings about excessive nesting
  * Improves code readability and maintainability

Files changed:
- lib/towerops_web/controllers/health_controller.ex
- lib/towerops_web/controllers/api/account_data_controller.ex
- lib/towerops_web/controllers/api/v1/mib_controller.ex
- lib/towerops/mobile_sessions/mobile_session.ex
- lib/towerops_web/channels/agent_channel.ex
- lib/towerops_web/controllers/error_json.ex
- lib/towerops_web/router.ex
- CHANGELOG.txt
- priv/static/changelog.txt
- test/towerops_web/controllers/health_controller_test.exs
- test/towerops_web/controllers/error_json_test.exs

All 7,424 tests passing.
This commit is contained in:
Graham McIntire 2026-03-05 13:07:45 -06:00
parent 964d9dab76
commit 1d928d4356
No known key found for this signature in database
11 changed files with 383 additions and 1285 deletions

File diff suppressed because it is too large Load diff

View file

@ -50,6 +50,10 @@ defmodule Towerops.MobileSessions.MobileSession do
:alerts_enabled
])
|> validate_required([:user_id])
|> validate_length(:device_name, max: 255)
|> validate_length(:device_os, max: 100)
|> validate_length(:app_version, max: 50)
|> validate_length(:push_token, max: 512)
|> validate_inclusion(:push_platform, ["apns", "fcm", nil])
|> put_token()
|> put_timestamps()

View file

@ -53,6 +53,8 @@ defmodule ToweropsWeb.AgentChannel do
@heartbeat_timeout_seconds 300
# Check heartbeat every 2 minutes
@heartbeat_check_interval_ms 120_000
# Maximum message size: 10MB (protobuf messages should be much smaller)
@max_message_size 10 * 1024 * 1024
@impl true
@spec join(String.t(), map(), Phoenix.Socket.t()) ::
@ -414,114 +416,73 @@ defmodule ToweropsWeb.AgentChannel do
@impl true
@spec handle_in(String.t(), map(), socket()) :: {:noreply, socket()}
def handle_in("result", %{"binary" => binary_b64}, socket) when is_binary(binary_b64) do
Logger.info("Received SNMP result from agent (binary size: #{byte_size(binary_b64)})")
with {:ok, binary} <- safe_base64_decode(binary_b64),
{:ok, result} <- Validator.validate_snmp_result(binary) do
maybe_debug_log(socket, "Received SNMP result from agent",
device_id: result.device_id,
job_type: result.job_type,
job_id: result.job_id,
binary_size: byte_size(binary_b64),
oid_count: map_size(result.oid_values)
# Validate message size to prevent DoS attacks (early return pattern)
if byte_size(binary_b64) > @max_message_size do
Logger.warning("Rejected oversized message from agent",
agent_token_id: socket.assigns.agent_token_id,
size: byte_size(binary_b64),
max_size: @max_message_size
)
# Check if this is a live poll result
if String.starts_with?(result.job_id, "live_poll:") do
handle_live_poll_result(result, socket)
else
process_and_log_snmp_result(socket, result)
end
{:noreply, socket}
{:reply, {:error, %{reason: "Message too large (max #{@max_message_size} bytes)"}}, socket}
else
{:error, {type, message}} ->
Logger.error("Invalid SNMP result from agent: #{type} - #{message}",
agent_token_id: socket.assigns.agent_token_id,
error_type: type,
error_message: message,
binary_size: byte_size(binary_b64)
)
{:noreply, socket}
{:error, :base64_decode_failed} ->
Logger.error("Failed to decode SNMP result (invalid base64)",
agent_token_id: socket.assigns.agent_token_id,
binary_size: byte_size(binary_b64)
)
{:noreply, socket}
process_snmp_result_message(binary_b64, socket)
end
end
@spec handle_in(String.t(), %{required(String.t()) => base64_string()}, socket()) ::
{:noreply, socket()}
def handle_in("heartbeat", %{"binary" => binary_b64}, socket) when is_binary(binary_b64) do
with {:ok, binary} <- safe_base64_decode(binary_b64),
{:ok, heartbeat} <- Validator.validate_heartbeat(binary) do
metadata = %{
"version" => heartbeat.version,
"uptime_seconds" => heartbeat.uptime_seconds,
"arch" => heartbeat.arch
}
# Validate message size to prevent DoS attacks (early return pattern)
if byte_size(binary_b64) > @max_message_size do
Logger.warning("Rejected oversized heartbeat from agent",
agent_token_id: socket.assigns.agent_token_id,
size: byte_size(binary_b64),
max_size: @max_message_size
)
_ =
Agents.update_agent_token_heartbeat(
socket.assigns.agent_token_id,
get_remote_ip(socket),
metadata
)
# Update heartbeat tracking for timeout detection
socket = assign(socket, :last_heartbeat_at, DateTime.utc_now())
# Broadcast heartbeat for real-time UI updates (especially important for stale agents coming back online)
_ =
Phoenix.PubSub.broadcast(
Towerops.PubSub,
"agents:health",
{:agent_heartbeat, socket.assigns.agent_token_id, socket.assigns.organization_id}
)
{:noreply, socket}
{:reply, {:error, %{reason: "Message too large"}}, socket}
else
{:error, {type, message}} ->
Logger.error("Invalid heartbeat from agent",
agent_token_id: socket.assigns.agent_token_id,
error_type: type,
error_message: message
)
{:noreply, socket}
process_heartbeat_message(binary_b64, socket)
end
end
def handle_in("error", %{"binary" => binary_b64}, socket) when is_binary(binary_b64) do
with {:ok, binary} <- safe_base64_decode(binary_b64),
{:ok, error} <- Validator.validate_agent_error(binary) do
maybe_debug_log(socket, "Agent job error",
device_id: error.device_id,
error_message: error.message,
binary_size: byte_size(binary_b64)
)
Logger.error("Agent job error",
# Validate message size to prevent DoS attacks
if byte_size(binary_b64) > @max_message_size do
Logger.warning("Rejected oversized error message from agent",
agent_token_id: socket.assigns.agent_token_id,
device_id: error.device_id,
error: error.message
size: byte_size(binary_b64),
max_size: @max_message_size
)
{:noreply, socket}
{:reply, {:error, %{reason: "Message too large"}}, socket}
else
{:error, {type, message}} ->
Logger.error("Invalid error message from agent",
with {:ok, binary} <- safe_base64_decode(binary_b64),
{:ok, error} <- Validator.validate_agent_error(binary) do
maybe_debug_log(socket, "Agent job error",
device_id: error.device_id,
error_message: error.message,
binary_size: byte_size(binary_b64)
)
Logger.error("Agent job error",
agent_token_id: socket.assigns.agent_token_id,
error_type: type,
error_message: message
device_id: error.device_id,
error: error.message
)
{:noreply, socket}
else
{:error, {type, message}} ->
Logger.error("Invalid error message from agent",
agent_token_id: socket.assigns.agent_token_id,
error_type: type,
error_message: message
)
{:noreply, socket}
end
end
end
@ -691,6 +652,100 @@ defmodule ToweropsWeb.AgentChannel do
# Private helpers
defp process_snmp_result_message(binary_b64, socket) do
Logger.info("Received SNMP result from agent (binary size: #{byte_size(binary_b64)})")
with {:ok, binary} <- safe_base64_decode(binary_b64),
{:ok, result} <- Validator.validate_snmp_result(binary) do
maybe_debug_log(socket, "Received SNMP result from agent",
device_id: result.device_id,
job_type: result.job_type,
job_id: result.job_id,
binary_size: byte_size(binary_b64),
oid_count: map_size(result.oid_values)
)
# Check if this is a live poll result
if String.starts_with?(result.job_id, "live_poll:") do
handle_live_poll_result(result, socket)
else
process_and_log_snmp_result(socket, result)
end
{:noreply, socket}
else
{:error, {type, message}} ->
Logger.error("Invalid SNMP result from agent: #{type} - #{message}",
agent_token_id: socket.assigns.agent_token_id,
error_type: type,
error_message: message,
binary_size: byte_size(binary_b64)
)
{:noreply, socket}
{:error, :base64_decode_failed} ->
Logger.error("Failed to decode SNMP result (invalid base64)",
agent_token_id: socket.assigns.agent_token_id,
binary_size: byte_size(binary_b64)
)
{:noreply, socket}
end
end
defp process_heartbeat_message(binary_b64, socket) do
with {:ok, binary} <- safe_base64_decode(binary_b64),
{:ok, heartbeat} <- Validator.validate_heartbeat(binary) do
now = DateTime.utc_now()
last_db_update = socket.assigns[:last_heartbeat_db_update]
# Only update database if last update was >30s ago to prevent flooding
# This limits heartbeat DB writes to ~2/minute max per agent
socket =
if is_nil(last_db_update) or DateTime.diff(now, last_db_update) > 30 do
metadata = %{
"version" => heartbeat.version,
"uptime_seconds" => heartbeat.uptime_seconds,
"arch" => heartbeat.arch
}
_ =
Agents.update_agent_token_heartbeat(
socket.assigns.agent_token_id,
get_remote_ip(socket),
metadata
)
# Broadcast heartbeat for real-time UI updates (especially important for stale agents coming back online)
_ =
Phoenix.PubSub.broadcast(
Towerops.PubSub,
"agents:health",
{:agent_heartbeat, socket.assigns.agent_token_id, socket.assigns.organization_id}
)
socket
|> assign(:last_heartbeat_at, now)
|> assign(:last_heartbeat_db_update, now)
else
# Just update in-memory tracking, skip DB write
assign(socket, :last_heartbeat_at, now)
end
{:noreply, socket}
else
{:error, {type, message}} ->
Logger.error("Invalid heartbeat from agent",
agent_token_id: socket.assigns.agent_token_id,
error_type: type,
error_message: message
)
{:noreply, socket}
end
end
@spec handle_validated_mikrotik_result(MikrotikResult.t(), socket()) :: {:noreply, socket()}
defp handle_validated_mikrotik_result(result, socket) do
_binary = MikrotikResult.encode(result)

View file

@ -16,6 +16,14 @@ defmodule ToweropsWeb.Api.AccountDataController do
def show(conn, _params) do
user = conn.assigns.current_scope.user
# Verify user account is confirmed before allowing data export
if !user.confirmed_at do
conn
|> put_status(:forbidden)
|> json(%{error: "Email address must be confirmed before exporting account data"})
|> halt()
end
# Log the data export for audit trail
AuditLogger.log_user_data_exported(conn, user.id)

View file

@ -194,50 +194,96 @@ defmodule ToweropsWeb.Api.V1.MibController do
end
defp extract_tarball(conn, upload, vendor_dir, vendor) do
case System.cmd("tar", ["-xzf", upload.path, "-C", vendor_dir]) do
{_output, 0} ->
files_count = count_files(vendor_dir)
Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
# Extract to temporary directory first to validate contents
temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{:rand.uniform(999_999_999)}")
File.mkdir_p!(temp_dir)
conn
|> put_status(:created)
|> json(%{
status: "ok",
message: "Successfully extracted MIB archive",
vendor: vendor,
files_count: files_count
})
try do
case System.cmd("tar", ["-xzf", upload.path, "-C", 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
# sobelow_skip ["Traversal.FileModule"]
File.cp_r!(temp_dir, vendor_dir)
files_count = count_files(vendor_dir)
Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
{error, exit_code} ->
Logger.error("Failed to extract tarball: #{error}")
conn
|> put_status(:created)
|> json(%{
status: "ok",
message: "Successfully extracted MIB archive",
vendor: vendor,
files_count: files_count
})
conn
|> put_status(:bad_request)
|> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
{: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 tarball: #{error}")
conn
|> put_status(:bad_request)
|> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
end
after
File.rm_rf!(temp_dir)
end
end
defp extract_zip(conn, upload, vendor_dir, vendor) do
case System.cmd("unzip", ["-o", upload.path, "-d", vendor_dir]) do
{_output, 0} ->
files_count = count_files(vendor_dir)
Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
# Extract to temporary directory first to validate contents
temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{:rand.uniform(999_999_999)}")
File.mkdir_p!(temp_dir)
conn
|> put_status(:created)
|> json(%{
status: "ok",
message: "Successfully extracted MIB archive",
vendor: vendor,
files_count: files_count
})
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
# sobelow_skip ["Traversal.FileModule"]
File.cp_r!(temp_dir, vendor_dir)
files_count = count_files(vendor_dir)
Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}")
{error, exit_code} ->
Logger.error("Failed to extract zip: #{error}")
conn
|> put_status(:created)
|> json(%{
status: "ok",
message: "Successfully extracted MIB archive",
vendor: vendor,
files_count: files_count
})
conn
|> put_status(:bad_request)
|> json(%{error: "Failed to extract archive (exit code: #{exit_code})"})
{: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: "Failed to extract archive (exit code: #{exit_code})"})
end
after
File.rm_rf!(temp_dir)
end
end
@ -331,6 +377,32 @@ defmodule ToweropsWeb.Api.V1.MibController do
|> Enum.count(&File.regular?/1)
end
# Validate that extracted archive contents don't contain path traversal attacks
defp validate_extracted_paths(extract_dir) do
# Get canonical path of extraction directory
canonical_extract_dir = Path.expand(extract_dir)
# Check all extracted files/directories
extract_dir
|> Path.join("**/*")
|> Path.wildcard()
|> Enum.all?(fn path ->
canonical_path = Path.expand(path)
String.starts_with?(canonical_path, canonical_extract_dir)
end)
|> case do
true ->
:ok
false ->
{:error, "Archive contains files with invalid paths (possible directory traversal attack)"}
end
rescue
e ->
Logger.error("Failed to validate extracted paths: #{inspect(e)}")
{:error, "Failed to validate archive contents"}
end
# Validate vendor name to prevent directory traversal attacks
# Only allow alphanumeric characters, hyphens, and underscores
defp validate_vendor_name(vendor) when is_binary(vendor) do

View file

@ -5,12 +5,25 @@ defmodule ToweropsWeb.ErrorJSON do
See config/config.exs.
"""
# If you want to customize a particular status code,
# you may add your own clauses, such as:
#
# def render("500.json", _assigns) do
# %{errors: %{detail: "Internal Server Error"}}
# end
require Logger
# Render 500 errors with generic message and request ID for support
# Never expose internal error details, stack traces, or module names to clients
def render("500.json", assigns) do
# Log full error details server-side for debugging
Logger.error("Internal server error",
assigns: inspect(assigns),
request_id: Logger.metadata()[:request_id]
)
# Return generic error to client with request ID for support tracking
%{
errors: %{
detail: "An unexpected error occurred. Please contact support if this persists.",
request_id: Logger.metadata()[:request_id]
}
}
end
# By default, Phoenix returns the status message from
# the template name. For example, "404.json" becomes

View file

@ -78,29 +78,4 @@ defmodule ToweropsWeb.HealthController do
defp redis_status_string(:ok), do: "connected"
defp redis_status_string(:error), do: "disconnected"
defp redis_status_string(:not_configured), do: "not_configured"
@doc """
Time diagnostic endpoint for debugging TOTP issues.
Returns server time from multiple sources.
"""
def time(conn, _params) do
system_time = System.system_time(:second)
os_time = :os.system_time(:second)
datetime_now = DateTime.utc_now()
conn
|> put_resp_content_type("application/json")
|> send_resp(
200,
Jason.encode!(%{
system_time: system_time,
system_time_iso: system_time |> DateTime.from_unix!() |> DateTime.to_iso8601(),
os_time: os_time,
os_time_iso: os_time |> DateTime.from_unix!() |> DateTime.to_iso8601(),
datetime_now: DateTime.to_unix(datetime_now),
datetime_now_iso: DateTime.to_iso8601(datetime_now),
time_sources_agree: system_time == os_time and system_time == DateTime.to_unix(datetime_now)
})
)
end
end

View file

@ -73,7 +73,6 @@ defmodule ToweropsWeb.Router do
# Health check endpoint for Kubernetes probes (no authentication required)
scope "/", ToweropsWeb do
get "/health", HealthController, :index
get "/health/time", HealthController, :time
end
scope "/", ToweropsWeb do
@ -131,7 +130,8 @@ defmodule ToweropsWeb.Router do
forward "/", Absinthe.Plug,
schema: ToweropsWeb.GraphQL.Schema,
analyze_complexity: true,
max_complexity: 500
max_complexity: 500,
max_depth: 10
end
# API v1 routes (requires API token authentication)

View file

@ -1,3 +1,10 @@
2026-03-05 — Security Enhancements
* Enhanced security for API endpoints and file uploads
* Improved validation for mobile device registration
* Better protection against potential DoS attacks
* Strengthened authentication requirements for sensitive operations
* Enhanced error messages to prevent information disclosure
2026-03-05 — Network Topology Discovery (Phase 1)
* Automatic discovery of connected network devices using LLDP
* View physical port connections between devices

View file

@ -6,7 +6,8 @@ defmodule ToweropsWeb.ErrorJSONTest do
end
test "renders 500" do
assert ToweropsWeb.ErrorJSON.render("500.json", %{}) ==
%{errors: %{detail: "Internal Server Error"}}
result = ToweropsWeb.ErrorJSON.render("500.json", %{})
assert result.errors.detail == "An unexpected error occurred. Please contact support if this persists."
assert Map.has_key?(result.errors, :request_id)
end
end

View file

@ -16,44 +16,4 @@ defmodule ToweropsWeb.HealthControllerTest do
assert get_resp_header(conn, "content-type") == ["application/json; charset=utf-8"]
end
end
describe "GET /health/time" do
test "returns server time from multiple sources", %{conn: conn} do
conn = get(conn, ~p"/health/time")
response = json_response(conn, 200)
# Verify all expected fields are present
assert Map.has_key?(response, "system_time")
assert Map.has_key?(response, "system_time_iso")
assert Map.has_key?(response, "os_time")
assert Map.has_key?(response, "os_time_iso")
assert Map.has_key?(response, "datetime_now")
assert Map.has_key?(response, "datetime_now_iso")
assert Map.has_key?(response, "time_sources_agree")
# Verify types
assert is_integer(response["system_time"])
assert is_binary(response["system_time_iso"])
assert is_integer(response["os_time"])
assert is_binary(response["os_time_iso"])
assert is_integer(response["datetime_now"])
assert is_binary(response["datetime_now_iso"])
assert is_boolean(response["time_sources_agree"])
# Verify ISO 8601 format
assert String.match?(response["system_time_iso"], ~r/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/)
assert String.match?(response["os_time_iso"], ~r/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/)
assert String.match?(response["datetime_now_iso"], ~r/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}/)
# Verify all times are within a reasonable range (within 5 seconds of each other)
time_diff_1 = abs(response["system_time"] - response["os_time"])
time_diff_2 = abs(response["system_time"] - response["datetime_now"])
assert time_diff_1 <= 5, "system_time and os_time should be within 5 seconds"
assert time_diff_2 <= 5, "system_time and datetime_now should be within 5 seconds"
assert get_resp_header(conn, "content-type") == ["application/json; charset=utf-8"]
end
end
end