Fixed many bugs

.claude/agents/security-reviewer.md

@@ -32,6 +32,12 @@ A security finding you cannot demonstrate is a guess. Before you report:
   pattern can be quadratic in isolation yet unreachable because an earlier step
   normalizes the input. Guard each probe with `signal.setitimer` so a true
   blowup fails fast instead of hanging.
+  **Complexity is not only regexes.** The dominant blowups in this codebase are
+  plain Python: a loop over attacker-chosen keys where each iteration rescans
+  the whole collection (`for k in keys: message.get_all(k)`) is O(keys × total)
+  with no regex involved. Vary the two axes *independently* — many distinct
+  header names vs. many repeats of one name — because a probe that only grows
+  the total byte count keeps the ratio flat and hides the quadratic term.
 - **Injection / traversal / write-primitive** — construct the malicious input
   and show the resulting command, path, or file write. A `../` that
   `os.path.basename` strips is not a finding.
@@ -62,6 +68,69 @@ A security finding you cannot demonstrate is a guess. Before you report:
 | Outlook `.msg` path (`extract_msg`, `msgconvert`)     | Untrusted `.msg` flows into third-party OLE/CFB + Perl parsers with their own CVE history — flag the transitive attack surface and the temp/subprocess handling around it.     |
 | Logging (`log.debug` of headers/filenames)            | Raw headers and attachment names are logged verbatim (PII / indicators). Data-handling note, not code-exec — report as Info.                                                   |
 | `eval`/`exec`/`pickle`/`yaml.load`/`__import__`       | Should be absent. Any occurrence is a finding until proven inert.                                                                                                              |
+| `getattr(self, X)` where `X` is a header/part name    | Reflective dispatch on an attacker-chosen name. See "Reflective dispatch" below — collision, recursion, and non-serializable leakage are all reachable from a 12-byte email.   |
+| `json.dumps` over a dynamically built dict            | Values arriving from reflection are not guaranteed JSON-safe. A bound method or `Message` object in the dict is an uncaught `TypeError` on a public property.                  |
+| `find()` / `in` / `split()` locating a security token | Naive substring search for a delimiter (`"by"`, a trust string) that an attacker also controls the *neighbouring* text of. See "Trust-boundary string parsing" below.          |
+
+## Reflective dispatch on attacker-controlled names
+
+`MailParser.__getattr__` exposes every header as an attribute, and `_make_mail`
+/ `headers` iterate `message.keys()` calling `getattr(self, name)`. The header
+name is attacker-chosen, so **the attacker picks which Python attribute is
+read**. Python resolves real class attributes *before* `__getattr__`, so any
+name in `dir(cls)` shadows the header path. Always run this probe:
+
+```python
+for a in [x for x in dir(MailParser) if not x.startswith("_")]:
+    try:
+        mailparser.parse_from_string(f"{a}: x\r\n\r\n").mail_json
+    except Exception as e:
+        print(a, type(e).__name__, e)
+```
+
+Three distinct bug classes fall out, and you must check for all three — finding
+one does not rule out the others:
+
+- **Method/property collision** — the dict gets a bound method or a live object
+  instead of a string. Downstream `json.dumps` raises `TypeError`. Crash-DoS on
+  a public API from a minimal message.
+- **Recursion cycle** — a property that itself iterates `message.keys()` and
+  calls `getattr` can re-enter itself when a header is named after that property
+  or one of its `_json` / `_raw` aliases. Check the cycle guard covers **every**
+  alias and is **case-insensitive**: `set(message.keys()) - {"headers"}` does not
+  exclude `Headers_json`, and `message.keys()` preserves the sender's casing.
+  A recursion cycle nested inside a per-key rescan multiplies the two costs —
+  measure it, it is usually the worst finding on the page.
+- **Side-effecting property** — reflection can *invoke* a property the caller
+  never asked for. Confirm no property in the collision set writes files, spawns
+  a subprocess, or mutates state.
+
+The fix to argue for is structural, not a denylist: header values must resolve
+through a lookup that never touches Python attributes. Reject patches that just
+add another name to an exclusion set — that is how the `headers_json` cycle
+survived the `headers` fix.
+
+## Trust-boundary string parsing
+
+`get_server_ipaddress(trust)` decides *which IP the mail came from* — its output
+is used for attribution and blocklisting, so a wrong answer is a security
+failure, not a cosmetic bug. Anywhere a security decision depends on locating a
+delimiter, check the search is anchored:
+
+- `header.find("by")` matches inside `derby.example.com` or `nearby.example.org`.
+  Hostnames come from the sender's HELO (no DNS control needed) and land in the
+  trusted MTA's own `Received` header.
+- Truncating the clause makes extraction fail on the *genuine* top hop, and the
+  loop then falls through to older, fully attacker-forged `Received` headers —
+  so the failure mode is not "returns nothing", it is "returns the attacker's
+  value". Always test the fall-through, not just the single-header case.
+- Use the existing anchored `const._CLAUSE_SPLITTER` rather than a bare `\bby\b`
+  (`\b` still matches inside `host.by.example`, since `.` is a non-word char).
+
+Prove these with a control matrix, not a single PoC: benign hostname, malicious
+hostname alone, forged header alone, and both together. If a benign hostname
+also misattributes, say so — it makes the finding a correctness bug too and
+raises the priority.
 
 ## Invariants that must stay true (verify, don't assume)
 

