fix(node): close three host-class gaps in the public-URL gate - #346
Open
beardthelion wants to merge 1 commit into
Open
fix(node): close three host-class gaps in the public-URL gate#346beardthelion wants to merge 1 commit into
beardthelion wants to merge 1 commit into
Conversation
is_public_http_url is the shared gate for every operator-supplied outbound URL: peer announce, webhook creation, and the boot-time peer prune. Three host forms got past it that the checks around them plainly meant to exclude. A name with no dot never matched the `.local` / `.internal` suffix rules, so `http://internal/` and `http://wpad/` were accepted and left to the resolver to complete from its search domain. The dotless rule sits on the branch where the host did not parse as an IP literal, which keeps bracketed IPv6 (also dotless) on the accepting path; the existing 6to4 and NAT64 accept cases pin that. fec0::/10 fell through the IPv6 arm because the link-local mask does not cover it: 0xfec0 & 0xffc0 is 0xfec0, not 0xfe80. RFC 3879 deprecated the range, which argues for rejecting it rather than ignoring it. Trailing root dots were stripped once, so `localhost..` reduced to `localhost.` and matched neither the equality nor the suffix check. Stripping to fixation closes that. Each of the three is covered by a test that reddens when its own production line is reverted, and the dotted forms stay as negative controls so the suffix checks remain proven.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesPublic HTTP URL validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The PR tightens public-URL host validation for dotless names, deprecated IPv6 site-local addresses, and repeated trailing dots; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:node
14 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #339.
is_public_http_urlis the shared host-class gate for every operator-supplied outbound URL: peer announce, webhook creation, and the boot-time peer prune. Three host forms got past it that the checks around them plainly meant to exclude.Dotless names.
.localand.internalare suffix tests, so a single label matches neither, andhttp://internal/,http://wpad/,http://metadata/were accepted and left to the resolver to complete from its search domain.POST /api/v1/peers/announceis unauthenticated and a peer row'shttp_urldrives outbound sync-notify fan-out, so the caller choosing that name need not be anyone in particular.The new rule sits on the branch where the host did not parse as an IP literal. That placement is the whole trick: a bracketed IPv6 literal is also dotless, and the existing 6to4 and NAT64 accept cases pin that it stays accepted.
fec0::/10. The IPv6 arm missed it because the link-local mask does not cover it:0xfec0 & 0xffc0is0xfec0, not0xfe80. RFC 3879 deprecated the range in 2004, which argues for rejecting it rather than ignoring it.Repeated root dots. Trailing dots were stripped once, so
localhost..reduced tolocalhost.and matched neither the equality nor the suffix check. Stripping to fixation closes it. I could not resolve that name locally, so it is a predicate gap rather than a demonstrated reachable target.Verification
Each of the three is covered by a test that goes red when its own production line is reverted, checked one at a time rather than as a batch:
rejects_dotless_single_label_hostsfec0::/10clauserejects_deprecated_site_local_v6rejects_repeated_trailing_root_dotsThe dotted forms stay in the tables as negative controls so the suffix checks remain proven, and the IPv6 accept cases guard against the dotless rule over-rejecting.
cargo test -p gitlawb-node --bin gitlawb-node --lockedis 826 passed / 0 failed, withfmtandclippy --workspace --all-targets -- -D warningsclean.Scope
Predicate only. The larger gap, that nothing validates the address a hostname actually resolves to, is #340 and needs a resolver policy on both outbound clients rather than a change here.
This touches the same span of
peers.rsas #333, so whichever lands second wants a rebase. The conflict is adjacent lines, not competing logic: #333 adds.localhostto the suffix list, this adds a dotless branch further down. Thenode.localhost..case is deliberately not asserted here, since.localhostis not a rejected suffix until #333 lands.Summary by CodeRabbit