Add rate limiting to Cloud Functions#166
Open
Hell1213 wants to merge 1 commit into
Open
Conversation
- Implemented Firestore-based rate limiting for setRole function - Added automatic cleanup to prevent data bloat - Applied 10 requests/minute limit for admin operations - Graceful error handling with retry-after messaging Fixes roshankumar0036singh#155
Contributor
📝 WalkthroughWalkthroughThis PR implements Firestore-backed rate limiting for Cloud Functions to prevent abuse and DoS attacks. It adds a configurable rate limiter middleware with rolling-window request tracking per user per function, integrates it into the setRole function, includes a scheduled cleanup job for stale records, and updates the build configuration to exclude tests. ChangesRate Limiting Feature
Sequence DiagramsequenceDiagram
participant SetRole
participant CheckRateLimit
participant Firestore
participant ErrorHandler
SetRole->>CheckRateLimit: checkRateLimit(userId, "setRole", ADMIN_WRITE)
CheckRateLimit->>Firestore: query rateLimits document
alt Document exists
Firestore-->>CheckRateLimit: existing requests array
CheckRateLimit->>CheckRateLimit: filter in-window timestamps
alt Limit exceeded
CheckRateLimit->>ErrorHandler: throw HttpsError("resource-exhausted")
ErrorHandler-->>SetRole: HttpsError with retryAfter, limit, window
else Limit not exceeded
CheckRateLimit->>Firestore: append timestamp, update lastUpdated
Firestore-->>CheckRateLimit: write complete
CheckRateLimit-->>SetRole: void (continue)
end
else Document missing
CheckRateLimit->>Firestore: initialize new document with timestamp
Firestore-->>CheckRateLimit: write complete
CheckRateLimit-->>SetRole: void (continue)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloud-functions/src/setRole.ts (1)
3-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure rate limiting is applied beyond
setRole(issue#155scope)Only
cloud-functions/src/setRole.tscallscheckRateLimit(...); the other callable functions (calculateReputation,getTopContributors,sendDailyDigest) don’t appear to be rate-limited. Also, I can’t findcreateEvent/updateAttendance/sendNotificationin this repo by name—please verify the intended issue#155endpoints are covered (or explicitly document this as a phased rollout).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/setRole.ts` around lines 3 - 21, The PR only applies rate limiting in setRole via checkRateLimit(context.auth.uid, "setRole", RATE_LIMITS.ADMIN_WRITE); ensure the other callable functions referenced in issue `#155`—namely calculateReputation, getTopContributors, sendDailyDigest (and if present: createEvent, updateAttendance, sendNotification)—also call checkRateLimit with an appropriate key and RATE_LIMITS constant; locate each function (e.g., calculateReputation, getTopContributors, sendDailyDigest) and add the same auth check plus await checkRateLimit(context.auth.uid, "<functionName>", <appropriate RATE_LIMITS.*>) at the top, or if those endpoints are intentionally excluded, add a short README note documenting this as a phased rollout and listing which functions remain unrate-limited.
🧹 Nitpick comments (2)
cloud-functions/src/middleware/rateLimiter.ts (2)
42-42: ⚡ Quick winPrefer optional chaining for conciseness.
As per the SonarCloud hint, use optional chaining to simplify the null/undefined check.
♻️ Proposed refactor
- if (data && data.requests) { + if (data?.requests) { const recentRequests = (data.requests as number[])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/middleware/rateLimiter.ts` at line 42, The conditional in rateLimiter.ts uses a manual null/undefined check ("if (data && data.requests) {"); replace it with optional chaining to simplify and clarify the intent by changing the condition to check data?.requests instead, updating the conditional in the function/block that references the variable "data" (the rate limiter request-count check) so it uses optional chaining.
24-85: ⚡ Quick winConsider logging when rate limits are exceeded.
The PR objectives mention "log violations for monitoring," but the current implementation only logs errors (line 83) and cleanup counts. Adding a log statement when the rate limit is actually hit (lines 51-59) would help detect abuse patterns and monitor the effectiveness of rate limiting.
📊 Proposed addition
if (recentRequests.length >= config.maxRequests) { const oldestRequest = recentRequests[0]; const retryAfterMs = oldestRequest + config.windowMs - now; const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); + console.warn(`Rate limit exceeded for user ${userId} on function ${functionName}. ` + + `Limit: ${config.maxRequests} requests per ${config.windowMs / 1000}s`); + throw new functions.https.HttpsError(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-functions/src/middleware/rateLimiter.ts` around lines 24 - 85, The rate-limit branch that throws the functions.https.HttpsError in checkRateLimit currently doesn't emit a monitoring log; add a structured log just before throwing (using the existing logging approach, e.g., console.warn or the app's logger) that records userId, functionName, retryAfterSeconds, config.maxRequests and config.windowMs (or seconds) and any identifying doc id (rateLimitRef.id) to surface violations for monitoring and analytics; ensure the log is emitted only in the exhausted-rate branch (the block that computes retryAfterSeconds and throws) so callers still receive the same HttpsError.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud-functions/src/index.ts`:
- Around line 16-21: The schedule string passed to functions.pubsub.schedule in
the cleanupRateLimits export is grammatically incorrect ("every 1 hours");
update the schedule argument to a correct form such as "every 1 hour" or simply
"every hour" in the cleanupRateLimits definition so the call to .schedule(...)
reads correctly while leaving the async onRun handler and cleanupOldRateLimits()
call unchanged.
In `@cloud-functions/src/middleware/rateLimiter.ts`:
- Around line 47-49: The retry-after calculation using oldestRequest,
retryAfterMs and retryAfterSeconds can yield zero or negative values; change the
computation so retryAfterSeconds = Math.max(1, Math.ceil((oldestRequest +
config.windowMs - now) / 1000)) to ensure the header is always at least 1
second; update the place where retryAfterSeconds is used to rely on this bounded
value.
- Around line 79-84: The catch block in the rate limiter swallows non-HttpsError
exceptions (current code checks functions.https.HttpsError then logs and
returns), which allows requests to bypass limits; change it to fail-closed by
rethrowing or converting every error into an HttpsError: in the catch in
rateLimiter middleware, after logging rethrow the original error or throw a new
functions.https.HttpsError('internal', 'Rate limit check failed', { message:
String(error) }) so any Firestore/network/permission failure aborts the request
instead of silently permitting it.
- Around line 88-108: The current cleanupOldRateLimits function only deletes up
to 500 documents because of the .limit(500) query; change it to repeatedly query
and delete in batches until no stale docs remain. In cleanupOldRateLimits, wrap
the query+batch-delete logic (the
admin.firestore().collection("rateLimits").where("lastUpdated", "<",
cutoffTime).limit(500).get() call, snapshot handling, and batch.commit()) in a
loop (e.g., while/for) that exits when snapshot.empty, so you run successive
queries and commits until all matching documents are removed; keep using
batch.delete per doc and batch.commit for each batch. Ensure you await each
batch.commit before re-querying to avoid race conditions.
- Around line 37-78: The current read-modify-write on rateLimitRef can race—wrap
the logic inside a Firestore transaction (firestore.runTransaction) and perform
transaction.get(rateLimitRef), compute recentRequests = (data.requests as
number[]).filter(...) within the transaction, check recentRequests.length
against config.maxRequests and if exceeded throw the same
functions.https.HttpsError, otherwise push now and use
transaction.set(rateLimitRef, { requests: recentRequests, lastUpdated: now })
(or create the doc if missing) so the get+check+write is atomic; keep using
config.maxRequests and config.windowMs and preserve the retryAfter calculation
and metadata fields.
---
Outside diff comments:
In `@cloud-functions/src/setRole.ts`:
- Around line 3-21: The PR only applies rate limiting in setRole via
checkRateLimit(context.auth.uid, "setRole", RATE_LIMITS.ADMIN_WRITE); ensure the
other callable functions referenced in issue `#155`—namely calculateReputation,
getTopContributors, sendDailyDigest (and if present: createEvent,
updateAttendance, sendNotification)—also call checkRateLimit with an appropriate
key and RATE_LIMITS constant; locate each function (e.g., calculateReputation,
getTopContributors, sendDailyDigest) and add the same auth check plus await
checkRateLimit(context.auth.uid, "<functionName>", <appropriate RATE_LIMITS.*>)
at the top, or if those endpoints are intentionally excluded, add a short README
note documenting this as a phased rollout and listing which functions remain
unrate-limited.
---
Nitpick comments:
In `@cloud-functions/src/middleware/rateLimiter.ts`:
- Line 42: The conditional in rateLimiter.ts uses a manual null/undefined check
("if (data && data.requests) {"); replace it with optional chaining to simplify
and clarify the intent by changing the condition to check data?.requests
instead, updating the conditional in the function/block that references the
variable "data" (the rate limiter request-count check) so it uses optional
chaining.
- Around line 24-85: The rate-limit branch that throws the
functions.https.HttpsError in checkRateLimit currently doesn't emit a monitoring
log; add a structured log just before throwing (using the existing logging
approach, e.g., console.warn or the app's logger) that records userId,
functionName, retryAfterSeconds, config.maxRequests and config.windowMs (or
seconds) and any identifying doc id (rateLimitRef.id) to surface violations for
monitoring and analytics; ensure the log is emitted only in the exhausted-rate
branch (the block that computes retryAfterSeconds and throws) so callers still
receive the same HttpsError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e82758f-2272-4299-8b7a-7f890fd2bd63
📒 Files selected for processing (4)
cloud-functions/src/index.tscloud-functions/src/middleware/rateLimiter.tscloud-functions/src/setRole.tscloud-functions/tsconfig.json
Comment on lines
+16
to
+21
| export const cleanupRateLimits = functions.pubsub | ||
| .schedule("every 1 hours") | ||
| .onRun(async () => { | ||
| await cleanupOldRateLimits(); | ||
| return null; | ||
| }); |
Contributor
There was a problem hiding this comment.
Fix grammatical error in schedule string.
Line 17 uses "every 1 hours" which is grammatically incorrect.
📝 Proposed fix
export const cleanupRateLimits = functions.pubsub
- .schedule("every 1 hours")
+ .schedule("every 1 hour")
.onRun(async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export const cleanupRateLimits = functions.pubsub | |
| .schedule("every 1 hours") | |
| .onRun(async () => { | |
| await cleanupOldRateLimits(); | |
| return null; | |
| }); | |
| export const cleanupRateLimits = functions.pubsub | |
| .schedule("every 1 hour") | |
| .onRun(async () => { | |
| await cleanupOldRateLimits(); | |
| return null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/index.ts` around lines 16 - 21, The schedule string
passed to functions.pubsub.schedule in the cleanupRateLimits export is
grammatically incorrect ("every 1 hours"); update the schedule argument to a
correct form such as "every 1 hour" or simply "every hour" in the
cleanupRateLimits definition so the call to .schedule(...) reads correctly while
leaving the async onRun handler and cleanupOldRateLimits() call unchanged.
Comment on lines
+37
to
+78
| const doc = await rateLimitRef.get(); | ||
|
|
||
| if (doc.exists) { | ||
| const data = doc.data(); | ||
|
|
||
| if (data && data.requests) { | ||
| const recentRequests = (data.requests as number[]) | ||
| .filter((timestamp: number) => timestamp > windowStart); | ||
|
|
||
| if (recentRequests.length >= config.maxRequests) { | ||
| const oldestRequest = recentRequests[0]; | ||
| const retryAfterMs = oldestRequest + config.windowMs - now; | ||
| const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); | ||
|
|
||
| throw new functions.https.HttpsError( | ||
| "resource-exhausted", | ||
| `Rate limit exceeded. You can try again in ${retryAfterSeconds} seconds.`, | ||
| { | ||
| retryAfter: retryAfterSeconds, | ||
| limit: config.maxRequests, | ||
| window: config.windowMs / 1000 | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| recentRequests.push(now); | ||
| await rateLimitRef.set({ | ||
| requests: recentRequests, | ||
| lastUpdated: now | ||
| }); | ||
| } else { | ||
| await rateLimitRef.set({ | ||
| requests: [now], | ||
| lastUpdated: now | ||
| }); | ||
| } | ||
| } else { | ||
| await rateLimitRef.set({ | ||
| requests: [now], | ||
| lastUpdated: now | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical race condition allows rate limit bypass.
The read-modify-write pattern (get document → filter/update array → set document) is not atomic. When multiple requests arrive concurrently for the same user+function, they can all read the same state, each determine they're under the limit, and all write back successfully—bypassing the rate limit entirely.
Use a Firestore transaction to ensure atomicity.
🔒 Proposed fix using a transaction
- try {
- const doc = await rateLimitRef.get();
-
- if (doc.exists) {
- const data = doc.data();
-
- if (data && data.requests) {
- const recentRequests = (data.requests as number[])
- .filter((timestamp: number) => timestamp > windowStart);
-
- if (recentRequests.length >= config.maxRequests) {
- const oldestRequest = recentRequests[0];
- const retryAfterMs = oldestRequest + config.windowMs - now;
- const retryAfterSeconds = Math.ceil(retryAfterMs / 1000);
-
- throw new functions.https.HttpsError(
- "resource-exhausted",
- `Rate limit exceeded. You can try again in ${retryAfterSeconds} seconds.`,
- {
- retryAfter: retryAfterSeconds,
- limit: config.maxRequests,
- window: config.windowMs / 1000
- }
- );
- }
-
- recentRequests.push(now);
- await rateLimitRef.set({
- requests: recentRequests,
- lastUpdated: now
- });
- } else {
- await rateLimitRef.set({
- requests: [now],
- lastUpdated: now
- });
- }
- } else {
- await rateLimitRef.set({
- requests: [now],
- lastUpdated: now
- });
- }
+ try {
+ await admin.firestore().runTransaction(async (transaction) => {
+ const doc = await transaction.get(rateLimitRef);
+
+ let recentRequests: number[] = [];
+
+ if (doc.exists) {
+ const data = doc.data();
+ if (data?.requests) {
+ recentRequests = (data.requests as number[])
+ .filter((timestamp: number) => timestamp > windowStart);
+ }
+ }
+
+ if (recentRequests.length >= config.maxRequests) {
+ const oldestRequest = recentRequests[0];
+ const retryAfterMs = oldestRequest + config.windowMs - now;
+ const retryAfterSeconds = Math.max(1, Math.ceil(retryAfterMs / 1000));
+
+ throw new functions.https.HttpsError(
+ "resource-exhausted",
+ `Rate limit exceeded. You can try again in ${retryAfterSeconds} seconds.`,
+ {
+ retryAfter: retryAfterSeconds,
+ limit: config.maxRequests,
+ window: config.windowMs / 1000
+ }
+ );
+ }
+
+ recentRequests.push(now);
+ transaction.set(rateLimitRef, {
+ requests: recentRequests,
+ lastUpdated: now
+ });
+ });
} catch (error) {🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 42-42: Prefer using an optional chain expression instead, as it's more concise and easier to read.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/middleware/rateLimiter.ts` around lines 37 - 78, The
current read-modify-write on rateLimitRef can race—wrap the logic inside a
Firestore transaction (firestore.runTransaction) and perform
transaction.get(rateLimitRef), compute recentRequests = (data.requests as
number[]).filter(...) within the transaction, check recentRequests.length
against config.maxRequests and if exceeded throw the same
functions.https.HttpsError, otherwise push now and use
transaction.set(rateLimitRef, { requests: recentRequests, lastUpdated: now })
(or create the doc if missing) so the get+check+write is atomic; keep using
config.maxRequests and config.windowMs and preserve the retryAfter calculation
and metadata fields.
Comment on lines
+47
to
+49
| const oldestRequest = recentRequests[0]; | ||
| const retryAfterMs = oldestRequest + config.windowMs - now; | ||
| const retryAfterSeconds = Math.ceil(retryAfterMs / 1000); |
Contributor
There was a problem hiding this comment.
Retry-after calculation can produce negative or zero values.
If the oldest request is about to expire, retryAfterMs can be negative or very small. Math.ceil of a negative number remains negative, which is invalid for a retry-after header.
Use Math.max(1, ...) to ensure at least 1 second.
⏱️ Proposed fix
const oldestRequest = recentRequests[0];
const retryAfterMs = oldestRequest + config.windowMs - now;
- const retryAfterSeconds = Math.ceil(retryAfterMs / 1000);
+ const retryAfterSeconds = Math.max(1, Math.ceil(retryAfterMs / 1000));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/middleware/rateLimiter.ts` around lines 47 - 49, The
retry-after calculation using oldestRequest, retryAfterMs and retryAfterSeconds
can yield zero or negative values; change the computation so retryAfterSeconds =
Math.max(1, Math.ceil((oldestRequest + config.windowMs - now) / 1000)) to ensure
the header is always at least 1 second; update the place where retryAfterSeconds
is used to rely on this bounded value.
Comment on lines
+79
to
+84
| } catch (error) { | ||
| if (error instanceof functions.https.HttpsError) { | ||
| throw error; | ||
| } | ||
| console.error("Rate limit check failed:", error); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent failure defeats rate limiting security.
Non-HttpsError exceptions (Firestore permissions, network failures, etc.) are logged but suppressed, allowing the request to proceed without rate limiting. This defeats the security purpose—if Firestore is unavailable or misconfigured, all rate limits are bypassed.
Either rethrow all errors to fail-closed, or use an in-memory fallback.
🛡️ Proposed fix: fail-closed by rethrowing all errors
} catch (error) {
if (error instanceof functions.https.HttpsError) {
throw error;
}
console.error("Rate limit check failed:", error);
+ throw new functions.https.HttpsError(
+ "internal",
+ "Rate limit check failed. Please try again.",
+ );
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/middleware/rateLimiter.ts` around lines 79 - 84, The
catch block in the rate limiter swallows non-HttpsError exceptions (current code
checks functions.https.HttpsError then logs and returns), which allows requests
to bypass limits; change it to fail-closed by rethrowing or converting every
error into an HttpsError: in the catch in rateLimiter middleware, after logging
rethrow the original error or throw a new functions.https.HttpsError('internal',
'Rate limit check failed', { message: String(error) }) so any
Firestore/network/permission failure aborts the request instead of silently
permitting it.
Comment on lines
+88
to
+108
| export async function cleanupOldRateLimits(olderThanMs: number = 60 * 60 * 1000): Promise<void> { | ||
| const cutoffTime = Date.now() - olderThanMs; | ||
|
|
||
| const snapshot = await admin.firestore() | ||
| .collection("rateLimits") | ||
| .where("lastUpdated", "<", cutoffTime) | ||
| .limit(500) | ||
| .get(); | ||
|
|
||
| if (snapshot.empty) { | ||
| return; | ||
| } | ||
|
|
||
| const batch = admin.firestore().batch(); | ||
| snapshot.docs.forEach(doc => { | ||
| batch.delete(doc.ref); | ||
| }); | ||
|
|
||
| await batch.commit(); | ||
| console.log(`Cleaned up ${snapshot.size} old rate limit records`); | ||
| } |
Contributor
There was a problem hiding this comment.
Incomplete cleanup when more than 500 stale records exist.
The function queries with .limit(500) and processes only that batch. If there are more than 500 stale records, they won't be cleaned up in this run. Given the hourly schedule, if stale records accumulate faster than 500/hour, they'll build up indefinitely, leading to storage bloat and degraded query performance.
Either run in a loop until all stale records are cleaned, or document this limitation and monitor for accumulation.
🔄 Proposed fix: loop until all stale records are cleaned
export async function cleanupOldRateLimits(olderThanMs: number = 60 * 60 * 1000): Promise<void> {
const cutoffTime = Date.now() - olderThanMs;
+ let totalCleaned = 0;
+ let hasMore = true;
- const snapshot = await admin.firestore()
- .collection("rateLimits")
- .where("lastUpdated", "<", cutoffTime)
- .limit(500)
- .get();
-
- if (snapshot.empty) {
- return;
- }
-
- const batch = admin.firestore().batch();
- snapshot.docs.forEach(doc => {
- batch.delete(doc.ref);
- });
-
- await batch.commit();
- console.log(`Cleaned up ${snapshot.size} old rate limit records`);
+ while (hasMore) {
+ const snapshot = await admin.firestore()
+ .collection("rateLimits")
+ .where("lastUpdated", "<", cutoffTime)
+ .limit(500)
+ .get();
+
+ if (snapshot.empty) {
+ hasMore = false;
+ break;
+ }
+
+ const batch = admin.firestore().batch();
+ snapshot.docs.forEach(doc => {
+ batch.delete(doc.ref);
+ });
+
+ await batch.commit();
+ totalCleaned += snapshot.size;
+
+ // If we got fewer than 500, we're done
+ if (snapshot.size < 500) {
+ hasMore = false;
+ }
+ }
+
+ if (totalCleaned > 0) {
+ console.log(`Cleaned up ${totalCleaned} old rate limit records`);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function cleanupOldRateLimits(olderThanMs: number = 60 * 60 * 1000): Promise<void> { | |
| const cutoffTime = Date.now() - olderThanMs; | |
| const snapshot = await admin.firestore() | |
| .collection("rateLimits") | |
| .where("lastUpdated", "<", cutoffTime) | |
| .limit(500) | |
| .get(); | |
| if (snapshot.empty) { | |
| return; | |
| } | |
| const batch = admin.firestore().batch(); | |
| snapshot.docs.forEach(doc => { | |
| batch.delete(doc.ref); | |
| }); | |
| await batch.commit(); | |
| console.log(`Cleaned up ${snapshot.size} old rate limit records`); | |
| } | |
| export async function cleanupOldRateLimits(olderThanMs: number = 60 * 60 * 1000): Promise<void> { | |
| const cutoffTime = Date.now() - olderThanMs; | |
| let totalCleaned = 0; | |
| let hasMore = true; | |
| while (hasMore) { | |
| const snapshot = await admin.firestore() | |
| .collection("rateLimits") | |
| .where("lastUpdated", "<", cutoffTime) | |
| .limit(500) | |
| .get(); | |
| if (snapshot.empty) { | |
| hasMore = false; | |
| break; | |
| } | |
| const batch = admin.firestore().batch(); | |
| snapshot.docs.forEach(doc => { | |
| batch.delete(doc.ref); | |
| }); | |
| await batch.commit(); | |
| totalCleaned += snapshot.size; | |
| // If we got fewer than 500, we're done | |
| if (snapshot.size < 500) { | |
| hasMore = false; | |
| } | |
| } | |
| if (totalCleaned > 0) { | |
| console.log(`Cleaned up ${totalCleaned} old rate limit records`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloud-functions/src/middleware/rateLimiter.ts` around lines 88 - 108, The
current cleanupOldRateLimits function only deletes up to 500 documents because
of the .limit(500) query; change it to repeatedly query and delete in batches
until no stale docs remain. In cleanupOldRateLimits, wrap the query+batch-delete
logic (the admin.firestore().collection("rateLimits").where("lastUpdated", "<",
cutoffTime).limit(500).get() call, snapshot handling, and batch.commit()) in a
loop (e.g., while/for) that exits when snapshot.empty, so you run successive
queries and commits until all matching documents are removed; keep using
batch.delete per doc and batch.commit for each batch. Ensure you await each
batch.commit before re-querying to avoid race conditions.
Owner
|
@Hell1213 handle the coderabbit suggestion wherever necessary |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Added rate limiting to Cloud Functions to prevent DoS attacks and abuse. Implemented Firestore-based tracking that works with Firebase's
https.onCallfunctions.Fixes #155
Type of change
How Has This Been Tested?
npm run buildManual Testing Plan:
setRole10 times rapidlyChecklist:
Summary by CodeRabbit
Release Notes
New Features
Chores