CLAUDE.md

@@ -61,6 +61,25 @@ Three access modes are supported for any attribute `X`:
 Address headers listed in `const.ADDRESSES_HEADERS` (`from`, `to`, `cc`, `bcc`, `reply-to`,
 `delivered-to`) return `list[tuple[str, str]]` (display_name, email) instead of plain strings.
 
+**Never resolve a header name with `getattr`, and never rewrite it**: `__getattr__` serves names
+the *caller* types, so it may fold `_`→`-`, honour the `_json`/`_raw` suffixes, and use `getattr`
+to reach computed parts (`attachments_json`). Names taken from a parsed message must go through
+`MailParser._header_value()`, which looks the name up literally in the header index and never
+touches a Python attribute. Both halves matter:
+
+- Python looks up real class attributes before `__getattr__`, so a sender naming a header `Parse`
+  or `Headers_json` otherwise picks which attribute is read — a bound method lands in the parsed
+  mail, or a property re-enters itself. Excluding individual names from a key set is not a fix;
+  that is what let `Headers_json` through after `headers` was excluded.
+- Suffix handling on a wire name is an amplifier and an evasion: each `_json` re-serializes the
+  previous result (five input bytes per doubling of the output), and `Subject_json:` reports the
+  value of `Subject` while silently dropping its own.
+
+**Header index**: `_build_header_index()` maps lowercased name → list of raw values once per
+parse. `Message.get_all()` is a linear scan, so the one call per distinct name previously made by
+`_make_mail()` cost O(distinct × total) on attacker-chosen names. Look up via the index inside any
+loop over header names.
+
 **RFC non-compliance fallback in `get_addresses()`**: Python's
 `email.utils.getaddresses(strict=True)` (hardened against CVE-2023-27043) rejects headers where
 the display name contains `@`. Since this is a forensics tool, `utils.py` applies
@@ -71,6 +90,28 @@ actually in the header.
 (`from`, `by`, `via`, `with`, `id`, `for`, `envelope-from`) using `const._CLAUSE_SPLITTER`.
 Output list is ordered first-hop first. Unparseable headers fall back to `{"raw": ...}`.
 
+**Sender-IP attribution fails closed**: `get_server_ipaddress()` walks trust-matching `Received`
+headers only while a hop names a *private* IP (an internal relay); a hop naming **no** IP ends the
+search with `None`. Never resume the walk on that case — older `Received` headers are written by
+the sender, so "extraction failed" would become "returns the sender's chosen IP".
+
+Candidate addresses come only from the `from` clause (`utils.get_from_clause()`, which ends at the
+next RFC 5321 keyword). Inside it, `_sender_ip_candidates()` applies one positional rule, because
+**text cannot be classified by what it looks like** — 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:
+
+- the **first token** is the HELO name, whatever its shape, and is never a candidate;
+- an explicit **HELO marker inside a comment group** (`(helo=x)`, `(account a@b HELO x)`) is
+  sender text and is excluded, located with `const._HELO_RE`;
+- a candidate must sit **inside a `(`/`[` group** (`utils.group_spans()`), which is what makes a
+  clause truncated by a multi-word HELO fail closed — what it leaves behind is bare;
+- IPv4 and IPv6 matches are merged **positionally**, never family-first: choosing IPv4 first let
+  one private literal at EHLO suppress the IPv6 scan and hide the real sender.
+
+Only concession: `from [ip] (helo=x)` (Exim/CommuniGate), accepted when there is no other
+candidate and the marker is in a group. Do not add lookbehind guards to `_HELO_RE` to patch new
+cases — three rounds of that each reopened a hole the previous one closed.
+
 **Defect detection**: During `parse()`, every MIME part is walked and `_append_defects()` records
 RFC violations. `EPILOGUE_DEFECTS` triggers special epilogue extraction to recover hidden payloads
 in malformed boundaries.
