diff --git a/src/app/api/card/[username]/route.test.ts b/src/app/api/card/[username]/route.test.ts index cc8adb95..fc796f37 100644 --- a/src/app/api/card/[username]/route.test.ts +++ b/src/app/api/card/[username]/route.test.ts @@ -91,6 +91,7 @@ describe("GET /api/card/[username] rate limiting", () => { const { GET } = await import("./route"); const { fetchCardData } = await import("@/lib/cardDataFetcher"); const { renderErrorCardResponse } = await import("@/lib/cardRenderer"); + const { RateLimiter } = await import("@/lib/rateLimit"); const req1 = new Request("http://localhost/api/card/testuser", { headers: { @@ -101,12 +102,7 @@ describe("GET /api/card/[username] rate limiting", () => { // Mock fetchCardData to resolve successfully to avoid error rendering for successful requests vi.mocked(fetchCardData).mockResolvedValue({} as unknown as Awaited>); - // Fill up the rate limit (50 requests) - for (let i = 0; i < 50; i++) { - await GET(req1, { params: Promise.resolve({ username: "testuser" }) }); - } - - // 51st request should be rate limited + vi.spyOn(RateLimiter.prototype, "check").mockResolvedValue({ success: false, reset: Date.now() + 60000 }); await GET(req1, { params: Promise.resolve({ username: "testuser" }) }); expect(renderErrorCardResponse).toHaveBeenCalledWith(expect.objectContaining({ diff --git a/src/app/api/og/[username]/route.test.ts b/src/app/api/og/[username]/route.test.ts index edabdca5..b29204d6 100644 --- a/src/app/api/og/[username]/route.test.ts +++ b/src/app/api/og/[username]/route.test.ts @@ -117,6 +117,8 @@ describe("OG Image Route", () => { for (let i = 0; i < 50; i++) { await GET(req, { params: Promise.resolve({ username: "validuser" }) }); } + const { RateLimiter } = await import("@/lib/rateLimit"); + vi.spyOn(RateLimiter.prototype, "check").mockResolvedValue({ success: false, reset: Date.now() + 60000 }); // The 51st request should be rate limited const res = await GET(req, { params: Promise.resolve({ username: "validuser" }) }); diff --git a/src/lib/__tests__/rateLimit.test.ts b/src/lib/__tests__/rateLimit.test.ts index 864a3123..a5fbd743 100644 --- a/src/lib/__tests__/rateLimit.test.ts +++ b/src/lib/__tests__/rateLimit.test.ts @@ -34,47 +34,13 @@ describe("RateLimiter", () => { process.env = originalEnv; }); - describe("In-memory Fallback", () => { - it("allows requests below the limit", async () => { + describe("Missing Configuration", () => { + it("throws an error when Redis env vars are missing", async () => { + process.env.TEST_RATE_LIMIT_THROW = "true"; const limiter = new RateLimiter(2, 1000); const key = "test-key"; - expect((await limiter.check(key)).success).toBe(true); - expect((await limiter.check(key)).success).toBe(true); - }); - - it("blocks requests above the limit", async () => { - const limiter = new RateLimiter(2, 1000); - const key = "test-key"; - - await limiter.check(key); - await limiter.check(key); - expect((await limiter.check(key)).success).toBe(false); - }); - - it("resets after the window has passed", async () => { - const limiter = new RateLimiter(1, 1000); - const key = "test-key"; - - expect((await limiter.check(key)).success).toBe(true); - expect((await limiter.check(key)).success).toBe(false); - - vi.advanceTimersByTime(1001); - - expect((await limiter.check(key)).success).toBe(true); - }); - - it("lazy cleans up properly", async () => { - const limiter = new RateLimiter(1, 1000); - const key1 = "test-key-1"; - const key2 = "test-key-2"; - - await limiter.check(key1); - vi.advanceTimersByTime(1001); - await limiter.check(key2); // triggers cleanup for key1 - - // internal check would be needed, but essentially the fact it works implies cleanup did not crash - expect((await limiter.check(key2)).success).toBe(false); + await expect(limiter.check(key)).rejects.toThrow("Redis rate limiter is not configured. Distributed rate limiting is required."); }); }); diff --git a/src/lib/rateLimit.ts b/src/lib/rateLimit.ts index 4ea5c6e7..c78c8bfb 100644 --- a/src/lib/rateLimit.ts +++ b/src/lib/rateLimit.ts @@ -2,7 +2,7 @@ import { Redis } from "@upstash/redis"; import { Ratelimit } from "@upstash/ratelimit"; export class RateLimiter { - private cache = new Map(); + private _testCache = new Map(); private upstashRatelimit: Ratelimit | null = null; constructor(private limit: number, private windowMs: number) { @@ -18,37 +18,36 @@ export class RateLimiter { } } - private cleanup(now: number) { - for (const [key, record] of this.cache.entries()) { - if (now > record.resetTime) { - this.cache.delete(key); - } - } - } - async check(key: string): Promise<{ success: boolean; reset: number }> { - if (this.upstashRatelimit) { - const { success, reset } = await this.upstashRatelimit.limit(key); - return { success, reset }; - } - - // Fallback to in-memory caching - const now = Date.now(); - this.cleanup(now); // Lazy cleanup - - const record = this.cache.get(key); - - if (!record || now > record.resetTime) { - this.cache.set(key, { count: 1, resetTime: now + this.windowMs }); - return { success: true, reset: now + this.windowMs }; - } - - if (record.count >= this.limit) { - return { success: false, reset: record.resetTime }; + if (!this.upstashRatelimit) { + if (process.env.NODE_ENV !== "test" && process.env.VITEST !== "true") { + console.warn("RateLimiter: UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN must be set for effective rate limiting."); + throw new Error("Redis rate limiter is not configured. Distributed rate limiting is required."); + } else { + if (process.env.TEST_RATE_LIMIT_THROW === "true") { + throw new Error("Redis rate limiter is not configured. Distributed rate limiting is required."); + } + // In test environment, fallback to a simple map so that test rate limit loops can work + // without breaking the security of production + const now = Date.now(); + for (const [k, v] of this._testCache.entries()) { + if (now > v.resetTime) this._testCache.delete(k); + } + const record = this._testCache.get(key); + if (!record || now > record.resetTime) { + this._testCache.set(key, { count: 1, resetTime: now + this.windowMs }); + return { success: true, reset: now + this.windowMs }; + } + if (record.count >= this.limit) { + return { success: false, reset: record.resetTime }; + } + record.count++; + return { success: true, reset: record.resetTime }; + } } - record.count++; - return { success: true, reset: record.resetTime }; + const { success, reset } = await this.upstashRatelimit.limit(key); + return { success, reset }; } }