fix(cli): validate the --subnet prefix and stop dropping the option

lib/tdig/cli.ex

@@ -208,12 +208,18 @@ defmodule Tdig.CLI do
     end
   end
 
+  # The subnet clause comes first because an ECS option needs EDNS to carry it.
+  # `parse_args/1` sets :edns through `Map.put_new/3`, so the key is always
+  # present and the `edns: false` clause below would otherwise shadow this one,
+  # dropping --subnet silently unless --edns was also given. dig behaves the
+  # same way: +subnet turns EDNS on even alongside an explicit +noedns.
+  # `Map.put_new/3` leaves an explicitly requested --bufsize alone.
+  def check_edns(%{subnet: subnet} = arg) when is_binary(subnet),
+    do: arg |> Map.put_new(:bufsize, DNS.edns_max_udpsize()) |> mk_edns_with_subnet
+
   def check_edns(%{bufsize: size} = arg) when is_integer(size), do: mk_edns(arg)
   def check_edns(%{edns: false} = arg), do: arg
 
-  def check_edns(%{subnet: subnet} = arg) when is_binary(subnet),
-    do: arg |> Map.put(:bufsize, DNS.edns_max_udpsize()) |> mk_edns_with_subnet
-
   def check_edns(arg) do
     arg
     |> Map.put(:bufsize, DNS.edns_max_udpsize())
@@ -249,15 +255,15 @@ defmodule Tdig.CLI do
            %{
              family: 1 | 2,
              client_subnet: :inet.ip_address(),
-             source_prefix: integer(),
+             source_prefix: non_neg_integer(),
              scope_prefix: 0
            }}
 
   @spec parse_subnet_option(String.t()) :: edns_client_subnet()
   def parse_subnet_option(subnet) do
-    case String.split(subnet, "/") do
-      [addr_str, prefix_str] ->
-        prefix = String.to_integer(prefix_str)
+    case split_subnet(subnet) do
+      {:ok, addr_str, prefix_str} ->
+        prefix = prefix_length!(subnet, prefix_str)
 
         case :inet.parse_address(String.to_charlist(addr_str)) do
           {:ok, {a, b, c, d}} ->
@@ -289,12 +295,58 @@ defmodule Tdig.CLI do
             System.halt(1)
         end
 
-      _ ->
+      :error ->
         IO.puts(:stderr, "Invalid subnet format. Use: address/prefix (e.g., 192.0.2.1/24)")
         System.halt(1)
     end
   end
 
+  @doc """
+  Splits a `--subnet` argument into its address and prefix parts.
+
+  Anything other than exactly one separator is a format error rather than a
+  prefix error, so `"192.0.2.1/24/8"` and `"192.0.2.1"` are both rejected here
+  and never reach `parse_prefix_length/1`.
+  """
+  @spec split_subnet(String.t()) :: {:ok, String.t(), String.t()} | :error
+  def split_subnet(subnet) do
+    case String.split(subnet, "/") do
+      [addr_str, prefix_str] -> {:ok, addr_str, prefix_str}
+      _ -> :error
+    end
+  end
+
+  defp prefix_length!(subnet, prefix_str) do
+    case parse_prefix_length(prefix_str) do
+      {:ok, prefix} -> prefix
+      :error -> invalid_argument("Invalid prefix length in #{subnet}: not a valid number")
+    end
+  end
+
+  @doc """
+  Parses a subnet prefix length, accepting only a bare non-negative number.
+
+  A value above the address family's width is *not* rejected here: dig caps it
+  at 32 or 128 rather than erroring, and `parse_subnet_option/1` does the same
+  with `min/2`. What dig does reject is anything that is not an unsigned
+  number, which is what this function screens out — `min/2` only caps the
+  upper end, so a negative prefix would otherwise reach the query untouched.
+
+  Digits only, so a sign is refused even where `Integer.parse/1` would accept
+  it (`"+24"`); leading zeros are fine. This matches dig, measured with 9.20.26:
+
+      192.0.2.1/+24 => dig: invalid prefix length in '192.0.2.1/+24': not a valid number
+      192.0.2.1/024 => CLIENT-SUBNET: 192.0.2.0/24/0
+  """
+  @spec parse_prefix_length(String.t()) :: {:ok, non_neg_integer()} | :error
+  def parse_prefix_length(prefix_str) do
+    if String.match?(prefix_str, ~r/\A\d+\z/) do
+      {:ok, String.to_integer(prefix_str)}
+    else
+      :error
+    end
+  end
+
   def check_args(%{help: true}), do: %{help: true, exit_code: 0}
   def check_args(%{name: nil, read: nil, version: nil}), do: %{help: true, exit_code: 1}
 

