diff --git a/src/auth/rate-limit.ts b/src/auth/rate-limit.ts index 99ae735a46..2e88caa76f 100644 --- a/src/auth/rate-limit.ts +++ b/src/auth/rate-limit.ts @@ -147,59 +147,8 @@ async function validateBearerForRateLimit(c: Context<{ Bindings: Env }>, token: } 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 { - 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 { diff --git a/test/unit/auth.test.ts b/test/unit/auth.test.ts index 6730afd088..f493f93550 100644 --- a/test/unit/auth.test.ts +++ b/test/unit/auth.test.ts @@ -141,7 +141,7 @@ describe("private-beta auth and rate limiting", () => { 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 @@ describe("private-beta auth and rate limiting", () => { ), ).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 @@ describe("private-beta auth and rate limiting", () => { ).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 @@ describe("private-beta auth and rate limiting", () => { 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 @@ describe("private-beta auth and rate limiting", () => { 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 @@ describe("private-beta auth and rate limiting", () => { 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" });