Skip to content

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

Closed
RealDiligent wants to merge 7 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-selfhost-ratelimit-ip-v2
Closed

fix(selfhost): stop client-spoofed cf-connecting-ip from bypassing rate limits#6550
RealDiligent wants to merge 7 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-selfhost-ratelimit-ip-v2

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

  • Root cause: Self-host binds Redis RATE_LIMITER, but clientIp() trusts cf-connecting-ip. On Node that header is attacker-controlled (Caddy only sets X-Real-IP / X-Forwarded-For).
  • Fix: At the Node serve({ fetch }) edge, overwrite cf-connecting-ip via withTrustedClientIp + peerRemoteAddress (documented @hono/node-server HttpBindings | Http2Bindings shape, unit-tested). Behind a private peer prefer X-Real-IP / XFF; on a public peer use the TCP address.
  • Impact: Self-host auth/webhook rate limits bind to the real client again.

Supersedes closed #6547 (CI + linked-issue requirements). Maintainer: contributors cannot open issues on this repo (CreateIssue denied); please open/label a gittensor:bug tracking issue so this PR can Closes #N.

Test plan

  • Unit tests: spoof rejection, Caddy hop, peerRemoteAddress shape, IPv4-mapped peers
  • fetch typed as HttpBindings | Http2Bindings
  • CI validate-code / codecov/patch green before merge

Risk / tradeoffs

  • Assumes Caddy on a private compose network is the public front (matches shipped Caddyfile).

RealDiligent and others added 2 commits July 16, 2026 21:04
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 16, 2026 13:19
@RealDiligent
RealDiligent force-pushed the fix/critical-issue-selfhost-ratelimit-ip-v2 branch 2 times, most recently from d586533 to b460278 Compare July 16, 2026 13:20
@superagent-security

Copy link
Copy Markdown
Contributor

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

RealDiligent and others added 2 commits July 16, 2026 21:23
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.82%. Comparing base (593959a) to head (dd36ad8).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6550      +/-   ##
==========================================
+ Coverage   93.59%   95.82%   +2.22%     
==========================================
  Files         673      591      -82     
  Lines       67687    47170   -20517     
  Branches    18618    15020    -3598     
==========================================
- Hits        63354    45200   -18154     
+ Misses       3360     1176    -2184     
+ Partials      973      794     -179     
Flag Coverage Δ
rees ?
shard-1 43.97% <100.00%> (-0.01%) ⬇️
shard-2 37.07% <0.00%> (+0.14%) ⬆️
shard-3 32.40% <0.00%> (-0.10%) ⬇️
shard-4 34.86% <0.00%> (-0.01%) ⬇️
shard-5 31.28% <0.00%> (+0.12%) ⬆️
shard-6 45.67% <0.00%> (+0.22%) ⬆️

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 83 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 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Caution

🛑 LoopOver review result - reject/close recommended

Review updated: 2026-07-16 13:47:21 UTC

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

🛑 Suggested Action - Reject/Close

Review summary
This adds a Node-only trusted-client-ip resolver that overwrites cf-connecting-ip at the serve({fetch}) edge using the TCP peer or, behind a private hop (Caddy), X-Real-IP/XFF, closing the spoof vector where a Node client can set cf-connecting-ip directly. The core logic in trusted-client-ip.ts is sound and well-tested for the cases shown (IPv4/IPv6 validation, IPv4-mapped stripping, private/link-local classification, header precedence). The main concrete issue is the failing codecov/patch gate (73.21% vs 99% target), and the design still hinges on the untested assumption that the deployed Caddy is always the sole private-network hop in front of the app — if a public interface is ever reachable directly while Caddy is also used, IP determination could be manipulated, but that's a deployment-topology risk already acknowledged in the PR description rather than a code defect.

Nits — 5 non-blocking
  • The `isPrivateOrLinkLocal` and IPv4/IPv6 validation functions use many bare numeric literals (4, 8, 127, 192, 168, 172, 16, 31, 169, 254) without named constants, which the external brief flags — consider extracting octet-range constants for readability (src/selfhost/trusted-client-ip.ts:100-108).
  • codecov/patch failed at 73.21% vs the 99% target for this PR — worth checking which branches in trusted-client-ip.ts or server.ts:895-899 are uncovered before merge.
  • src/server.ts is already at ~905 lines (long-file smell); not introduced by this diff, but the new fetch-wrapping logic adds more surface to an already large file worth extracting review-wise.
  • The withTrustedClientIp doc comment says 'never trust cf-connecting-ip on Node' but doesn't mention what happens to other Cloudflare-only headers (e.g., cf-ray) that might also be spoofable — outside this PR's scope but worth a follow-up note.
  • Consider adding an integration-level test (or at least a comment) confirming server.ts:899's `withTrustedClientIp(request, peerRemoteAddress(nodeEnv))` call site is exercised by the boot smoke test, since unit tests only cover the pure functions in isolation.

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 #6547
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: 342 registered-repo PR(s), 166 merged, 31 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 342 PR(s), 31 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: significant
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: 342 PR(s), 31 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

RealDiligent and others added 3 commits July 16, 2026 21:38
…p patch

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

loopover-orb Bot commented Jul 16, 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.

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.

1 participant