fix(link,internet): reject a bool at the four remaining maker call sites (#540) - #568
Conversation
|
❌ NEEDS CHANGES Cross-model review (Opus 5; the PR was authored on Sonnet), head The fix itself is correct, complete, and correctly scoped. I verified the scope personally: 7 files, +238/−11, touching One change needed, plus one process matter that I want on the record. 1. An undocumented behaviour change beyond the bool fix. The four sites now pass So inside the IPv6-only RPL header, 2. The fails-without proof was obtained with Two smaller notes. The body labels On the #561 overlap: it is better than you claim. The body says the conflict "should be trivial to resolve". I ran the trial merge myself — Detail, including a weak assertion worth strengthening, follows. |
Detailed cross-review — #568 @
|
…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.
fb3b8ec to
6d2ceed
Compare
Correction from me on the
|
|
✅ GOOD TO MERGE Re-review after the force-push, head My blocking item is addressed, and addressed accurately. The delta is the two changelog files only — The new sentences say exactly what needed saying, with the real numbers:
And I checked the scoping rather than taking it, because "the two
So the entry documents the change that a user can actually observe and stays quiet about the two that they cannot. That is better than enumerating all four would have been.
The three body corrections from my first verdict are recommendations rather than blockers and I am not holding the PR for them: the "Full CI unit tier" label on what is a reduced selection, the stale And my The one remaining suggestion, non-blocking and for whenever that file is next open: Nothing further from me. |
Summary
Closes #540. Follow-up to #508/#539/#552:
boolis anintsubclass, so a bareipaddress.IPv4Address/IPv6Address/ip_addresscall inside a_make_*helper silently launderedTrue/Falseinto an address (0.0.0.1,::1) instead of raising. #539 fixed seven such sites but deliberately left out four whose files were owned by other work at the time; this PR fixes those four, all through the existingparse_ip_addresshelper (pcapkit/corekit/fields/ipaddress.py), the same pattern #539 and #552 used -- fixing the pattern in one pass rather than one site at a time, per the issue's own warning about how #481's single-site fix survived to become #491 and then #508.All four sites confirmed against
origin/mainat8cfd6ab01before fixing:ARP._make_proto_resolve--pcapkit/protocols/link/arp.py:399,401(post-fix). Before:_make_proto_resolve(True, IPv4)packed00000001(0.0.0.1),_make_proto_resolve(True, IPv6)packed::1-- no exception either way.IPv6_Route._make_data_type_rpl--pcapkit/protocols/internet/ipv6_route.py, five call sites (originally lines 737/745/753/764/768). Worse than a plain packed-address defect:cmpr_i/cmpr_eare derived from the laundered value, so a bool corrupts the compression metadata alongside the address list. Before:_make_data_type_rpl(ip=[True])->cmpr_i=0 cmpr_e=0 addresses=['00000001'];_make_data_type_rpl(dst=2001:db8::1, ip=[True])->cmpr_i=16 cmpr_e=0 addresses=['00000001'].IPv6_Route.make'sdstparameter -- originally line 314, the most reachable of the four since it's on the publicmake()entry point. Before:dst=Trueconverted to::1with no exception.OSPF._make_id_numbers-- originally line 328 (now 329 post-fix). Latent: nothing in the codebase calls it except a unit test (tests/protocols/link/test_link_unit.py:856pre-fix). Fixed anyway so it doesn't resurface the defect the moment a future caller reaches it -- the same kind of omission is how mh: reject a wrong-type MN-ID identifier in-library, not as a stdlib leak #481's fix survived to become IP address fields silently accept bool and corrupt the packet — unfixed sibling of #469/#481 at the shared root #491 and then Address-typed SwitchField attributes bypass #491's bool guard: 3 mh.py makers still corrupt #508.A second, smaller behaviour change beyond bool rejection
Passing
version=6toparse_ip_addresson the twoIPv6_Routesites (_make_data_type_rplandmake'sdst) does more than reject a bool -- it pins the address family for a plain integer too. Both previously converted through the bare, family-inferringipaddress.ip_address, under whichip=[258]packed a 4-octet IPv4 address (0.0.1.2) inside an IPv6-only header. Pinned toversion=6, the same input now packs the 16-octet IPv6 form (::102) instead:This is a deliberate improvement -- an IPv6-only header should not be able to hold a v4 address -- but it is a real change in packed output for non-bool integer input, called out here since it is not implied by "rejects a bool".
ARP._make_proto_resolveandOSPF._make_id_numbersare unaffected: ARP already dispatched to the explicitIPv4Address/IPv6Addressconstructor perptypebefore this fix, and OSPF's small-integer router/area IDs already resolved to IPv4 under the old family-inferring call, so pinningversion=4there reproduces the prior behaviour for every value that reached it (it is latent, so nothing did).Why
self.__class__.__name__instead ofself.aliasin ARP/OSPFIPv6_Route.aliasis a static string ('IPv6-Route'), so it's safe to use in theFieldValueErrormessage exactly like the existingProtocolErrorraises elsewhere in that file.ARP.aliasandOSPF.alias, however, readself._acnm/self._version, whichread()only assigns -- unavailable on a construction-only instance that never went throughread()(confirmed:object.__new__(ARP).make(...)and a fullARP(spa=..., tpa=...)construction both callmake()/pack()beforeunpack()/read()ever runs). Usingself.aliasthere would turn every call into anAttributeError, bool or not, and would have broken the pre-existing testtest_arp_make_builds_schema_with_resolved_addresses. Usedself.__class__.__name__instead, which needs no instance state.Overlap with #561
#561 is open and unmerged, and also touches
pcapkit/protocols/internet/ipv6_route.py(in_read_data_type_rpl, the read-path length guard) and adds tests to the same file this PR does,tests/protocols/internet/test_ipv6_extension_unit.py. This PR's edits toipv6_route.pystay entirely insidemake()and_make_data_type_rpl()(the construction path), a different method from_read_data_type_rpl. Verified rather than asserted:git merge-tree --write-tree <this-head> <561-head>exits0with a written tree and no conflict markers, so the two merge cleanly regardless of which lands first -- no manual resolution needed either way.Coverage
Four new tests, each proven to fail without its fix. Reverted the three production files only, using a uniquely-tagged
git stash push -u -m "<tag>" -- <the three files>, captured the stash entry's SHA immediately, restored withgit stash apply <sha>(neverpop), and dropped the entry once restored -- the shared stash stack this repo's other work may also be using was never touched by a barestash/pop. ConfirmedAssertionError: FieldValueError not raisedat every site before restoring:tests/protocols/link/test_link_unit.py::test_arp_proto_resolve_rejects_a_booltests/protocols/link/test_link_unit.py::test_ospf_id_numbers_rejects_a_booltests/protocols/internet/test_ipv6_extension_unit.py::test_ipv6_route_make_dst_rejects_a_booltests/protocols/internet/test_ipv6_extension_unit.py::test_ipv6_route_rpl_source_addresses_reject_a_bool(covers all three_make_data_type_rplshapes:dst=None,dstgiven with the bool as the sole/cmpr_eaddress, anddstgiven with the bool as acmpr_i-prefix address)tests/protocols/test_option_roundtrip_unit.py'sEXPECTED_FAILURESis unaffected -- the one IPv6-Route entry there (ipv6-route-type/RPL_Source_Route_Header) names a schema-layer defect inRPL.post_process(addressed by #561, not this PR's protocol-layer sites); confirmed unchanged by importingEXPECTED_FAILURESdirectly and by the roundtrip test still passing all 322 subtests.mypy --ignore-missing-importson the three changed production files: 7 pre-existing errors, same 7 after (one relocated from a bareipaddress.ip_addresscall toparse_ip_address, itself pre-existing onmainfor the samebytearray-vs-stub mismatch -- not a regression, not touched, out of this PR's narrowed scope).Test plan
pytest -q tests/protocols/link/test_link_unit.py tests/protocols/internet/test_ipv6_extension_unit.py-- 81 passed, 99 subtests passedpytest -q tests/protocols/test_option_roundtrip_unit.py -k round_trip_is_identity-- 322/322 subtests, unchangedpytest -q tests/project/test_changelog_md.pyandpython util/changelog_md.py --check--CHANGELOG.mdin step withdocs/source/changelog/1.5.0.rstpytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'-- 1120 passed, 8 skipped, 2673 subtests passedpytest tests-- 1264 passed, 17 skipped, 2855 subtests passed, exit 0 (samples regenerated first viapython examples/generators/make_samples.py, since a fresh worktree's*_runtime.py/regression fixtures are not committed)mypy --ignore-missing-importson the three changed modules -- no new errors vs.main