fix(auth): ignore spoofable proxy IP headers

src/auth/rate-limit.ts

@@ -1,4 +1,4 @@
 import type { Context } from "hono";
 import { DurableObject } from "cloudflare:workers";
 import { recordAuditEvent } from "../db/repositories";
 import { authenticateInternalToken, authenticatePrivateToken, extractBearerToken, hashToken } from "./security";
@@ -147,59 +147,8 @@
 }
 
 function clientIp(c: Context<{ Bindings: Env }>): string {
-  const cloudflareIp = normalizeIpAddress(c.req.header("cf-connecting-ip"));
-  if (cloudflareIp) return cloudflareIp;
-
-  if (!isTrustedProxyRequest(c)) return "unknown-ip";
-
-  const proxyCount = trustedProxyCount(c.env.RATE_LIMIT_TRUSTED_PROXY_COUNT);
-  return (
-    firstValidIp([
-      c.req.header("x-real-ip"),
-      trustedForwardedForIp(c.req.header("x-forwarded-for"), proxyCount),
-    ]) ?? "unknown-ip"
-  );
-}
-
-function isTrustedProxyRequest(c: Context<{ Bindings: Env }>): boolean {
-  const trustedProxies = parseTrustedProxyList(c.env.RATE_LIMIT_TRUSTED_PROXIES);
-  if (trustedProxies.length === 0) return false;
-  const chain = forwardedForCandidates(c.req.header("x-forwarded-for"))
-    .map((entry) => normalizeIpAddress(entry))
-    .filter((entry): entry is string => Boolean(entry));
-  const peer = chain[chain.length - 1];
-  return peer ? trustedProxies.includes(peer) : false;
-}
-
-function parseTrustedProxyList(value: string | undefined): string[] {
-  return (value ?? "")
-    .split(",")
-    .map((entry) => normalizeIpAddress(entry))
-    .filter((entry): entry is string => Boolean(entry));
-}
-
-function trustedProxyCount(value: string | undefined): number {
-  const parsed = Number(value?.trim());
-  return Number.isInteger(parsed) && parsed > 0 ? parsed : 1;
-}
-
-function trustedForwardedForIp(header: string | undefined, trustedProxyCountValue: number): string | undefined {
-  const chain = forwardedForCandidates(header);
-  if (chain.length < trustedProxyCountValue) return undefined;
-  return chain[chain.length - trustedProxyCountValue];
-}
-
-function forwardedForCandidates(header: string | undefined): string[] {
-  if (!header?.trim()) return [];
-  return header.split(",").map((part) => part.trim()).filter(Boolean);
-}
-
-function firstValidIp(candidates: Array<string | undefined>): string | undefined {
-  for (const candidate of candidates) {
-    const valid = normalizeIpAddress(candidate);
-    if (valid) return valid;
-  }
-  return undefined;
+  // Only trust Cloudflare-populated client IPs. Proxy fallback headers can be supplied by clients in Workers.
+  return normalizeIpAddress(c.req.header("cf-connecting-ip")) ?? "unknown-ip";
 }
 
 function normalizeIpAddress(value: string | undefined): string | undefined {

test/unit/auth.test.ts

@@ -1,4 +1,4 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { completeGitHubWebOAuth, createSessionFromGitHubToken, pollGitHubDeviceFlow, startGitHubDeviceFlow, startGitHubWebOAuth } from "../../src/auth/github-oauth";
 import { enforceRateLimit, RateLimiter, routeClassForPath } from "../../src/auth/rate-limit";
 import { authenticatePrivateToken, buildBrowserSessionCookie, createSessionForGitHubUser, extractCookieValue, isAuthorizedGitHubSessionLogin, revokeSession, timingSafeEqual } from "../../src/auth/security";
@@ -141,7 +141,7 @@
     expect(observedKeys[0]).toMatch(/^normal:\/v1\/public\/github\/repos\/:owner\/:repo\/stats:ip:/);
   });
 
-  it("keys pre-auth routes by proxy fallback headers when cf-connecting-ip is absent", async () => {
+  it("ignores proxy fallback headers when cf-connecting-ip is absent", async () => {
     const observedKeys: string[] = [];
     const env = rateLimitTestEnv({}, observedKeys);
 
@@ -158,7 +158,7 @@
       ),
     ).resolves.toBeNull();
     expect(observedKeys).toHaveLength(2);
-    expect(observedKeys[0]).not.toBe(observedKeys[1]);
+    expect(observedKeys[0]).toBe(observedKeys[1]);
     expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/session:ip:/);
 
     observedKeys.length = 0;
@@ -170,36 +170,12 @@
     ).resolves.toBeNull();
     await expect(
       enforceRateLimit(
-        fakeContext(env, "/v1/auth/github/session", trustedProxyHeaders({ "x-real-ip": "198.51.100.3" })),
-        "strict",
-      ),
-    ).resolves.toBeNull();
-    expect(observedKeys).toHaveLength(2);
-    expect(observedKeys[0]).toBe(observedKeys[1]);
-
-    observedKeys.length = 0;
-    await expect(
-      enforceRateLimit(
-        fakeContext(env, "/v1/auth/github/session", trustedProxyHeaders({ "x-real-ip": "203.0.113.44", "x-forwarded-for": "198.51.100.99" })),
+        fakeContext(env, "/v1/auth/github/session", trustedProxyHeaders({ "x-real-ip": "203.0.113.44" })),
         "strict",
       ),
     ).resolves.toBeNull();
