Skip to content

Commit d00d89a

Browse files
beardtheliont
andauthored
fix(node): gate POST /api/v1/sync/trigger and rate-limit the peer-sync routes (#82) (#161)
* feat(node): add per-client-IP limiters + knobs for the peer-sync routes (#82) Plumb two dedicated per-IP RateLimiters into config/state so the peer-sync enqueue routes can be braked independently: sync_trigger_rate_limiter (tight, GITLAWB_SYNC_TRIGGER_RATE_LIMIT, default 60/hr) and peer_write_rate_limiter (generous, GITLAWB_PEER_WRITE_RATE_LIMIT, default 600/hr). Both are bounded and swept by the periodic cleanup loop, keyed via the node-wide push_limiter_trust. No route is wired to them yet; that lands next. * fix(node): require a signature on /sync/trigger and rate-limit the peer-sync routes (#82) /api/v1/sync/trigger was reachable unauthenticated in the default config and drove an O(peers) outbound fan-out + per-repo sync_queue enqueue — an anonymous amplification/DoS. The same enqueue_sink is reachable via /sync/notify, which also accepts unsigned known-peer requests and had no rate limit. - Move /sync/trigger into its own always-signed route group (add_auth_layers), independent of require_signed_peer_writes, and give it a tight per-IP brake. - Add a separate, generous per-IP brake to the peer-write group (/peers/announce, /sync/notify); notify's signature behavior is unchanged (rolling-upgrade compat), it only gains the brake. Separate buckets so an unsigned notify flood can't drain the signed trigger caller's quota. - Per-IP (not per-DID): a did:key farm self-registers, so a signature/per-DID gate does not cap cost (INV-10); the brake keys on the client IP before auth. Adds an adversarial test matrix (unsigned->401 both modes, IP flood->429, DID-agnostic brake, forged X-Forwarded-For can't bypass, notify flood->429, distinct-IP not throttled, cross-route non-exhaustion, 0-disables-but-sig-holds, announce still accepts unsigned). * fix(gl): surface node denial on sync trigger and sanitize the error body (#82) Now that the node gates /api/v1/sync/trigger behind a signature and rate limit, 'gl sync trigger' must not render a 401/429 as a fabricated '✓ sync triggered / 0 peers' success. Check resp.status() before parsing; on a non-2xx, extract the node's message and bail. Strip C0/C1 control bytes from the node-supplied string before printing (INV-6) — a hostile node could embed ANSI/OSC escapes in the body. Adds mockito tests: 401/429 surface as errors (RED without the status check), control bytes are stripped, and the 200 path is unchanged. * fix(review): bound gl error-body read, strip bidi controls, genericize 429 message (#82) Code-review follow-ups on the #82 change: - gl sync trigger: read the node error body with a capped reader (read_body_capped) instead of resp.text(), so a hostile/broken node can't force an unbounded allocation to surface a denial (INV-6, read half). - sanitize_node_msg: also strip Unicode bidi/format controls (U+202A-202E, U+2066-2069, LRM/RLM/ALM) — is_control() only covers the Cc category, but a right-to-left override can reorder the displayed error line. - too_many_requests(): the shared per-IP 429 middleware now serves the peer-sync routes too, so drop the push-specific wording ('rate limit exceeded'); the push-specific 429 in the receive-pack handler is unchanged. Adds tests for the bidi/length sanitize and the oversized-body path. * test(#82): close reasoned-not-run gaps with load-bearing tests Executes the branches that were previously only reasoned: - read_body_capped byte-cap test (RED against resp.text() — 2MB body -> ≤cap): vets the bounded-read fix, not just the display cap. - sync-route 429 body asserts 'rate limit exceeded' and NOT 'push' (RED before the message was genericized); also finishes genericizing the warn log, which the response-body change had missed. - real RFC 9421 signature through the full router reaches the handler (200) in both config modes — the positive path through the actual gate, not a direct-mount with an injected DID. - /peers/announce flood -> 429 (the peer_write brake covers announce, not just notify). - trigger_counts extraction unit test (happy + malformed -> 0,0), closing the count-formatting gap in the 200 path. No production behavior change beyond the warn-log wording. * fix(#82): require an identity for sync trigger and fix the rollout docs Address review feedback on the always-signed /sync/trigger route. - gl sync trigger now requires a local identity via load_keypair_from_dir and fails locally with "identity not found" before issuing a request, instead of .ok() silently sending an unsigned request that can only 401 remotely. Add a test proving it fails before dialing the node. - Split /sync/trigger out of the staged unsigned peer-write rollout in the README and the OSS-readiness audit; only /peers/announce and /sync/notify keep the rolling-upgrade unsigned behavior. - Reorder the two peer-sync rate-limit vars in .env.example for dotenv-linter. - Bump crossbeam-epoch to 0.9.20 (RUSTSEC-2026-0204), matching main, so cargo audit passes. --------- Co-authored-by: t <t@t>
1 parent 67ad2b8 commit d00d89a

13 files changed

Lines changed: 851 additions & 23 deletions

File tree

‎.env.example‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,16 @@ GITLAWB_MAX_PACK_BYTES=2147483648
9595
# IP per hour. 0 disables. Default 600.
9696
GITLAWB_PUSH_RATE_LIMIT=600
9797

98+
# ── Peer-sync rate limiting (per client IP, uses GITLAWB_TRUSTED_PROXY below) ─
99+
# /api/v1/peers/announce and /api/v1/sync/notify accept unsigned requests from
100+
# known peers and run at higher frequency, so a generous bucket. Separate from
101+
# the trigger bucket so an unsigned notify flood can't drain trigger's quota.
102+
# 0 disables. Default 600.
103+
GITLAWB_PEER_WRITE_RATE_LIMIT=600
104+
# /api/v1/sync/trigger requires a signature and fans out to every peer per call,
105+
# so it gets a tight bucket. 0 disables. Default 60.
106+
GITLAWB_SYNC_TRIGGER_RATE_LIMIT=60
107+
98108
# Which forwarded header the edge is trusted to set, used to resolve the real
99109
# client IP for the push limiter. One of:
100110
# (unset) — no trusted proxy: key on the socket peer address, ignore headers.

‎Cargo.lock‎

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎README.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -302,20 +302,21 @@ POST /api/v1/bounties/{id}/...
302302
POST /{owner}/{repo}/git-receive-pack
303303
```
304304

305-
Peer write routes support staged rollout:
305+
These peer write routes support staged rollout:
306306

307307
```txt
308308
POST /api/v1/peers/announce
309309
POST /api/v1/sync/notify
310-
POST /api/v1/sync/trigger
311310
```
312311

313-
When `GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false`, unsigned legacy peers are accepted, but signed requests are verified when signature headers are present. Once all live peers upgrade, operators can set:
312+
When `GITLAWB_REQUIRE_SIGNED_PEER_WRITES=false`, unsigned legacy peers are accepted on those two routes, but signed requests are verified when signature headers are present. Once all live peers upgrade, operators can set:
314313

315314
```bash
316315
GITLAWB_REQUIRE_SIGNED_PEER_WRITES=true
317316
```
318317

318+
`POST /api/v1/sync/trigger` is not part of the staged rollout: it always requires a signature in both config modes and returns 401 without one, because each call drives an O(peers) outbound fan-out.
319+
319320
---
320321

321322
## Configuration

0 commit comments

Comments
 (0)