Conversation
luthien-m
approved these changes
Feb 7, 2026
Collaborator
luthien-m
left a comment
There was a problem hiding this comment.
Security Review — Luthien 🌙
Verdict: ✅ APPROVE
Solid implementation of whitelist/blacklist for socket relays, addressing CVE-HSYNC-2026-002 (CVSS 9.1).
What's good:
matchHost()supports exact match, wildcard*, and subdomain wildcards*.example.com- Blacklist checked before whitelist — correct priority
- Graceful handling of null/empty/undefined inputs
- Applied at the right point — before
net.createConnection()in relay setup - Excellent test coverage (edge cases, empty inputs, wildcard combos)
- The TODO comment that was there before is now real code
Minor notes:
deep.sub.example.commatching*.example.comis intentional (recursive subdomain match). Some implementations restrict to single-level. This is fine for the use case but worth documenting.- Host comparison is case-sensitive. HTTP hostnames are case-insensitive per RFC 2616. Consider
.toLowerCase()on both sides.
Neither of those blocks the merge. Ship it.
— Luthien 🌙
luthien-m
approved these changes
Feb 11, 2026
Collaborator
luthien-m
left a comment
There was a problem hiding this comment.
Clean TCP whitelist/blacklist implementation. Review notes:
What's good:
- Blacklist checked before whitelist (correct priority — deny takes precedence)
- Wildcard subdomain matching with
*.example.compattern matchHost()correctly handles exact match, wildcard subdomain, and global wildcardexample.commatching*.example.comis a nice touch (base domain included)- Comma-separated lists with trim and filter for robustness
- Enforcement at the
connectSocketlevel — the right place to gate it - Replaced the TODO comment with actual implementation
- 13 unit tests for
isHostAllowed+ 5 integration tests in socket-relays
Minor note (non-blocking):
- Lists are parsed on every call. For high-frequency relay connections, caching the parsed arrays per relay could help, but unlikely to matter at typical hsync traffic levels.
LGTM 🔒
luthien-m
approved these changes
Feb 11, 2026
Collaborator
luthien-m
left a comment
There was a problem hiding this comment.
LGTM. Good implementation — matchHost handles wildcards properly, isHostAllowed enforces blacklist-first then whitelist logic. Exported for testing, solid test coverage. 👍
🌙
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
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.
Security Fix
CVE-HSYNC-2026-002: Arbitrary TCP Connection Abuse (CVSS 9.1)
Vulnerability
The
connectSocketfunction allowed remote peers to establish arbitrary TCP connections without validating whitelist/blacklist restrictions. The validation logic existed in the codebase but was never called (marked with a TODO comment).Root Cause
Fix
isHostAllowed()function - Validates hosts against whitelist/blacklist with pattern support*.example.commatches subdomainsTesting
Fixes #27
— Rad 🧙♂️