Skip to content

fix(selfhost): stop client-spoofed cf-connecting-ip from bypassing rate limits - #7392

Closed
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-selfhost-ratelimit-ip-v3
Closed

fix(selfhost): stop client-spoofed cf-connecting-ip from bypassing rate limits#7392
RealDiligent wants to merge 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-selfhost-ratelimit-ip-v3

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

Root cause: Self-host binds Redis RATE_LIMITER, but clientIp() in src/auth/rate-limit.ts trusts cf-connecting-ip. On Node that header is client-controlled (Caddy only sets X-Real-IP / X-Forwarded-For). Attackers can rotate it to bypass strict pre-auth buckets (10/min on /v1/auth/*); honest clients without it collapse to a shared unknown-ip bucket.

Fix: At the Node serve({ fetch }) edge, overwrite cf-connecting-ip via applySelfHostFetchTrustedClientIp (src/selfhost/trusted-client-ip.ts): delete any client-supplied value; behind a private/link-local peer (Caddy) prefer X-Real-IP / leftmost X-Forwarded-For; on a public peer use the TCP address. Cloudflare Workers path unchanged.

Impact: Self-host auth/webhook rate limits bind to the real client again.

Fixes #477

Test plan

  • test/unit/trusted-client-ip.test.ts — spoof rejection, Caddy hop, peerRemoteAddress shape, IPv4-mapped peers, malformed IP rejection
  • CI validate-code / codecov/patch green

Risk / tradeoffs

  • Assumes the only public front is Caddy on a private compose network (matches shipped Caddyfile). Direct public expose correctly uses the TCP peer and ignores proxy headers.

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 20, 2026 03:13
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.18%. Comparing base (47b34e9) to head (3b2cfbe).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7392   +/-   ##
=======================================
  Coverage   91.18%   91.18%           
=======================================
  Files         716      717    +1     
  Lines       72697    72754   +57     
  Branches    20423    20454   +31     
=======================================
+ Hits        66288    66340   +52     
- Misses       5367     5368    +1     
- Partials     1042     1046    +4     
Flag Coverage Δ
shard-1 36.25% <0.00%> (+0.32%) ⬆️
shard-2 40.94% <0.00%> (-0.56%) ⬇️
shard-3 39.95% <100.00%> (+0.91%) ⬆️
shard-4 38.94% <0.00%> (-0.99%) ⬇️
shard-5 28.32% <0.00%> (+1.13%) ⬆️
shard-6 30.68% <0.00%> (-0.67%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/selfhost/trusted-client-ip.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 20, 2026
@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-20 03:21:35 UTC

3 files · 1 AI reviewer · 1 blocker · CI green · clean

🛑 Suggested Action - Reject/Close

Review summary
This fix correctly moves IP trust resolution to the Node `serve({ fetch })` edge, deleting any client-supplied `cf-connecting-ip` and replacing it with a value derived from either the TCP peer or, when the peer is private/link-local (i.e. behind Caddy), the leftmost trusted proxy header. The IPv4/IPv6 validation and private-range classification are thorough and well covered by the accompanying unit tests, and the Workers path is left untouched. The design assumes a single private-network Caddy hop as the only public front, which matches the shipped Caddyfile and is called out explicitly as a documented tradeoff.

Nits — 5 non-blocking
  • src/selfhost/trusted-client-ip.ts: `resolveTrustedClientIp` only reads the first X-Forwarded-For hop and the peer's private/link-local check, so if a self-hoster puts a second proxy in front of Caddy (not the shipped topology) the leftmost XFF entry is still attacker-controlled — worth a comment noting this is intentionally out of scope.
  • src/selfhost/trusted-client-ip.ts: the numeric octet-range checks in `isPrivateOrLinkLocal` (10/8, 172.16-31/12, 192.168/16, 169.254/16) could use named constants for the CIDR boundaries to aid future readability, per the external brief.
  • src/server.ts: the `fetch` handler doc comment is long relative to the one-line change; consider trimming to the key invariant (spoof deletion + peer fallback) for scan-ability.
  • Consider exporting the private-IP ranges as named constants (e.g., `PRIVATE_IPV4_RANGES`) in trusted-client-ip.ts for readability and easier future edits (e.g. adding CGNAT 100.64/10).
  • If multi-hop proxy chains behind Caddy become a real deployment scenario, extend `resolveTrustedClientIp` to walk XFF from the right until it exits the trusted set rather than trusting the leftmost entry.

Why this is blocked

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #477
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 398 registered-repo PR(s), 190 merged, 33 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 398 PR(s), 33 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
Linked issue satisfaction

Partially addressed
The PR fixes the collapse-to-unknown-ip scenario by injecting a resolved cf-connecting-ip header at the Node serve edge (using X-Real-IP/X-Forwarded-For behind a trusted private hop, or the TCP peer otherwise), which does prevent same-bucket collapse when those headers are present, but it does not modify `clientIp()`/`rateLimitIdentity` in src/auth/rate-limit.ts as the issue explicitly requested,

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, JavaScript, Ruby, Svelte, TypeScript, Markdown, MDX
  • Official Gittensor activity: 398 PR(s), 33 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Keep the PR focused and include validation evidence before maintainer review.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb

loopover-orb Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

LoopOver is closing this pull request on the maintainer's behalf (No linked issue detected). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed.

@loopover-orb loopover-orb Bot closed this Jul 20, 2026
@RealDiligent

Copy link
Copy Markdown
Contributor Author

CI is fully green (validate, codecov/patch, self-host smoke). Auto-closed for linked issue #477 being closed as not_planned — this PR uses the Node-edge trusted-client-ip approach (supersedes closed #6550) which is the correct fix for self-host without trusting XFF on Workers. Maintainer: please reopen #477 (or file a new gittensor:bug issue) so this can merge. Happy to update the PR body with the new issue number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Pre-auth rate limits collapse to unknown-ip when cf-connecting-ip is absent, so one client can throttle everyone behind fallback proxies

1 participant