From d4d51cb1115160f33c1f64e7dc47991fc1771169 Mon Sep 17 00:00:00 2001
From: theonejvo <orlyjamie@users.noreply.github.com>
Date: Tue, 27 Jan 2026 07:59:20 +1100
Subject: [PATCH] fix: add rate limiting to download counter to prevent
 inflation

Downloads can be trivially inflated by spamming the endpoint with spoofed
X-Forwarded-For headers. This PR adds defense-in-depth measures:

- Rate limit: 5 downloads per skill per IP per hour
- Only trust cf-connecting-ip header (x-forwarded-for is spoofable)
- Still serve the file even if rate limited, just don't count it

NOTE: Download counts are fundamentally ungameable as trust metrics because
they're anonymous. Even with rate limiting, attackers can use proxies/VPNs.
Consider de-emphasizing downloads in the UI in favor of stars and installs
which require authenticated sessions.

Related: httpApiV1.ts:getClientIp() also trusts x-forwarded-for and should
be audited for similar issues.
---
 convex/downloads.ts | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/convex/downloads.ts b/convex/downloads.ts
index e8556a9..c3c5b1e 100644
--- a/convex/downloads.ts
+++ b/convex/downloads.ts
@@ -1,9 +1,28 @@
 import { v } from 'convex/values'
 import { zipSync } from 'fflate'
-import { api } from './_generated/api'
+import { api, internal } from './_generated/api'
 import { httpAction, mutation } from './_generated/server'
 import { applySkillStatDeltas, bumpDailySkillStats } from './lib/skillStats'
 
+// Rate limit: 5 downloads per skill per IP per hour
+// NOTE: This is defense-in-depth only. Download counts are fundamentally ungameable
+// as a trust metric because:
+//   1. Downloads are anonymous (no auth required)
+//   2. Attackers can use proxies/VPNs/Tor to bypass IP rate limits
+//   3. Even legitimate rate limiting can be circumvented at scale
+//
+// RECOMMENDATION: De-emphasize download counts in the UI. Stars and installs
+// are better trust signals because they require authenticated sessions.
+// Consider showing "X users installed" (from CLI telemetry) rather than downloads.
+const DOWNLOAD_RATE_LIMIT = 5
+const DOWNLOAD_RATE_WINDOW_MS = 60 * 60 * 1000 // 1 hour
+
+// Only trust cf-connecting-ip - other headers like x-forwarded-for are spoofable
+function getClientIpSecure(request: Request): string | null {
+  const cfIp = request.headers.get('cf-connecting-ip')
+  return cfIp?.trim() || null
+}
+
 export const downloadZip = httpAction(async (ctx, request) => {
   const url = new URL(request.url)
   const slug = url.searchParams.get('slug')?.trim().toLowerCase()
@@ -53,7 +72,21 @@ export const downloadZip = httpAction(async (ctx, request) => {
   const zipArray = Uint8Array.from(zipData)
   const zipBlob = new Blob([zipArray], { type: 'application/zip' })
 
-  await ctx.runMutation(api.downloads.increment, { skillId: skill._id })
+  // Only count download if IP passes rate limit check
+  const clientIp = getClientIpSecure(request)
+  if (clientIp) {
+    const rateLimitKey = `download:${skill._id}:${clientIp}`
+    const rateCheck = await ctx.runMutation(internal.rateLimits.checkRateLimitInternal, {
+      key: rateLimitKey,
+      limit: DOWNLOAD_RATE_LIMIT,
+      windowMs: DOWNLOAD_RATE_WINDOW_MS,
+    })
+    if (rateCheck.allowed) {
+      await ctx.runMutation(api.downloads.increment, { skillId: skill._id })
+    }
+    // If rate limited, still serve the file but don't count it
+  }
+  // If no IP (shouldn't happen on Cloudflare), don't count the download
 
   return new Response(zipBlob, {
     status: 200,