Skip to content

Conversation

@ctate
Copy link
Collaborator

@ctate ctate commented Dec 2, 2025

No description provided.

@vercel
Copy link
Contributor

vercel bot commented Dec 2, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
workflow-builder Ready Ready Preview Comment Dec 2, 2025 8:19pm

Comment on lines 80 to 95
const currentCount = result?.count ?? 0;
const remaining = Math.max(0, config.maxRequests - currentCount);

if (currentCount >= config.maxRequests) {
return {
allowed: false,
remaining: 0,
resetAt,
};
}

// Record this request
await db.insert(rateLimitEvents).values({
userId,
eventType: "ai_generate",
});
Copy link
Contributor

@vercel vercel bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AI rate limit check has a race condition that allows exceeding the configured limit. Between checking the current count and inserting a new record, concurrent requests can both pass the check and both insert records, resulting in counts exceeding the maximum.

View Details
📝 Patch Details
diff --git a/lib/rate-limit.ts b/lib/rate-limit.ts
index af919eb..a405e7a 100644
--- a/lib/rate-limit.ts
+++ b/lib/rate-limit.ts
@@ -66,39 +66,64 @@ export async function checkAIRateLimit(
   );
   const resetAt = new Date(Date.now() + config.windowInHours * 60 * 60 * 1000);
 
-  const [result] = await db
-    .select({ count: count(rateLimitEvents.id) })
-    .from(rateLimitEvents)
-    .where(
-      and(
-        eq(rateLimitEvents.userId, userId),
-        eq(rateLimitEvents.eventType, "ai_generate"),
-        gte(rateLimitEvents.createdAt, windowStart)
-      )
-    );
+  try {
+    const result = await db.transaction(
+      async (tx) => {
+        const [result] = await tx
+          .select({ count: count(rateLimitEvents.id) })
+          .from(rateLimitEvents)
+          .where(
+            and(
+              eq(rateLimitEvents.userId, userId),
+              eq(rateLimitEvents.eventType, "ai_generate"),
+              gte(rateLimitEvents.createdAt, windowStart)
+            )
+          );
 
-  const currentCount = result?.count ?? 0;
-  const remaining = Math.max(0, config.maxRequests - currentCount);
+        const currentCount = result?.count ?? 0;
+        const remaining = Math.max(0, config.maxRequests - currentCount);
 
-  if (currentCount >= config.maxRequests) {
-    return {
-      allowed: false,
-      remaining: 0,
-      resetAt,
-    };
-  }
+        if (currentCount >= config.maxRequests) {
+          return {
+            allowed: false,
+            remaining: 0,
+            resetAt,
+          };
+        }
 
-  // Record this request
-  await db.insert(rateLimitEvents).values({
-    userId,
-    eventType: "ai_generate",
-  });
+        // Record this request
+        await tx.insert(rateLimitEvents).values({
+          userId,
+          eventType: "ai_generate",
+        });
 
-  return {
-    allowed: true,
-    remaining: remaining - 1,
-    resetAt,
-  };
+        return {
+          allowed: true,
+          remaining: remaining - 1,
+          resetAt,
+        };
+      },
+      {
+        isolationLevel: "serializable",
+      }
+    );
+
+    return result;
+  } catch (error) {
+    // If a serialization error occurs, the transaction is aborted and we should
+    // deny the request to be safe (the rate limit was likely exceeded)
+    if (
+      error instanceof Error &&
+      error.message.includes("serialization failure")
+    ) {
+      return {
+        allowed: false,
+        remaining: 0,
+        resetAt,
+      };
+    }
+    throw error;
+  }
 }
 
 /**

Analysis

Race condition in checkAIRateLimit allows exceeding rate limit with concurrent requests

What fails: checkAIRateLimit() in lib/rate-limit.ts performs a TOCTOU (Time-Of-Check-Time-Of-Use) check: it queries the current count of rate limit events, checks if it's below the limit, then inserts a new record. Between the query and insert, concurrent requests can both pass the check and insert, causing the actual count to exceed the configured maximum of 50 requests per hour.

How to reproduce:

  1. Configure two concurrent HTTP requests to /api/ai/generate from the same user
  2. Time them so both read the event count (e.g., 49) before either inserts
  3. Both will pass the check (currentCount < maxRequests)
  4. Both will insert records
  5. Actual count becomes 51, exceeding the limit of 50

What happens: Without transaction isolation, both concurrent requests are allowed, resulting in 51 total requests instead of the enforced maximum of 50. This defeats the rate limiting protection.

Root cause: The checkAIRateLimit() function consists of separate database operations (SELECT count, then INSERT) without wrapping them in an atomic transaction. PostgreSQL defaults to READ COMMITTED isolation level, which allows phantom reads and concurrent modifications between statements.

Fix: Wrapped the count check and insert in a SERIALIZABLE transaction. This ensures that if concurrent transactions both try to insert beyond the limit, the second will fail with a serialization conflict, which is caught and results in denying the request. This prevents any request from exceeding the configured rate limit.

Comment on lines +10 to +13
// Create Redis client with KV_ prefix
const redis = new Redis({
url: process.env.KV_REST_API_URL ?? "",
token: process.env.KV_REST_API_TOKEN ?? "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Create Redis client with KV_ prefix
const redis = new Redis({
url: process.env.KV_REST_API_URL ?? "",
token: process.env.KV_REST_API_TOKEN ?? "",
// Validate required environment variables
if (!process.env.KV_REST_API_URL || !process.env.KV_REST_API_TOKEN) {
throw new Error(
"Missing required environment variables for rate limiting: KV_REST_API_URL and KV_REST_API_TOKEN. " +
"Please configure your Upstash Redis credentials in your environment."
);
}
// Create Redis client with KV_ prefix
const redis = new Redis({
url: process.env.KV_REST_API_URL,
token: process.env.KV_REST_API_TOKEN,

Missing validation for required environment variables KV_REST_API_URL and KV_REST_API_TOKEN. Using empty string defaults will cause rate limiting to fail at runtime with unhelpful error messages.

View Details

Analysis

Missing validation for required Upstash Redis environment variables

What fails: Rate limiting fails at runtime when KV_REST_API_URL or KV_REST_API_TOKEN environment variables are not set. The rate limiter module initializes successfully without these credentials (lazy-evaluated), but any attempt to check rate limits throws an unhelpful error.

How to reproduce:

  1. Set KV_REST_API_URL and KV_REST_API_TOKEN to empty/undefined in environment
  2. Start the application (it starts without error)
  3. Make any request that triggers rate limit checking (POST to /api/ai/generate)
  4. Request fails with error caught by try-catch block

Result: Error Failed to parse URL from /pipeline is thrown when aiRatelimit.limit() is called, which gets caught by the route's try-catch and returned as a generic 500 error to the client with message: "Failed to parse URL from /pipeline".

Expected: Module should fail fast at startup with clear error message: "Missing required environment variables for rate limiting: KV_REST_API_URL and KV_REST_API_TOKEN. Please configure your Upstash Redis credentials in your environment."

Why this matters: Without validation at startup, developers see a cryptic error message at runtime instead of knowing exactly which environment variables are missing. This makes debugging deployments much harder. Compare to industry standard practice where required configuration is validated on application startup.

Tested with: @upstash/redis 1.35.7, @upstash/ratelimit 2.0.7 - confirmed that empty string credentials pass initialization but fail on first API call with Invalid URL error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants