fix(node): gate POST /api/v1/sync/trigger and rate-limit the peer-sync routes (#82)#161
Conversation
…es (#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.
…er-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).
…ody (#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.
…e 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.
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds dedicated per-IP rate limiters and mandatory signature enforcement for ChangesNode server sync/trigger security and rate limiting
Estimated code review effort: 4 (Complex) | ~60 minutes gl CLI sync trigger response handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SyncTriggerRoutes
participant AuthLayer
participant SyncTriggerRateLimiter
participant TriggerSyncHandler
Client->>SyncTriggerRoutes: POST /api/v1/sync/trigger
SyncTriggerRoutes->>AuthLayer: enforce signature
AuthLayer-->>SyncTriggerRoutes: 401 if unsigned
SyncTriggerRoutes->>SyncTriggerRateLimiter: check per-IP quota
SyncTriggerRateLimiter-->>SyncTriggerRoutes: 429 if exceeded
SyncTriggerRoutes->>TriggerSyncHandler: forward request
TriggerSyncHandler-->>Client: 200 with sync counts
sequenceDiagram
participant User
participant GlCli
participant NodeServer
User->>GlCli: gl sync trigger
GlCli->>NodeServer: POST /api/v1/sync/trigger
NodeServer-->>GlCli: HTTP response
alt non-success status
GlCli->>GlCli: read_body_capped()
GlCli->>GlCli: extract message/error
GlCli->>GlCli: sanitize_node_msg()
GlCli-->>User: error with status and sanitized message
else success
GlCli->>GlCli: trigger_counts()
GlCli-->>User: peers_reached, repos_enqueued
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
beardthelion
added
crate:gl
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/gitlawb-node/src/main.rs (1)
216-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWiring is correct; consider deduplicating limiter setup.
The new
sync_trigger_rate_limiter/peer_write_rate_limiterconstruction,AppStatewiring, and cleanup-loop integration are all consistent with the existingpush_rate_limiterpattern and correctly implement "0 disables" semantics. As an optional follow-up, the four near-identicalRateLimiter::new_bounded(...)+ "if limit == 0, warn" blocks (main limiter, push, sync-trigger, peer-write) could be collapsed into a small helper to reduce copy-paste and the risk of the warning/limiter values drifting out of sync in future edits.♻️ Example helper to reduce duplication
+fn bounded_limiter_with_warning( + limit: usize, + window: std::time::Duration, + max_keys: usize, + env_var: &str, + label: &str, +) -> rate_limit::RateLimiter { + if limit == 0 { + tracing::warn!("{env_var}=0 — {label} rate limiting disabled"); + } + rate_limit::RateLimiter::new_bounded(limit, window, max_keys) +}Also applies to: 257-258, 320-321, 330-331
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/main.rs` around lines 216 - 238, The limiter setup in main is correct, but the repeated RateLimiter::new_bounded plus “if limit == 0, warn” pattern is duplicated across the main, push, sync_trigger_rate_limiter, and peer_write_rate_limiter blocks. Extract that repeated logic into a small helper around the existing RateLimiter construction so the warning text and configured limit stay in sync and future edits only need to change one place..env.example (1)
98-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReorder env vars to satisfy dotenv-linter.
GITLAWB_PEER_WRITE_RATE_LIMITshould be declared beforeGITLAWB_SYNC_TRIGGER_RATE_LIMITper the linter's key-ordering rule.🧹 Proposed reorder
-# /api/v1/sync/trigger requires a signature and fans out to every peer per call, -# so it gets a tight bucket. 0 disables. Default 60. -GITLAWB_SYNC_TRIGGER_RATE_LIMIT=60 -# /api/v1/peers/announce and /api/v1/sync/notify accept unsigned requests from -# known peers and run at higher frequency, so a generous bucket. Separate from -# the trigger bucket so an unsigned notify flood can't drain trigger's quota. -# 0 disables. Default 600. -GITLAWB_PEER_WRITE_RATE_LIMIT=600 +# /api/v1/peers/announce and /api/v1/sync/notify accept unsigned requests from +# known peers and run at higher frequency, so a generous bucket. Separate from +# the trigger bucket so an unsigned notify flood can't drain trigger's quota. +# 0 disables. Default 600. +GITLAWB_PEER_WRITE_RATE_LIMIT=600 +# /api/v1/sync/trigger requires a signature and fans out to every peer per call, +# so it gets a tight bucket. 0 disables. Default 60. +GITLAWB_SYNC_TRIGGER_RATE_LIMIT=60🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 98 - 107, The environment variable declarations in the .env.example rate-limiting section are out of the linter’s required key order. Reorder the related entries so GITLAWB_PEER_WRITE_RATE_LIMIT appears before GITLAWB_SYNC_TRIGGER_RATE_LIMIT, keeping the surrounding comments aligned with the moved variables.Source: Linters/SAST tools
crates/gitlawb-node/src/server.rs (1)
280-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute split and layering look correct.
Signature gate is unconditional for
/sync/triggerin both config modes, and the per-IP brake is outermost so it runs before signature verification — matches the documented threat model (DID farms can't buy cost immunity via a valid signature, and the IP brake can't be bypassed by rotating DIDs).One minor naming nit:
state.push_limiter_trustis now reused for the sync-trigger and peer-write limiters too (in addition to push). Functionally fine since it's a single trusted-proxy topology, but the name reads push-specific. Consider renaming to something likeip_limiter_trustin a follow-up for clarity (touchesstate.rs/main.rs/auth/mod.rsas well, not shown in this diff).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/server.rs` around lines 280 - 314, The rate-limit trust flag used in the new sync-trigger and peer-write route layers is semantically broader than “push” now that it is shared across multiple IP limiters. Update the shared state field referenced in server::sync_trigger_routes and peer_write_routes from the push-specific name to a generic one like ip_limiter_trust, and propagate that rename through the state initialization and any related config/auth plumbing so the naming matches its actual scope.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.env.example:
- Around line 98-107: The environment variable declarations in the .env.example
rate-limiting section are out of the linter’s required key order. Reorder the
related entries so GITLAWB_PEER_WRITE_RATE_LIMIT appears before
GITLAWB_SYNC_TRIGGER_RATE_LIMIT, keeping the surrounding comments aligned with
the moved variables.
In `@crates/gitlawb-node/src/main.rs`:
- Around line 216-238: The limiter setup in main is correct, but the repeated
RateLimiter::new_bounded plus “if limit == 0, warn” pattern is duplicated across
the main, push, sync_trigger_rate_limiter, and peer_write_rate_limiter blocks.
Extract that repeated logic into a small helper around the existing RateLimiter
construction so the warning text and configured limit stay in sync and future
edits only need to change one place.
In `@crates/gitlawb-node/src/server.rs`:
- Around line 280-314: The rate-limit trust flag used in the new sync-trigger
and peer-write route layers is semantically broader than “push” now that it is
shared across multiple IP limiters. Update the shared state field referenced in
server::sync_trigger_routes and peer_write_routes from the push-specific name to
a generic one like ip_limiter_trust, and propagate that rename through the state
initialization and any related config/auth plumbing so the naming matches its
actual scope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ada22aa-e672-4cef-b14a-38ffb55a243a
📒 Files selected for processing (10)
.env.examplecrates/gitlawb-node/src/api/peers.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/config.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/rate_limit.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/state.rscrates/gitlawb-node/src/test_support.rscrates/gl/src/sync.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P2] Update the peer-write rollout docs for the always-signed trigger route
README.md:305
The README still listsPOST /api/v1/sync/triggerunder the staged peer-write rollout and says unsigned legacy peers are accepted whenGITLAWB_REQUIRE_SIGNED_PEER_WRITES=false;docs/OSS-READINESS-AUDIT.mdalso still saysgl sync triggerfalls back to an unsigned request for live compatibility. This PR intentionally moves/sync/triggerinto its own always-signed route group, so operators following the docs will expect the flag to preserve unsigned trigger compatibility even though the route now returns 401 without a signature. Please split/sync/triggerout of the staged peer-write list and document that only/peers/announceand/sync/notifykeep the rolling-upgrade unsigned behavior. -
[P2] Stop silently sending unsigned
gl sync triggerrequests
crates/gl/src/sync.rs:33
gl sync triggerstill loads the keypair with.ok()and then buildsNodeClient::new(&args.node, keypair), so a missing or unreadable identity is silently converted into an unsigned request. Since this PR makes/api/v1/sync/triggeralways require a signature, that path can only fail remotely with a 401 and keeps the legacy unsigned fallback alive in the client instead of telling the operator to create or fix their identity before calling the route. Please makeTriggerrequire a keypair locally, as the other signed CLI writes do, and cover the missing-identity path so the command fails before issuing an unsigned request.
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.
|
@jatmn both addressed in c84c703:
Also bumped crossbeam-epoch to 0.9.20 (RUSTSEC-2026-0204) so cargo audit is green again. |
|
@coderabbitai the |
|
Tip For best results, initiate chat on the files or code changes.
Understood on deferring the limiter-setup dedup helper and the server.rs route-grouping note to a separate cleanup PR rather than expanding this security fix's diff — that's a reasonable call given the PR's focus on closing If you'd like, I can open a follow-up GitHub issue to track those two cleanup items so they don't get lost. Just let me know and I'll file it (tagging you as the requester and linking back to this PR/thread). |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the previously discussed paths and do not see any remaining actionable issues from my side.
3 participants
POST /api/v1/sync/triggerwas reachable unauthenticated in the default config and drove an O(peers) outbound fan-out plus onesync_queueenqueue per repo, an anonymous amplification/DoS (#82). The sameenqueue_syncsink is also reachable through/sync/notify, which accepts unsigned known-peer requests and had no rate limit, so gating only trigger would have left an equivalent hole.What changed:
/sync/triggernow requires a valid RFC 9421 signature in both config modes (its own always-signed route group), independent ofrequire_signed_peer_writes.announce+notifygroup. Per-IP rather than per-DID, because a did:key farm self-registers and never trips a per-DID bucket; the brake is keyed before auth so DID rotation can't bypass it.notify's signature behavior is unchanged for rolling-upgrade compat, it only gains the brake. Knobs:GITLAWB_SYNC_TRIGGER_RATE_LIMIT,GITLAWB_PEER_WRITE_RATE_LIMIT(0 disables).gl sync triggernow checks HTTP status before parsing (a 401/429 can no longer render as a fake "sync triggered" success), bounds the error-body read, and strips control and Unicode bidi characters from the node's error before printing.Known limitation: the per-IP brake caps a single-source flood. A distributed multi-IP notify flood stays open until
/peers/announceis gated (the free peer self-registration that enables it), which is tracked separately.Tests are an adversarial RED to GREEN matrix: unsigned to 401 in both modes, IP flood to 429, DID-farm and forged-header resistance, cross-bucket isolation, disabled-when-0, the notify and announce brakes, the real-signature positive through the full router, and the client 401/429/sanitize/oversized-body paths.
Closes #82.
Summary by CodeRabbit
POST /api/v1/sync/triggerand peer-write routes (/api/v1/peers/announce,/api/v1/sync/notify), controlled via new CLI/env settings (including optional disable via0).sync/triggerinto its own authenticated route with dedicated throttling.gl sync triggerto fail locally when no identity is configured and to surface server errors correctly.sync/triggersignature requirements and staged-rollout behavior.