Conversation
There was a problem hiding this comment.
Pull request overview
Adds per-IP websocket connection/attempt caps and tighter request parameter limits to make websocket handling more robust and less abuse-prone.
Changes:
- Introduces an in-memory per-IP websocket connection limiter (active connections + connection attempt rate).
- Improves websocket client IP extraction (Cloudflare headers + RemoteAddr parsing/canonicalization).
- Adds caps and tests for websocket request parameters (estimateFee blocks, subscribeAddresses size, getAccountInfo paging/gap clamping).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/websocket.go | Implements IP parsing/canonicalization, connection limiter, and new request caps/clamps in websocket handlers. |
| server/websocket_test.go | Adds unit tests for getIP(), the connection limiter behavior, and new websocket parameter caps. |
| server/public_test.go | Extends tests around paging/gap sanitization used by websocket getAccountInfo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func getIP(r *http.Request) string { | ||
| ip := r.Header.Get("cf-connecting-ip") | ||
| if ip != "" { | ||
| if ip, ok := parseIP(r.Header.Get("CF-Connecting-IPv6")); ok { | ||
| return ip | ||
| } | ||
| ip = r.Header.Get("X-Real-Ip") | ||
| if ip != "" { | ||
| if ip, ok := parseIP(r.Header.Get("CF-Connecting-IP")); ok { | ||
| return ip |
There was a problem hiding this comment.
getIP() trusts CF-Connecting-* headers unconditionally. Because the websocket connection caps are keyed off getIP(), a client can bypass the limiter by spoofing these headers unless the server is guaranteed to sit behind Cloudflare (or another trusted proxy that strips/overwrites them). Consider only honoring these headers when the TCP peer (RemoteAddr) is in a configured set of trusted proxy CIDRs, or gate this behavior behind an explicit "trust proxy headers" config.
| func (l *websocketConnectionLimiter) cleanupLocked(now time.Time) { | ||
| if !l.lastCleanup.IsZero() && now.Sub(l.lastCleanup) < websocketConnectionLimiterCleanupInterval { | ||
| return | ||
| } | ||
| l.lastCleanup = now | ||
| for ip, client := range l.clients { | ||
| client.trimAttempts(now) | ||
| if client.active == 0 && now.Sub(client.lastSeen) > websocketConnectionLimiterTTL { | ||
| delete(l.clients, ip) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
websocketConnectionLimiter cleanup only runs on accept()/release() calls. If the server experiences a burst of many unique IPs and then becomes idle (no new connections), stale entries older than websocketConnectionLimiterTTL will never be evicted, so the clients map can retain memory indefinitely. Consider running cleanup on a background ticker, or otherwise ensuring eviction happens without requiring subsequent connection activity.
Honor X-Real-Ip when the TCP peer is on a loopback/RFC1918/ULA/link-local network, i.e. an upstream proxy on the same host or LAN. For direct internet peers the header stays ignored so it can't be used to spoof past the per-IP rate limiter. Auto-detected via netip predicates, no config.
Background ticker that calls limiter.sweep() every cleanupInterval, so TTL-expired idle entries are evicted even when no new connections arrive to drive cleanup. The goroutine is started once in NewWebsocketServer
Add <NETWORK>_WS_TRUSTED_PROXIES env var to extend X-Real-Ip trust beyond loopback/RFC1918 for non-Cloudflare deployments. Fails startup on /0 or otherwise overly broad prefixes (< /8 IPv4, < /16 IPv6) so misconfig can't silently turn the header into a spoofing primitive.
Wrap the four DB-touching go ... spawn sites with a sync.WaitGroup gate and add WebsocketServer.Shutdown(ctx) that flips a shuttingDown flag, closes all registered channels, and waits for in-flight goroutines to drain. PublicServer.Shutdown now drives it after http.Server.Shutdown, so a long getAccountInfo can no longer race rocksdb_close in cgo and SIGSEGV on graceful restart.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ip = r.Header.Get("X-Real-Ip") | ||
| if ip != "" { | ||
| return ip | ||
| addr, err := netip.ParseAddr(value) |
There was a problem hiding this comment.
I think the review comment is mostly incorrect for our current Go/runtime.
I verified locally:
netip.ParseAddr("fe80::1%eth0") -> ok
IsLinkLocalUnicast() -> true
And for real http.Request.RemoteAddr, the usual form would be:
[fe80::1%eth0]:12345
net.SplitHostPort turns that into:
fe80::1%eth0
Then netip.ParseAddr accepts it, and isTrustedProxy recognizes it as link-local. So the specific claim “link-local peers won’t be recognized” does not hold with this Go version.
There is one adjacent cleanup worth considering, but it is not urgent: normalize away the zone after parsing:
addr = addr.WithZone("")That avoids carrying %eth0 into rate-limit keys and makes configured prefix matching behave more predictably, because netip.Prefix.Contains does not match zoned addresses against unzoned prefixes. I would not split on % manually; parse first, then WithZone("").
So my take:
- Not a blocker as written.
- Reasonable small hardening: strip zone after successful
netip.ParseAddr. - Add a test for
getIPwithRemoteAddr: "[fe80::1%eth0]:12345"andX-Real-Ipto prove link-local proxy trust works.
There was a problem hiding this comment.
Done in commit 9d5aba7. Added addr.WithZone("") in parseAddr so all returned addresses are zone-free — rate-limit keys stay clean and netip.Prefix.Contains works correctly against link-local peers. Also added a TestGetIP case with RemoteAddr: "[fe80::1%eth0]:12345" + X-Real-Ip confirming the link-local proxy is trusted and the header is honored.
| - `<coin shortcut>_WS_GETACCOUNTINFO_LIMIT` - Limits the number of `getAccountInfo` requests per websocket connection to reduce server abuse. Accepts number as input. | ||
|
|
||
| - `<coin shortcut>_WS_ALLOWED_ORIGINS` - Comma-separated list of allowed WebSocket origins (e.g. `https://example.com`, `http://localhost:3000`). If omitted, all origins are allowed and it is the operator's responsibility to enforce origin access (for example via proxy). | ||
|
|
||
| - `<network>_WS_TRUSTED_PROXIES` - Comma-separated list of trusted proxy CIDRs whose `X-Real-Ip` header should be used as the WebSocket client IP. This IP is used by per-IP WebSocket connection and connection-attempt limits. |
| var voutFilter = api.AddressFilterVoutOff | ||
| page := validateIntParam(r.URL.Query().Get("page"), 0, 0, maxPageNumber) | ||
| pageSize := validateIntParam(r.URL.Query().Get("pageSize"), maxPageSize, 0, maxPageSize) | ||
| page, pageSize = sanitizePagingParams(page, pageSize, maxPageSize, maxPageSize) | ||
| page, pageSize = sanitizeAccountPagingParams(page, pageSize, maxPageSize, maxPageSize) | ||
| from := validateIntParam(r.URL.Query().Get("from"), 0, 0, 10000000000) |
There was a problem hiding this comment.
@copilot change the PR description accordingly
There was a problem hiding this comment.
Updated the PR description to explicitly call out that sanitizeAccountPagingParams is now used by the HTTP /api/.../address endpoint as well, replacing the previous maxSafePagingOffset-based clamp and applying consistent capping across both WebSocket and HTTP API.
Agent-Logs-Url: https://github.com/trezor/blockbook/sessions/0d3d9125-200e-45e5-ae57-a54919829a60 Co-authored-by: pragmaxim <8983344+pragmaxim@users.noreply.github.com>
Agent-Logs-Url: https://github.com/trezor/blockbook/sessions/0d3d9125-200e-45e5-ae57-a54919829a60 Co-authored-by: pragmaxim <8983344+pragmaxim@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Relatively simple caps and improved origin detection for ws connections that makes capping more reliable