Fixed many bugs

[fedelemantuano]

Summary

Fixes 21 security findings in the header-parsing and sender-IP-attribution paths, all reachable from parse_from_string() / parse_from_bytes() on attacker-controlled input.

Three were reported by an external audit against 4.6.1; a fourth of the same family surfaced while reproducing them. The remaining 17 were found by four rounds of security-reviewer on the fix itself — including one High regression introduced by an earlier iteration of this branch (F11) and one pre-existing bug that was masking a broken test (F14).

Every finding has a reproducing PoC and a regression test. No public API signature changed, but several behaviours did — see Behaviour changes.

Root causes

Two mechanisms account for all 21 findings.

1. Header values were resolved through Python attribute lookup. MailParser exposes headers dynamically via __getattr__, and _make_mail() / headers iterated the sender's header names calling getattr(self, name). Python resolves real class attributes before __getattr__, so the sender chose which attribute was read — and the caller-facing conveniences (_→- folding, _json / _raw suffixes) were applied to names off the wire.

Fixed by MailParser._header_value(), a literal-lookup-only resolver backed by a per-parse header index. It performs no attribute access and no name rewriting. __getattr__ keeps the conveniences for names the caller types.

2. Sender-IP attribution tried to classify text by what it looks like. get_server_ipaddress() located the by clause with received_header.find("by"), then successive iterations of this branch tried to tell MTA-written text from sender-written text with substring searches, regex deletion, and unbalanced-delimiter heuristics. Each guard reopened a hole the previous one closed (F8→F11, F12→F16/F18, F9→F19), because a closed [...] pair the sender wrote is byte-identical to one the MTA wrote and EHLO [8.8.8.8] is a form RFC 5321 §4.1.3 requires of a client with no FQDN.

Replaced with one positional rule in _sender_ip_candidates():

