fix(cli): validate the --subnet prefix and stop dropping the option - #87
Conversation
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
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # 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 |
There was a problem hiding this comment.
ℹ️ [LOW] 確認です。--subnet と --bufsize を両方指定した場合、subnet 節が先頭で一致して mk_edns_with_subnet に入ります。この経路で --bufsize の値が実際に EDNS リクエストに反映されるか（つまり mk_edns_with_subnet が bufsize を尊重するか）を念のため確認しておくと安全です。テストは parse_args 出力の :bufsize を検証しているものの、最終的に送出される EDNS の UDP サイズまでは検証していないように見えます。
There was a problem hiding this comment.
確認しました。反映されますが、「テストが parse_args 出力までしか見ていない」というご指摘は妥当なので、テストを OPT レコードまで伸ばしました（0b3430c）。
bufsize は lib/tdig.ex の Tdig.check_edns/1（CLI 側の同名関数とは別物）で OPT 疑似レコードの payload_size になります。
# lib/tdig.ex:67
payload_size: arg.bufsize,--bufsize 1232 --subnet 192.0.2.1/24 を実際に通すと bufsize=1232 が保たれ、mk_edns_with_subnet は bufsize を触りません（Map.put_new/3 に変えたのはまさにこのためです）。
追加したテストは中間状態ではなく送出される OPT レコードそのものを検証します。
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.rdatamix test → 74 passed。
| case String.split(subnet, "/") do | ||
| [addr_str, prefix_str] -> | ||
| prefix = String.to_integer(prefix_str) | ||
| prefix = prefix_length!(subnet, prefix_str) |
There was a problem hiding this comment.
ℹ️ [LOW] 補足の指摘です。IPv6 で prefix を省略した記法（例: 2001:db8::1/）や、/ が複数含まれるケース（String.split(subnet, "/") が 3 要素以上を返す場合）は、この節に一致せず既存の Invalid subnet format パスへ流れる想定でしょうか。空 prefix 文字列 "" は parse_prefix_length/1 が :error を返すため適切に弾かれますが、複数スラッシュのケースの扱いを一度確認しておくとよいと思います。
There was a problem hiding this comment.
ご想定のとおりです。実測して 0b3430c でテストに固定しました。
"192.0.2.1/24/8" split=["192.0.2.1", "24", "8"] → 3 要素 → Invalid subnet format
"192.0.2.1//24" split=["192.0.2.1", "", "24"] → 3 要素 → Invalid subnet format
"2001:db8::1/" split=["2001:db8::1", ""] → 2 要素 → Invalid prefix length
"2001:db8::1/64" split=["2001:db8::1", "64"] → 正常
複数スラッシュは [addr_str, prefix_str] の節に一致しないので既存の _ -> 節に落ち、Invalid subnet format. Use: address/prefix (e.g., 192.0.2.1/24) になります。エラーの種類として妥当（prefix の問題ではなく形式の問題）なので、この分岐のままにしました。
IPv6 は区切りが : なのでアドレス部にスラッシュは現れず、2001:db8::1/ は 2 要素になってご指摘のとおり parse_prefix_length("") が :error を返します。escript でも確認済みです。
$ ./tdig example.com A --subnet 192.0.2.1/
Invalid prefix length in 192.0.2.1/: not a valid number （exit=1）
| # 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 |
There was a problem hiding this comment.
✨ [POSITIVE] Map.put_new/3 への変更は良い判断です。--bufsize を明示した場合にその値を尊重できるようになり、テストでも --subnet + --bufsize 1232 のケースを検証していて意図が明確です。
| 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 |
There was a problem hiding this comment.
✨ [POSITIVE] parse_prefix_length/1 の正規表現 \A\d+\z による検証は、Integer.parse/1 の符号許容という落とし穴を的確に回避しており、dig の挙動（+24 を拒否、先頭ゼロは許容）と厳密に整合しています。docstring も根拠が明快で優れています。
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
| assert ecs.source_prefix == 128 | ||
| end | ||
|
|
||
| test "an extra slash is a format error, not a prefix error" do |
There was a problem hiding this comment.
an extra slash is a format error のテストが、実際に parse_subnet_option/1 を呼んでフォーマットエラー（stderr + exit 1）になることを検証せず、String.split/2 の出力形状のみを assert しています。これでは「3要素になったときに実際にどの節へフォールスルーし、どう扱われるか」を検証できておらず、意図を説明するコメントと実際のテスト内容に乖離があります。可能なら parse_subnet_option("192.0.2.1/24/8") が期待するエラー動作をするところまで検証することを推奨します。
There was a problem hiding this comment.
ご指摘のとおりです。あのテストは String.split/2（標準ライブラリ）の戻り値の形を assert していて、tdig 側の分岐を何も検証していませんでした。92a73b7 で直しました。
parse_subnet_option/1 が 2 つのエラーメッセージを選び分けている地点を split_subnet/1 として切り出し、そこを直接検証します。System.halt/1 を呼ぶ関数はそのままでは ExUnit で検証できないため、判断だけを純粋関数に出すという parse_prefix_length/1 と同じ手を使いました。
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"}
# 空 prefix は分割としては正常なので、prefix 側のエラーになる
assert Tdig.CLI.split_subnet("2001:db8::1/") == {:ok, "2001:db8::1", ""}
assert Tdig.CLI.parse_prefix_length("") == :errorこれで「3 要素になったときにどちらのメッセージになるか」が実際にテストで固定されました。escript での確認も一致します。
$ ./tdig example.com A --subnet 192.0.2.1/24/8
Invalid subnet format. Use: address/prefix (e.g., 192.0.2.1/24) （exit=1）
$ ./tdig example.com A --subnet 192.0.2.1/
Invalid prefix length in 192.0.2.1/: not a valid number （exit=1）
| """ | ||
| @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 |
There was a problem hiding this comment.
ℹ️ [LOW] parse_prefix_length/1 で String.match?/2 の後に String.to_integer/1 を呼んでいますが、正規表現 \A\d+\z にマッチした時点で必ず整数化可能なので安全です。ただし極端に長い桁数（例: "999999..."）でも巨大整数として受理され、その後の min/2 で 32/128 にクランプされるため実害はありません。念のためこの挙動が意図通りか（dig と同じくクランプで良いか）はテストで担保しておくとより堅牢です。
There was a problem hiding this comment.
「テストで担保しておくとより堅牢」の点、92a73b7 で追加しました。ただし裏を取ったところ、dig はこの領域で clamp ではなくエラーにしますので、意図的な差異として記録します。
dig 9.20.26 の実測で境界を特定しました。
/255 => CLIENT-SUBNET: 192.0.2.1/32/0
/65536 => CLIENT-SUBNET: 192.0.2.1/32/0
/4294967295 => CLIENT-SUBNET: 192.0.2.1/32/0
/4294967296 => dig: invalid prefix length in '...': out of range
境界は 32 bit 符号なしの上限ちょうどで、not a valid number とは別のメッセージです。つまり dig は「uint32 として読めなければ out of range、読めたらファミリ幅にクランプ」という二段構えで、上限は C の整数幅に由来する実装上の産物です。
tdig にこの上限は設けず、クランプのままにしました。理由は、dig が受理する全ての値について送出されるオプションが一致するためです（ファミリ幅以上はすべて 32 / 128 になる）。差が出るのは 2^32 以上という、どちらにせよ無意味な入力に対してだけで、そこに 4294967295 という定数を持ち込む価値は薄いと判断しました。
挙動が意図どおりであることをテストで固定しています。
{: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 == 32out of range まで再現すべきというご判断であれば対応しますので、その場合はお知らせください。
| # 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 |
There was a problem hiding this comment.
✨ [POSITIVE] check_edns/1 の subnet 節を先頭に移動し、Map.put を Map.put_new に変えた修正は、シャドウイングバグの本質を正確に捉えており、--bufsize の明示値を尊重する点でも適切です。コメントで意図（ECS が EDNS を含意すること、dig の挙動との整合）が明記されており、将来の保守者にとって非常に価値があります。
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
| family: 1 | 2, | ||
| client_subnet: :inet.ip_address(), | ||
| source_prefix: integer(), | ||
| source_prefix: non_neg_integer(), |
There was a problem hiding this comment.
ℹ️ [LOW] source_prefix の型を non_neg_integer() に厳格化したのは実装が非負を保証するようになったことと整合しており良い変更です。ただし現状ではクランプ後の値のみが格納されるため、より厳密には 0..128 のような範囲型で表現することも検討できます（dialyzer 上は現状で十分です）。
There was a problem hiding this comment.
0..128 はより厳密にはならないので据え置きます。
この型は IPv4 と IPv6 の両方の ECS オプションを表しており、実際の上限はファミリごとに違います。
family: 1（IPv4）→min(prefix, 32)なので 0..32family: 2（IPv6）→min(prefix, 128)なので 0..128
0..128 と書くと IPv4 の場合に 33..128 という到達し得ない値を許すことになるため、精度が上がるのではなく別の形で不正確になります。正確に表現するにはファミリごとに型を分ける（edns_client_subnet を 2 つの型の union にする）必要があり、family と client_subnet と source_prefix の 3 つが連動する構造をこの目的のために分割するのは割に合わないと判断しました。
non_neg_integer() は「どちらのファミリでも成り立つ、嘘のない上界なしの下界」なので、現状ではこれが妥当と考えます。
| """ | ||
| @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 |
There was a problem hiding this comment.
ℹ️ [LOW] parse_prefix_length/1 で正規表現 ~r/\A\d+\z/ を用いて数字のみを受理し、負値・符号付き・非数値を弾く方針は dig の挙動と一致しており的確です。なお \d は Unicode の数字（例: 全角数字やアラビア数字）にもマッチし得るため、ASCII 数字のみに限定したい場合は ~r/\A\d+\z/ に代えて String.to_charlist で範囲チェックするか、(?-u) フラグの利用を検討してもよいでしょう。実害は限定的ですが一応の指摘です。
There was a problem hiding this comment.
既に ASCII 数字のみです。 Elixir の正規表現は Unicode 対応が既定で off で、u 修飾子を付けたときだけ \d が Unicode の数字に広がります。実測です。
全角 ２４: 既定 => false /u 付き => true
アラビア ٢٤: 既定 => false /u 付き => true
デーヴァナーガリー २४: 既定 => false /u 付き => true
ASCII 24: 既定 => true /u 付き => true
parse_prefix_length/1 を直接通しても弾かれます。
"２４" => :error
"٢٤" => :error
"24" => {:ok, 24}
(?-u) は既定の状態を明示するだけで挙動を変えないため、追加しません。もし将来 u を付ける変更が入れば String.to_integer/1 側が ArgumentError を投げて気づけます（String.to_integer("２４") は失敗する）。
| # 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 |
There was a problem hiding this comment.
✨ [POSITIVE] check_edns/1 の subnet 節を先頭に移動して shadowing を解消した修正は的確です。コメントで dig の +subnet が +noedns があっても EDNS を有効にする挙動と一致させた理由まで明記されており、意図が明確に伝わります。Map.put → Map.put_new によって明示的な --bufsize を尊重する変更も適切です。
1 participant
概要
--subnetがdigの+subnetと揃うようにする。issue #86 が想定していた仕様はdigの実挙動と違っていたので、実測に基づいて修正内容を組み直した。あわせて、prefix 検証を足すだけでは意味がなくなる別の不具合（--subnetが黙って捨てられる）も直す。Resolves #86
1.
digの実挙動（DiG 9.20.26 実測）issue の案 1 は「範囲外をエラーにする（dig に合わせる）」だったが、
digは範囲超過をエラーにしない。+qrで実際に送るオプションを見ると、範囲超過はファミリ幅にクランプされている。つまり「数値として妥当でないものは拒否、範囲超過はクランプ」の二段構え。tdig の
min(prefix, 32)によるクランプは既にdigと同じなので維持し、直すのは負値・非数値だけとした。2. prefix が検証されていなかった
--subnet 192.0.2.1/abc→String.to_integer/1がArgumentErrorを投げ、利用者にスタックトレースが出ていた--subnet 192.0.2.1/-5→min/2は上限しか抑えないのでsource_prefix: -5がそのままクエリに乗る。RFC 7871 の SOURCE PREFIX-LENGTH は 8 bit 符号なしで、そもそも表現できないparse_prefix_length/1を追加し、数字のみを受理する。Integer.parse/1は符号を許すため使わない（digは+24を拒否する）。先頭ゼロはdig同様に受理する。エラー表示は近隣の subnet エラーと同じ流儀（stderr + 終了コード 1）に揃えた。
3.
--subnetが黙って捨てられていたこれを直さないと 2 の検証は効かない（
--subnet 192.0.2.1/abc単体ではparse_subnet_option/1が呼ばれない）ので、同じ PR に含めた。修正前の実測。3 通りのうち 2 通りで ECS が送られていなかった。
check_edns/1の節順が原因。parse_args/1がMap.put_new(:edns, false)するので:ednsキーは常に存在し、--ednsを付けない限り 2 番目の節が一致してしまう。subnet の節を先頭に移した。3 番目の節は自分で
bufsizeを設定してmk_edns_with_subnetを呼んでおり、--subnetが EDNS を含意する意図は明らかなので、設計変更ではなく順序の取り違えの修正と判断した。digも同じで、+subnetは明示的な+noednsがあっても EDNS を有効にする（実測確認済み）。あわせて
Map.putをMap.put_newに変え、--bufsizeを明示した場合はその値を尊重するようにした。なぜ既存テストで気づけなかったか
check_edns enables EDNS with subnet optionが%{subnet: "192.0.2.1/24"}という**:ednsキーを持たない手組みの map** を渡していたため。parse_args/1の出力とは形が違うので、影を作っていた節を素通りしていた。新しいテストはparse_args/1を通し、CLI が実際に作る形で検証する。変更内容
parse_prefix_length/1を追加（数字のみ受理）。parse_subnet_option/1からprefix_length!/2経由で使うcheck_edns/1の subnet 節を先頭へ移動、Map.put→Map.put_newsource_prefixの型をinteger()→non_neg_integer()（実装が非負を保証するようになったため。PR fix(cli): narrow parse_subnet_option/1 spec to the value it returns #85 のレビュー指摘が本 PR で成立した）parse_args経由の 3 通りの指定）検証
escript を実際にビルドして 5 パターンの終了コードと stderr を確認済み（上記 2 の出力）。
スコープ外（別 issue）
ECS のアドレスがマスクされていない。
digは192.0.2.1/24を192.0.2.0/24として送るが、tdig はclient_subnet: {192, 0, 2, 1}とホスト部を残す。RFC 7871 は SOURCE PREFIX-LENGTH を超えるビットを 0 にすることを求めており、prefix で精度を落とすという ECS の目的に反する。prefix 検証とは独立なので #88 に分離した。https://claude.ai/code/session_01YAjSJR67tWTvYDJLcQThbQ