feat(node): per-IP write-surface rate brake + purge-spam admin tool

[beardthelion]

What

Two related pieces of the creation-surface abuse-resistance work:

1. A per-IP write-surface rate brake. Every authenticated non-creation write sink now sits behind a per-client-IP flood brake on its own bucket: pull/issue comments, labels, stars, merges, protect/unprotect, replicas, visibility, agent deregister, tasks, bounties, profile, and GraphQL mutations. Previously these route groups mounted with no rate limit at all, so a single-source flood on any of them was uncounted (only repo/agent creation, git-push, and peer-sync were braked). The bucket is configured by GITLAWB_WRITE_RATE_LIMIT (default 600/hr, 0 disables), keyed on the resolved client IP via the existing GITLAWB_TRUSTED_PROXY boundary.

2. A purge-spam admin subcommand for cleaning up a specific empty spam-burst that predates the mirror bot. Dry-run by default; --execute deletes.

Key decisions

• Separate bucket from creation, not shared: a write flood cannot drain the repo-create budget and vice versa (same rationale as the existing sync_trigger vs peer_write split). A shared bucket would also self-throttle a legitimate session doing repo + issues + comments.
• IP-only, no per-DID on the write groups: a did:key farm never trips a per-DID limit (the same reason git-receive-pack deliberately omits it), and a per-DID brake would false-positive busy legitimate agents.
• GraphQL POST braked at the HTTP layer. An HTTP-layer brake cannot tell a query POST from a mutation POST, so it counts every /graphql request; /graphql/ws subscriptions stay unbraked. MutationRoot was the largest previously-uncounted write vector.
• purge-spam safety: per-repo empty verification (zero git refs, with a bare-repo-marker guard so git's upward repo discovery can't read a parent checkout's refs and delete a live repo), a hard exclusion gate evaluated before the empty check, a re-verify immediately before delete (TOCTOU), and on-disk dir removal so DB and disk stay consistent. Dry-run is the default and prints the candidate list with ref-count evidence.

Verification

• cargo test --workspace green; cargo fmt --all --check and cargo clippy --workspace --all-targets -D warnings clean.
• Real-node deny path driven through a client: a write sink returns 429 before signature verification (brake outermost), and under-limit writes reach the handler.
• Adversarial TrustedProxy coverage: a spoofed leftmost X-Forwarded-For hop cannot rotate the bucket key, distinct trusted hops get distinct buckets, and an absent header falls back to the socket peer.
• The load-bearing tests were RED-verified (removing the brake, the exclusion gate, the bare-repo guard, or the SQL normalization each turns the relevant test red).

Scope

This is the write-surface brake coverage plus the burst cleanup. iCaptcha enforcement, storage/size quotas, and moving the gate toward enforce-by-default are separate follow-ups, not in this PR.

Summary by CodeRabbit

• New Features

  • Added an optional per-IP hourly limit for authenticated write actions, configurable with GITLAWB_WRITE_RATE_LIMIT.
  • Added a purge-spam administrator command with safe dry-run behavior and optional execution.
  • Added protections to prevent shared or spoofed client IPs from bypassing rate limits.

• Bug Fixes

  • Ensured write limits are separate from repository-creation limits.
  • Excluded GraphQL WebSocket connections from write throttling.

[coderabbitai]

📝 Walkthrough

Walkthrough

Changes

The pull request adds a purge-spam admin command with dry-run and execute modes, safe empty-repository checks, per-row deletion, and integration tests. It also adds configurable per-client-IP rate limiting for authenticated non-creation writes across REST and GraphQL HTTP routes.

Spam purge administration

Layer / File(s)	Summary
Admin command and database surface crates/gitlawb-node/src/config.rs, crates/gitlawb-node/src/main.rs, crates/gitlawb-node/src/db/mod.rs	Adds purge-spam CLI parsing, direct admin dispatch, and repository listing/deletion database APIs.
Candidate selection and safety checks crates/gitlawb-node/src/admin.rs	Normalizes DIDs, applies exclusions, verifies exact bare-repository markers, counts refs, and rechecks candidates for TOCTOU safety.
Dry-run and deletion execution crates/gitlawb-node/src/admin.rs	Lists candidates in dry-run mode and, when enabled, deletes database rows and matching repository directories with error handling and integration coverage.

Authenticated write rate limiting

Layer / File(s)	Summary
Write limiter configuration and state .env.example, crates/gitlawb-node/src/state.rs, crates/gitlawb-node/src/main.rs, crates/gitlawb-node/src/{auth,test_support}.rs	Defines GITLAWB_WRITE_RATE_LIMIT, adds the limiter to application state, constructs it at startup, and updates test fixtures.
Write middleware route wiring crates/gitlawb-node/src/server.rs	Applies the per-IP write brake to authenticated REST write groups and GraphQL HTTP while leaving GraphQL WebSockets outside the brake.
Rate-limit behavior validation crates/gitlawb-node/src/api/repos.rs, crates/gitlawb-node/src/rate_limit.rs	Tests throttling, bucket separation, disabled limits, route coverage, and trusted-proxy client-IP keying.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Router
  participant WriteLimiter
  participant WriteHandler
  Client->>Router: authenticated write request
  Router->>WriteLimiter: check client-IP bucket
  WriteLimiter->>WriteHandler: forward request when permitted

Loading

sequenceDiagram
  participant AdminCLI
  participant PurgeRunner
  participant Database
  participant RepositoryDirectory
  AdminCLI->>PurgeRunner: purge-spam
  PurgeRunner->>Database: list target-owned repositories
  PurgeRunner->>RepositoryDirectory: verify zero refs
  PurgeRunner->>Database: delete repository row
  PurgeRunner->>RepositoryDirectory: remove repository directory

Loading

Possibly related PRs

• Gitlawb/node#25: Adds repository visibility routes affected by the new write middleware placement.
• Gitlawb/node#87: Adds GraphQL optional-signature middleware used before the new write brake.
• Gitlawb/node#152: Introduces related rate_limit and trusted-proxy infrastructure.

Suggested labels: sev:medium, crate:node, kind:feature, kind:security, subsystem:api, needs-tests

Suggested reviewers: gravirei

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately captures both main changes: the write-surface rate brake and purge-spam admin tool.
Description check	✅ Passed	The description covers the main changes, rationale, verification, and scope, though it doesn't follow the template headings exactly.
Docstring Coverage	✅ Passed	Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/write-sink-brake-coverage

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/gitlawb-node/src/main.rs (1)

440-455: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

write_rate_limiter is missing from the periodic cleanup loop.

Every other limiter (rate_limiter, create_ip_rate_limiter, push_rate_limiter, sync_trigger_rate_limiter, peer_write_rate_limiter) is swept here, but the newly added write_rate_limiter is not. It is bounded, so no unbounded leak, yet stale keys are only evicted lazily on a capacity miss instead of proactively like its peers — an inconsistency worth closing.

🧹 Proposed fix

         let peer_write_rl = state.peer_write_rate_limiter.clone();
+        let write_rl = state.write_rate_limiter.clone();
         let db = state.db.clone();

                         peer_write_rl.cleanup().await;
+                        write_rl.cleanup().await;

🤖 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 440 - 455, Include the
state.write_rate_limiter in the periodic cleanup task alongside the other cloned
rate limiters. Clone it before spawning the task and invoke its cleanup method
in the five-minute tokio::select! interval, preserving the existing cleanup
behavior for all other limiters.

🧹 Nitpick comments (1)

crates/gitlawb-node/src/api/repos.rs (1)

2629-2966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting a shared test-request helper to reduce duplication.

All 9 new tests repeat the same Request::builder() + ConnectInfo insertion + oneshot() + status-assert boilerplate. rate_limit.rs in this same PR already factored this pattern into post_from/post_with helpers — a similar helper here (e.g., send_from(&router, method, uri, body, peer)) would cut most of the repetition across write_route_is_rate_limited_by_ip, write_flood_does_not_drain_creation_budget, graphql_post_is_rate_limited_by_ip, issue_comment_is_rate_limited_by_ip, under_limit_write_is_not_throttled, write_rate_limit_zero_disables_the_brake, task_bounty_profile_writes_are_rate_limited, graphql_ws_is_not_braked, and every_write_group_passes_under_limit.

🤖 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/api/repos.rs` around lines 2629 - 2966, Reduce
duplicated request setup across the listed rate-limit tests by adding a shared
test helper such as send_from that accepts the router, HTTP method, URI, body,
and peer, inserts ConnectInfo, sends via oneshot, and returns the response or
status. Update each named test to use this helper while preserving its existing
request bodies, methods, URIs, and status assertions.

🤖 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.

Outside diff comments:
In `@crates/gitlawb-node/src/main.rs`:
- Around line 440-455: Include the state.write_rate_limiter in the periodic
cleanup task alongside the other cloned rate limiters. Clone it before spawning
the task and invoke its cleanup method in the five-minute tokio::select!
interval, preserving the existing cleanup behavior for all other limiters.

---

Nitpick comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 2629-2966: Reduce duplicated request setup across the listed
rate-limit tests by adding a shared test helper such as send_from that accepts
the router, HTTP method, URI, body, and peer, inserts ConnectInfo, sends via
oneshot, and returns the response or status. Update each named test to use this
helper while preserving its existing request bodies, methods, URIs, and status
assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e186f361-729a-419c-b633-a90e12b77a44

📥 Commits

Reviewing files that changed from the base of the PR and between ad7c2b2 and 27b4a2a.

📒 Files selected for processing (11)

• .env.example
• crates/gitlawb-node/src/admin.rs
• crates/gitlawb-node/src/api/repos.rs
• crates/gitlawb-node/src/auth/mod.rs
• crates/gitlawb-node/src/config.rs
• crates/gitlawb-node/src/db/mod.rs
• crates/gitlawb-node/src/main.rs
• crates/gitlawb-node/src/rate_limit.rs
• crates/gitlawb-node/src/server.rs
• crates/gitlawb-node/src/state.rs
• crates/gitlawb-node/src/test_support.rs

[jatmn]

High — fix before merge

H1. Read/fork handlers run blocking git subprocesses directly on the tokio runtime (no spawn_blocking, no timeout)

• fork_repo — crates/gitlawb-node/src/api/repos.rs:1567-1575 invokes std::process::Command::new("git").args(["clone","--mirror",…]).output() inside the async handler. output() blocks the calling worker thread; git clone --mirror of a non-trivial repo can take minutes and there is no timeout, unlike every other git walk in this file which correctly uses tokio::task::spawn_blocking (repos.rs:67, 110, 661, 1143) or tokio::process::Command (info/refs, upload-pack, receive-pack).
• Read metadata handlers — list_commits/get_tree/get_blob call store::{resolve_head, log, ls_tree, read_file} (api/repos.rs:360-361, 397, 443, 481) which are synchronous std::process::Command::output() calls (crates/gitlawb-node/src/git/store.rs:92-231). A slow disk or large repo blocks a worker thread for seconds; with a finite worker pool a handful of concurrent /commits, /tree, /blob, or a single /fork can starve the whole runtime, including /health and /ready (same runtime in build_router).
• Impact: availability/reliability regression, not just latency. git_service_timeout_secs is bypassed on these paths.
• Fix: wrap the blocking store calls (and the fork clone) in tokio::task::spawn_blocking and bound with tokio::time::timeout(git_service_timeout_secs, …), returning AppError::Timeout.

H2. write_rate_limiter is never reaped by the background cleanup loop

• crates/gitlawb-node/src/main.rs:440-455 — the periodic task clones exactly five limiters (rate_limiter, create_ip_rate_limiter, push_rate_limiter, sync_trigger_rate_limiter, peer_write_rate_limiter) and calls .cleanup() on each. state.write_rate_limiter (declared at main.rs:329-333, wired into AppState at :404) is omitted, even though it protects the entire authenticated non-creation write surface + /graphql (server.rs:84-98, 110, 154+).
• The RateLimiter design makes the inline capacity sweep amortized and explicitly relies on the background loop for full reclamation (rate_limit.rs:36-39, 94-114). With no background sweep, write_rate_limiter only reclaims expired keys once its map hits the 200,000-key cap. Until then every distinct client IP persists as a stale Window for the full 1-hour window and beyond → steady memory growth (bounded at the cap) and a latent correctness cliff: once at cap, the amortized inline sweep becomes the only reclaimer and a distinct-IP write flood serializes behind O(max_keys) scans while legitimate new IPs get spuriously 429'd between sweeps.
• Fix: add let write_rl = state.write_rate_limiter.clone(); and write_rl.cleanup().await; inside the cleanup tick beside the other five.

---

Medium — should fix

M1. fork_repo / create_repo orphan on-disk repos (and a spent proof / uploaded object) on late DB failure

• fork_repo ordering (api/repos.rs:1555 → 1567 → 1585 → 1605): spend proof → git clone --mirror to disk_path → release_after_write (Tigris upload) → db.create_repo. If create_repo fails, the on-disk mirror and the already-uploaded Tigris object are left with no DB row, and the single-use iCaptcha proof is already consumed.
• create_repo (api/repos.rs:208 → 210-219 → 236): proof.consume then repo_store.init (blocking git init) happen before db.create_repo. A DB failure orphans the freshly-initialized directory. init itself is also a blocking git call on the async runtime (same class as H1).
• Fix: make disk materialization and proof spend adjacent to a guaranteed-successful DB write, and on db.create_repo failure best-effort remove_dir_all(disk_path) (and prefer deferring the Tigris upload until after the row commits).

M2. TrustedProxy IP trust is config-asserted, not network-enforced — header rotation defeats every per-IP brake

• crates/gitlawb-node/src/rate_limit.rs:198-218 (client_key): in Fly/XForwardedFor modes the limiter key is taken entirely from Fly-Client-IP or the rightmost X-Forwarded-For hop, and only falls back to the socket peer when the header is absent. There is no check that the socket peer (ConnectInfo) is actually the operator's proxy. If GITLAWB_TRUSTED_PROXY is set but the node's HTTP port is reachable directly (misconfigured firewall, exposed Fly port, second ingress, internal attacker), a client can set Fly-Client-IP: <random> / append any rightmost X-Forwarded-For hop and get a fresh bucket per request, evading the creation/write/push/sync brakes (the only thing stopping a single-source spam/push flood). The unit tests cover header parsing but never assert "peer not the proxy → key on peer."
• Fix: honor the forwarded header only when ConnectInfo peer is in a configured proxy CIDR/IP allowlist; otherwise key on the socket peer (fail closed). Add a test for "trust mode set, request from a non-proxy IP."
• Note (validation): exploitability is deployment-dependent (Fly's edge does overwrite Fly-Client-IP), so rated Medium not Blocker.

M3. purge-spam --execute deletes via remove_dir_all on an unsanitized repo_name (path traversal → arbitrary repo deletion)

• crates/gitlawb-node/src/git/store.rs:450-453 — repo_disk_path sanitizes owner_did (replace([':', '/'], "_")) but joins repo_name verbatim via format!("{repo_name}.git") with no ../slash/dot validation.
• crates/gitlawb-node/src/admin.rs:243-244 calls std::fs::remove_dir_all(store::repo_disk_path(&repos_dir, &c.owner_did, &c.name)) on that path. The fail-closed marker guard (admin.rs:128) only requires a HEAD file + objects/ dir — i.e. any existing legitimate bare repo satisfies it. API names are validated (api/repos.rs:189-197), but smart_http_repo_name explicitly notes repos are "creatable via the peer mirror path, which skips API name validation" (api/repos.rs:490-494), so a burst-DID row with a ../ name can be planted, then deleted outside repos_dir.
• Fix: validate repo_name (and re-validate owner_did) the same way the serving paths do (repo_store::validate_repo_name/validate_path_components), and assert the resolved path is canonically inside repos_dir before any remove_dir_all. Ideally make repo_disk_path itself reject unsafe names.
• Note: a destructive remove_dir_all should never run on a DB-sourced, unsanitized name. End-to-end exploitability is narrow (requires the burst DID + marker check at the traversed path), so rated Low/Medium — but it is a clear defense-in-depth defect.

M4. purge-spam TOCTOU: emptiness re-checked once as a batch snapshot, not immediately before each delete — races a live push

• crates/gitlawb-node/src/admin.rs:221-235 — partition_for_delete (:224-226) re-verifies emptiness for all candidates up front, producing to_delete; the actual delete_repo_by_id runs later in a loop (:235). Between the snapshot and a given delete, a concurrent git-receive-pack on the running node can add a ref, making the repo non-empty — yet it is still deleted (silent data loss). The comment claims it "re-verify[s] emptiness immediately before deleting," but the recheck is a single batch pass, not a per-candidate check immediately before delete_repo_by_id.
• The execute path also does not acquire the node's acquire_write advisory lock (api/repos.rs:927), so it races a live push: a push can leave orphaned Tigris objects / ref-certificate rows referencing a now-deleted repos.id, and remove_dir_all can race an in-flight receive-pack writing objects/.
• Fix: re-verify ref_count_on_disk immediately before each delete_repo_by_id (or wrap recheck+delete of each repo in a repo_store.acquire_write guard / DB transaction re-checking a ref signal). Document that the tool must be run against a quiesced node for the target DID.

M5. iCaptcha gate fails open in enforce mode when the public key cannot load

• crates/gitlawb-node/src/icaptcha.rs:302-304 — inside decide, if v.key.is_none() { return Decision::Allow; } runs before the mode match, so in Mode::Enforce a missing key (iCaptcha unreachable at startup, DNS/BGP block, bad ICAPTCHA_PUBKEY) allows every create/registration request with no proof. init() (icaptcha.rs:160-166) leaves key = None and only warns. Since the gate defaults to off, the default deployment already has no captcha; enforce-mode is now trivially bypassable by denying the key fetch, directly weakening the abuse protection this PR leans on after PoW removal.
• Fix: in enforce mode fail closed (reject) when key.is_none(); or refuse to start in enforce mode without a key. At minimum document that enforce+no-key == fail-open.

M6. purge-spam deletes the DB row, then removes the dir; a disk failure is swallowed while the deleted counter is already incremented

• crates/gitlawb-node/src/admin.rs:235-248 — on Ok(n) the row is counted as deleted before remove_dir_all, which on error only warn!s. The final "deleted N repo row(s)" summary therefore reports success even when the on-disk repo survives → DB/disk drift and a misleading operator-facing count. No retry / non-zero exit signal.
• Fix: remove the on-disk dir first (or track disk-failure separately from the DB-deleted count) and surface "N deleted, M failed."

M7. delete_repo_by_id (and purge-spam) leave dependent rows orphaned

• crates/gitlawb-node/src/db/mod.rs:1288-1294 — DELETE FROM repos WHERE id = $1 with no cascade. Child tables keyed on repo_id (branch_cids, ref_certificates, visibility_rules, repo_stars, encrypted_blobs, received_ref_updates, repo_replicas, arweave_anchors, webhooks, pull_requests) keep orphan rows that accumulate and can be re-joined if a repo with the same id is later created.
• Fix: add ON DELETE CASCADE (or explicit child deletes) for the repo_id FKs, ideally within the same transaction as the row delete.

---

Low / Info — nice to have

• L1. Rate-limiter bounded-map DoS. rate_limit.rs:96-114 rejects new keys when at the 200k cap; a distinct-key flood (IPv6 variation or the M2 header rotation) then 429s legitimate new clients for up to a full window. Consider evicting expired entries to admit new keys, or a metric on cap-full rejections.
• L2. Limiter state is per-process / in-memory (rate_limit.rs:24-41). In multi-machine Fly deployments the effective per-IP/per-DID limit is N × configured and a restart wipes buckets. Known tradeoff — document explicitly; consider shared counters for the spam-relevant paths.
• L3. Non-atomic create_repo. api/repos.rs:203-236 check-then-insert with no UNIQUE(owner_did, name); the idx_repos_owner_key_name is a plain index (db/mod.rs:474-490). A concurrent create race surfaces a raw UNIQUE(disk_path) violation mapped to a generic 500 instead of RepoExists (409).
• L4. Read handlers mask git errors as empty 200 OK. api/repos.rs:361 (store::log(...).unwrap_or_default()) and the get_tree siblings coerce real backend failures into empty lists, unlike get_blob (api/repos.rs:398-410) which classifies errors. Map Err to AppError::Git/NotFound.
• L5. parse_ref_updates silently tolerates malformed/truncated bodies (api/repos.rs:1651-1699) → empty Vec<RefUpdate>; receive-pack then proceeds with zero ref updates (skipping branch-protection/owner-push checks). Return 400 on a non-empty body that parses short.
• L6. Push bookkeeping errors swallowed (api/repos.rs:959, 977, 982, 999-1005 let _ = / only-warn!) — transient DB blips silently lose record_push/update_trust_score. At least raise log severity / add a durable outbox.
• L7. git_info_refs receive-pack brake skipped when client_key returns None (api/repos.rs:556 / rate_limit.rs:198): if no trusted header and no ConnectInfo peer, the if let Some(key) guard is skipped and the request proceeds unthrottled. Unreachable in prod (socket peer always present) but a divergent contract from rate_limit_by_ip; document or reject instead of skip.
• L8. Malformed rate-limit env vars silently fall back to defaults. main.rs:304-307, 325-328, 343-346 parse GITLAWB_CREATE/WRITE/PUSH_RATE_LIMIT with .ok().and_then(parse).unwrap_or(default); a fat-fingered value silently disables the intended (stricter) brake with no warning, unlike the clap-validated siblings. Warn (or fail fast) on present-but-unparseable values.
• L9. .env.example docs drift. Missing GITLAWB_IPFS_API, GITLAWB_TIGRIS_BUCKET, GITLAWB_METRICS_ADDR, GITLAWB_SHUTDOWN_GRACE_SECS (parsed in config.rs:88,161,188,194). Reverse direction is clean. Add with defaults.
• L10. Weak dry-run safety test. admin.rs:471-492 (dry_run_deletes_nothing) seeds repos with an empty repos_dir, so run_purge_spam hits the no-candidate early return before any delete logic — it does not exercise the if !execute { return } guard when candidates exist. Add a dry-run test with a real empty on-disk target repo so a regression that deletes in dry-run is caught.
• L11. Unauthenticated /metrics (main.rs:828-875, config.rs:187 warns it is unauthenticated). If bound to a non-loopback addr it leaks peer/repo/push counts. Operator-controlled; note in hardening guidance.
• L12. Git error strings returned to clients (AppError::Git(e.to_string()) sites, e.g. api/repos.rs:398-410). Can surface internal paths; return opaque messages, keep detail in server-side logs.

---