fix: correct decapsulate for repeated protocols and substring collisions

[sumanjeet0012]

Fixes #109

Description

This pull request fixes a bug where Multiaddr.decapsulate() produced incorrect results when the same protocol appears multiple times in the address, or when the decapsulation target is a substring of a protocol value. The implementation has been updated to compare addresses component-by-component using bytes_iter() instead of performing a raw string search.

Changes

• Updated Multiaddr.decapsulate in multiaddr/multiaddr.py to match components iteratively instead of using str.rindex().
• Added tests to tests/test_multiaddr.py containing edge cases for:
  • Repeated protocols
  • Substring collisions
  • Protocol value collisions
• Added a towncrier newsfragment newsfragments/109.bugfix.

[acul71]

Thanks @sumanjeet0012

Full review here:

AI PR Review: #124 — fix: correct decapsulate for repeated protocols and substring collisions

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

---

1. Summary of Changes

Aspect	Detail
Type	Bug fix
Modules	multiaddr/multiaddr.py (Multiaddr.decapsulate), tests/test_multiaddr.py, newsfragments/109.bugfix
Linked issue	#109 — decapsulate() used string rindex, causing wrong splits when protocol values contained substrings that looked like other multiaddr segments
Breaking changes	None intended; behavior changes for pathological inputs (see Issues)

The PR replaces string-based suffix detection with component-by-component comparison via bytes_iter(), aligning with go-multiaddr Decapsulate for the cases described in #109.

---

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 still recommended so the branch is not behind.

---

3. Strengths

• Correct root cause: Moving off str.rindex() fixes real bugs (substring collisions in DNS names and repeated /ip4/… segments) that cannot be solved at the string layer.
• Spec-aligned algorithm: Sliding-window match over parsed components with “last match wins” mirrors go-multiaddr’s Decapsulate loop structure.
• Binary-safe: Slicing self._bytes[:offset] preserves canonical bytes instead of re-parsing a truncated string.
• Tests: Three focused cases from decapsulate() uses string rindex — produces incorrect results for repeated protocols #109 are added to existing test_decapsulate(); existing encapsulate/decapsulate tests still pass.
• Newsfragment content: User-facing wording is appropriate for a bugfix entry; issue number 109 is correct.
• 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 #109 and the existing test suite.

Major

- File: newsfragments/109.bugfix
- Line(s): N/A (filename)
- Issue: Fragment is named `109.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: .../109.bugfix". This script is invoked from `Makefile` during release notes (`make notes`), so the fragment will block the release workflow even though towncrier draft build accepts the file today.
- Suggestion: Rename to `newsfragments/109.bugfix.rst` (content unchanged). Confirm with `python newsfragments/validate_files.py` and `towncrier build --draft`.

- File: multiaddr/multiaddr.py
- Line(s): 247–260
- Issue: When `other` has zero components (e.g. `decapsulate("")` or `decapsulate(Multiaddr(""))`), the inner loop never runs and `match` stays `True`, so every start index is treated as a match and the last index wins—typically stripping the final protocol instead of no-op or error. go-multiaddr leaves `foundMatch` false for an empty `rightParts` and returns the original multiaddr unchanged.
- Suggestion: After building `other_components`, if it is empty, either `return self` (Go parity) or `raise ValueError` (consistent with Python’s “not contained” errors). Add a test documenting the chosen behavior.

Minor

- File: tests/test_multiaddr.py
- Line(s): 622
- Issue: Redundant `import pytest` inside `test_decapsulate()`; module already imports `pytest` at line 1.
- Suggestion: Remove the inner import.

- File: multiaddr/multiaddr.py
- Line(s): 259–260
- Issue: Variable name `last_match_end` holds the byte offset at the *start* of the matched suffix (correct for slicing, but slightly misleading for readers).
- Suggestion: Rename to `last_match_start` or add a one-line comment that the offset is the cut point before the matched suffix.

- File: tests/test_multiaddr.py
- Line(s): 616–629
- Issue: No explicit test that the *last* occurrence wins when the same suffix appears twice (only prefix-style repeated `/ip4` case). go-multiaddr semantics use the last matching index.
- Suggestion: Optional test, e.g. decapsulate `/tcp/80` on `/ip4/1.1.1.1/tcp/80/ip4/2.2.2.2/tcp/80` → `/ip4/1.1.1.1/tcp/80/ip4/2.2.2.2`.

---

5. Security Review

Check	Result
DNS/resolver paths	Unchanged
New external I/O	None
Input validation	Multiaddr(addr) parsing still applies; component comparison uses parsed binary values

- Risk: Decapsulate on attacker-controlled strings could previously produce wrong peer/transport views (logic bug), enabling mis-routing in higher layers—not classic memory safety, but correctness-sensitive for libp2p stacks.
- Impact: low (after fix); medium for unfixed substring cases in DNS-like values
- Mitigation: Merge component-level decapsulate; no additional security controls required beyond existing Multiaddr parsing.

---

6. Documentation and Examples

• Public API behavior of decapsulate is unchanged in signature and docstring; the fix is behavioral.
• No README/examples update required for this internal correctness fix.
• make docs-ci: Passed locally (Sphinx -W, no warnings).

---

7. Newsfragment Requirement

Criterion	Status
Issue number 109	✅
Type bugfix	✅
ReST-oriented user text	✅
Trailing newline	✅ (single line)
Filename per validate_files.py / README (*.bugfix.rst)	❌ — currently 109.bugfix

---

8. Tests and Validation

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

Tests cover success paths and one failure path (/tcp/8 on /dns4/example.com/tcp/80). Edge case for empty other is not covered.

---

9. Recommendations for Improvement

1. Rename newsfragment to 109.bugfix.rst and run python newsfragments/validate_files.py.
2. Guard len(other_components) == 0 in decapsulate and add a test (prefer return self for go-multiaddr parity, unless maintainers want strict ValueError).
3. Remove duplicate pytest import in tests.
4. Optionally add a “last suffix wins” test for repeated trailing segments.
5. Merge or rebase latest origin/master (branch is one commit behind).

---

10. Questions for the Author

1. Was 109.bugfix without .rst intentional? CI lint tox passed, but release validate_files.py will reject it—can you rename to match other fragments?
2. For an empty decapsulation target, should Python follow go-multiaddr (return self) or raise ValueError?
3. Did you compare behavior against js-multiaddr for the “decapsulate prefix → empty multiaddr” case already covered in test_encapsulate (comment references JavaScript)?

---

11. Overall Assessment

Dimension	Rating
Quality	Good — core fix is sound and well-motivated
Security impact	None (correctness fix; low residual risk once merged)
Merge readiness	Needs fixes — newsfragment filename must be corrected before release tooling; empty-other behavior should be decided and tested
Confidence	High on #109 scenarios; Medium on edge cases (empty other, release validation)

Recommendation: Approve direction after renaming the newsfragment and addressing empty-subaddress behavior (small follow-up). No maintainer comments on the PR yet; none required for this review pass.