@@ -87,6 +128,10 @@ Accessible as `parser.mail` / `parser.mail_partial`.
 To make a header always appear in partial output, add its lowercase name to `OTHERS_PARTS` in
 `const.py`. Address-type headers (returning parsed name/email tuples) go in `ADDRESSES_HEADERS`.
 
+If the new part is computed by a `MailParser` property rather than read off the wire, add it to
+`COMPUTED_PARTS` as well — that set is exactly what `_make_mail()` is allowed to resolve through
+attribute access.
+
 ## Workflow
 
 After every change:

README.md

@@ -179,9 +179,16 @@ pipelines.
 - `body` - Complete message body
 - `text_html` - HTML body parts (list)
 - `text_plain` - Plain text body parts (list)
-- `headers` - All headers as a structured object
+- `headers` - All headers as a structured object. Keys are the header names exactly as they
+  appear in the message, and every header is reported — including names that match a
+  `MailParser` method or property (`Parse`, `Message`, `Headers_json`) and names containing
+  underscores (`X_Spam_Flag`). Such names are always resolved as headers, never as attributes,
+  and are never rewritten.
 - `attachments` - Complete attachment metadata and payloads
-- `get_server_ipaddress()` - Reliable sender IP extraction with trust levels
+- `get_server_ipaddress()` - Reliable sender IP extraction with trust levels. Only the `from`
+  clause of the first trusted `Received` header is searched, with the sender-supplied HELO name
+  removed, and the result is `None` when that hop names no public IP. Attribution never falls
+  back to older `Received` headers, which the sender is free to forge.
 - `to_domains` - Extracted recipient domains for analysis
 - `timezone` - Detected timezone information
 - `defects` - RFC compliance issues for security analysis
