diff --git a/lib/tdig/cli.ex b/lib/tdig/cli.ex index cb3fc87..d330d0b 100644 --- a/lib/tdig/cli.ex +++ b/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} diff --git a/test/tdig_test.exs b/test/tdig_test.exs index e7cfff8..0a06298 100644 --- a/test/tdig_test.exs +++ b/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.