fix(ethernet,pcapng): reject a non-string MAC or EUI address (#2222) by JarryShaw · Pull Request #2232 · JarryShaw/PyPCAPKit

JarryShaw · 2026-10-26T03:35:35Z

Searched for similar pull requests
Followed the coding style
make test passes, and a test case covers the change
Changelog entry: N/A — centralised in docs(changelog): shared 1.5.0 changelog — long-lived, merges last (#610, #616, #617, #618, #620) #657

What is the purpose of your pull request?

fix — bug fix

Description

Closes #2222. Ethernet._make_mac_addr sent a non-string address straight to PAT_MAC_ADDR.fullmatch, so Ethernet(src=True), dst=5, src=None and dst=1.5 raised a bare TypeError.

Fix: anything not str, bytes or bytearray, a bool included, raises ProtocolError. Every message in the three makers carries the protocol prefix (Ethernet: invalid MAC address: must be a str, bytes or bytearray, not True). int stays unaccepted, as in every release.
Test: tests/protocols/link/test_ethernet_mac_type_unit.py pins the message for bool, int, None and float over src and dst, and for a malformed string, and checks that str, bytes and bytearray still pack.
PCAP-NG: PCAPNG._make_mac_addr and PCAPNG._make_eui_addr had the same gap and get the same check and prefix. Their only callers are the IDB if_MACaddr and if_EUIaddr option builders, which tests/protocols/misc/test_pcapng_addr_type_2222_unit.py drives.
Fail-without-fix: on origin/main, the two test files give 23 failed: the bare TypeError, and the unprefixed malformed-string message.
Tests run: tests/protocols/link gives 112 passed, tests/protocols/misc/test_pcapng* 211 passed, and the docstring contract and tier guard 140 passed.

JarryShaw · 2026-10-26T03:39:56Z

Review: GOOD TO GO at 8bffb2992 (Sonnet).

Behaviour, run on the head: src=True and src=False raise FieldValueError. dst=5, None and 1.5 raise ProtocolError. str, bytes and bytearray still pack.
Consistency: the order matches ARP._make_addr_resolve's Ethernet branch.
Hierarchy: both exceptions subclass ValueError, not TypeError. The only caller is Ethernet.make, and no test pins the old TypeError.
Fail-without-fix: on origin/main, 10 failed with TypeError: expected string or bytes-like object.
Follow-up, in scope: PCAPNG._make_mac_addr and PCAPNG._make_eui_addr have the same gap, with no type check before fullmatch. They are being added to this PR, and a delta review follows.

JarryShaw · 2026-10-26T03:50:20Z

Review: GOOD TO GO at 068165323 (Sonnet delta review of 8bffb2992→068165323).

PCAP-NG: _make_mac_addr and _make_eui_addr check bool first (FieldValueError), then non-str, bytes or bytearray (ProtocolError). The docstrings match.
Callers: they are called only by the IDB if_MACaddr and if_EUIaddr option builders. The only PAT_* fullmatch sites under pcapkit/protocols are these two plus Ethernet and ARP, so no unguarded sibling remains.
Fail-without-fix: on origin/main, 10 failed (TypeError: expected string or bytes-like object).
Tests: test_pcapng*, tests/protocols/link, the docstring contract and the tier guard give 467 passed.

github-actions · 2026-10-26T06:19:44Z

Coverage: 91.56% (unit tier, Python 3.15, 9fd05a104, Unit Tests run success)

Package	Statements	Missed	Branches	Partial	Cover
`pcapkit (top level)`	104	4	20	2	95.16%
`pcapkit/const`	19599	881	2618	867	92.11%
`pcapkit/corekit`	2589	74	816	37	96.51%
`pcapkit/dumpkit`	282	3	102	3	98.44%
`pcapkit/foundation`	3023	23	1044	25	98.82%
`pcapkit/interface`	138	4	52	3	96.32%
`pcapkit/protocols`	17440	218	4590	185	98.15%
`pcapkit/toolkit`	869	36	298	10	95.20%
`pcapkit/utilities`	504	7	146	5	98.15%
`pcapkit/vendor`	4432	1988	1018	167	51.87%

Per-file detail: the coverage-html artifact of this run.

JarryShaw · 2026-10-26T06:45:03Z

Ready to merge at 068165323. Closes #2222, along with the PCAP-NG MAC and EUI makers.

Review: Sonnet GOOD TO GO, then a delta review of the PCAP-NG extension.
CI: all checks complete, Required checks passed SUCCESS, 0 failed.
KTLO: 12 legs, no warnings.
Ordering: merged into main and run. protocols leg: 1934 tests, 0 failures.
Conflicts: none against the current main.

JarryShaw · 2026-10-26T13:58:57Z

if we enforce str/bytes/bytearray, then why we'd still need the bool check? are we dropping the acceptance of int values?

JarryShaw · 2026-10-26T14:06:06Z

No int is dropped. None of the three makers has ever accepted one. At every release tag from v1.0.0 to v1.5.0b14, and on main, the body is addr.encode() if isinstance(addr, str) else addr, then PAT_*_ADDR.fullmatch. So an int raises a bare TypeError. This PR turns that into ProtocolError. Raw 6-octet bytes was never accepted either, since only the hex-text forms match.

You're right that the bool branch is redundant. bool is not a subclass of str or bytes, so the type check already rejects it. The branch only changes the class to FieldValueError. That class is a sibling of ProtocolError (both are BaseError, ValueError), so a caller catching ProtocolError would miss it.

Why the guard exists: it exists for ipaddress (fix: reject a bool IP address in-library instead of silently corrupting the packet #500, Address-typed SwitchField attributes bypass #491's bool guard: 3 mh.py makers still corrupt #508), where ip_address(True) silently gives 0.0.0.1. The MAC makers convert no int, so that can't happen here.
Why ARP has it: ARP keeps it only to match its IP sibling, _make_proto_resolve (fix(arp): a bool or int protocol address with a non-IP ptype raises TypeError, not FieldValueError #2166).

Recommendation: drop the bool branch, so that anything other than str, bytes or bytearray, bool included, raises ProtocolError. Accepting int would be a new feature, with open questions on byte order and 48- versus 64-bit, so it belongs in its own issue if you want it. I'll push the change once you confirm.

JarryShaw · 2026-10-26T14:49:11Z

take your recommendations and make sure the error message follows the conventions.

JarryShaw · 2026-10-26T15:06:50Z

Review: GOOD TO GO at 9fd05a104 (Sonnet). This revision takes the ruling: the bool check is dropped and the messages carry the protocol prefix.

Behaviour: True, False, 5, None and 1.5 raise ProtocolError in all three makers. str, bytes and bytearray still pack.
Messages: they follow the house <prefix>: invalid X: must be a …, not {v!r} form. {self.alias} gives Ethernet, and PCAP-NG matches the file's other errors.
Dependants: nothing in pcapkit/, tests/ or docs/ depends on the old strings or on FieldValueError.
Fail-without-fix: on origin/main, 23 failed (bare TypeError, and the unprefixed malformed-string message).
Tests: tests/protocols/link, the PCAP-NG tests, the docstring contract and the tier guard give 366 passed.
Follow-up: ARP._make_addr_resolve's Ethernet branch still has unprefixed Invalid MAC address messages. I'm filing that as its own issue.

JarryShaw · 2026-10-26T17:18:40Z

Ready to merge at 9fd05a104. Closes #2222, along with the PCAP-NG MAC and EUI makers.

Review: Sonnet GOOD TO GO on the ruling revision: the bool branch is dropped and the messages are prefixed.
CI: all checks complete, Required checks passed SUCCESS, 0 failed.
KTLO: 12 legs, no warnings.
Ordering: merged into main and run. protocols leg: 1940 tests, 0 failures.
Conflicts: none against the current main.

… ProtocolError (#2256) (#2257) ARP._make_addr_resolve raised unprefixed, capitalised messages ("Invalid MAC address: ...", "Invalid hardware address: ..."), gave a wrong type the same message as a malformed string, and rejected a bool with a separate FieldValueError that "except ProtocolError" missed. Drop the bool check, as #2232 did for Ethernet and PCAP-NG: any address that is not a str, bytes or bytearray raises "ARP: invalid MAC address: must be a str, bytes or bytearray, not ..." ("hardware address" for a non-Ethernet htype), and a malformed string raises "ARP: invalid MAC address: ...". Accepted forms are unchanged. The 1.5.0 changelog entry for #2166 and the generated CHANGELOG.md are corrected to match. Test: TestARPHardwareAddressType pins the exact messages for bool, int, None, float and a malformed string on Ethernet and Fibre Channel.

…2265) - #876 item 3: state why the no-else-raise pragma stays, not "the belief" - #652: the flagless HTTP/2 dump reads "Flags::0 [0]", as #648 renders it - #2172: section_context raises only on a block holding no section - add the missing #2222/#2232 MAC/EUI maker entry; cite #2257 on ARP - add the #1055/#2244 engine-tests and required-checks entry that #849's and #856's job descriptions now need - #800 names HTTP._guess_version, where the method lives; #631 states the old MANIFEST.in in the past tense - drop mentions of never-released names (pcapkit.utilities.timestamp, a test constant) - citation range runs to #2257; CHANGELOG.md regenerated

) No ProtocolError in pcapkit.protocols.internet.esp carried the "<prefix>: <lowercase description>: ... {value!r}" form #2232 and #2257 gave Ethernet, PCAP-NG and ARP: none had an "ESP:" prefix, some put "ESP" mid-sentence, and most printed the value bare. Every message raised by _resolve, CipherSuite.get, IntegritySuite.get, SecurityAssociation (SPI, destination, AEAD plus integrity, ICV length, compute_icv, decrypt, encrypt, _split_key), ESPContext.register and ESP.make now reads "ESP: <description>: ...", with the offending value in !r form. SecurityAssociation.unavailable gives the same prefixed reason. The traceback quoted in the module docstring and esp.rst follows. Test: ESPErrorMessageTests pins each message whole; the HAS_CRYPTO gate count in test_tier_guard rises by its one gated test.

IPv6_Ext's version gate raised "only valid for IPv6, got version=4" -- a bespoke tail unlike the <prefix>: <lowercase description>: must be X, not {value!r} form #2232/#2257 established, and it never named the version that was wanted. The same method's docstring quoted the message verbatim for the AH/ESP case, so it was stale by the same edit. Message is now "invalid version: must be 6, not {version!r}". Test: test_version_gate_message_is_in_the_house_form pins the message whole on both routes into the gate -- a bare-bytes call and the BytesIO/alias=/extension=True stream form.

…pe in option guards (#2398) #2300: the guards that reject a file's magic number or byte order had no PCAP:/PCAP-NG: prefix, unlike every other message in those modules, and PCAP's magic-number guard didn't say what it read. Four sites: schema/misc/pcap/header.py's magic_number_callback, misc/pcap/header.py's _make_magic (byte order unresolved/unknown), and two byte-order sites in schema/misc/pcapng.py that were the only two of nine messages in that file still missing PCAP-NG:. #2301: 128 PCAP-NG option guards in misc/pcapng.py ended in a full stop, unlike the house <prefix>: ... form (#2232, #2257), and the placement guard formatted self._type -- a plain int at make time, not the Enum_ BlockType it's cast to -- with bare {}, showing a raw number. A new _block_type_name() helper resolves either form through Enum_BlockType, idempotent for the enum case; applying the int form via .name directly would raise AttributeError, verified against pristine main. Also fixed: the if_tsresol/if_tzone option guards carried the identical full-stop defect, outside the issue's literal checklist but left would have made this consistency fix inconsistent with itself -- the file's full-stop count is now 0, which a new test asserts as an invariant. CHANGELOG.md regenerated (util/changelog_md.py) after correcting one docs/source/changelog/1.5.0.rst entry that quoted the old literal message. Tests: new tests/protocols/misc/test_capture_message_prefix_2300_2301_unit.py, 14 cases, 13 fail / 1 passes against pristine main (the one passing case is a characterisation pin of _type's runtime type, true on both trees).

…ne_name__ (#2399) The pcapkit.toolkit adapters put the engine name mid-sentence instead of leading with it, unlike the <prefix>: <lowercase description>: ... {value!r} form #2232/#2257 established, and hand-typed the name rather than reading it off the engine class's own __engine_name__. Each adapter module gains _ENGINE = <EngineClass>.__engine_name__ (plus the import), so a rename in pcapkit/foundation/engines/ cannot leave the adapter spelling the old name. Four modules, five sub-bugs: - pcap_ct.py/pypcap.py: "IPv4 reassembly is not supported by the pcap-ct engine: ..." -> "PCAP_CT: IPv4 reassembly is not supported: ..." (and the PyPCAP sibling). - pypcapfile.py: "invalid PyPCAPFile IPv4 address: 5" -> "PyPCAPFile: invalid IPv4 address: 5" (same for MAC address); the IPv6 UnsupportedCall and the "Frame N: TCP over IPv6 ..." warning get the same treatment. - pyshark.py: boolean-field, frame.time_epoch and no-frame-layer messages all reordered the same way. - scapy.py: the import-time warning reordered; the quoted package name is lowercased to 'scapy' to match the actual failing import and the sibling dependency warnings elsewhere in the codebase, which quote the installable name. The engine classes' toolkit imports are all function-local, so the new toolkit -> foundation.engines import introduces no cycle. Tests: new tests/toolkit/test_engine_message_prefix_2304_unit.py (9 tests) pins each message whole and asserts each module's _ENGINE is the engine class's own attribute. Existing pins in the four engines' test files updated for the new text.

JarryShaw added bug Issues reporting a defect (set by the bug report template; a default, not an assessment) fix Pull requests that fix a defect (fix: subject prefix) review: running A cross-review is in flight against the current head - no verdict yet labels Oct 26, 2026

github-project-automation Bot added this to PyPCAPKit Oct 26, 2026

JarryShaw added review: pending No verdict for the current head - never reviewed, or the head moved since the last one and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 26, 2026

JarryShaw added this to the PyPCAPKit v1.5 milestone Oct 26, 2026

JarryShaw changed the title ~~fix(ethernet): reject a non-string MAC address with ProtocolError (#2222)~~ fix(ethernet,pcapng): reject a non-string MAC or EUI address (#2222) Oct 26, 2026

JarryShaw force-pushed the fix/2222-ethernet-mac-type branch from 8bffb29 to 0681653 Compare October 26, 2026 03:46

JarryShaw added review: running A cross-review is in flight against the current head - no verdict yet and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 26, 2026

JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 26, 2026

JarryShaw moved this to In review in PyPCAPKit Oct 26, 2026

JarryShaw mentioned this pull request Oct 26, 2026

docs(api): document context, IPX._next_exthdr_depth and the pcapng schema helpers (#2234) #2251

Merged

5 tasks done

JarryShaw added the needs: decision Waiting on the maintainer to decide — not blocked by other work label Oct 26, 2026

JarryShaw moved this from In review to Needs decision in PyPCAPKit Oct 26, 2026

JarryShaw added wip Work in flight - a covering PR is open or an agent is actively on it and removed needs: decision Waiting on the maintainer to decide — not blocked by other work review: good-to-go Cross-review at the current head says ready; CI state is separate labels Oct 26, 2026

JarryShaw removed the wip Work in flight - a covering PR is open or an agent is actively on it label Oct 26, 2026

JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: running A cross-review is in flight against the current head - no verdict yet labels Oct 26, 2026

This was referenced Oct 26, 2026

fix(arp): hardware-address messages lack the protocol prefix, and a bool gets FieldValueError #2256

Closed

fix(arp): prefix the hardware-address messages and reject a bool with ProtocolError (#2256) #2257

Merged

JarryShaw merged commit a5864a6 into main Oct 26, 2026
65 checks passed

JarryShaw deleted the fix/2222-ethernet-mac-type branch October 26, 2026 18:13

github-project-automation Bot moved this from In review to Done in PyPCAPKit Oct 26, 2026

JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 26, 2026

JarryShaw mentioned this pull request Oct 27, 2026

fix(esp): give ESP's errors the ESP: prefix and !r values (#2297) #2366

Merged

5 tasks done

Navigation Menu

Repository navigation

fix(ethernet,pcapng): reject a non-string MAC or EUI address (#2222) - #2232

fix(ethernet,pcapng): reject a non-string MAC or EUI address (#2222)#2232
JarryShaw merged 1 commit into
mainfrom
fix/2222-ethernet-mac-type

JarryShaw commented Oct 26, 2026 •

edited

Loading

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026 •

edited

Loading

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

JarryShaw commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of your pull request?

Description

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

JarryShaw commented Oct 26, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

JarryShaw commented Oct 26, 2026 •

edited

Loading

github-actions Bot commented Oct 26, 2026 •

edited

Loading