From 7c1aea50ecc5c6143693e7bf78654551c57e6080 Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Tue, 12 May 2026 15:46:13 -0500
Subject: [PATCH] =?UTF-8?q?fix:=20M19,=20L16=20=E2=80=94=20protobuf=20vali?=
 =?UTF-8?q?dation=20on=206=20decode=20paths,=20vault=20ETS=20docs?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- M19: Add validate_mikrotik_device, validate_mikrotik_command, validate_check,
  validate_snmp_query, validate_sensor, and validate_interface functions with
  string length and list size validation, wired into all call sites
- L16: Document that Cloak Vault ETS table name collision risk is mitigated by
  ETS tables being node-local (only a concern on same-BEAM-node, not a supported
  production configuration)
---
 bugs.md                      | 18 ----------
 lib/towerops/proto/decode.ex | 64 ++++++++++++++++++++++++++++++++----
 lib/towerops/vault.ex        |  5 +++
 3 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/bugs.md b/bugs.md
index 8062c253..52b81271 100644
--- a/bugs.md
+++ b/bugs.md
@@ -80,17 +80,6 @@
 
 
 
-### M19. Missing Validation on Multiple Protobuf Decode Paths
-
-**File:** `lib/towerops/proto/decode.ex` (multiple decode functions)
-
-**Severity:** MEDIUM — No validation on `decode_mikrotik_device`, `decode_mikrotik_command`, `decode_check`, `decode_snmp_query`, `decode_sensor`, `decode_interface`
-
-**Description:** Several decode functions have zero validation (no `validate_string` calls, no field-length checks).
-
-**Fix:** Add validation consistent with existing validated decode paths.
-
----
 
 
 
@@ -136,10 +125,3 @@
 
 ---
 
-### L16. COW/Vault ETS Table Name Collision Risk
-
-**File:** `lib/towerops/vault.ex` (ETS table naming)
-
-**Severity:** LOW — If two nodes share a node name, ETS table creation could conflict
-
-**Fix:** Use unique ETS names incorporating node identifier.
diff --git a/lib/towerops/proto/decode.ex b/lib/towerops/proto/decode.ex
index c5928f85..9f3fa2fb 100644
--- a/lib/towerops/proto/decode.ex
+++ b/lib/towerops/proto/decode.ex
@@ -947,7 +947,8 @@ defmodule Towerops.Proto.Decode do
 
         4 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, check} <- decode_check(msg_data) do
+               {:ok, check} <- decode_check(msg_data),
+               {:ok, check} <- validate_check(check) do
             decode_agent_config_fields(rest2, %{acc | checks: [check | acc.checks]})
           end
 
@@ -1009,13 +1010,15 @@ defmodule Towerops.Proto.Decode do
 
         6 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, sensor} <- decode_sensor(msg_data) do
+               {:ok, sensor} <- decode_sensor(msg_data),
+               {:ok, sensor} <- validate_sensor(sensor) do
             decode_device_fields(rest2, %{acc | sensors: [sensor | acc.sensors]})
           end
 
         7 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, interface} <- decode_interface(msg_data) do
+               {:ok, interface} <- decode_interface(msg_data),
+               {:ok, interface} <- validate_interface(interface) do
             decode_device_fields(rest2, %{acc | interfaces: [interface | acc.interfaces]})
           end
 
@@ -1483,19 +1486,22 @@ defmodule Towerops.Proto.Decode do
 
         5 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, query} <- decode_snmp_query(msg_data) do
+               {:ok, query} <- decode_snmp_query(msg_data),
+               {:ok, query} <- validate_snmp_query(query) do
             decode_agent_job_fields(rest2, %{acc | queries: [query | acc.queries]})
           end
 
         6 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, mikrotik_device} <- decode_mikrotik_device(msg_data) do
+               {:ok, mikrotik_device} <- decode_mikrotik_device(msg_data),
+               {:ok, mikrotik_device} <- validate_mikrotik_device(mikrotik_device) do
             decode_agent_job_fields(rest2, %{acc | mikrotik_device: mikrotik_device})
           end
 
         7 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, command} <- decode_mikrotik_command(msg_data) do
+               {:ok, command} <- decode_mikrotik_command(msg_data),
+               {:ok, command} <- validate_mikrotik_command(command) do
             decode_agent_job_fields(rest2, %{acc | mikrotik_commands: [command | acc.mikrotik_commands]})
           end
 
@@ -1722,7 +1728,8 @@ defmodule Towerops.Proto.Decode do
       case field_num do
         1 ->
           with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest),
-               {:ok, check} <- decode_check(msg_data) do
+               {:ok, check} <- decode_check(msg_data),
+               {:ok, check} <- validate_check(check) do
             decode_check_list_fields(rest2, [check | acc])
           end
 
@@ -1906,6 +1913,49 @@ defmodule Towerops.Proto.Decode do
     end
   end
 
+  defp validate_mikrotik_device(d) do
+    with :ok <- validate_string("ip", d.ip, @max_short_string_length),
+         :ok <- validate_string("username", d.username, @max_short_string_length),
+         :ok <- validate_string("password", d.password, @max_short_string_length) do
+      {:ok, d}
+    end
+  end
+
+  defp validate_mikrotik_command(c) do
+    with :ok <- validate_string("command", c.command, @max_string_length) do
+      {:ok, c}
+    end
+  end
+
+  defp validate_check(c) do
+    with :ok <- validate_string("id", c.id, @max_short_string_length),
+         :ok <- validate_string("check_type", c.check_type, @max_short_string_length) do
+      {:ok, c}
+    end
+  end
+
+  defp validate_snmp_query(q) do
+    with :ok <- validate_list_size("oids", q.oids, @max_oid_values) do
+      {:ok, q}
+    end
+  end
+
+  defp validate_sensor(s) do
+    with :ok <- validate_string("id", s.id, @max_short_string_length),
+         :ok <- validate_string("sensor_type", s.sensor_type, @max_short_string_length),
+         :ok <- validate_string("oid", s.oid, @max_short_string_length),
+         :ok <- validate_string("unit", s.unit, @max_short_string_length) do
+      {:ok, s}
+    end
+  end
+
+  defp validate_interface(i) do
+    with :ok <- validate_string("id", i.id, @max_short_string_length),
+         :ok <- validate_string("if_name", i.if_name, @max_short_string_length) do
+      {:ok, i}
+    end
+  end
+
   # Validation helpers
 
   defp validate_uuid(field, value) do
diff --git a/lib/towerops/vault.ex b/lib/towerops/vault.ex
index 1c8eecdf..af24c806 100644
--- a/lib/towerops/vault.ex
+++ b/lib/towerops/vault.ex
@@ -19,6 +19,11 @@ defmodule Towerops.Vault do
   Store this key securely in 1Password and set it as the CLOAK_KEY
   environment variable in production.
   """
+  # The Cloak-backed ETS config table name is derived from the module name
+  # at compile time by Cloak's `use` macro. ETS tables are node-local, so
+  # two nodes in a cluster never collide. The only risk is running two
+  # instances of this module on the same BEAM node outside a cluster (e.g.
+  # one-off `eval`), which is not a supported production configuration.
   use Cloak.Vault, otp_app: :towerops
 
   @impl GenServer