-    await expect(
-      enforceRateLimit(fakeContext(env, "/v1/auth/github/session", trustedProxyHeaders({ "x-real-ip": "203.0.113.44" })), "strict"),
-    ).resolves.toBeNull();
     expect(observedKeys).toHaveLength(2);
     expect(observedKeys[0]).toBe(observedKeys[1]);
-
-    observedKeys.length = 0;
-    await expect(
-      enforceRateLimit(fakeContext(env, "/v1/auth/github/session", { "x-forwarded-for": "198.51.100.1" }), "strict"),
-    ).resolves.toBeNull();
-    await expect(
-      enforceRateLimit(fakeContext(env, "/v1/auth/github/session", { "x-forwarded-for": "198.51.100.2" }), "strict"),
-    ).resolves.toBeNull();
-    expect(observedKeys).toHaveLength(2);
-    expect(observedKeys[0]).toBe(observedKeys[1]);
-    expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/session:ip:/);
   });
 
   it("does not treat spoofed cf-ray as trusted proxy proof", async () => {
@@ -226,7 +202,7 @@
     expect(observedKeys[0]).toMatch(/^strict:\/v1\/auth\/github\/session:ip:/);
   });
 
-  it("keys pre-auth routes by configured trusted proxy IPs without cf-ray", async () => {
+  it("ignores configured trusted proxy IPs without Cloudflare client IP", async () => {
     const observedKeys: string[] = [];
     const env = createTestEnv({
       RATE_LIMITER: rateLimiterNamespace({ status: 200, body: {} }, observedKeys) as unknown as DurableObjectNamespace,
@@ -246,7 +222,7 @@
     await expect(
       enforceRateLimit(
         fakeContext(env, "/v1/auth/github/session", {
-          "x-forwarded-for": "198.51.100.2, 198.51.100.99",
+          "x-forwarded-for": "198.51.100.3, 198.51.100.99",
         }),
         "strict",
       ),
@@ -424,6 +400,31 @@
     expect(observedKeys[0]).toBe(unknownIpKey);
   });
 
+  it("normalizes only valid Cloudflare client IP headers", async () => {
+    const observedKeys: string[] = [];
+    const env = rateLimitTestEnv({}, observedKeys);
+
+    await expect(enforceRateLimit(fakeContext(env, "/v1/auth/github/session", { "cf-connecting-ip": " 203.0.113.9 " }), "strict")).resolves.toBeNull();
+    const ipv4Key = observedKeys[0];
+
+    observedKeys.length = 0;
+    await expect(enforceRateLimit(fakeContext(env, "/v1/auth/github/session", { "cf-connecting-ip": "[2001:db8::1]" }), "strict")).resolves.toBeNull();
+    await expect(enforceRateLimit(fakeContext(env, "/v1/auth/github/session", { "cf-connecting-ip": "2001:db8::1" }), "strict")).resolves.toBeNull();
+    expect(observedKeys).toHaveLength(2);
+    expect(observedKeys[0]).toBe(observedKeys[1]);
+    expect(observedKeys[0]).not.toBe(ipv4Key);
+
+    observedKeys.length = 0;
+    await expect(enforceRateLimit(fakeContext(env, "/v1/auth/github/session"), "strict")).resolves.toBeNull();
+    const unknownIpKey = observedKeys[0];
+
+    for (const value of ["", "not-an-ip", "1.2.3", "256.0.0.1", "1::2::3", "1:2:3:4:5:6:7:8:9", "::"]) {
+      observedKeys.length = 0;
+      await expect(enforceRateLimit(fakeContext(env, "/v1/auth/github/session", { "cf-connecting-ip": value }), "strict")).resolves.toBeNull();
+      expect(observedKeys[0]).toBe(unknownIpKey);
+    }
+  });
+
   it("enforces route limits with session and IP keys plus retry headers", async () => {
     const env = createTestEnv();
     const noLimiter = fakeContext(env, "/v1/repos/123/pulls/456", { authorization: "Bearer session-token" });