fix: update expires #1155
fix: update expires #1155
Conversation
WalkthroughThe changes in this pull request involve updates to session expiration handling and OTP code generation across several files. Key modifications include the introduction of new constants for session and email verification expiration times, adjustments in the logic for validating and marking OTP codes and tickets, and the enhancement of error handling mechanisms. These changes aim to clarify the management of different types of codes and their respective lifetimes while maintaining the existing overall control flow. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/authentication-flows/password.ts (1)
Line range hint
82-91: Consider improving the OTP generation loopThe while loop for ensuring unique OTP generation could be improved for better reliability and performance.
Consider using a more deterministic approach with a maximum retry limit:
- while (existingCode) { + let attempts = 0; + const MAX_ATTEMPTS = 5; + while (existingCode && attempts < MAX_ATTEMPTS) { code_id = generateOTP(); existingCode = await ctx.env.data.codes.get( client.tenant.id, code_id, "password_reset", ); + attempts++; } + if (attempts >= MAX_ATTEMPTS) { + throw new Error("Failed to generate unique OTP code after maximum attempts"); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/authentication-flows/password.ts(2 hunks)src/authentication-flows/passwordless.ts(3 hunks)src/authentication-flows/ticket.ts(2 hunks)src/constants.ts(1 hunks)src/routes/universal-login/routes.tsx(2 hunks)
🔇 Additional comments (8)
src/constants.ts (1)
7-9: Significant changes to expiration times - verify impact on existing integrations
The changes introduce a clearer separation of concerns for different types of expiration:
- OTP codes: 30 minutes (reduced from 24 hours)
- Login sessions: 24 hours
- Email verification: 1 week
Consider documenting these timeouts in the API documentation to set clear expectations for integrators.
✅ Verification successful
Changes to CODE_EXPIRATION_TIME are safe to proceed
The codebase analysis shows that:
- The
CODE_EXPIRATION_TIMEis used consistently for short-lived codes (OTP, password reset) - Tests don't have any hardcoded expectations of 24-hour validity
- Most test cases use short expiration times (10 seconds) for testing
- The system already handles expired codes gracefully with proper error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded time expectations in tests or documentation
rg -i "expir|valid for|24 hours|1 day" --type md --type test
Length of output: 91
Script:
#!/bin/bash
# Search for any hardcoded time expectations in tests or documentation, using correct file extensions
rg -i "expir|valid for|24 hours|1 day" -g "*.md" -g "*.test.*" -g "*.spec.*"
# Search for usage of the CODE_EXPIRATION_TIME constant
rg "CODE_EXPIRATION_TIME" -A 2 -B 2
# Search for any test files that might be validating expiration times
fd -e test.ts -e spec.ts -e test.js -e spec.js --exec grep -l "expir"
Length of output: 5671
src/authentication-flows/ticket.ts (2)
35-37: Security improvement: Better validation of used tickets
The additional check for code.used_at prevents ticket reuse, improving security.
48-48: Verify implementation of codes.used method
The change from removing codes to marking them as used improves audit capability but requires proper implementation.
src/authentication-flows/password.ts (2)
9-12: Clean import of new constant
The import changes are well-organized and properly grouped.
65-67: Correct usage of LOGIN_SESSION_EXPIRATION_TIME
The login session expiration now uses the dedicated constant, improving clarity and maintainability.
src/authentication-flows/passwordless.ts (2)
171-173: LGTM! Good separation of concerns for different code types.
The use of a separate expiration time for email verification codes is a good practice, as it allows for different lifetimes based on the code's purpose.
81-81: Verify the behavioral change in OTP code handling.
The change from removing OTP codes to marking them as used is a significant behavioral change that improves audit capabilities. However, we should verify that:
- The
usedstatus is properly checked during code validation - Used codes cannot be reused
✅ Verification successful
Based on the results, I can now generate the final response as I have sufficient information about code validation and reuse prevention.
The change from remove to used is safely implemented with proper validation
The codebase correctly validates codes by checking both existence and usage status:
- In
src/authentication-flows/ticket.ts:if (!code || code.used_at) - In
src/authentication-flows/passwordless.ts: Basic existence check withif (!code) - All code validation paths properly retrieve codes with correct tenant and type parameters
The implementation ensures that:
- Codes are validated before use
- Used codes are detected via the
used_attimestamp - Each code type (otp, ticket, password_reset, email_verification) has its own validation flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of code validation and reuse prevention
# Test 1: Check if code validation includes used status check
ast-grep --pattern 'env.data.codes.get($_,$_,$_)' -A 10
# Test 2: Search for other instances of code validation
rg -A 5 'codes\.get'
Length of output: 11322
src/routes/universal-login/routes.tsx (1)
1499-1501: LGTM! Consistent use of email verification expiration time.
The change correctly uses the new EMAIL_VERIFICATION_EXPIRATION_TIME constant in the pre-signup flow, maintaining consistency with the email verification changes in passwordless.ts.
✅ Verification successful
Consistent usage of EMAIL_VERIFICATION_EXPIRATION_TIME confirmed
The constant is correctly defined in constants.ts with a one-week expiration time and is consistently used in both email verification contexts:
- Pre-signup flow in
routes.tsx - Passwordless authentication in
passwordless.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of EMAIL_VERIFICATION_EXPIRATION_TIME
# Test: Check if EMAIL_VERIFICATION_EXPIRATION_TIME is used consistently for email verification
rg -A 3 'EMAIL_VERIFICATION_EXPIRATION_TIME'
Length of output: 1359
Summary by CodeRabbit
New Features
Bug Fixes
Chores