Repository navigation
port(upstream#1974): per-IP limits and deny list on the /ws upgrade - #47
Conversation
…ade (Kpa-clawbot#1974) Closes Kpa-clawbot#1794. Follow-up to Kpa-clawbot#1793, decided **before** the upgrade because the handshake is the resource being protected. - Deny list of addresses and CIDRs → 403 - Per-IP concurrent connection cap → 403 - Per-IP upgrade rate limit over a rolling minute → **429**, not 403: a temporary refusal should not read as "never come back" - Rejection counters split by cause in `/api/stats` under `websocket` ### The decision this feature lives or dies on Most CoreScope installs sit behind nginx, Caddy, Traefik or an ingress. `cdn_detection.go` says so in as many words: it deliberately excludes `X-Forwarded-For` from its CDN signals precisely because *every* reverse-proxied install sets it. For those deployments `r.RemoteAddr` is the proxy, `127.0.0.1` for every visitor on earth. A per-IP cap keyed on that address protects nobody and hands the sixth legitimate browser tab a 403. That is a self-inflicted outage wearing the costume of hardening. So: - **`X-Forwarded-For` is believed only from an address listed in `webSocket.trustedProxies`.** From anywhere else it is attacker-supplied, and trusting it would let anyone mint a fresh source IP per connection, which is strictly worse than having no limit at all. - **When the peer looks like a local reverse proxy and no `trustedProxies` is set, the per-IP limits are skipped**, and one warning names the setting that fixes it. Silently refusing real users is the worse failure. - **The deny list still applies there**, because it is the operator's explicit instruction rather than an inference. That is the answer to @mcode6726's question on the thread: it is neither "always the socket address" nor "always the header", and the operator decides which by naming their proxy. ### Two deliberate departures from the issue body **`maxConnsPerIP` ships as 0 (off), not 5.** Carrier-grade NAT puts thousands of unrelated mobile subscribers behind a single public IPv4. A cap of 5 refuses real visitors on phones while a scraper simply rents more addresses: all of the cost, none of the benefit. `upgradesPerMinPerIP` ships at **30 and on**, because that one *is* safe under CGNAT: a real client upgrades a handful of times per minute even while reconnecting, so 30 leaves ordinary traffic untouched while flattening a reconnect loop. A pointer type distinguishes "unset" from an explicit `0` that turns it off. **The default deny list is not shipped.** The thread proposed seeding 44 CIDRs for one VPS provider after a single scraper was seen at `23.111.177.6`. I have left it out: blanket-blocking a hosting provider by default breaks legitimate operators who host there, is undiscoverable by the person locked out (they see a bare 403), and ages badly as ranges get reassigned. The mechanism is here and `config.example.json` shows exactly how to configure it, so any operator who wants that list can have it in one line. If you want it shipped as a default anyway, that is your call as maintainer and it is a one-line change. ### Verification 19 tests, including all five the issue specifies as TDD requirements, each marked with the issue's own wording. Beyond those five: - a **bare address** in the deny list works, not just CIDR form. Operators write `1.2.3.4`, and silently ignoring that would be the worst possible failure for a deny list: it looks configured and blocks nothing - an unparseable deny entry is skipped and logged, not fatal. One typo must not take the server down - one client behind a trusted proxy does **not** exhaust another client's budget behind the same proxy, which is the entire point of honouring XFF - changing a forged XFF from an untrusted peer buys no fresh budget - `release` frees a slot and is **idempotent**, because `Unregister` can run twice for one client and double-crediting would leak slots - a **rejected** upgrade does not consume rate budget, or a retrying client could never recover once its window cleared - limits skipped for loopback and private peers; deny list applies anyway - a nil limiter allows everything, so a `Hub` built without `ConfigureLimits` behaves exactly as before - idle per-IP state is collected, while a record with a live connection never is Full `cmd/server` suite green, `gofmt` clean. ### Not done - No runtime config reload; restart required. Listed as optional in the issue. - No `WS_DENY_IPS` env override. Also listed as optional. - From the OWASP expansion in the first comment: `maxPayload` and the idle/read timeout are **already in master** (`SetReadLimit`, `SetReadDeadline`). The ping/pong heartbeat is not, and is not in this PR either; it is a separate change to the read/write pumps and belongs in its own review. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 1ffaad8) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verificeret lokalt — de tre sikkerhedsgarantier er ægte dækketBranchen er synkroniseret med master Mutationstest af det, der kan forårsage et selvforskyldt udfaldPR'ens egen kommentar siger det selv: et per-IP-loft bag en ukonfigureret reverse proxy tæller hele internettet som én klient og giver den sjette legitime browserfane 403. Det er den farligste egenskab i ændringen, så jeg testede, om den faktisk er pinned — ved at genindsætte mutationen og se, at en navngiven test fejler på en rigtig assertion, ikke en compilefejl:
Den første er den vigtigste: den beviser, at udelukkelsesbeskyttelsen ikke bare er en kommentar, men en testet invariant. Den anden beviser den bevidste asymmetri — deny-listen er operatørens egen udtrykkelige instruks og gælder derfor stadig, mens de udledte per-IP-grænser slås fra. De 19 tests i Hvad der ikke kan afgøres her
Hvorfor PARKERETOpgavens regler kræver staging før merge for WebSocket-/netværksgrænser, med rollback og kontrolleret load-test — og med den udtrykkelige betingelse, at Caddy og produktion ikke røres. Staging kan ikke nås fra den maskine, arbejdet kører på: docker-dæmonen kører ikke, Konkret testplan ligger i 🤖 Generated with Claude Code |
Split out of #25 (commit
c489d70bthere). This branch holds exactly one upstream change so it can be reviewed, tested and reverted on its own.Upstream
1ffaad8eb1d98d8ac2bff48ff3f7f74a93abb175git cherry-pick -xonto masterfda24ca5; upstream authorship kept, and the commit message carries the(cherry picked from commit …)line.cmd/server/{config,main,routes,types,websocket}.go, newws_limits.go+ test,config.example.json.Problem
The
/wsupgrade had only the browser-facingCheckOriginallowlist. Non-browser clients can omit or forgeOrigin, connect in a loop and sit in the hub.Change
New
webSocketconfig block evaluated before the handshake:deny: addresses/CIDRs → 403maxConnsPerIP: concurrent cap, default 0 (off)upgradesPerMinPerIP: rolling-minute upgrade rate → 429, default 30 (on)trustedProxies: only from these peers isX-Forwarded-ForbelievedRejections are counted in
/api/statsunderwebsocket.Adaptation to this fork
None. The cherry-pick applied without conflicts and the changed lines are identical to upstream.
Notes for review
Safe with our Docker image as-is: the bundled Caddy proxies to
localhost:3000, so the Go server's peer is loopback. WithtrustedProxiesempty,clientIP()marks loopback/private peers as indistinguishable, the per-IP limits are skipped and one warning is logged; onlydenystill applies. To actually enforce the limits, settrustedProxiesto the proxy address(es). Only do that if the proxy'sX-Forwarded-Forreally carries the visitor IP; behind a CDN in front of Caddy, visitors could otherwise share an edge IP and be throttled together.One local test failure, classified as a pre-existing flake: the first local
-racerun of the server suite on this branch failedTestIssue1008_HandlerReturns503WhileSubpathIndexLoading(status = 200, want 503), while master's run passed. This PR does not touch that handler, the index build or the test file; itsroutes.gohunk is confined tohandleHealth. Reproduction under identical conditions, see the verification table for the counts:-racererun passes on both master and this branch;-race -cpu=1fail on master 2/200 and on this branch 1/200, with the same assertion.It is therefore a timing race in the test itself, not a regression.
Dependencies and merge order
fda24ca5and needs no other PR from this split.TestPruneOldNeighborMetricsdeterministic). If test(ingestor): make neighbor metrics pruning deterministic #33 lands first, the expected CI failure named below disappears; nothing in this PR depends on it.Verification
Local run of the same commands as CI's “Go Build & Test” job (server tests with
-race), on this branch and on masterfda24ca5under the same conditions (same machine, run one after another):fda24ca5go-server-build-vetgo-server-test-raceTestIssue1008_HandlerReturns503WhileSubpathIndexLoadingTestIssue1008_HandlerReturns503WhileSubpathIndexLoadingis a timing race in the test itself (it asserts the not-ready window without waiting for the background index build). 200 iterations each with-race -cpu=1: masterfda24ca5failed 2/200, this branch 1/200, same assertionstatus = 200, want 503. With-cpu=2: 0/200 on both. A full-racererun of the server suite passes on bothchannel-lib-testdecrypt-cli-build-testdockerfile-copy-invariantsdeclare -A), macOS has 3.2; identical on masterstaging-disk-monitorcss-vars-lintBaseline failures (fail identically on master; not introduced or changed here): see rows marked baseline failure, unchanged.
Browser validation (local, fixture DB, no staging/production): Not applicable (no frontend change).
Not run:
eslint(not installed locally; CI installs it on the fly).Expected GitHub CI: “Go Build & Test” is expected to fail on
TestPruneOldNeighborMetrics, which already fails on master (see #25's run). Downstream jobs (Playwright, image build) are therefore skipped. “Deploy Staging” and all GHCR publish steps only run onpushtomasterand cannot run for this PR.Two further ingestor tests have failed intermittently in this split's CI on branches whose
cmd/ingestortree is byte-identical to master (#27, #28), so they can also appear here without being caused by this change:TestBackfillTxLastSeen_ResolvesFromMaxObservationTimestamp: also reproduced locally on unmodified master.TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749: the suite flake that upstream test(ingestor): join the watchdog loop goroutine instead of only asking it to stop Kpa-clawbot/CoreScope#2003 (also split out of port(upstream): 26 clean upstream fixes — prune batching, /ws limits, observer liveness, watchdog race #25) addresses.GitHub CI result: run 34752482084 on
93f37b91. Go Build & Test: failure; all downstream jobs incl. Deploy Staging skipped. Failed tests:TestPruneOldNeighborMetrics: fails on master, documented baseline🤖 Generated with Claude Code