Skip to content

fix(event): remove shared NAT rate limits #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
AsperforMias wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(event): remove shared NAT rate limits #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/app/api/roast/route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,7 @@ const mocks = vi.hoisted(() => ({
acquireRoastLock: vi.fn(),
buildRoastMessages: vi.fn(),
checkRoastRateLimit: vi.fn(),
checkRoastNetworkRateLimit: vi.fn(),
checkRoastRequestRateLimit: vi.fn(),
checkRoastRequestNetworkRateLimit: vi.fn(),
chat: vi.fn(),
defaultLlmConfig: vi.fn(),
fallbackLlmConfig: vi.fn(),
Expand Down Expand Up @@ -40,9 +38,7 @@ vi.mock("@/lib/rank", () => ({ getRankCached: mocks.getRankCached }));
vi.mock("@/lib/redis", () => ({
acquireRoastLock: mocks.acquireRoastLock,
checkRoastRateLimit: mocks.checkRoastRateLimit,
checkRoastNetworkRateLimit: mocks.checkRoastNetworkRateLimit,
checkRoastRequestRateLimit: mocks.checkRoastRequestRateLimit,
checkRoastRequestNetworkRateLimit: mocks.checkRoastRequestNetworkRateLimit,
clearCachedRoast: vi.fn(),
getCachedRoast: mocks.getCachedRoast,
getCachedScan: mocks.getCachedScan,
Expand Down Expand Up @@ -76,8 +72,6 @@ describe("POST /api/roast quick score contract", () => {
beforeEach(() => {
mocks.checkRoastRequestRateLimit.mockResolvedValue({ success: true });
mocks.checkRoastRateLimit.mockResolvedValue({ success: true });
mocks.checkRoastRequestNetworkRateLimit.mockResolvedValue({ success: true });
mocks.checkRoastNetworkRateLimit.mockResolvedValue({ success: true });
mocks.rateLimitHeaders.mockReturnValue({});
mocks.anonymousSessionPrincipal.mockReturnValue(null);
mocks.defaultLlmConfig.mockReturnValue({ baseURL: "https://llm.example.test", apiKey: "key", model: "model" });
Expand Down Expand Up @@ -123,7 +117,7 @@ describe("POST /api/roast quick score contract", () => {
expect(mocks.checkRoastRateLimit).toHaveBeenCalled();
});

it("uses the signed browser session while retaining the shared network budgets", async () => {
it("uses the signed browser session without a shared-NAT budget", async () => {
mocks.anonymousSessionPrincipal.mockReturnValue("anon:session-fixture");
const response = await POST(new NextRequest("https://example.test/api/roast", {
method: "POST",
Expand All @@ -134,9 +128,7 @@ describe("POST /api/roast quick score contract", () => {
expect(response.status).toBe(200);
await new Response(response.body).text();
expect(mocks.checkRoastRequestRateLimit).toHaveBeenCalledWith("anon:session-fixture");
expect(mocks.checkRoastRequestNetworkRateLimit).toHaveBeenCalledWith("198.51.100.10");
expect(mocks.checkRoastRateLimit).toHaveBeenCalledWith("anon:session-fixture");
expect(mocks.checkRoastNetworkRateLimit).toHaveBeenCalledWith("198.51.100.10");
});

it("keeps machine-authenticated callers on their IP budget", async () => {
Expand Down
31 changes: 1 addition & 30 deletions src/app/api/roast/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ import {
acquireRoastLock,
checkRoastRequestRateLimit,
checkRoastRateLimit,
checkRoastNetworkRateLimit,
checkRoastRequestNetworkRateLimit,
clearCachedRoast,
getCachedRoast,
getCachedScan,
Expand Down Expand Up @@ -624,7 +622,7 @@ export async function POST(req: NextRequest) {
}
const ip = clientIp(req);
// CLI/MCP callers retain their IP budget. Only an interactive browser that
// completed Turnstile can exchange its shared-NAT IP key for a signed session.
// completed Turnstile receives a signed session with its own budget.
const principal = auth === "absent" ? anonymousSessionPrincipal(req) ?? ip : ip;

// This protects every path, including BYO: it runs before the snapshot and
Expand All @@ -640,20 +638,6 @@ export async function POST(req: NextRequest) {
},
);
}
const networkRequestLimit = await checkRoastRequestNetworkRateLimit(ip);
if (!networkRequestLimit.success) {
return NextResponse.json(
{
error: networkRequestLimit.unavailable ? "rate_limit_unavailable" : "rate_limited",
useByoKey: true,
},
{
status: networkRequestLimit.unavailable ? 503 : 429,
headers: { ...rateLimitHeaders(networkRequestLimit), "Cache-Control": "no-store" },
},
);
}

const lang = normLang(body.lang);

// A verified v5/v5/v3 artifact is a read-only continuity path when the quick
Expand Down Expand Up @@ -805,19 +789,6 @@ export async function POST(req: NextRequest) {
},
);
}
const networkGenerationLimit = await checkRoastNetworkRateLimit(ip);
if (!networkGenerationLimit.success) {
return NextResponse.json(
{
error: networkGenerationLimit.unavailable ? "rate_limit_unavailable" : "rate_limited",
useByoKey: true,
},
{
status: networkGenerationLimit.unavailable ? 503 : 429,
headers: { ...rateLimitHeaders(networkGenerationLimit), "Cache-Control": "no-store" },
},
);
}
isLeader = await acquireRoastLock(username, lang);
if (isLeader) {
if (refreshHonored) await clearCachedRoast(username, lang);
Expand Down
6 changes: 1 addition & 5 deletions src/app/api/scan/route.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import type { ScanResult } from "@/lib/types";
const mocks = vi.hoisted(() => ({
buildScanResult: vi.fn(),
checkRateLimit: vi.fn(),
checkScanNetworkRateLimit: vi.fn(),
coalesceScan: vi.fn(),
getCachedScan: vi.fn(),
getLegacyReadFallbackScan: vi.fn(),
Expand All @@ -29,7 +28,6 @@ vi.mock("@/lib/db", () => ({
}));
vi.mock("@/lib/redis", () => ({
checkRateLimit: mocks.checkRateLimit,
checkScanNetworkRateLimit: mocks.checkScanNetworkRateLimit,
coalesceScan: mocks.coalesceScan,
getCachedScan: mocks.getCachedScan,
rateLimitHeaders: mocks.rateLimitHeaders,
Expand Down Expand Up @@ -61,7 +59,6 @@ describe("POST /api/scan immediate quick contract", () => {
beforeEach(() => {
process.env.GITHUB_ROAST_CLI_API_KEY = "test-key";
mocks.checkRateLimit.mockResolvedValue({ success: true });
mocks.checkScanNetworkRateLimit.mockResolvedValue({ success: true });
mocks.rateLimitHeaders.mockReturnValue({});
mocks.getCachedScan.mockResolvedValue(null);
mocks.coalesceScan.mockImplementation(async (_handle: string, produce: () => unknown) => produce());
Expand Down Expand Up @@ -112,7 +109,7 @@ describe("POST /api/scan immediate quick contract", () => {
});
});

it("uses a Turnstile-issued browser session before the shared network budget", async () => {
it("uses a Turnstile-issued browser session without a shared-NAT budget", async () => {
delete process.env.GITHUB_ROAST_CLI_API_KEY;
mocks.establishAnonymousSession.mockReturnValue({ id: "session-fixture", issued: true });

Expand All @@ -124,7 +121,6 @@ describe("POST /api/scan immediate quick contract", () => {

expect(response.status).toBe(200);
expect(mocks.checkRateLimit).toHaveBeenCalledWith("anon:session-fixture");
expect(mocks.checkScanNetworkRateLimit).toHaveBeenCalledWith("198.51.100.10");
expect(mocks.attachAnonymousSession).toHaveBeenCalledWith(
expect.any(Response),
{ id: "session-fixture", issued: true },
Expand Down
9 changes: 0 additions & 9 deletions src/app/api/scan/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
} from "@/lib/db";
import {
checkRateLimit,
checkScanNetworkRateLimit,
coalesceScan,
getCachedScan,
rateLimitHeaders,
Expand Down Expand Up @@ -181,14 +180,6 @@ export async function POST(req: NextRequest) {
headers: { ...idem, ...rlHeaders, "Cache-Control": "no-store" },
});
}
const networkLimit = await checkScanNetworkRateLimit(ip);
if (!networkLimit.success) {
return apiError(networkLimit.unavailable ? "rate_limit_unavailable" : "rate_limited", {
status: networkLimit.unavailable ? 503 : 429,
headers: { ...idem, ...rateLimitHeaders(networkLimit), "Cache-Control": "no-store" },
});
}

const cached = await getCachedScan(username);
if (cached) {
if (!(await persistQuickScan(cached, Date.now()))) {
Expand Down
8 changes: 1 addition & 7 deletions src/lib/__tests__/redis-rate-limit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,16 @@ describe("production rate-limit availability", () => {
vi.stubEnv("VERCEL_ENV", "production");
unsetRedisEnv();
const error = vi.spyOn(console, "error").mockImplementation(() => {});
const { checkRateLimit, checkScanNetworkRateLimit, rateLimitHeaders } = await loadRedis();
const { checkRateLimit, rateLimitHeaders } = await loadRedis();

const result = await checkRateLimit("198.51.100.10");
const networkResult = await checkScanNetworkRateLimit("198.51.100.10");

expect(result).toMatchObject({ success: false, unavailable: true, retryAfter: 15 });
expect(networkResult).toMatchObject({ success: false, unavailable: true, retryAfter: 15 });
expect(rateLimitHeaders(result)).toEqual({ "Retry-After": "15" });
expect(error).toHaveBeenCalledWith(
"rate_limit_unavailable",
expect.objectContaining({ limiter: "scan", reason: "missing_redis_config" }),
);
expect(error).toHaveBeenCalledWith(
"rate_limit_unavailable",
expect.objectContaining({ limiter: "scan_network", reason: "missing_redis_config" }),
);
});

it("fails closed when a configured Redis limiter request errors", async () => {
Expand Down
84 changes: 0 additions & 84 deletions src/lib/redis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,11 @@ import type { RoastJudgeResult, RoastLine, ScanResult } from "./types";

let redis: Redis | null = null;
let scanLimiter: Ratelimit | null = null;
let scanNetworkLimiter: Ratelimit | null = null;
let campaignLeaderboardReadLimiter: Ratelimit | null = null;
let mcpLimiter: Ratelimit | null = null;
let roastRequestLimiter: Ratelimit | null = null;
let roastRequestNetworkLimiter: Ratelimit | null = null;
let roastMinuteLimiter: Ratelimit | null = null;
let roastDayLimiter: Ratelimit | null = null;
let roastNetworkMinuteLimiter: Ratelimit | null = null;
let roastNetworkDayLimiter: Ratelimit | null = null;
let verdictMinuteLimiter: Ratelimit | null = null;
let verdictDayLimiter: Ratelimit | null = null;

Expand Down Expand Up @@ -271,29 +267,6 @@ export async function checkRateLimit(principal: string): Promise<RateLimitResult
}
}

/** Wider second-line scan budget for browsers sharing one public network. */
export async function checkScanNetworkRateLimit(ip: string): Promise<RateLimitResult> {
const r = getRedis();
if (!r) return unavailableRateLimitResult("scan_network", "missing_redis_config");
if (!scanNetworkLimiter) {
scanNetworkLimiter = new Ratelimit({
redis: r,
limiter: Ratelimit.slidingWindow(60, "60 s"),
prefix: "rl:scan-network",
analytics: false,
});
}
try {
const { success, limit, remaining, reset } = await scanNetworkLimiter.limit(ip);
return { success, limit, remaining, reset };
} catch (error) {
return unavailableRateLimitResult(
"scan_network",
error instanceof Error ? error.name : "redis_request_failed",
);
}
}

/**
* Public event leaderboard refreshes fan out from many browsers that may share
* one venue NAT. Keep them off the scan budget while still bounding origin reads.
Expand Down Expand Up @@ -355,29 +328,6 @@ export async function checkRoastRequestRateLimit(principal: string): Promise<Rat
}
}

/** Wider second-line request budget for browsers sharing one public network. */
export async function checkRoastRequestNetworkRateLimit(ip: string): Promise<RateLimitResult> {
const r = getRedis();
if (!r) return unavailableRateLimitResult("roast_request_network", "missing_redis_config");
if (!roastRequestNetworkLimiter) {
roastRequestNetworkLimiter = new Ratelimit({
redis: r,
limiter: Ratelimit.slidingWindow(120, "60 s"),
prefix: "rl:roast-request-network",
analytics: false,
});
}
try {
const { success, limit, remaining, reset } = await roastRequestNetworkLimiter.limit(ip);
return { success, limit, remaining, reset };
} catch (error) {
return unavailableRateLimitResult(
"roast_request_network",
error instanceof Error ? error.name : "redis_request_failed",
);
}
}

/**
* Per-IP limiter for the MCP server. Tighter than the web scan limiter — the
* tools are unauthenticated and callable in a loop by an autonomous agent, so we
Expand Down Expand Up @@ -443,40 +393,6 @@ export async function checkRoastRateLimit(principal: string): Promise<RateLimitR
}
}

/** Wider network-level generation budget, separate from each signed browser. */
export async function checkRoastNetworkRateLimit(ip: string): Promise<RateLimitResult> {
const r = getRedis();
if (!r) return unavailableRateLimitResult("roast_generation_network", "missing_redis_config");
if (!roastNetworkMinuteLimiter) {
roastNetworkMinuteLimiter = new Ratelimit({
redis: r,
limiter: Ratelimit.slidingWindow(48, "60 s"),
prefix: "rl:roast-network:m",
analytics: false,
});
}
if (!roastNetworkDayLimiter) {
roastNetworkDayLimiter = new Ratelimit({
redis: r,
limiter: Ratelimit.slidingWindow(480, "1 d"),
prefix: "rl:roast-network:d",
analytics: false,
});
}
try {
const [minute, day] = await Promise.all([
roastNetworkMinuteLimiter.limit(ip),
roastNetworkDayLimiter.limit(ip),
]);
return { success: minute.success && day.success };
} catch (error) {
return unavailableRateLimitResult(
"roast_generation_network",
error instanceof Error ? error.name : "redis_request_failed",
);
}
}

/** Cached roast: the LLM-written report + deterministic score metadata, keyed
* by every input contract plus language and username (24h). */
export interface CachedRoast {
Expand Down
Loading