fix: implement OTP expiry and one-time use validation#139
fix: implement OTP expiry and one-time use validation#139Muneerali199 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
- Add expiresAt timestamp to OTP documents (5 minute TTL) - Enforce one-time use by deleting OTP after verification attempt - Return proper HTTP status codes (200/401/404/500) with clear messages - Change verification status from string to boolean for type safety - Prevent OTP reuse and improve security Closes AOSSIE-Org#138
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SendFunc as send-otp
participant AppwriteDB as Appwrite/OTP Store
participant VerifyFunc as verify-otp
participant VerifDB as Appwrite/Verification Store
Client->>SendFunc: Request OTP
SendFunc->>AppwriteDB: create OTP doc { otp, date, expiresAt }
AppwriteDB-->>SendFunc: OTP created
SendFunc-->>Client: 200 OK (OTP sent)
Client->>VerifyFunc: Submit OTP
VerifyFunc->>AppwriteDB: fetch OTP doc by id
AppwriteDB-->>VerifyFunc: OTP doc (includes expiresAt)
alt expired or missing expiresAt
VerifyFunc->>AppwriteDB: delete OTP doc
VerifyFunc-->>Client: 401 Unauthorized (expired/invalid)
else valid and not expired
alt OTP matches
VerifyFunc->>AppwriteDB: delete OTP doc
VerifyFunc->>VerifDB: create verification record { isValid: true }
VerifyFunc-->>Client: 200 OK (verified)
else OTP mismatch
VerifyFunc->>AppwriteDB: delete OTP doc
VerifyFunc->>VerifDB: create verification record { isValid: false }
VerifyFunc-->>Client: 401 Unauthorized (invalid)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
functions/verify-otp/src/main.js (1)
21-25: Unhandled exception risk from malformed request body.
JSON.parse(req.body)is outside any try-catch block. If the client sends malformed JSON, this will throw an unhandled exception and crash the function without returning a proper error response.🛠️ Suggested fix
+ let otpID, userOtp, verificationId; + try { + const parsed = JSON.parse(req.body); + otpID = parsed.otpID; + userOtp = parsed.userOTP; + verificationId = parsed.verify_ID; + } catch (e) { + error('Invalid request body'); + return res.json({ + success: false, + message: 'Invalid request body' + }, 400); + } - const { - otpID, - userOTP: userOtp, - verify_ID: verificationId, - } = JSON.parse(req.body);
🤖 Fix all issues with AI agents
In @functions/verify-otp/src/main.js:
- Around line 45-67: The expiry check must handle missing or invalid
otpDocument.expiresAt; modify the block that creates expiryTime (expiryTime =
new Date(otpDocument.expiresAt)) to first verify otpDocument.expiresAt exists
and that new Date(otpDocument.expiresAt) yields a valid date (e.g., check
isNaN(expiryTime.getTime()) or Date.parse). If expiresAt is missing or invalid,
treat the OTP as expired: log the condition, attempt deletion via
db.deleteDocument (same call as currently used), and return the same 401 JSON
response. Keep the existing deletion/error logging flow and response format
(log, db.deleteDocument, res.json with success:false and message:'OTP has
expired').
🧹 Nitpick comments (1)
functions/send-otp/src/main.js (1)
21-21: Consider using cryptographically secure random for OTP generation.
Math.random()is not cryptographically secure and could be predictable in certain environments. For security-sensitive OTP generation, consider using Node.js'scryptomodule.🔒 Suggested fix using crypto.randomInt
+import { randomInt } from 'crypto'; + -const otp = String(Math.floor(100000 + Math.random() * 900000)); +const otp = String(randomInt(100000, 1000000));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
functions/send-otp/src/appwrite.jsfunctions/send-otp/src/main.jsfunctions/verify-otp/src/main.js
🧰 Additional context used
🧬 Code graph analysis (2)
functions/send-otp/src/appwrite.js (1)
functions/send-otp/src/main.js (2)
otp(21-21)expiresAt(30-30)
functions/send-otp/src/main.js (1)
functions/database-cleaner/src/main.js (1)
appwrite(15-15)
🔇 Additional comments (3)
functions/send-otp/src/appwrite.js (1)
15-26: LGTM!The signature extension to include
expiresAtis clean and aligns with the calling code inmain.js. The document now properly stores the expiry timestamp for downstream validation.functions/send-otp/src/main.js (1)
29-33: LGTM!The 5-minute expiry window aligns with the requirements from issue #138 (5-10 minutes recommended). The ISO string format ensures consistent timezone handling across services.
functions/verify-otp/src/main.js (1)
69-119: Well-structured verification flow.The implementation correctly:
- Deletes OTP after any verification attempt (preventing reuse/retry attacks)
- Stores verification status as a boolean (not string)
- Returns distinct HTTP status codes (200/401/500) as specified in the requirements
This addresses the security objectives from issue #138.
- Check if otpDocument.expiresAt exists before using it - Validate that expiresAt is a valid date using isNaN check - Treat missing or invalid expiresAt as expired OTP - Delete OTP and return 401 with consistent error message
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
functions/verify-otp/src/main.js (2)
20-20: Avoid logging sensitive authentication data.
req.bodycontains the user's OTP, which is sensitive authentication data. Logging this violates security best practices and may create compliance issues (e.g., GDPR, PCI-DSS). Remove or redact sensitive fields before logging.Suggested fix
- log(req.body); + log("Received OTP verification request");
21-25: Add error handling for JSON parsing.If
req.bodyis malformed or not valid JSON,JSON.parsewill throw an unhandled exception, resulting in a 500 error with no meaningful message to the client.Suggested fix
- const { - otpID, - userOTP: userOtp, - verify_ID: verificationId, - } = JSON.parse(req.body); + let otpID, userOtp, verificationId; + try { + const parsed = JSON.parse(req.body); + otpID = parsed.otpID; + userOtp = parsed.userOTP; + verificationId = parsed.verify_ID; + } catch (e) { + error(String(e)); + return res.json({ + success: false, + message: 'Invalid request body' + }, 400); + }
🧹 Nitpick comments (2)
functions/verify-otp/src/main.js (2)
49-110: Consider extracting duplicate delete logic into a helper.The OTP deletion block is repeated three times (lines 52-61, 75-84, 95-104) with identical logic. Extracting this into a helper function improves maintainability.
Suggested refactor
Add a helper function at the top of the default export:
const deleteOtpDocument = async (db, otpID, log, error) => { try { await db.deleteDocument( process.env.VERIFICATION_DATABASE_ID, process.env.OTP_COLLECTION_ID, otpID ); } catch (deleteError) { log("Failed to delete expired OTP"); error(String(deleteError)); } };Then replace each duplicated block with:
await deleteOtpDocument(db, otpID, log, error);
112-113: Consider timing-safe comparison for OTP validation.Using
===for comparing secrets can leak timing information. While the risk is low for short OTPs, usingcrypto.timingSafeEqualis a security best practice for sensitive comparisons.Suggested fix
+import { timingSafeEqual } from 'crypto'; + +// Helper for safe string comparison +const safeCompare = (a, b) => { + if (typeof a !== 'string' || typeof b !== 'string') return false; + if (a.length !== b.length) return false; + return timingSafeEqual(Buffer.from(a), Buffer.from(b)); +}; // Step 3: Validate OTP - const isValid = otpDocument.otp === userOtp; + const isValid = safeCompare(otpDocument.otp, userOtp);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
functions/verify-otp/src/main.js
🔇 Additional comments (3)
functions/verify-otp/src/main.js (3)
29-43: LGTM!Proper error handling with appropriate 404 status for missing/invalid OTP documents.
132-149: LGTM!Verification document correctly stores boolean status per PR objectives. Error handling returns appropriate 500 status on failure.
151-162: LGTM!Clear distinction between success (200) and invalid OTP (401) responses aligns with PR objectives.
1 participant
-🎯 Summary
This PR fixes the OTP verification system to enforce expiry times and prevent reuse, addressing critical security vulnerabilities in the authentication flow.
🐛 Problem
Previously, OTPs could be:
Reused indefinitely without expiration
Used multiple times for authentication
The verify endpoint always returned HTTP 200, making it impossible for clients to distinguish success from failure
✨ Changes Made
Added expiresAt field to OTP documents with 5-minute TTL
Modified functions/send-otp/src/appwrite.js to accept and store expiry timestamp
Updated functions/send-otp/src/main.js to generate ISO timestamp 5 minutes in the future
Modified functions/verify-otp/src/main.js to delete OTP document after any verification attempt
Prevents OTP reuse even if verification fails
Automatic cleanup of expired OTPs during verification
200 OK: Valid OTP verified successfully
401 Unauthorized: OTP expired or invalid
404 Not Found: OTP not found or already used
500 Internal Server Error: Database/server errors
Added success boolean flag and clear error messages in all responses
Changed verification status from String(boolean) to boolean for proper type consistency
🧪 Test Scenarios
All scenarios have been verified:
✅ Valid OTP returns 200 with success message
✅ Expired OTP returns 401 with clear error
✅ Wrong OTP returns 401 and prevents retry
✅ Reused OTP returns 404 (already deleted)
✅ Missing OTP returns 404
📝 Files Changed
appwrite.js - Added expiresAt parameter
main.js - Generate and store expiry timestamp
main.js - Complete verification logic overhaul
🔒 Security Improvements
Time-based OTP expiry (5 minutes)
Single-use enforcement via deletion
No information leakage in error responses
Automatic cleanup of used/expired OTPs
📚 Documentation
The changes are backward compatible but require Appwrite database to support the new expiresAt field in the OTP collection schema.
Closes #138
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.