Skip to content

fix(traits): match RFC 9637 documentation /20 in is_public_ipv6#917

Draft
singhlovepreet9 wants to merge 1 commit into
marmot-protocol:masterfrom
singhlovepreet9:fix/ipv6-doc-range-mask
Draft

fix(traits): match RFC 9637 documentation /20 in is_public_ipv6#917
singhlovepreet9 wants to merge 1 commit into
marmot-protocol:masterfrom
singhlovepreet9:fix/ipv6-doc-range-mask

Conversation

@singhlovepreet9

@singhlovepreet9 singhlovepreet9 commented Jul 18, 2026

Copy link
Copy Markdown

Fixes #913

Problem

is_public_ipv6 mis-classifies a large slice of global-unicast space as non-routable. The guard meant to exclude the RFC 9637 documentation range 3fff::/20 was written as:

// crates/traits/src/app_components/host_safety.rs
if (first & 0xfff0) == 0x3ff0 {
    return false;
}

0xfff0 masks off only the low nibble of the first hextet, so the condition matches every first hextet in 0x3ff0..=0x3fff — i.e. 3ff0::/16 through 3fff::/16 — regardless of the second hextet, instead of the intended 3fff::/20.

Root cause

3fff::/20 fixes the full first hextet (0x3fff) plus the top nibble of the second hextet. The masked comparison collapses that to "first hextet in 0x3ff0..=0x3fff", over-rejecting:

  • 3ff0:: through 3ffe:: (entire hextets of ordinary global unicast), and
  • 3fff:: addresses past the /20 boundary (second hextet >= 0x1000).

All of these are inside global-unicast 2000::/3, so is_public_ipv6 returns false and the avatar-URL / encrypted-media SSRF validators (reject_non_routable_ipv6) wrongly reject legitimately-routable hosts. It fails closed (no SSRF exposure), but it is a concrete deviation from the stated RFC and the code comment, latent until IANA allocates 3xxx:: to the RIRs.

Fix

Match the actual /20: first hextet 0x3fff with the top nibble of the second hextet zero.

if first == 0x3fff && (second & 0xf000) == 0 {
    return false;
}

The guard still fails closed for the real documentation block and never newly accepts a non-routable address.

Verification

Toolchain pinned 1.90.0; just isn't installed locally so the equivalent cargo gates were run directly.

# regression added first — FAILS on the buggy mask (3ff0::1 wrongly rejected):
cargo test -p cgka-traits --test host_safety_public
  → thread panicked: 3ff0::1 should be accepted

# after the fix:
cargo test -p cgka-traits --test host_safety_public   # 6 passed
cargo test -p cgka-traits                              # lib 69 passed, integration + doctests ok
cargo fmt -p cgka-traits -- --check                    # clean
cargo clippy -p cgka-traits --all-targets              # clean, no warnings

Added regression coverage in tests/host_safety_public.rs locking both edges of the /20:

  • now accepted (were wrongly rejected): 3ff0::1, 3ffe::1, 3fff:1000::1, 3fff:ffff::1
  • still rejected (documentation range): 3fff::1 (lower edge) and 3fff:0fff::1 (upper edge)

The one existing assertion that depended on the buggy mask — 3fff:ffff::1 in group_avatar_url_rejects_documentation_ipv6_ranges (it is not documentation space) — was replaced with 3fff:0fff::1, a genuine in-range documentation address, so that test keeps asserting exactly what its name says.

Scope / risk

Three files, all in crates/traits. No previously-public address becomes rejected; the change only stops rejecting genuinely-routable space, so there is no new SSRF surface and no public-API change.


Open in Stage

Summary by CodeRabbit

  • Bug Fixes
    • Improved IPv6 address classification around the RFC 9637 documentation-only range.
    • Correctly rejects addresses within 3fff:0000::/20 while accepting adjacent global-unicast ranges.
    • Updated validation coverage for IPv6 addresses used in avatar URLs.

The 3fff::/20 documentation guard masked the first hextet with 0xfff0,
which matches every first hextet in 0x3ff0..=0x3fff regardless of the
second hextet. That covers 3ff0::/16 through 3fff::/16 rather than the
intended 3fff::/20, so global-unicast space (3ff0:: through 3ffe::, and
3fff:: past the /20 boundary) is classified as non-routable. The
avatar-URL and encrypted-media SSRF validators (reject_non_routable_ipv6)
then wrongly reject those legitimately-routable hosts.

Match the actual /20 instead: first hextet 0x3fff with the top nibble of
the second hextet zero. The guard still fails closed for the real
documentation block and never newly accepts a non-routable address.
@stage-review

stage-review Bot commented Jul 18, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 3 individual chapters for you:

Title
1 Correct the RFC 9637 IPv6 documentation mask
2 Update internal tests for documentation range
3 Verify routable and documentation IPv6 boundaries
Open in Stage

Chapters generated by Stage for commit 56a5074 on Jul 18, 2026 2:13am UTC.

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0c12b3ca-bd6c-4400-a054-750e4f813486

📥 Commits

Reviewing files that changed from the base of the PR and between e673cf8 and 56a5074.

📒 Files selected for processing (3)
  • crates/traits/src/app_components/host_safety.rs
  • crates/traits/src/app_components/tests.rs
  • crates/traits/tests/host_safety_public.rs

Walkthrough

The IPv6 classifier now identifies RFC 9637’s 3fff::/20 documentation range using both hextets. Tests cover addresses inside the range, neighboring public ranges, and avatar URL rejection.

Changes

IPv6 documentation-range classification

Layer / File(s) Summary
Correct the documentation prefix match
crates/traits/src/app_components/host_safety.rs
is_public_ipv6 now rejects only the 3fff::/20 documentation range.
Validate IPv6 range boundaries
crates/traits/tests/host_safety_public.rs, crates/traits/src/app_components/tests.rs
Tests verify rejected documentation addresses, accepted neighboring global-unicast addresses, and avatar URL rejection.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly states the core fix: correcting the RFC 9637 IPv6 documentation /20 handling in is_public_ipv6.
Linked Issues check ✅ Passed The code now matches 3fff::/20 precisely and the tests cover accepted and rejected boundary cases, satisfying #913.
Out of Scope Changes check ✅ Passed The changes stay focused on the IPv6 classifier and its tests, with no unrelated scope introduced.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

traits: is_public_ipv6 documentation-range mask rejects far more than RFC 9637 3fff::/20 (over-broad, latent host rejection)

1 participant