test/tdig_test.exs

@@ -335,6 +335,133 @@ defmodule TdigTest do
     end
   end
 
+  describe "subnet prefix length validation (Issue #86)" do
+    # dig parses the prefix as an unsigned number and refuses anything else
+    # ("invalid prefix length in '...': not a valid number"), so a negative or
+    # non-numeric value is an error rather than something to coerce.
+    test "parse_prefix_length/1 accepts a bare non-negative number" do
+      assert Tdig.CLI.parse_prefix_length("24") == {:ok, 24}
+      assert Tdig.CLI.parse_prefix_length("0") == {:ok, 0}
+      # dig accepts leading zeros: 192.0.2.1/024 => CLIENT-SUBNET: 192.0.2.0/24/0
+      assert Tdig.CLI.parse_prefix_length("024") == {:ok, 24}
+    end
+
+    test "parse_prefix_length/1 rejects a negative number" do
+      assert Tdig.CLI.parse_prefix_length("-5") == :error
+      assert Tdig.CLI.parse_prefix_length("-1") == :error
+    end
+
+    test "parse_prefix_length/1 rejects anything that is not a bare number" do
+      for input <- ["abc", "", " 24", "24x", "2.4", "+24"] do
+        assert Tdig.CLI.parse_prefix_length(input) == :error,
+               "expected #{inspect(input)} to be rejected"
+      end
+    end
+
+    # An over-range prefix is NOT an error in dig: it is capped at the address
+    # family's width. Measured with dig 9.20.26 via +qr:
+    #   192.0.2.1/999   => CLIENT-SUBNET: 192.0.2.1/32/0
+    #   2001:db8::1/200 => CLIENT-SUBNET: 2001:db8::1/128/0
+    test "an over-range IPv4 prefix is clamped to 32 rather than rejected" do
+      {:edns_client_subnet, ecs} = Tdig.CLI.parse_subnet_option("192.0.2.1/999")
+      assert ecs.source_prefix == 32
+
+      {:edns_client_subnet, ecs} = Tdig.CLI.parse_subnet_option("192.0.2.1/33")
+      assert ecs.source_prefix == 32
+    end
+
+    test "an over-range IPv6 prefix is clamped to 128 rather than rejected" do
+      {:edns_client_subnet, ecs} = Tdig.CLI.parse_subnet_option("2001:db8::1/200")
+      assert ecs.source_prefix == 128
+    end
+
+    test "an extra slash is a format error, not a prefix error" do
+      # split_subnet/1 is where parse_subnet_option/1 decides between its two
+      # error messages. Asserting it directly, rather than the halting call,
+      # keeps the distinction testable: :error here means the input never
+      # reaches parse_prefix_length/1 and gets the format message instead.
+      assert Tdig.CLI.split_subnet("192.0.2.1/24/8") == :error
+      assert Tdig.CLI.split_subnet("192.0.2.1//24") == :error
+      assert Tdig.CLI.split_subnet("192.0.2.1") == :error
+
+      assert Tdig.CLI.split_subnet("192.0.2.1/24") == {:ok, "192.0.2.1", "24"}
+      # an empty prefix is a well-formed split, so it is a prefix error
+      assert Tdig.CLI.split_subnet("2001:db8::1/") == {:ok, "2001:db8::1", ""}
+      assert Tdig.CLI.parse_prefix_length("") == :error
+    end
+
+    test "a prefix far above the family width is still clamped" do
+      # dig stops at a 32-bit unsigned and reports "out of range" beyond it
+      # (4294967295 clamps to 32, 4294967296 errors), which is an artefact of
+      # its C parsing rather than a protocol limit. tdig has no such ceiling;
+      # every value at or above the family width produces the same option, so
+      # the emitted query matches dig for everything dig accepts.
+      {:edns_client_subnet, ecs} = Tdig.CLI.parse_subnet_option("192.0.2.1/4294967295")
+      assert ecs.source_prefix == 32
+
+      {:edns_client_subnet, ecs} = Tdig.CLI.parse_subnet_option("192.0.2.1/99999999999999999999")
+      assert ecs.source_prefix == 32
+    end
+  end
+
+  describe "subnet option reaches the query (Issue #86)" do
+    # parse_args/1 sets :edns via Map.put_new, so the key is always present.
+    # These go through parse_args rather than calling check_edns/1 with a
+    # hand-built map, because the shadowing bug was invisible to a map that
+    # omitted :edns.
+    defp ecs_options(argv), do: Tdig.CLI.parse_args(argv)[:options]
+
+    test "--subnet alone carries the ECS option" do
+      assert [{:edns_client_subnet, ecs}] =
+               ecs_options(["example.com", "--subnet", "192.0.2.1/24"])
+
+      assert ecs.family == 1
+      assert ecs.source_prefix == 24
+    end
+
+    test "--subnet combined with --bufsize carries the ECS option" do
+      argv = ["example.com", "--bufsize", "1232", "--subnet", "192.0.2.1/24"]
+      assert [{:edns_client_subnet, ecs}] = ecs_options(argv)
+      assert ecs.source_prefix == 24
+    end
+
+    test "--subnet keeps an explicitly requested bufsize" do
+      argv = ["example.com", "--bufsize", "1232", "--subnet", "192.0.2.1/24"]
+      assert Tdig.CLI.parse_args(argv)[:bufsize] == 1232
+    end
+
+    test "--subnet with --bufsize reaches the OPT record that gets sent" do
+      # parse_args/1 alone does not prove the value is emitted: the OPT
+      # pseudo-record is built later, in Tdig.check_edns/1.
+      argv = ["example.com", "--bufsize", "1232", "--subnet", "192.0.2.1/24"]
+      assert [opt] = argv |> Tdig.CLI.parse_args() |> Tdig.check_edns()
+      assert opt.type == :opt
+      assert opt.payload_size == 1232
+      assert [{:edns_client_subnet, ecs}] = opt.rdata
+      assert ecs.source_prefix == 24
+    end
+
+    test "--subnet combined with --edns still carries the ECS option" do
+      argv = ["example.com", "--edns", "--subnet", "192.0.2.1/24"]
+      assert [{:edns_client_subnet, _}] = ecs_options(argv)
+    end
+
+    test "--subnet turns EDNS on, as dig's +subnet does" do
+      assert Tdig.CLI.parse_args(["example.com", "--subnet", "192.0.2.1/24"])[:edns] == true
+    end
+
+    test "EDNS without a subnet carries no options" do
+      assert ecs_options(["example.com", "--edns"]) == []
+      assert ecs_options(["example.com", "--bufsize", "1232"]) == []
+    end
+
+    test "a plain query still has EDNS off" do
+      args = Tdig.CLI.parse_args(["example.com"])
+      assert args[:edns] == false
+      assert args[:options] == nil
+    end
+  end
+
   describe "version reporting (Issue #49)" do
     test "version/0 returns the mix.exs project version" do
       # Guards against stale hardcoded version strings drifting from mix.exs.