• the first token of the from clause is the HELO name, whatever its shape — never a candidate;
• an explicit HELO marker inside a comment group ((helo=x), (account a@b HELO x)) is sender text — excluded;
• a candidate must sit inside a (/[ group — which is what makes a clause truncated by a multi-word HELO fail closed, since what it leaves behind is bare;
• IPv4 and IPv6 matches are merged positionally, never family-first.

Findings

Reported by the audit

#	Finding	Severity	Evidence
1	Quadratic CPU per distinct header name. Each getattr rescanned the whole header list via Message.get_all → O(distinct × total)	Medium 5.3, CWE-407	16,000 distinct names: 5.83 s → 0.033 s. 32,000 repeats of one name: 0.057 s (confirms distinct-name count is the driver, not size)
2	get_server_ipaddress() sender-IP spoofing via find("by") matching inside a hostname	7.5, CWE-345	derby.attacker.com → returned forged 6.6.6.6 instead of 1.2.3.4; benign nearby.example.org → None
3	mail_json crash: a header named after a method stored a bound method in the mail dict	Medium 5.3, CWE-407	Parse: x\r\n\r\n (12 bytes) → TypeError: Object of type method is not JSON serializable; 9 names confirmed

Finding 2 is rated higher than the 5.3 the audit proposed. The failure mode is not "returns nothing" — truncation kills extraction on the genuine hop, the loop falls through, and it returns the attacker's forged IP. That is an integrity failure in a value used for attribution and blocklisting, while the docstring promises a trust boundary. The HELO name needs no DNS control.

Found while reproducing

#	Finding	Severity	Evidence
4	Unbounded recursion via a Headers_json: header. The - {"headers"} guard did not cover the _json alias, and each cycle rebuilt the full header dict, compounding with finding 1	High 7.5, CWE-407	32 KB → 48.3 s (~1500× amplification, vs ~50× for finding 1). Fires during parse(), not lazily. Headers_json_json recurses too

Found by review of the fix

#	Finding	Severity	Evidence
F1	_json suffix applied to a wire name re-serialized the previous result — one output doubling per five input bytes	High 7.5, CWE-405	148-byte header → 536 MB string; 40 headers at depth 30 (~7 KB) → ~80 GB
F2	X_raw on an 8-bit header dumped an email.header.Header, an uncaught TypeError outside the MailParser* hierarchy	High 7.5, CWE-248	b"Subject: caf\xe9\r\nSubject_raw: x\r\n\r\n" (39 bytes) kills the worker
F3	HELO name left in the scanned clause; check[-1] takes the last IP, and Exim/CommuniGate record the HELO after the genuine IP	High 7.5, CWE-345	helo=[8.8.8.8] and HELO 8.8.8.8 both returned the attacker's address
F4	Clause with no from surfaced an IP from by / for / with / id — and for holds the envelope recipient, chosen at RCPT TO	Medium 5.9	for <bounce+8.8.8.8@x> → 8.8.8.8
F5	Wire header names were rewritten before lookup, so values vanished or were replaced by another header's	Medium 5.3, CWE-436	Subject_json: reported Subject's value; X_Spam_Flag: YES → ""; indicators absent from every output surface with has_defects == False
F6	Message.__len__ is the header count, so a header-less message is falsy: parse() returned before _reset() and every property answered ""	Medium 5.3, CWE-754	parse_from_string("Click http://evil.example/pay.exe now") → body == "", mail == "", no error
F7	headers deduped on exact spelling while values resolve case-insensitively — n casings of one name gave n keys each holding an n-element list	High 7.5, CWE-407	271 KB of headers → 7.6 s and 540 MB of JSON; 16,000 variants → 30.2 s → 0.0043 s
F8	Ending the clause at the first by let a multi-word HELO inject a clause keyword and truncate before the MTA-written IP	Medium	EHLO evil 8.8.8.8 by z → 8.8.8.8
F9	_HELO_RE's \S+ ate the MTA-written IP when the HELO name was literally helo	Medium 5.3	from helo ([45.33.32.156]) → None
F10	Failing closed on the first trusted hop lost the real sender IP on legitimate multi-hop chains through internal relays	Low 3.7	4 corpus chains regressed vs HEAD
F11	Regression from F8's fix. Extending the clause to the last by dragged in the for and envelope-from values, both sender-chosen	High 7.5, CWE-345	MAIL FROM:<"x 8.8.8.8 by q"@evil.example> → 8.8.8.8; the private-IP variant reached a fully forged header
F12	_HELO_RE deleted text rather than marking it, so an attacker token equal to helo before the MTA-written IP removed it	Medium 5.9, CWE-185	sendmail "may be forged" layout → attacker's 8.8.8.8
F13	REGXIP.findall(...) or REGXIP6.findall(...) is family-first: one IPv4 literal at EHLO suppressed the IPv6 scan entirely	High 7.5, CWE-345	EHLO 10.0.0.1 on an IPv6 hop → private → walk resumed into the attacker's header
F14	REGXIP6 matched inside the RFC 5321 IPv6: tag	Medium 5.3, CWE-20	[IPv6:2a00:1450:4864:20::32] → 6:2a00:1450:4864:20::, a different valid routable address
F15	Headers named after a computed part are dropped from mail / mail_json	Info, CWE-436	Documented; value remains in headers
F16	Bracketed EHLO [8.8.8.8] passed the unbalanced-delimiter test — a closed pair the sender wrote is indistinguishable from one the MTA wrote	High 7.5, CWE-345	[8.8.8.8] → reported verbatim; [10.0.0.1] → private → fall-through to forged header
F17	lone_token whitelisted a clause truncated to a single token	High 7.5, CWE-345	EHLO 8.8.8.8 by z → 8.8.8.8
F18	(?![\[(]) made _HELO_RE miss a genuine bracketed HELO argument	High 7.5, CWE-185	HELO [8.8.8.8] → sender's literal became the last candidate
F19	The width-preserving IPv6: blank used spaces, manufacturing a _HELO_RE lookbehind position inside the sender's own token	High 7.5, CWE-345	EHLO [8.8.8.8]IPv6:helo= → 8.8.8.8
F20	REGXIP6 alternation ordered ascending by trailing-group count; re takes the first match, not the longest	Medium, CWE-185	15 of 41 valid compressed forms truncated — 2001:470:1f0b:16c0::2:1 → …::2, still public, silently attributed
F21	mta_written sliced the whole prefix per match → O(n·m)	Medium 5.3, CWE-407	2.9 MiB header → 6.6 s, ratio ≥3 per doubling

Behaviour changes

Consumers should read these before upgrading.

get_server_ipaddress() — semantics changed materially, and deliberately.

• Walks trust-matching hops only while a hop names a private IP (internal relay). A hop naming no IP ends the search with None rather than falling through to older, sender-written headers.
• Only the from clause is searched, and only addresses inside a (/[ group written after the first token.
• A bare address clause (from 139.88.66.159, no HELO marker anywhere) now returns None. It is byte-identical to EHLO 139.88.66.159, so it fails closed. Exim/CommuniGate from [ip] (helo=x) still resolves, because the marker in the comment identifies the first token as MTA-written.
• Callers passing a trust string that matches multiple hops (e.g. a recipient domain rather than the border MTA) may now get None where they previously got an address from a lower hop.

headers

• Keys are deduped case-insensitively, keeping the first spelling seen; the value carries every occurrence, matching Message.get_all().
• Every header is reported, including names matching a MailParser method or property (Parse, Message, Headers_json) and names containing underscores (X_Spam_Flag). They are resolved as headers, never as attributes, and never rewritten.

mail / mail_json

• Header names are looked up literally — no _→- folding, no _json / _raw suffix interpretation.
• Headers named defects, defects_categories or has_defects are skipped so they cannot shadow the defect metadata with a value of a different type. Still reachable via headers and <name>_raw.

X_raw — always a JSON array ([] for an absent header, previously null), and 8-bit values are coerced to str instead of raising.

Attribute access — names beginning with _ now raise AttributeError instead of being answered as an absent header. This stops internal bugs being masked as "", and incidentally makes copy.deepcopy / pickle work.

Header-less messages — parse_from_string("body only") now parses the body and records the defect, instead of returning an object whose every property is "".

Tests

258 passing, 100% statement and branch coverage on all src/mailparser modules.

Every finding has a regression test asserting the failure mode, not just the fix — scaling assertions for the algorithmic ones, control matrices for the attribution ones, and a dir(MailParser) collision sweep for the reflective ones.

Three pre-existing tests were changed, each because it encoded a bug:

• test_issue_139 — the original fix for Processing breaks if a "headers:" email header exists #139 hid the literal headers: header from output rather than resolving it. Recursion is now impossible structurally, so the header is reported like any other. Expectation gained "headers": "hello-world".
• test_extract_ip_ipv6_fallback — passed only because of F14: REGXIP6 matched 6:2001:db8:: (public) instead of the real 2001:db8::1, which Python classifies as private (documentation range). The test's comment asserting "it is not private" was factually wrong. Now uses a genuinely public address and asserts the exact value.
• test_extract_ip_invalid_ip_returns_none — mocked REGXIP.findall, which the refactor replaced with finditer, so it silently stopped exercising the ValueError branch. Replaced with a direct _public_ip test needing no mocks.

Also updated

• .claude/agents/security-reviewer.md — the reviewer missed all four original findings. Its complexity guidance was regex-only and it had no notion of reflective dispatch or trust-boundary parsing. Added: probes must vary distinct-key count and total size independently (a size-only probe keeps the ratio flat and hides exactly this quadratic); a mandatory dir(MailParser) collision sweep covering collision / recursion cycle / side-effecting property; a 4-cell control matrix for attribution bugs that tests the fall-through, not just the single-header case; and an explicit instruction to reject denylist patches — which is how headers_json survived the headers fix.
• CLAUDE.md — the two invariants above, written as rules for future changes, including "do not add lookbehind guards to _HELO_RE", since three rounds of that each reopened a hole.
• README.md — the behaviour changes above.

Notes for release

Given the DoS findings are remotely triggerable on unauthenticated input, this warrants a patch release and GHSA advisories rather than riding to the next feature release.

[coveralls]

coverage: 100.0%. remained the same — security-issues into develop