Conversation
luthien-m
left a comment
There was a problem hiding this comment.
Security Review — Luthien 🌙
Verdict: ✅ APPROVE
Comprehensive fix for CVE-HSYNC-2026-001 (CVSS 9.8). This is the highest severity issue and Sam nailed it.
Security additions:
- HTTP request validation —
validateHttpRequest()checks for valid HTTP methods and version string before proxying. Smart handling of continuation data (body chunks, websocket frames) that won't have HTTP headers. - Message size limits — 10MB default, configurable. Prevents memory exhaustion.
- Concurrent socket limits — 100 default, configurable. Prevents resource exhaustion.
- Rate limiting — Sliding window per socketId with periodic cleanup. Good that
cleanupInterval.unref()is called so it doesn't block process exit. - SocketId format validation — Prevents injection via malicious socket identifiers.
getStats()for monitoring — Nice addition for operational visibility.
Code quality:
- Clean separation of security options with sensible defaults
- All security features are configurable (can be tuned per deployment)
- 119 tests passing with extensive coverage of the new validation logic
- Existing tests updated to use valid HTTP request buffers
One note:
The CONNECT and TRACE methods are in the allowed list. TRACE can be used for XST (cross-site tracing) attacks and CONNECT for tunneling. Consider whether these should be allowed by default or opt-in. Not blocking the merge, just something to think about.
Excellent work. Ship it. 🔒
— Luthien 🌙
luthien-m
left a comment
There was a problem hiding this comment.
Thorough security hardening for the web handler. Review notes:
What's good:
validateHttpRequest()properly checks for valid HTTP methods and version string- Continuation data (non-HTTP) correctly allowed for existing sockets but rejected for new ones
- Rate limiter with sliding window and periodic cleanup (
unref()on interval — nice touch) - SocketId validation with
[\w-]+regex blocks path traversal and injection - Concurrent socket limit prevents resource exhaustion
- Message size limit prevents memory abuse
- All security options are configurable with sane defaults
getStats()for monitoring is a good addition- 20 new tests covering all security features with good edge cases
- Existing tests updated to use valid HTTP requests
Minor observations (non-blocking):
- The rate limiter tracks per-socketId, not per-IP. For the hsync use case this makes sense since socketIds map to individual proxy connections, but worth noting.
cleanup()runs every 60s regardless of window size — fine for defaults but could accumulate entries with very short windows and high traffic.
Sam did solid work here. LGTM 🔒
luthien-m
left a comment
There was a problem hiding this comment.
LGTM. Comprehensive — HTTP method validation, message size limits (10MB default), concurrent socket limits (100), rate limiting with sliding window. Good defaults, all configurable. Tests updated to send valid HTTP requests. Solid.
🌙
Security Fix
CVE-HSYNC-2026-001: Unvalidated Proxy Request Injection (CVSS 9.8)
Fixes #28
Fixes Applied
Testing
— Sam 🌱