fix(corekit): reject a bool where a maker converts an address itself (#508) - #539
Conversation
…508) - add `parse_ip_address()` to `pcapkit.corekit.fields.ipaddress`, which calls the existing `_reject_bool` before converting, and takes an optional `version` so a caller that pins the address family widens an `int` to the right one - route the seven `_make_*` sites that convert a caller-supplied address *before* the schema is built through it: `MH._make_opt_bid`, `MH._make_opt_lmaa`, `MH._make_fid_suboption`, `MH._make_opt_dmnp`, `MH._make_opt_lma_up`, `HIP._make_param_locator_set` and `TCP._make_mptcp_addaddr`. Each derives its option length or family flag from the converted address, so a bare `ipaddress.ip_address(True)` became `0.0.0.1` and #500's field-level guard could no longer tell it from a real address - leave `SwitchField` untouched: its `pre_process` delegates to the resolved field and is not reached on the pack path at all, so the guard #508 proposes putting there would be dead code - drop the now-unused `import ipaddress` from `tcp.py`, and document `parse_ip_address` on the ipaddress fields page Adds tests over three files, including one that re-derives the ten address-typed `SwitchField` declarations from `Schema.__fields__` so a new one cannot be added unnoticed. CI-equivalent unit suite: 1056 passed, with only the pre-existing `test_docstring_contract` failure that main already has.
9aac76c to
6e2387b
Compare
|
✅ GOOD TO MERGE — independently confirmed the issue's own proposed mechanism is wrong: on |
Detailed review (independent verification, falsify-not-bless)Head sha reviewed: The refuted mechanism — independently reproduced, not taken on the PR's wordThe PR's central claim is that #508 misdiagnosed the defect: it says a sf = SwitchField(selector=lambda pkt: IPv4AddressField())
resolved = sf({})
resolved.pack(True, {}) # -> FieldValueError: invalid IP address: must not be a bool...
resolved.pre_process(True, {}) # -> FieldValueError: invalid IP address: must not be a bool...Both raise on Re-derived count: 10 address-typed
|
- `TSOption.post_process` converted `ts_data` entries to addresses with a bare
`ipaddress.ip_address`, which takes a `bool` as the `int` it subclasses, so
`ts_data=[True, 5]` packed and reported `IPv4Address('0.0.0.1')` with no
exception. It runs on the packing path too, and `IPv4.make` accepts a
caller-built option schema, so this was reachable from the public API. All
three conversions now go through `parse_ip_address`, the fifth site of the
defect #481, #500, #539 and #540 fixed before it.
- `quick_start_data_selector` sized the nested Quick-Start suboption with a
hardcoded `SchemaField(length=5)` -- the width of a Request's `ttl` and
`nonce` alone. A well-formed 8-octet option decoded its nonce as 55 rather
than 933982136 and left three octets to be read as a fabricated option, so
the datagram failed with `ProtocolError`. The length now comes from
`quick_start_option_length`, computed from the resolved suboption, and
`QuickStartReportOption` gains the RFC 4782 section 3.1 `Not Used` octet it
was missing, which had made it seven octets wide against the `length=8` both
`_make_opt_qs` and `_read_opt_qs` use.
- `_make_opt_ts` passed `data=` where the schema field is `ts_data`, so every
timestamp was dropped with an `UnknownFieldWarning` and the Timestamp option
was unbuildable through `make`. The `TYPE_CHECKING` `__init__` stub that
advertised `data` is corrected too.
Three new tests, each shown to fail without its fix; `ipv4-option/TS` deleted
from `EXPECTED_FAILURES` now that it round-trips. Full unit tier green, 1107
passed with 2666 subtests; both changed modules at 100% statement and branch
coverage.
- `TSOption.post_process` converted `ts_data` entries to addresses with a bare
`ipaddress.ip_address`, which takes a `bool` as the `int` it subclasses, so
`ts_data=[True, 5]` packed and reported `IPv4Address('0.0.0.1')` with no
exception. It runs on the packing path too, and `IPv4.make` accepts a
caller-built option schema, so this was reachable from the public API. All
three conversions now go through `parse_ip_address`, the fifth site of the
defect #481, #500, #539 and #540 fixed before it.
- `quick_start_data_selector` sized the nested Quick-Start suboption with a
hardcoded `SchemaField(length=5)` -- the width of a Request's `ttl` and
`nonce` alone. A well-formed 8-octet option decoded its nonce as 55 rather
than 933982136 and left three octets to be read as a fabricated option, so
the datagram failed with `ProtocolError`. The length now comes from
`quick_start_option_length`, computed from the resolved suboption, and
`QuickStartReportOption` gains the RFC 4782 section 3.1 `Not Used` octet it
was missing, which had made it seven octets wide against the `length=8` both
`_make_opt_qs` and `_read_opt_qs` use.
- `_make_opt_ts` passed `data=` where the schema field is `ts_data`, so every
timestamp was dropped with an `UnknownFieldWarning` and the Timestamp option
was unbuildable through `make`. The `TYPE_CHECKING` `__init__` stub that
advertised `data` is corrected too.
Three new tests, each shown to fail without its fix; `ipv4-option/TS` deleted
from `EXPECTED_FAILURES` now that it round-trips. Full unit tier green, 1107
passed with 2666 subtests; both changed modules at 100% statement and branch
coverage.
…tes (#540) Follow-up to #508/#539/#552: bool is an int subclass, so a bare ipaddress.IPv4Address/IPv6Address/ip_address call in a _make_* helper silently laundered True/False into 0.0.0.1/::1 instead of raising. Four sites were deliberately left out of #539 because their files were owned by other work at the time; all four are now routed through the existing parse_ip_address helper, the same pattern #539 and #552 used. - ARP._make_proto_resolve (pcapkit/protocols/link/arp.py): addr=True packed as 00000001 (IPv4) or ::1 (IPv6) with no exception. - IPv6_Route._make_data_type_rpl (pcapkit/protocols/internet/ipv6_route.py): worse than a packed-address defect, since cmpr_i/cmpr_e are derived from the laundered value -- ip=[True] packed with cmpr_e=0 and an address of 00000001 instead of raising. - IPv6_Route.make's dst parameter: the most reachable of the four, on the public make() entry point; dst=True converted to ::1 silently. - OSPF._make_id_numbers: latent, no production caller today, fixed anyway so it does not resurface the defect the moment one is added. ARP and OSPF use self.__class__.__name__ rather than self.alias in the FieldValueError message, because their alias properties read state (_acnm, _version) that read() only assigns -- unavailable to a construction-only instance that never went through read(). Beyond bool rejection, pinning version=6 on the two IPv6_Route sites is a second, smaller behaviour change: both previously converted a plain integer through the bare, family-inferring ipaddress.ip_address, so ip=[258] packed a 4-octet IPv4 address (0.0.1.2) inside an IPv6-only header; it now packs the 16-octet IPv6 form (::102) instead, which is what an IPv6-only header should hold regardless of what an int happens to fit as an IPv4 address. Overlaps PR #561 (open, unmerged) in ipv6_route.py, which touches _read_data_type_rpl; this change stays inside make()/_make_data_type_rpl so the conflict on merge should be trivial. Four new tests, each shown to fail without its fix (FieldValueError not raised). Full unit tier green: 1120 passed, 8 skipped, 2673 subtests passed. CHANGELOG.md regenerated via util/changelog_md.py from docs/source/changelog/1.5.0.rst.
…tes (#540) (#568) Follow-up to #508/#539/#552: bool is an int subclass, so a bare ipaddress.IPv4Address/IPv6Address/ip_address call in a _make_* helper silently laundered True/False into 0.0.0.1/::1 instead of raising. Four sites were deliberately left out of #539 because their files were owned by other work at the time; all four are now routed through the existing parse_ip_address helper, the same pattern #539 and #552 used. - ARP._make_proto_resolve (pcapkit/protocols/link/arp.py): addr=True packed as 00000001 (IPv4) or ::1 (IPv6) with no exception. - IPv6_Route._make_data_type_rpl (pcapkit/protocols/internet/ipv6_route.py): worse than a packed-address defect, since cmpr_i/cmpr_e are derived from the laundered value -- ip=[True] packed with cmpr_e=0 and an address of 00000001 instead of raising. - IPv6_Route.make's dst parameter: the most reachable of the four, on the public make() entry point; dst=True converted to ::1 silently. - OSPF._make_id_numbers: latent, no production caller today, fixed anyway so it does not resurface the defect the moment one is added. ARP and OSPF use self.__class__.__name__ rather than self.alias in the FieldValueError message, because their alias properties read state (_acnm, _version) that read() only assigns -- unavailable to a construction-only instance that never went through read(). Beyond bool rejection, pinning version=6 on the two IPv6_Route sites is a second, smaller behaviour change: both previously converted a plain integer through the bare, family-inferring ipaddress.ip_address, so ip=[258] packed a 4-octet IPv4 address (0.0.1.2) inside an IPv6-only header; it now packs the 16-octet IPv6 form (::102) instead, which is what an IPv6-only header should hold regardless of what an int happens to fit as an IPv4 address. Overlaps PR #561 (open, unmerged) in ipv6_route.py, which touches _read_data_type_rpl; this change stays inside make()/_make_data_type_rpl so the conflict on merge should be trivial. Four new tests, each shown to fail without its fix (FieldValueError not raised). Full unit tier green: 1120 passed, 8 skipped, 2673 subtests passed. CHANGELOG.md regenerated via util/changelog_md.py from docs/source/changelog/1.5.0.rst.
1 participant
Closes #508.
The issue's stated mechanism is wrong, and correcting it is most of this change
#508 reads the defect as a dispatch problem: because
SwitchFieldpicks the concrete field at runtime, aboolsupposedly never reaches_IPAddressField.pre_process, so #500's guard cannot fire — which would put the fix inSwitchField. Measured, that is not what happens.SwitchField.pre_process(pcapkit/corekit/fields/misc.py:432-445, a range I re-verified is still accurate) inspects onlyself._fieldand delegates straight to the resolved field:So a
boolthat genuinely arrives at an address-selecting switch is already rejected, on every address-typed branch of every such switch — #500 covers it. A guard inSwitchFieldwould be dead code twice over, becauseSwitchField.packdelegates toself._field.pack(...), which calls the resolved field'spre_processdirectly;SwitchField.pre_processis not even invoked on the pack path.The real cause is upstream of the schema entirely. Seven
_make_*methods have to know the address family before they can build the schema — to size an option whose length is the only thing on the wire carrying that family — so they convert the argument themselves with bareipaddress.ip_address(). That conversion happens before the schema, so it laundersTrueinto a perfectly ordinaryIPv4Address('0.0.0.1')that #500's guard can then only see as a legitimate address. Worse, the option length and family flag are then derived from the laundered address, so the selector picks the matchingIPv4AddressFieldand no mismatch is left for anything to detect.What changed
parse_ip_address(value, description, version=None)added topcapkit/corekit/fields/ipaddress.py. It calls the existing_reject_boolas its first statement, then converts;versionpins the family where the wire format fixes it, so anintwidens correctly (258is::102forversion=6, but0.0.1.2for family-agnosticip_address). It raisesFieldValueError— the same class_IPAddressField.pre_processraises for the identical value, so a caller sees one exception whether theboolarrived through the schema or through a maker. (BoolErrorwould be wrong here: it means "must be a bool", and sits withIntError/BytesErrorin theTypeErrorblock.)misc.pyis untouched, for the reason above.import ipaddressdropped fromtcp.py(verified no remaining runtime use; theTYPE_CHECKINGimport is separate).parse_ip_addressdocumented indocs/source/pcapkit/corekit/fields/ipaddress.rst, naming the defining module rather than a re-export.Re-derived counts
Both of the issue's counts are wrong, in opposite directions.
Address-typed
SwitchFieldattributes: 10, not 7. Derived twice independently — statically, and at runtime fromSchema.__fields__:internet.hipLocator.valueinternet.hopoptSMFIdentificationBasedDPDOption.tidinternet.ipv6_optsSMFIdentificationBasedDPDOption.tidinternet.mhBindingIdentifierOption.addressinternet.mhDelegatedMNPOption.prefixinternet.mhLMAAddressOption.addressinternet.mhLMAUserPlaneAddressOption.addressinternet.mhMNIDOption.identifierinternet.mhTargetCareofAddressSuboption.addresstransport.tcpMPTCPAddAddress.addressThe three the issue's table misses are
hopopt/ipv6_optsSMFIdentificationBasedDPDOption.tidandmhBindingIdentifierOption.address— allConditionalField-wrapped, e.g.so a grep anchored on
= SwitchField(cannot see them. (The issue does mentionBindingIdentifierOptionin prose while omitting it from the table.)test_the_address_typed_switch_table_is_completenow pins this at 10 by rediscovering them fromSchema.__fields__, so the next one added cannot go unnoticed — that "nobody listed it" failure mode is what #491 and #508 share.Corrupting call sites: 7 makers, not 3. The issue lists 3, all in
mh.py. The four it misses areMH._make_fid_suboption,MH._make_opt_dmnp,HIP._make_param_locator_setandTCP._make_mptcp_addaddr. Observed wire output before the fix:address=TrueproducedMH._make_opt_bid23080001000000000001MH._make_opt_lmaa2906010000000001(vs2906010001020304for'1.2.3.4')MH._make_fid_suboption0506000000000001MH._make_opt_dmnp3706801800000001MH._make_opt_lma_up3b06000000000001HIP._make_param_locator_setip='::1'TCP._make_mptcp_addaddraddress=IPv4Address('0.0.0.1'),version=4_make_opt_dmnpneedsprefix_length <= 32to show it; the defaultprefix_length=64hides it behind the range check, which is why the issue's own sweep read that site as already guarded.HIP._make_param_locator_setis the most damning:ip=Trueandip='::1'were literally indistinguishable on the wire.TCP._make_mptcp_addaddrcannot be constructed end to end at all (#541), so its corruption was only ever visible on the schema the maker returns — #508 was right that it needed its own test, and it has one.Fails-without evidence
pytest-subtestsis not installed and pytest 9.1.1's native subtests print a failing subtest's parent asPASSED, so the exit code is the only honest signal. Read from a file, never through a pipe.All three runs below were measured on this branch after it was rebased onto
691f12ab5, rather than carried over from the pre-rebase measurement.Baseline, fix in place: exit code 0 — 80 passed, 584 subtests passed.
Proof 1 — revert all seven call sites, keep the helper: exit code 1. 18 failing subtests; only one parent produced a
FAILEDline. The other two tests (test_switch_backed_address_makers_reject_a_bool,test_both_bool_guards_stay_catchable_as_value_error_and_as_base_error) had failing subtests and were counted in "79 passed".Those seven distinct labels are the independent confirmation of the 7-maker count.
Proof 2 — remove only
_reject_bool(value, description)from insideparse_ip_address, leave all routing in place: exit code 1, 21 failing subtests, and zeroFAILEDparent lines — every parent reported as passing. This is the starkest form of the pytest hazard, and it shows the helper's guard is load-bearing rather than the routing alone.After each revert the tree was restored and verified byte-identical to the committed one.
Every measurement was taken with
PYTHONSAFEPATH=1andPYTHONPATHset to this worktree, withpcapkit.__file__printed and asserted to start with the worktree root first. That matters here: an editable install (__editable___pypcapkit_1_4_1_post2_finder) is present in the venv, and I confirmed it appends tosys.meta_path(:76), soPathFinderstill wins and the measurements are of this tree rather than the main checkout.Coverage
coverage run -m pytest(nopytest-cov) over the three test files:pcapkit/corekit/fields/ipaddress.py— 93%, missing lines67, 294, 401, 439, 489, 534. The newparse_ip_addressspans 115–216, so none of its lines are missing.pcapkit/protocols/internet/mh.py— 92%; the five changed makers all covered.hip.py(3081, 3086, 3091) andtcp.py(2907) are all covered.EXPECTED_FAILURESintests/protocols/test_option_roundtrip_unit.pyis still 55 (imported, not grepped — it uses**unpacking), i.e. this change neither fixes nor breaks an option round-trip.Local suite
Rebased onto
691f12ab5. The CI-equivalent unit selection (--ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py') is green: exit code 0 — 1058 passed, 8 skipped, 2602 subtests passed, zero failures, with nothing to disclaim.An earlier revision of this description reported one pre-existing failure on
main—tests/test_docstring_contract.py::DocstringParameterTests::test_known_defects_are_still_defects, from a staleKNOWN_DEFECTSentry forpcapkit/vendor/ipx/packet.py::process. #538 has since removed that entry, so it is gone. Re-measured on this rebased branch: that file alone runs at exit code 0, 7 passed, 13 subtests passed.GitHub Actions checks on this PR have not started — the queue is currently wedged rather than merely slow, so the run being absent says nothing about the change.
Found but deliberately NOT fixed
Everything in this section is a real, verified defect that is out of scope for this PR. Each was reproduced on this branch's tree, i.e. these are not fixed by the change above. All are now tracked:
ARP._make_proto_resolve(pcapkit/protocols/link/arp.py:400,402,True→00000001for IPv4 and…0001for IPv6, against01020304for a real address);IPv6_Route._make_data_type_rpl(pcapkit/protocols/internet/ipv6_route.py:737,745,753,764,768, worse than a wrong address — the bool becomes a 4-byteIPv4Addresswhich then corrupts the derived compression metadata:dst=IPv6Address('::1'), ip=[True]gavecmpr_i=16 cmpr_e=3 addresses=['01']);IPv6_Route.make'sdst(ipv6_route.py:314, same shape inmakerather than a_make_*); and the latentOSPF._make_id_numbers(pcapkit/protocols/link/ospf.py:318— same conversion, butgrep -rn '_make_id_numbers' pcapkit/returns only thedef, so no live caller).TCP._make_mptcp_addaddrcannot be constructed end to end (KeyError: 'length'frompcapkit/protocols/schema/transport/tcp.py:790). Independent of this fix and left alone; noted in a code comment at the call site.They were left out of this PR because its file ownership does not extend to
arp.py,ipv6_route.pyorospf.py, and because #481 fixing exactly one call site is how the root cause survived to become #491 — piecemeal is the failure mode, so the remaining sites deserve their own review rather than being tacked on here.parse_ip_addressis now the sanctioned way to do this conversion, so #540 has somewhere to route them.