Attach structured diagnostics to every ProtocolError - #127
Merged
Conversation
Strictness is this library's security story -- it raises where scapy silently fills in defaults -- but every exception carried only a formatted string. Verified: the full attribute set on a raised TruncatedHeaderError was args, add_note, with_traceback. There was no offset, no field, no protocol, no expected/actual, so a fuzzing harness, conformance suite, or protocol-validation tool that wanted to know where a parse failed had to regex the message. Every raise site in src/ now attaches: try: packet = decode_frame(frame) except ProtocolError as e: print(e.protocol, e.field, e.offset, e.frame_offset, e.expected, e.actual) # <class 'netprotocols.layer3.ip.IPv4'> ihl 0 14 >=5 0 protocol is set at all 62 raise sites bar one, verified by an AST sweep rather than by eye (tests/test_diagnostics.py pins the sweep itself as a regression test, confirmed to fail when a site is reverted). The one exception is random_mac()'s manufacturer check, which validates a free-standing argument belonging to no header -- documented at the site and asserted by its own test. offset is scoped honestly rather than uniformly: relative to the data argument of a decode() call for a fixed-header error, relative to the bytes attribute field names (options, body, sections) for an on-demand parse -- a TCP option error's offset is relative to header.options, never the frame -- and None, never guessed, for a __post_init__ validation error, which sees field values and never the bytes they came from. decode_frame is the only code holding the cursor needed to rebase a layer-relative offset to the whole frame, so it is the only thing that sets frame_offset: added to the walk's except block, running whether the walk is strict or lax, so a caller catching the re-raise in strict mode also gets a correct frame_offset. A bare SomeClass.decode() call leaves it None -- there is no frame to rebase against. validate_mac_addr() and validate_ipv4_addr() gained optional protocol=/field= parameters so the three __post_init__ call sites (Ethernet, ARP, IPv4) can attach the field they are validating; both defaults keep the utilities usable standalone with no context. No message string changed to make any of this true -- every existing str(err) and match= assertion in the suite holds unchanged, which tests/test_diagnostics.py also pins directly. New public name: MaxDepthExceededError, introduced by decode_frame, gets the same structured attributes as every other ProtocolError subclass. Closes#91. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QJnVMNGwTRDktC4rkABtgt
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Sep 4, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Strictness is this library's security story — it raises where scapy silently
fills in defaults — but every exception carried only a formatted string.
Verified in the issue: the full attribute set on a raised
TruncatedHeaderErrorwasargs,add_note,with_traceback. A fuzzingharness, conformance suite, or protocol-validation tool that wanted to know
where a parse failed had to regex the message.
Closes#91.
What's included
ProtocolErrorgains five optional attributes —protocol,field,offset,frame_offset,expected/actual— all defaulting toNone, allsubclasses inheriting the same
__init__automatically.protocolis set at all 62 raise sites bar one, verified by an AST sweeprather than by eye —
tests/test_diagnostics.py::TestEveryRaiseSiteIsInstrumentedpins the sweep itself as a regression test (confirmed to fail when a site is
reverted, see Notes). The one exception is
random_mac()'s manufacturer-codecheck, which validates a free-standing argument belonging to no header;
documented at the site and asserted by its own test.
offsetis scoped honestly rather than uniformly:dataargument of adecode()call, for a fixed-headererror (
_unpack_fixed, IHL/data-offset checks).bytesattributefieldnames (options,body,sections), for an on-demand parse — a TCP option error's offset isrelative to
header.options, never the frame.None, never guessed, for a__post_init__validation error, which seesfield values and never the bytes they came from.
decode_framerebases toframe_offset. It's the only code holding thecursor needed to translate a layer-relative offset to the whole frame, so it's
the only thing that sets it — added to the walk's
exceptblock, runningwhether the walk is strict or lax, so a caller catching the re-raise in strict
mode also gets a correct
frame_offset. A bareSomeClass.decode()callleaves it
None— there's no frame to rebase against.validate_mac_addr()/validate_ipv4_addr()gained optionalprotocol=/field=parameters so the three__post_init__call sites(
Ethernet,ARP,IPv4) can attach the field they're validating; bothdefaults keep the utilities usable standalone with no context, unchanged.
Docs — README gains a short example; ARCHITECTURE.md's error-hierarchy
section explains the offset-scoping rule and the
MaxDepthExceededErrorbranch it was missing; CHANGELOG under
## [Unreleased];docs/CLAIMS.mdgains 5.9.
Verification
uv run ruff checkanduv run ruff format --checkare cleanuv run mypyis clean (strict)uv run pytestpasses locally — 916 passed, coverage 99.88% (gate 98%)CHANGELOG.mdhas an entry under## [Unreleased]No message string changed. Every existing
str(err)andmatch=assertion in the suite holds unchanged —
test_message_text_is_unaffected_by_the_kwargspins this directly, and the full 916-test run (all pre-existing assertions
included) is the broader proof.
New protocol or dispatch change — also:
decode()/__post_init__behaviour changed, only what accompanies a raise
EtherType/IPProtocolnumberstests/test_fuzz.py::ALL_PROTOCOLSis unchangedNotes
On "carries at least protocol" being genuinely enforced, not just asserted.
TestEveryRaiseSiteIsInstrumentedwalks the AST of every file undersrc/,finds every raise of a
ProtocolErrorsubclass, and assertsprotocol=is akeyword at each one (with a named, tested exemption for the one that isn't). I
verified the test actually catches a regression before trusting it: reverted
one site's kwargs locally, watched the test fail naming that exact
file:line, then restored it. That's stronger than the acceptance criterionasks for — the criterion is satisfiable by a one-time sweep, this makes it a
standing gate.
On the offset design. The issue asks the two named design questions to be
decided: "Decide whether the exception carries a relative offset that
decode_framerebases, or whether both are exposed" (both —offsetstayslayer-relative and immutable regardless of call path,
frame_offsetis thewalker's rebased view, set only when a walker was involved) and "Keep the
message strings as they are" (verified directly, see above). The corpus figure
in the issue's own count (
~55) was off — the real count is 62, whichdocs/CLAIMS.md5.9 states precisely rather than repeating the estimate.On
fieldfor on-demand parses. TCP options, IPv4 options, DHCP options,IGMP group records, ICMP NDP options, and DNS's section parsing all raise
from a property reading an already-materialized
bytesattribute, not fromdecode()itself. Rather than either omittingoffsetthere (losing realinformation) or falsely claiming it's relative to a
decode()buffer thatisn't in scope,
fieldnames which attribute the offset is relative to —consistent across all six call sites, and it's what let the DHCP/TCP/IPv4/DNS
tests assert exact offsets rather than just "an offset was set."
🤖 Generated with Claude Code
https://claude.ai/code/session_01QJnVMNGwTRDktC4rkABtgt
Generated by Claude Code