fix: validate binary input in _from_bytes()

[sumanjeet0012]

Fixes #110

Description

This pull request adds strict validation to the binary multiaddr parser. It updates _from_bytes() to parse components iteratively using bytes_iter(), ensuring invalid bytes are rejected immediately upon initialization rather than failing later during conversion. It also updates bytes_iter() to catch truncated streams.

Changes

• Updated Multiaddr._from_bytes() to validate raw binary components fully and raise a BinaryParseError upon encountering unexpected bytes.
• Updated bytes_iter() to raise a BinaryParseError if a component's byte length is shorter than expected.
• Added test_invalid_bytes() to tests/test_multiaddr.py to test various invalid multiaddr byte strings.
• Added a towncrier newsfragment newsfragments/110.bugfix.

[acul71]

Thanks @sumanjeet0012 for this PR — it's a solid fix for #110. Validating in _from_bytes() via bytes_iter() and tightening short reads in bytes_iter() matches go-multiaddr's fail-fast behavior, and the three new tests cover the cases described in the issue. CI looks green, and the branch merges cleanly with master (rebasing onto latest master would still be good since you're one commit behind).

Before merge, a couple of items to address:

1. Newsfragment filename — Rename newsfragments/110.bugfix to newsfragments/110.bugfix.rst so python newsfragments/validate_files.py passes (make notes / release tooling). Towncrier draft already picks up the content today, but release validation will fail with the current name.
2. BinaryParseError handling in _from_bytes() — The broad except Exception re-wraps errors already raised by bytes_iter(), which duplicates messages and forces protocol: 0 on the outer error. Prefer letting BinaryParseError propagate (e.g. except BinaryParseError: raise).

Nice-to-have follow-ups: drop the redundant BinaryParseError import in test_invalid_bytes(), and optionally add a trailing-bytes-after-valid-prefix test.

Full review below.

---

AI PR Review: #125 — fix: validate binary input in _from_bytes()

PR: #125
Author: sumanjeet0012
Reviewer: AI-assisted review (prompt: downloads/py-multiaddr-pr-review-prompt.md)
Date: 2026-07-29
Branch reviewed: fix-issue-110 (local checkout)

---

1. Summary of Changes

Aspect	Detail
Type	Bug fix
Modules	multiaddr/multiaddr.py (_from_bytes), multiaddr/transforms.py (bytes_iter), tests/test_multiaddr.py, newsfragments/110.bugfix
Linked issue	#110 — _from_bytes() stored raw bytes without validation; invalid binary multiaddrs only failed on later str() / conversion
Breaking changes	Behavior change (intentional): Multiaddr(invalid_bytes) now raises BinaryParseError at construction instead of deferring failure. Callers that relied on lazy acceptance of bad bytes will need to handle errors earlier.

The PR validates binary input by walking components with bytes_iter(), checks for trailing unconsumed bytes (Go NewMultiaddrBytes parity), and strengthens bytes_iter() to raise when read() returns fewer bytes than the protocol requires.

---

2. Branch Sync Status and Merge Conflicts

Sync vs origin/master: 1 1 (one commit behind, one commit ahead — diverged by one commit each side).

Merge test: git merge --no-commit --no-ff origin/master completed with automatic merge of tests/test_multiaddr.py and no conflicts.

✅ No merge conflicts detected. The PR branch can be merged cleanly into origin/master.

Note: Rebasing or merging origin/master before merge is recommended so the branch is not behind.

---

3. Strengths

• Addresses the right failure mode: Fail-fast at Multiaddr(bytes) matches #110 and go-multiaddr's NewMultiaddrBytes() "read everything or error" model.
• Reuses existing parser: Validation via bytes_iter() avoids duplicating varint/protocol/codec logic.
• bytes_iter() truncation fix: Checking len(part) != size closes a gap where short reads could yield partial values silently.
• Tests: Three cases from the issue (bad varint, unknown protocol, short ip4 payload) assert immediate BinaryParseError on init.
• Docstring: _from_bytes() now documents BinaryParseError in Raises (helpful for API consumers).
• CI: All GitHub Actions checks on the PR were green at review time (tox core/docs/lint, Windows matrix).

---

4. Issues Found

Critical

None identified for the scenarios covered by #110 and the existing test suite.

Major

- File: newsfragments/110.bugfix
- Line(s): N/A (filename)
- Issue: Fragment is named `110.bugfix` but `newsfragments/validate_files.py` only allows extensions such as `.bugfix.rst` (see `ALLOWED_EXTENSIONS`). Running `python newsfragments/validate_files.py` fails with "Unexpected file: .../110.bugfix". This script is invoked from `Makefile` during release notes (`make notes`), so the fragment will block the release workflow even though `towncrier build --draft` accepts the file today.
- Suggestion: Rename to `newsfragments/110.bugfix.rst` (content unchanged). Confirm with `python newsfragments/validate_files.py` and `towncrier build --draft`.

- File: multiaddr/multiaddr.py
- Line(s): 519–520
- Issue: Broad `except Exception` re-wraps `BinaryParseError` already raised by `bytes_iter()` (and the new truncation check), producing nested, duplicated messages and forcing `protocol: 0` on the outer error. Example for unknown protocol 0x99: outer message includes both "protocol 0" and inner "protocol 153".
- Suggestion: Use `except exceptions.BinaryParseError: raise` before a narrower handler, or only catch non-`BinaryParseError` exceptions from `bytes_iter`. Preserve the original exception when already a `BinaryParseError`.

Minor

- File: tests/test_multiaddr.py
- Line(s): 594–595
- Issue: Redundant `from multiaddr.exceptions import BinaryParseError` inside `test_invalid_bytes()`; module already imports `BinaryParseError` at line 5.
- Suggestion: Remove the inner import.

- File: tests/test_multiaddr.py
- Line(s): 594–603
- Issue: No test for trailing bytes after a valid prefix (Go's "bytes leftover" case). In practice trailing garbage often fails during the next component parse (e.g. `ord()` / unknown protocol) rather than via the `consumed != len(addr)` branch; behavior is still fail-fast but error text may differ from Go's explicit leftover message.
- Suggestion: Add a case such as `Multiaddr(Multiaddr("/ip4/127.0.0.1")._bytes + b"\xff")` and document expected message (or assert only `BinaryParseError`).

- File: tests/test_multiaddr.py
- Line(s): 591
- Issue: `Multiaddr(1)` vs prior `Multiaddr(42)` — both invalid; change is unrelated to #110.
- Suggestion: Revert to `42` or leave as-is; no functional impact.

- File: multiaddr/multiaddr.py
- Line(s): 522–527
- Issue: The `unexpected extra data` guard is largely defensive if `bytes_iter()` always consumes the buffer or raises; still valuable for Go parity and future parser drift.
- Suggestion: No change required; optional comment linking to go-multiaddr leftover check.

---

5. Security Review

Area	Assessment
DNS/resolver paths	Unchanged
New external I/O	None
Input validation	Improved — invalid binary multiaddrs rejected at construction

- Risk: Previously, hostile or corrupted binary input could be stored and only fail at string conversion or downstream use, making failures harder to attribute and potentially allowing inconsistent internal state in callers that inspect `_bytes` without converting.
- Impact: low (correctness / fail-fast; not memory safety)
- Mitigation: Merge this validation; consider clearer `BinaryParseError` messages (avoid double-wrapping) for operators debugging bad peer records.

---

6. Documentation and Examples

• User-visible behavior change (immediate error on bad bytes) is reflected in _from_bytes() docstring; no README change strictly required.
• make docs-ci: Passed locally (Sphinx -W, no warnings).

---

7. Newsfragment Requirement

Criterion	Status
Issue number 110	✅
Type bugfix	✅
ReST-oriented user text	✅
Trailing newline	✅ (single line)
Filename per validate_files.py / README (*.bugfix.rst)	❌ — currently 110.bugfix
towncrier build --draft	✅ Includes #110 entry

---

8. Tests and Validation

Check	Result	Log
make lint	Pass	downloads/AI-PR-REVIEWS/125/lint_output.log
make typecheck	Pass	downloads/AI-PR-REVIEWS/125/typecheck_output.log
make test	364 passed, 1 skipped	downloads/AI-PR-REVIEWS/125/test_output.log
make docs-ci	Pass	downloads/AI-PR-REVIEWS/125/docs_output.log
newsfragments/validate_files.py	Fail (filename)	Run locally on PR branch

Tests cover the three invalid-byte cases from #110. Trailing-extra-bytes messaging and bytes_iter() truncation are only indirectly covered via the short ip4 case.

---

9. Recommendations for Improvement

1. Rename newsfragment to 110.bugfix.rst and run python newsfragments/validate_files.py.
2. Avoid re-wrapping BinaryParseError in _from_bytes() so protocol codes and messages stay accurate.
3. Remove duplicate BinaryParseError import in test_invalid_bytes().
4. Add a trailing-bytes test (valid prefix + garbage suffix) to lock in fail-fast behavior.
5. Merge or rebase latest origin/master (branch is one commit behind).

---

10. Questions for the Author

1. Was 110.bugfix without .rst intentional? towncrier picks it up, but validate_files.py / README expect 110.bugfix.rst — can you align with release tooling?
2. For trailing bytes after a valid component, is matching Go's exact "unexpected extra data" message a goal, or is any BinaryParseError at init acceptable?
3. Should BinaryParseError from bytes_iter() propagate unchanged (recommended), or should all binary init failures share a single outer message shape?

---

11. Overall Assessment

Dimension	Rating
Quality	Good — correct direction, aligns with #110 and go-multiaddr
Security impact	Low (improved validation; positive)
Merge readiness	Needs fixes — newsfragment filename for release tooling; polish exception handling
Confidence	High on #110 scenarios; Medium on error-message consistency and leftover-byte paths

Recommendation: Approve direction after renaming the newsfragment and avoiding double-wrapped BinaryParseError. No maintainer comments on the PR yet; none required for this review pass.