@@ -209,6 +216,12 @@ access the `X-MSMail-Priority` header:
 mail.X_MSMail_Priority
 ```
 
+This underscore-for-hyphen convenience, and the `_json` / `_raw` suffixes, apply only to
+attribute access written by you. Names read out of a message — the keys of `mail` and `headers` —
+are looked up literally, so a header genuinely named `X_Spam_Flag` or `Subject_json` keeps its own
+name and its own value. Attribute names beginning with an underscore are not headers and raise
+`AttributeError`.
+
 The `received` header is intelligently parsed into individual hops, revealing the complete email
 routing path. Each hop contains structured fields:
 

src/mailparser/const.py

@@ -25,16 +25,21 @@
 )
 
 # IPv6 pattern - matches standard and common compressed forms per RFC 5952
+# Alternation order matters: Python's ``re`` takes the first alternative
+# that matches, not the longest.  The "trailing ::" branch therefore has to
+# come *after* every branch that continues past the ``::`` — with it listed
+# early, ``2a00:1450:4864:20::32`` matched only as ``2a00:1450:4864:20::``,
+# reporting a different, valid, routable address as the sender.
 REGXIP6 = re.compile(
     r"(?:(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}"  # full form
-    r"|(?:[0-9a-fA-F]{1,4}:){1,7}:"  # trailing ::
-    r"|(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}"  # :: with 1 group after
-    r"|(?:[0-9a-fA-F]{1,4}:){1,5}(?::[0-9a-fA-F]{1,4}){1,2}"
-    r"|(?:[0-9a-fA-F]{1,4}:){1,4}(?::[0-9a-fA-F]{1,4}){1,3}"
-    r"|(?:[0-9a-fA-F]{1,4}:){1,3}(?::[0-9a-fA-F]{1,4}){1,4}"
+    r"|[0-9a-fA-F]{1,4}:(?::[0-9a-fA-F]{1,4}){1,6}"  # 6 groups after ::
     r"|(?:[0-9a-fA-F]{1,4}:){1,2}(?::[0-9a-fA-F]{1,4}){1,5}"
-    r"|[0-9a-fA-F]{1,4}:(?::[0-9a-fA-F]{1,4}){1,6}"
+    r"|(?:[0-9a-fA-F]{1,4}:){1,3}(?::[0-9a-fA-F]{1,4}){1,4}"
+    r"|(?:[0-9a-fA-F]{1,4}:){1,4}(?::[0-9a-fA-F]{1,4}){1,3}"
+    r"|(?:[0-9a-fA-F]{1,4}:){1,5}(?::[0-9a-fA-F]{1,4}){1,2}"
+    r"|(?:[0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}"  # 1 group after ::
     r"|:(?::[0-9a-fA-F]{1,4}){1,7}"  # ::x:x...
+    r"|(?:[0-9a-fA-F]{1,4}:){1,7}:"  # trailing ::
     r"|::)"  # just ::
 )
 
@@ -75,6 +80,35 @@
 # not collapse spaces, so this guard must live at the point of use.
 _WS_RUN_RE = re.compile(r"\s+")
 
+# Matches the HELO/EHLO name an MTA records inside the ``from`` clause,
+# in the two shapes seen in the wild:
+#   from evil.example ([203.0.113.9]:45321 helo=[8.8.8.8]) by ...   (Exim)
+#   from [203.0.113.9] (account x@y HELO 8.8.8.8) by ...      (CommuniGate)
+# The HELO argument is chosen by the sender and may be an RFC 5321 address
+# literal, so it must be removed before scanning the clause for the sender
+# IP — otherwise the sender simply appends the IP they want reported.
+# Used to locate the span, never to delete it: the trailing \S+ would
+# otherwise swallow whatever follows, and a sender whose rDNS or HELO is
+# literally ``helo`` can place that word right before the MTA-written IP.
+# Two guards keep the span off MTA-written text.  The lookbehind: the
+# clause value *starts* with the HELO name, so at offset 0 the word is the
+# name itself, not a label introducing one.  The ``(?![\[(])`` on the
+# space-separated form: an MTA writes its own address inside brackets or
+# parentheses, so a following group is never part of the HELO argument.
+# Exim's ``helo=[8.8.8.8]`` is a genuine bracketed HELO argument and keeps
+# no such guard.
+_HELO_RE = re.compile(
+    r"(?<=[(\s])e?helo\s*=\S*"  # Exim "helo=value": after "(" or space
+    r"|(?<=\s)e?helo\s+\S+",  # space form: only after whitespace
+    re.I,
+)
+
+# RFC 5321 §4.1.3 tags an IPv6 literal as ``[IPv6:2a00:...]``.  REGXIP6
+# happily starts matching at the ``6`` of the tag and yields
+# ``6:2a00:1450:4864:20::`` — a different, valid, routable address that an
+# analyst would then act on.  Blank the tag before scanning.
+_IPV6_TAG_RE = re.compile(r"IPv6:", re.I)
+
 # Extracts envelope-from email: envelope-from <addr>
 _ENVELOPE_FROM_RE = re.compile(r"<([^>]+)>")
 
@@ -108,3 +142,26 @@
         "x-original-to",
     ]
 )
+
+# Subset of OTHERS_PARTS that MailParser computes itself: each maps to a
+# property, not to a header read off the wire.  ``_make_mail()`` resolves
+# only these names through attribute access; every other key it handles is
+# a header name chosen by the sender and goes through
+# ``MailParser._header_value()``, which never touches an attribute.  Keep
+# this set in sync with OTHERS_PARTS and with the properties in core.py.
+#
+# These names shadow a header of the same name in ``mail`` / ``mail_json``:
+# a message carrying a literal ``Body:`` header reports the computed body
+# there, not the header value.  The wire value is never lost — it is in
+# ``headers`` / ``headers_json`` under its own name — but a consumer
+# reading only ``mail_json`` will not see it.
+COMPUTED_PARTS = set(
+    [
+        "attachments",
+        "body",
+        "date",
+        "received",
+        "timezone",
+        "to_domains",
+    ]
+)