feat: implement 60s backend rate-limiting for OTP requests (#140).#141
feat: implement 60s backend rate-limiting for OTP requests (#140).#141cannonerd007 wants to merge 5 commits intoAOSSIE-Org:mainfrom
Conversation
📝 WalkthroughWalkthroughProject manifest pruned to retain only a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as OTP Handler<br/>(main.js)
participant Service as AppwriteService
participant DB as Database
participant Mailer as Email Service
Client->>Handler: POST /send-otp (email)
Handler->>Service: getUserByEmail(email)
Service->>DB: Query documents (email filter)
DB-->>Service: User document | null
Service-->>Handler: User | null
alt User exists
Handler->>Handler: Check elapsed time since last_otp_sent
alt Within 60 seconds
Handler-->>Client: 429 Too Many Requests (wait time)
else Beyond 60 seconds
Handler->>Handler: Generate OTP
Handler->>Mailer: Send OTP email
Mailer-->>Handler: Email sent
Handler->>Service: updateUserLastOtpSent(userId, timestamp)
Service->>DB: Update last_otp_sent
DB-->>Service: Update confirmed
Handler-->>Client: 200 OK (OTP sent)
end
else User not found
Handler->>Handler: Generate OTP
Handler->>Mailer: Send OTP email
Mailer-->>Handler: Email sent
Handler-->>Client: 200 OK (OTP sent)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@appwrite.json`:
- Around line 101-109: The users collection with ID 64a52f0a6c41ded09def is
missing the required attribute last_otp_sent used for OTP rate-limiting; update
the collection schema to add a new attribute named last_otp_sent (type:
datetime/timestamp or string in ISO format depending on your DB schema
constraints), ensure it is nullable by default and indexed if you query by it,
and deploy/migrate the collection so the UsersCollectionID referenced by the
environment variable is updated before merging.
In `@functions/send-otp/src/appwrite.js`:
- Around line 41-55: The try-catch in updateUserLastOtpSent is contradictory
(comment says "log but don't throw" yet it re-throws), so remove the unnecessary
try-catch and let errors propagate to the caller; locate the async method
updateUserLastOtpSent that calls this.databases.updateDocument and delete the
try-catch block around that call (or alternatively replace the catch body with a
proper log and return if you intend to swallow errors), ensuring main.js
continues to handle errors as currently designed.
🧹 Nitpick comments (1)
functions/send-otp/src/appwrite.js (1)
27-39: Fail-open design is acceptable but consider logging errors.The method silently returns
nullfor any error, which is a reasonable fail-open approach for rate-limiting (users can still authenticate if the DB is temporarily unavailable). However, consider logging the error before returningnullto aid debugging connectivity issues.♻️ Optional: Add error logging
async getUserByEmail(email) { try { const response = await this.databases.listDocuments( process.env.UserDataDatabaseID, process.env.UsersCollectionID, [Query.equal('email', email), Query.limit(1)] ); return response.documents.length > 0 ? response.documents[0] : null; } catch (e) { - // Return null if user not found or any error occurs + // Return null if user not found or any error occurs (fail-open for rate-limiting) + console.warn(`getUserByEmail failed for lookup: ${e.message}`); return null; } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
appwrite.jsonfunctions/send-otp/src/appwrite.jsfunctions/send-otp/src/main.js
🧰 Additional context used
🧬 Code graph analysis (2)
functions/send-otp/src/main.js (1)
functions/database-cleaner/src/main.js (1)
appwrite(15-15)
functions/send-otp/src/appwrite.js (1)
functions/join-room/src/main.js (1)
userId(19-19)
🔇 Additional comments (3)
functions/send-otp/src/appwrite.js (1)
1-1: LGTM - Query import added correctly.functions/send-otp/src/main.js (2)
12-13: LGTM - Environment variable validation added.
23-40: Rate-limiting implementation looks correct.The logic properly checks for existing users with a
last_otp_senttimestamp and enforces the 60-second cooldown. The 429 response with remaining seconds provides good UX feedback.Note that new users (where
userDocis null) or users without alast_otp_sentfield will bypass rate-limiting on their first OTP request, which is the expected behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
appwrite.json
Outdated
| }, | ||
| { | ||
| "key": "UserDataDatabaseID", | ||
| "value": "64a1319104a149e16f5c" | ||
| }, | ||
| { | ||
| "key": "UsersCollectionID", | ||
| "value": "64a52f0a6c41ded09def" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check if appwrite.json exists and get basic info
if [ -f appwrite.json ]; then
echo "=== File structure ==="
wc -l appwrite.json
echo ""
echo "=== Lines 95-115 to see context ==="
sed -n '95,115p' appwrite.json
else
echo "appwrite.json not found in repository root"
fiRepository: AOSSIE-Org/Resonate-Backend
Length of output: 804
🏁 Script executed:
#!/bin/bash
# Check if jq is available and examine the file structure
if command -v jq &> /dev/null; then
echo "=== Checking collections ==="
cat appwrite.json | jq '.collections[] | {id: ."$id", name: .name, attributes: [.attributes[] | .key]}' 2>/dev/null
else
echo "jq not available, trying alternative approach"
cat appwrite.json | head -200
fiRepository: AOSSIE-Org/Resonate-Backend
Length of output: 3738
🏁 Script executed:
#!/bin/bash
# Check for the specific collection ID 64a52f0a6c41ded09def and its attributes
cat appwrite.json | jq '.collections[] | select(."$id" == "64a52f0a6c41ded09def") | {id: ."$id", name: .name, attributes: [.attributes[] | {key: .key, type: .type}]}' 2>/dev/nullRepository: AOSSIE-Org/Resonate-Backend
Length of output: 776
Critical: Missing required last_otp_sent attribute in users collection.
The environment variables UserDataDatabaseID and UsersCollectionID are correctly configured. However, the users collection (64a52f0a6c41ded09def) is missing the last_otp_sent attribute required for the rate-limiting feature. Current collection attributes are: name, dob, username, profileImageUrl, email, profileImageID, ratingCount, ratingTotal, followers, friends. Add the last_otp_sent attribute to the collection schema before merging.
🤖 Prompt for AI Agents
In `@appwrite.json` around lines 101 - 109, The users collection with ID
64a52f0a6c41ded09def is missing the required attribute last_otp_sent used for
OTP rate-limiting; update the collection schema to add a new attribute named
last_otp_sent (type: datetime/timestamp or string in ISO format depending on
your DB schema constraints), ensure it is nullable by default and indexed if you
query by it, and deploy/migrate the collection so the UsersCollectionID
referenced by the environment variable is updated before merging.
|
Did you test your PR and this function locally? |
Hi @M4dhav, to be honest, I haven't performed a live local execution of the function yet as I wanted to get the logic up for review first. However, I have verified the logic through static analysis and ensured it passed the CI build checks. I’m currently setting up my local environment to perform a live test and confirm the 429 response and timestamp update behavior. I'll update you as soon as I've confirmed it on my end! |
Please note that for the future, all code should be tested locally before opening PRs |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@appwrite.json`:
- Around line 13-15: The function currently only has the "users.read" scope; add
the required database scopes so the API key can perform
listDocuments/createDocument/updateDocument. Update the "scopes" array in
appwrite.json to include the appropriate database permissions for your Appwrite
version (e.g., "databases.read" and "databases.write" or, if using the newer
rows model, "rows.read" and "rows.write") alongside "users.read" so the function
can list, create, and update documents.
In `@functions/send-otp-backup/src/appwrite.js`:
- Around line 41-55: The updateUserLastOtpSent method currently re-throws errors
despite the comment and PR intent; change the catch block to log the error
instead of throwing so DB update failures don't interrupt authentication. In the
catch, call the existing logger (or console) with a clear message including the
function name, userId and timestamp and the caught error (e.g.,
"updateUserLastOtpSent failed for userId=... timestamp=..."), then swallow the
error (do not re-throw) so the primary flow continues; keep the call to
this.databases.updateDocument and the same payload structure.
- Around line 27-39: The getUserByEmail function currently swallows all
exceptions and returns null, making network/auth/config errors indistinguishable
from "user not found"; change it to distinguish those cases by catching errors,
logging the error (include the exception and context such as email and the
DB/collection IDs), and either rethrowing the error or returning a distinct
error result so callers (e.g., OTP rate-limiter) can treat failures differently
from "no user"; update the catch around this.databases.listDocuments in
getUserByEmail to log the error and propagate it (or return a structured
failure) instead of silently returning null, while keeping the existing
successful-path behavior of returning the document when found.
In `@functions/send-otp-backup/src/main.js`:
- Around line 6-14: The environment variable list passed to throwIfMissing is
inconsistent with the main send-otp function; update the array in the
throwIfMissing call to match the main function by removing
"VERIFICATION_DATABASE_ID" and "OTP_COLLECTION_ID" and adding
"APPWRITE_FUNCTION_PROJECT_ID", while keeping "APPWRITE_API_KEY", "SENDER_MAIL",
"SENDER_PASSWORD", "UserDataDatabaseID", and "UsersCollectionID" so both
functions require the same set of env vars and are drop-in compatible.
In `@functions/send-otp/README.md`:
- Around line 1-46: Update the README to reflect the actual send-otp function:
replace the generic /ping and / endpoints with the real OTP endpoint(s) and
sample request/response payloads, document rate-limiting behavior (e.g., limits,
window, and error responses), list all required env vars used in src/main.js
(APPWRITE_API_KEY, APPWRITE_FUNCTION_PROJECT_ID, SENDER_MAIL, SENDER_PASSWORD,
UserDataDatabaseID, UsersCollectionID, and any others present), and fix the
runtime to match appwrite.json (node-16.0) or update appwrite.json to Node 18 if
intended; ensure configuration (entrypoint, build commands, timeout,
permissions) matches the project files and include minimal examples for success
and error responses.
In `@functions/send-otp/src/appwrite.js`:
- Around line 15-25: createOtpDocument is writing OTPs to
process.env.UserDataDatabaseID / process.env.UsersCollectionID while verify-otp
expects them in process.env.VERIFICATION_DATABASE_ID /
process.env.OTP_COLLECTION_ID; change the database/collection arguments in
createOtpDocument to use process.env.VERIFICATION_DATABASE_ID and
process.env.OTP_COLLECTION_ID so OTP documents are stored where verifyOtp
retrieves them, keeping the same parameters (otpId, otp, date) and call to
this.databases.createDocument in the createOtpDocument method.
♻️ Duplicate comments (4)
functions/send-otp/src/appwrite.js (1)
41-55: Inconsistent error handling: comment contradicts code.The comment on line 52 says "log but don't throw" but line 53 immediately re-throws the exception. This try-catch block is effectively a no-op.
🔧 Proposed fix: Remove unnecessary try-catch
Since the caller in
main.jsalready handles the error with its own try-catch, let the error propagate:async updateUserLastOtpSent(userId, timestamp) { - try { - await this.databases.updateDocument( - process.env.UserDataDatabaseID, - process.env.UsersCollectionID, - userId, - { - last_otp_sent: timestamp - } - ); - } catch (e) { - // If update fails, log but don't throw (non-critical) - throw e; - } + await this.databases.updateDocument( + process.env.UserDataDatabaseID, + process.env.UsersCollectionID, + userId, + { + last_otp_sent: timestamp + } + ); }appwrite.json (1)
1-25: Missinglast_otp_sentattribute in users collection.As previously noted, the rate-limiting feature requires a
last_otp_sentattribute in the users collection schema. Ensure this attribute is added to the Appwrite collection before deploying.functions/send-otp-backup/src/main.js (1)
23-40: Rate-limiting gap for registration flow remains unaddressed.The rate-limiting check only applies when
userDocexists. For registration flows where the user document hasn't been created yet,getUserByEmailreturnsnull, completely bypassing rate limiting. An attacker could spam OTP requests for non-existent accounts without restriction.Consider implementing email-based rate limiting (e.g., tracking OTP requests per email in a separate collection) that applies regardless of user existence.
functions/send-otp/src/main.js (1)
22-39: Rate-limiting gap for registration flow still present.As flagged in the previous review, the rate-limiting check only applies when
userDocexists. Registration flows (where the user doesn't exist yet) bypass rate limiting entirely, allowing attackers to spam OTP requests for non-existent email addresses.This is a known issue from the prior review that should be addressed before merging.
🧹 Nitpick comments (7)
functions/send-otp/package.json (1)
2-4: Package name inconsistent with function.The package name
"starter-template"doesn't match the actual function name"send-otp". This appears to be leftover from scaffolding.🔧 Proposed fix
{ - "name": "starter-template", + "name": "send-otp", "version": "1.0.0", - "description": "", + "description": "OTP sending function with 60s rate limiting",appwrite.json (1)
7-7: Runtimenode-16.0is end-of-life.Node.js 16 reached end-of-life in September 2023. Consider upgrading to a supported LTS version like
node-18.0ornode-20.0for security patches and continued support.🔧 Proposed fix
- "runtime": "node-16.0", + "runtime": "node-18.0",functions/send-otp-backup/package.json (1)
1-17: Package name should reflect the backup function identity.The package name is
"send-otp"but this is in thesend-otp-backupdirectory. This could cause confusion or conflicts. Consider renaming to"send-otp-backup"for clarity.Additionally, there's a significant version mismatch with the main
send-otpfunction:
- Backup:
node-appwrite@^17.0.0,nodemailer@^6.9.5- Main:
node-appwrite@20.3.0,nodemailer@7.0.12This inconsistency could lead to different behavior between the two implementations.
Proposed fix
{ - "name": "send-otp", + "name": "send-otp-backup", "version": "1.0.0", "description": "", "main": "src/main.js", "type": "module", "scripts": { "start": "node src/main.js", "format": "prettier --write ." }, "dependencies": { - "node-appwrite": "^17.0.0", - "nodemailer": "^6.9.5" + "node-appwrite": "^20.3.0", + "nodemailer": "^7.0.12" }, "devDependencies": { "prettier": "^3.0.0" } }functions/send-otp-backup/README.md (1)
36-86: Documentation is incomplete for the new rate-limiting feature.The README is missing several important details:
Missing environment variables: The code requires
UserDataDatabaseID,UsersCollectionID, and (per the main function)APPWRITE_FUNCTION_PROJECT_ID, but these are not documented.Missing 429 response documentation: The rate-limiting feature returns HTTP 429 when requests are made within 60 seconds, but only the 200 response is documented.
Consider updating the documentation to reflect the complete configuration and response behaviors.
functions/send-otp-backup/src/main.js (1)
42-42: Consider using cryptographically secure random number generation.
Math.random()is not cryptographically secure. While a 6-digit OTP has limited entropy regardless, usingcrypto.randomInt()would be a more secure approach for security-sensitive operations.Suggested improvement
+import { randomInt } from 'crypto'; + - const otp = String(Math.floor(100000 + Math.random() * 900000)); + const otp = String(randomInt(100000, 1000000));functions/send-otp-backup/src/appwrite.js (2)
4-13: Constructor looks good, but missing env vars could cause silent issues.If
APPWRITE_FUNCTION_PROJECT_IDorAPPWRITE_API_KEYare undefined, the client will be configured withundefinedvalues, potentially causing cryptic errors later. Consider validating these required env vars or using thethrowIfMissingutility (available inutils.jsper the PR summary) at initialization.💡 Suggested improvement
constructor() { + const projectId = process.env.APPWRITE_FUNCTION_PROJECT_ID; + const apiKey = process.env.APPWRITE_API_KEY; + if (!projectId || !apiKey) { + throw new Error('Missing required Appwrite environment variables'); + } const client = new Client() .setEndpoint( process.env.APPWRITE_ENDPOINT ?? 'https://cloud.appwrite.io/v1' ) - .setProject(process.env.APPWRITE_FUNCTION_PROJECT_ID) - .setKey(process.env.APPWRITE_API_KEY); + .setProject(projectId) + .setKey(apiKey); this.databases = new Databases(client); }
15-25: Inconsistent environment variable naming convention.This method uses
VERIFICATION_DATABASE_IDandOTP_COLLECTION_ID(SCREAMING_SNAKE_CASE), whilegetUserByEmailandupdateUserLastOtpSentuseUserDataDatabaseIDandUsersCollectionID(PascalCase). Consider standardizing the naming convention across the codebase for maintainability.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
functions/send-otp-backup/package-lock.jsonis excluded by!**/package-lock.jsonfunctions/send-otp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
appwrite.jsonfunctions/send-otp-backup/README.mdfunctions/send-otp-backup/package.jsonfunctions/send-otp-backup/src/appwrite.jsfunctions/send-otp-backup/src/mail.jsfunctions/send-otp-backup/src/main.jsfunctions/send-otp-backup/src/utils.jsfunctions/send-otp/.gitignorefunctions/send-otp/.prettierrc.jsonfunctions/send-otp/README.mdfunctions/send-otp/package.jsonfunctions/send-otp/src/appwrite.jsfunctions/send-otp/src/main.js
✅ Files skipped from review due to trivial changes (2)
- functions/send-otp/.gitignore
- functions/send-otp/.prettierrc.json
🧰 Additional context used
🧬 Code graph analysis (5)
functions/send-otp-backup/src/mail.js (2)
functions/send-otp-backup/src/main.js (2)
recipientEmail(21-21)otp(42-42)functions/send-otp/src/main.js (2)
recipientEmail(20-20)otp(41-41)
functions/send-otp-backup/src/appwrite.js (3)
functions/send-otp-backup/src/main.js (1)
otp(42-42)functions/send-otp/src/main.js (1)
otp(41-41)functions/join-room/src/main.js (1)
userId(19-19)
functions/send-otp-backup/src/main.js (2)
functions/send-otp-backup/src/utils.js (2)
throwIfMissing(1-11)throwIfMissing(1-11)functions/send-otp/src/main.js (11)
throwIfMissing(5-66)appwrite(15-15)mailService(16-16)recipientEmail(20-20)userDoc(23-23)lastOtpSentTime(26-26)currentTime(27-27)timeDifference(28-28)remainingSeconds(31-31)otp(41-41)currentDate(46-46)
functions/send-otp-backup/src/utils.js (2)
functions/send-otp-backup/src/main.js (1)
throwIfMissing(5-67)functions/send-otp/src/main.js (1)
throwIfMissing(5-66)
functions/send-otp/src/appwrite.js (1)
functions/join-room/src/main.js (1)
userId(19-19)
🪛 markdownlint-cli2 (0.18.1)
functions/send-otp/README.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
21-21: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
functions/send-otp-backup/README.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
16-16: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (9)
functions/send-otp-backup/src/utils.js (1)
1-11: LGTM!The utility function is clean and serves its purpose for environment variable validation. Note that the condition
!obj[key]will treat empty strings,0, andfalseas missing, which is appropriate for the required API keys and database IDs being validated inmain.js.functions/send-otp/src/appwrite.js (1)
27-39: LGTM!The
getUserByEmailmethod correctly usesQuery.equalandQuery.limit(1)to fetch user documents. Returningnullon errors is appropriate since the caller inmain.jshandles this case gracefully by skipping the rate limit check for non-existent users.functions/send-otp/package.json (1)
11-12: No action needed. MailService uses the Gmail SMTP transport (service: 'gmail'), which has no breaking changes between nodemailer v6 and v7. The breaking change in v7 only affects the SES transport (switched to AWS SDK v3), which is not used here.Likely an incorrect or invalid review comment.
functions/send-otp-backup/README.md (1)
30-32: Build command appears incorrect for serverless function.The build command
npm install && npm run startwould attempt to execute the function during build. For Appwrite functions, typically onlynpm installis needed as the build command.functions/send-otp-backup/src/main.js (1)
52-59: Graceful error handling for timestamp update is appropriate.The try-catch wrapper ensures that failures in updating the
last_otp_senttimestamp don't break the primary OTP flow. This is the correct approach for non-critical metadata updates.functions/send-otp-backup/src/mail.js (1)
3-23: Implementation looks correct.The MailService correctly initializes a nodemailer transport with Gmail and provides an async
sendMailmethod. Errors will propagate to the caller which handles them appropriately in main.js.functions/send-otp/src/main.js (2)
6-13: Environment variable changes look appropriate.The updated required environment variables (
APPWRITE_FUNCTION_PROJECT_ID,UserDataDatabaseID,UsersCollectionID) align with the new AppwriteService methods for user lookup and timestamp updates.
51-59: Timestamp update with graceful error handling is well implemented.The try-catch wrapper correctly ensures that timestamp update failures don't interrupt the primary OTP flow, with appropriate warning logging.
functions/send-otp-backup/src/appwrite.js (1)
57-58: LGTM!Default export is appropriate for this service class.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async getUserByEmail(email) { | ||
| try { | ||
| const response = await this.databases.listDocuments( | ||
| process.env.UserDataDatabaseID, | ||
| process.env.UsersCollectionID, | ||
| [Query.equal('email', email), Query.limit(1)] | ||
| ); | ||
| return response.documents.length > 0 ? response.documents[0] : null; | ||
| } catch (e) { | ||
| // Return null if user not found or any error occurs | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Swallowing all errors may mask real issues and bypass rate limiting.
Returning null for any error means network failures, authentication issues, or misconfiguration are indistinguishable from "user not found." For the rate-limiting feature, if this query fails, the calling code may allow an OTP to be sent (bypassing the rate limit).
Consider logging the error for observability, or distinguishing between "not found" and actual failures.
🔧 Suggested improvement
async getUserByEmail(email) {
try {
const response = await this.databases.listDocuments(
process.env.UserDataDatabaseID,
process.env.UsersCollectionID,
[Query.equal('email', email), Query.limit(1)]
);
return response.documents.length > 0 ? response.documents[0] : null;
} catch (e) {
- // Return null if user not found or any error occurs
+ // Log error for debugging but return null to allow flow to continue
+ console.error('getUserByEmail failed:', e.message);
return null;
}
}📝 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.
| async getUserByEmail(email) { | |
| try { | |
| const response = await this.databases.listDocuments( | |
| process.env.UserDataDatabaseID, | |
| process.env.UsersCollectionID, | |
| [Query.equal('email', email), Query.limit(1)] | |
| ); | |
| return response.documents.length > 0 ? response.documents[0] : null; | |
| } catch (e) { | |
| // Return null if user not found or any error occurs | |
| return null; | |
| } | |
| } | |
| async getUserByEmail(email) { | |
| try { | |
| const response = await this.databases.listDocuments( | |
| process.env.UserDataDatabaseID, | |
| process.env.UsersCollectionID, | |
| [Query.equal('email', email), Query.limit(1)] | |
| ); | |
| return response.documents.length > 0 ? response.documents[0] : null; | |
| } catch (e) { | |
| // Log error for debugging but return null to allow flow to continue | |
| console.error('getUserByEmail failed:', e.message); | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@functions/send-otp-backup/src/appwrite.js` around lines 27 - 39, The
getUserByEmail function currently swallows all exceptions and returns null,
making network/auth/config errors indistinguishable from "user not found";
change it to distinguish those cases by catching errors, logging the error
(include the exception and context such as email and the DB/collection IDs), and
either rethrowing the error or returning a distinct error result so callers
(e.g., OTP rate-limiter) can treat failures differently from "no user"; update
the catch around this.databases.listDocuments in getUserByEmail to log the error
and propagate it (or return a structured failure) instead of silently returning
null, while keeping the existing successful-path behavior of returning the
document when found.
| async updateUserLastOtpSent(userId, timestamp) { | ||
| try { | ||
| await this.databases.updateDocument( | ||
| process.env.UserDataDatabaseID, | ||
| process.env.UsersCollectionID, | ||
| userId, | ||
| { | ||
| last_otp_sent: timestamp | ||
| } | ||
| ); | ||
| } catch (e) { | ||
| // If update fails, log but don't throw (non-critical) | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
Comment contradicts the actual code behavior.
The comment states "If update fails, log but don't throw (non-critical)" but the code does the opposite: it throws the error and performs no logging. The PR description states that "database errors during the update do not interrupt the primary authentication flow," which aligns with the comment, not the code.
🐛 Fix to align code with intended behavior per PR description
async updateUserLastOtpSent(userId, timestamp) {
try {
await this.databases.updateDocument(
process.env.UserDataDatabaseID,
process.env.UsersCollectionID,
userId,
{
last_otp_sent: timestamp
}
);
} catch (e) {
- // If update fails, log but don't throw (non-critical)
- throw e;
+ // If update fails, log but don't throw (non-critical for auth flow)
+ console.error('Failed to update last_otp_sent:', e.message);
}
}📝 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.
| async updateUserLastOtpSent(userId, timestamp) { | |
| try { | |
| await this.databases.updateDocument( | |
| process.env.UserDataDatabaseID, | |
| process.env.UsersCollectionID, | |
| userId, | |
| { | |
| last_otp_sent: timestamp | |
| } | |
| ); | |
| } catch (e) { | |
| // If update fails, log but don't throw (non-critical) | |
| throw e; | |
| } | |
| } | |
| async updateUserLastOtpSent(userId, timestamp) { | |
| try { | |
| await this.databases.updateDocument( | |
| process.env.UserDataDatabaseID, | |
| process.env.UsersCollectionID, | |
| userId, | |
| { | |
| last_otp_sent: timestamp | |
| } | |
| ); | |
| } catch (e) { | |
| // If update fails, log but don't throw (non-critical for auth flow) | |
| console.error('Failed to update last_otp_sent:', e.message); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@functions/send-otp-backup/src/appwrite.js` around lines 41 - 55, The
updateUserLastOtpSent method currently re-throws errors despite the comment and
PR intent; change the catch block to log the error instead of throwing so DB
update failures don't interrupt authentication. In the catch, call the existing
logger (or console) with a clear message including the function name, userId and
timestamp and the caught error (e.g., "updateUserLastOtpSent failed for
userId=... timestamp=..."), then swallow the error (do not re-throw) so the
primary flow continues; keep the call to this.databases.updateDocument and the
same payload structure.
| throwIfMissing(process.env, [ | ||
| "APPWRITE_API_KEY", | ||
| "VERIFICATION_DATABASE_ID", | ||
| "OTP_COLLECTION_ID", | ||
| "SENDER_MAIL", | ||
| "SENDER_PASSWORD", | ||
| "UserDataDatabaseID", | ||
| "UsersCollectionID", | ||
| ]); |
There was a problem hiding this comment.
Environment variable requirements differ from main send-otp function.
The backup function requires VERIFICATION_DATABASE_ID and OTP_COLLECTION_ID, but the main send-otp function (per the AI summary) removed these in favor of only UserDataDatabaseID and UsersCollectionID. Also, the main function requires APPWRITE_FUNCTION_PROJECT_ID which is missing here.
This inconsistency means the two functions aren't drop-in replacements for each other.
🤖 Prompt for AI Agents
In `@functions/send-otp-backup/src/main.js` around lines 6 - 14, The environment
variable list passed to throwIfMissing is inconsistent with the main send-otp
function; update the array in the throwIfMissing call to match the main function
by removing "VERIFICATION_DATABASE_ID" and "OTP_COLLECTION_ID" and adding
"APPWRITE_FUNCTION_PROJECT_ID", while keeping "APPWRITE_API_KEY", "SENDER_MAIL",
"SENDER_PASSWORD", "UserDataDatabaseID", and "UsersCollectionID" so both
functions require the same set of env vars and are drop-in compatible.
Hi @M4dhav, I apologize for not providing the local test results earlier. I have now successfully set up the local environment and verified the rate-limiting logic. As shown in the attached execution logs, the function now correctly returns a 429 Too Many Requests status code when a user attempts to request a second OTP within the 60-second window. I have also verified that the database correctly creates and updates the last_otp_sent timestamp to manage this behavior. Thank you for the guidance, and I will ensure all future PRs are tested locally before submission. Ready for your review! 
|
M4dhav
left a comment
M4dhav
left a comment
There was a problem hiding this comment.
What is the send_otp_backup funtion?
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@appwrite.json`:
- Line 7: Update the Node runtime declared in appwrite.json by replacing the
outdated "node-16.0" value with a supported LTS version string such as "node-20"
or "node-22" (e.g., change the "runtime" property from "node-16.0" to "node-22"
or "node-20"); ensure any deployment/CI configs and Dockerfiles that rely on the
runtime are checked for compatibility and updated consistently after changing
the "runtime" value.
In `@functions/send-otp/src/appwrite.js`:
- Around line 27-39: getUserByEmail is querying
VERIFICATION_DATABASE_ID/OTP_COLLECTION_ID which currently mixes ephemeral OTP
records with persistent user rate-limit metadata; separate concerns by creating
a dedicated users collection (e.g., USERS_COLLECTION_ID) to store per-email
state like last_otp_sent and change getUserByEmail to query that users
collection for the email (or, if you must keep a single collection, change
getUserByEmail to return the most-recent OTP document by email sorted by
timestamp); also fix updateUserLastOtpSent so it does not re-throw exceptions
(remove the re-throw and handle/log the error or return a failure indicator) to
match the "don't throw" comment.
♻️ Duplicate comments (2)
functions/send-otp/src/appwrite.js (1)
41-55: Inconsistent error handling remains unaddressed.The comment on line 52 says "log but don't throw" but line 53 immediately re-throws. This was flagged in a previous review and still needs to be fixed.
functions/send-otp/src/main.js (1)
22-39: Rate limiting bypassed for unregistered emails.When
userDocisnull(no existing document found), the rate-limiting check is entirely skipped. This was flagged in a previous review—an attacker can spam OTP requests for any email address that doesn't have an existing record.Consider tracking rate limits per email in a dedicated collection, independent of user document existence.
appwrite.json
Outdated
| "name": "Upcoming Rooms Time Checker", | ||
| "$id": "6968b4f000222d5d8d93", | ||
| "name": "send-otp", | ||
| "runtime": "node-16.0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
**Consider upgrading Node.js runtime.**Node.js 16 reached end-of-life on September 11, 2023, and "there have not been any bug fixes or security updates to these versions." Appwrite now supports Node.js 22 runtime (22.18).
Upgrade to a supported LTS version (Node.js 20 or 22) for security patches and continued support.
🔧 Suggested fix
- "runtime": "node-16.0",
+ "runtime": "node-22.0",📝 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.
| "runtime": "node-16.0", | |
| "runtime": "node-22.0", |
🤖 Prompt for AI Agents
In `@appwrite.json` at line 7, Update the Node runtime declared in appwrite.json
by replacing the outdated "node-16.0" value with a supported LTS version string
such as "node-20" or "node-22" (e.g., change the "runtime" property from
"node-16.0" to "node-22" or "node-20"); ensure any deployment/CI configs and
Dockerfiles that rely on the runtime are checked for compatibility and updated
consistently after changing the "runtime" value.
| async getUserByEmail(email) { | ||
| try { | ||
| const response = await this.databases.listDocuments( | ||
| process.env.VERIFICATION_DATABASE_ID, | ||
| process.env.OTP_COLLECTION_ID, | ||
| [Query.equal('email', email), Query.limit(1)] | ||
| ); | ||
| return response.documents.length > 0 ? response.documents[0] : null; | ||
| } catch (e) { | ||
| // Return null if user not found or any error occurs | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Locating appwrite.json ==="
find . -name "appwrite.json" -o -name "*appwrite*" -type f | head -20
echo ""
echo "=== Checking for database/collection configuration files ==="
find . -name "*.json" | grep -E "(config|db|collection|schema)" | head -20Repository: AOSSIE-Org/Resonate-Backend
Length of output: 345
🏁 Script executed:
#!/bin/bash
echo "=== Searching for collection ID references in JS files ==="
rg "COLLECTION_ID|DATABASE_ID" functions/send-otp/src/ -A 2 -B 2
echo ""
echo "=== Searching for users collection or user data references ==="
rg -i "(users|user_data|profile).*collection" functions/send-otp/src/ -B 2 -A 2Repository: AOSSIE-Org/Resonate-Backend
Length of output: 1825
🏁 Script executed:
#!/bin/bash
echo "=== Examining the full appwrite.js file to see all collection usage ==="
cat -n functions/send-otp/src/appwrite.jsRepository: AOSSIE-Org/Resonate-Backend
Length of output: 2272
🏁 Script executed:
#!/bin/bash
echo "=== Searching for last_otp_sent field usage across codebase ==="
rg "last_otp_sent" . -n
echo ""
echo "=== Searching for updateUserLastOtpSent implementation ==="
rg "updateUserLastOtpSent" . -B 5 -A 10Repository: AOSSIE-Org/Resonate-Backend
Length of output: 2881
🏁 Script executed:
#!/bin/bash
echo "=== Examining appwrite.json for collection schemas ==="
cat appwrite.json | head -200Repository: AOSSIE-Org/Resonate-Backend
Length of output: 902
🏁 Script executed:
#!/bin/bash
echo "=== Searching for main.js to understand the flow ==="
cat -n functions/send-otp/src/main.js | head -80Repository: AOSSIE-Org/Resonate-Backend
Length of output: 2942
Data model design issue: OTP_COLLECTION_ID dual-purposes for both OTP records and user rate-limit metadata.
getUserByEmail queries VERIFICATION_DATABASE_ID/OTP_COLLECTION_ID, which stores both OTP data (created per-request) and user metadata (last_otp_sent). This conflates concerns:
- Unclear semantics: Documents are created by unique
otpIDbut queried/updated by email, conflating per-request OTP records with user state. - Rate limiting fails on first request: New users have no existing document, so rate limiting is bypassed for their first OTP request (only works from second request onward).
- Fragile design: Storing user rate-limit metadata in a collection designed for ephemeral OTP records couples unrelated concerns.
Separate user state (email, last_otp_sent) into a dedicated users collection, or redesign the rate-limiting approach to store state alongside each OTP record (and check the most recent OTP's timestamp instead of querying by email).
Also fix error handling in updateUserLastOtpSent (lines 51–55): comment says "don't throw" but code re-throws the exception.
🤖 Prompt for AI Agents
In `@functions/send-otp/src/appwrite.js` around lines 27 - 39, getUserByEmail is
querying VERIFICATION_DATABASE_ID/OTP_COLLECTION_ID which currently mixes
ephemeral OTP records with persistent user rate-limit metadata; separate
concerns by creating a dedicated users collection (e.g., USERS_COLLECTION_ID) to
store per-email state like last_otp_sent and change getUserByEmail to query that
users collection for the email (or, if you must keep a single collection, change
getUserByEmail to return the most-recent OTP document by email sorted by
timestamp); also fix updateUserLastOtpSent so it does not re-throw exceptions
(remove the re-throw and handle/log the error or return a failure indicator) to
match the "don't throw" comment.
|
Hey @M4dhav, sorry for the delay! To answer your question—that send-otp-backup folder was just a local backup I made while testing that I accidentally included in the commit. I've gone ahead and deleted it now. I also took a look at the automated feedback and finished polishing the rest of the PR. I restored the standard variable names like VERIFICATION_DATABASE_ID, updated the runtime to Node 18, and fixed the database scopes in appwrite.json. I also did a full write-up for the README so the environment variables and rate-limiting are properly documented. Everything is passing the checks now and should be ready for review! |
3 participants
I have implemented a 60-second server-side rate limit for the send-otp function to address the security concerns raised in #140. Following the maintainer's feedback, I integrated an "early exit" strategy at the start of the function that checks the user's last_otp_sent timestamp in the database before any mail is generated or sent. If a user attempts to request an OTP too quickly, the system now returns a 429 Too Many Requests error, specifically informing them of the remaining cooldown time. To support this, I updated the AppwriteService with methods to fetch and update user metadata and ensured that the timestamp update is handled gracefully so it doesn't interrupt the core authentication flow if a database error occurs.
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.