From d2466b9f66532ed2dd30a718017a2cd6a66f73f2 Mon Sep 17 00:00:00 2001 From: Toshihiko SHIMOKAWA Date: Tue, 11 Aug 2026 22:24:58 +0900 Subject: [PATCH 1/3] fix(cli): validate the --subnet prefix and stop dropping the option Two defects kept --subnet from behaving like dig's +subnet. The prefix length was not validated. String.to_integer/1 raised ArgumentError on a non-numeric value, so `--subnet 192.0.2.1/abc` showed a stack trace, and a negative value passed straight through because min/2 only caps the upper end -- `/-5` reached the query as source_prefix: -5, which RFC 7871 cannot even encode. Measured against dig 9.20.26, an over-range prefix is not an error there: it is capped at the family width (999 -> 32, 200 -> 128 for IPv6), which min/2 already did correctly. What dig rejects is anything that is not an unsigned number, including a leading sign. Screen the prefix with that rule and report it the way the neighbouring subnet errors do, on stderr with exit 1. The option was also being dropped. check_edns/1 matched `%{edns: false}` before the subnet clause, and parse_args/1 sets :edns through Map.put_new/3, so the key is always present: --subnet was silently ignored unless --edns was also passed, and the bufsize clause swallowed it too. Two of the three ways to ask for ECS sent no ECS at all. Move the subnet clause first, matching dig, where +subnet enables EDNS even next to an explicit +noedns. The existing check_edns test missed this because it called the function with a hand-built map that omitted :edns. The new tests go through parse_args/1 so they see what the CLI actually produces. source_prefix is now non-negative by construction, so the type says non_neg_integer() rather than integer(). Resolves #86 Claude-Session: https://claude.ai/code/session_01YAjSJR67tWTvYDJLcQThbQ --- lib/tdig/cli.ex | 47 ++++++++++++++++++++++--- test/tdig_test.exs | 88 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 5 deletions(-) diff --git a/lib/tdig/cli.ex b/lib/tdig/cli.ex index cb3fc87..49eaac6 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,7 +255,7 @@ defmodule Tdig.CLI do %{ family: 1 | 2, client_subnet: :inet.ip_address(), - source_prefix: integer(), + source_prefix: non_neg_integer(), scope_prefix: 0 }} @@ -257,7 +263,7 @@ defmodule Tdig.CLI do def parse_subnet_option(subnet) do case String.split(subnet, "/") do [addr_str, prefix_str] -> - prefix = String.to_integer(prefix_str) + prefix = prefix_length!(subnet, prefix_str) case :inet.parse_address(String.to_charlist(addr_str)) do {:ok, {a, b, c, d}} -> @@ -295,6 +301,37 @@ defmodule Tdig.CLI do 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..2f7072b 100644 --- a/test/tdig_test.exs +++ b/test/tdig_test.exs @@ -335,6 +335,94 @@ 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 + 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 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. From 0b3430c099bb1eae3519cf53dd86783fee036cf8 Mon Sep 17 00:00:00 2001 From: Toshihiko SHIMOKAWA Date: Tue, 11 Aug 2026 22:27:48 +0900 Subject: [PATCH 2/3] test(cli): assert the emitted OPT record, not just the parsed args Reviewing #86 raised a fair gap: the bufsize test stopped at parse_args/1 output, which does not show whether the value reaches the OPT pseudo-record. That record is built later, in Tdig.check_edns/1, so assert payload_size and the ECS rdata there. Also pin the extra-slash cases, which fall through to the existing "Invalid subnet format" message rather than the new prefix one. Claude-Session: https://claude.ai/code/session_01YAjSJR67tWTvYDJLcQThbQ --- test/tdig_test.exs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/tdig_test.exs b/test/tdig_test.exs index 2f7072b..72249ab 100644 --- a/test/tdig_test.exs +++ b/test/tdig_test.exs @@ -374,6 +374,13 @@ defmodule TdigTest 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 + # String.split/2 yields three parts, which no address/prefix clause + # matches, so this falls through to the existing format message. + assert String.split("192.0.2.1/24/8", "/") == ["192.0.2.1", "24", "8"] + assert String.split("192.0.2.1//24", "/") == ["192.0.2.1", "", "24"] + end end describe "subnet option reaches the query (Issue #86)" do @@ -402,6 +409,17 @@ defmodule TdigTest do 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) From 92a73b75ef077c7e76da2d19b0bccfdf426b0402 Mon Sep 17 00:00:00 2001 From: Toshihiko SHIMOKAWA Date: Tue, 11 Aug 2026 22:30:55 +0900 Subject: [PATCH 3/3] test(cli): assert tdig's own split, not String.split/2 The extra-slash test asserted the shape String.split/2 returns, which checks the standard library rather than anything in this module, and did not show which of the two error messages the input would get. Extract split_subnet/1, the point where parse_subnet_option/1 chooses between the format error and the prefix error, and assert that. Same approach as parse_prefix_length/1: keep the decision in a pure function so it stays testable around the halting call. Also pin the clamping of very large prefixes. dig stops at a 32-bit unsigned and reports "out of range" past it, an artefact of its C parsing; tdig clamps instead, which yields the same option for every value dig accepts. Claude-Session: https://claude.ai/code/session_01YAjSJR67tWTvYDJLcQThbQ --- lib/tdig/cli.ex | 21 ++++++++++++++++++--- test/tdig_test.exs | 29 +++++++++++++++++++++++++---- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/lib/tdig/cli.ex b/lib/tdig/cli.ex index 49eaac6..d330d0b 100644 --- a/lib/tdig/cli.ex +++ b/lib/tdig/cli.ex @@ -261,8 +261,8 @@ defmodule Tdig.CLI do @spec parse_subnet_option(String.t()) :: edns_client_subnet() def parse_subnet_option(subnet) do - case String.split(subnet, "/") do - [addr_str, 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 @@ -295,12 +295,27 @@ 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 diff --git a/test/tdig_test.exs b/test/tdig_test.exs index 72249ab..0a06298 100644 --- a/test/tdig_test.exs +++ b/test/tdig_test.exs @@ -376,10 +376,31 @@ defmodule TdigTest do end test "an extra slash is a format error, not a prefix error" do - # String.split/2 yields three parts, which no address/prefix clause - # matches, so this falls through to the existing format message. - assert String.split("192.0.2.1/24/8", "/") == ["192.0.2.1", "24", "8"] - assert String.split("192.0.2.1//24", "/") == ["192.0.2.1", "", "24"] + # 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