feat(auth): implement rate limiting for OTP verification & lint/format errors #174
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
WalkthroughThis pull request introduces several technical changes across the project. New environment variables for Upstash Redis are added to the sample configuration file. The OTP confirmation route is enhanced with improved error handling and rate limiting via a new Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AuthRoute as Auth Route
participant RateLimiter as RateLimiter
participant Redis as Redis
participant AuthErrorHandler as AuthErrorHandler
User->>AuthRoute: GET request (token, type)
AuthRoute->>AuthRoute: Validate parameters
AuthRoute->>RateLimiter: Check rate limit (increment)
RateLimiter->>Redis: Update/check rate limit counter
Redis-->>RateLimiter: Return status (allowed/blocked)
alt Allowed
AuthRoute->>AuthRoute: Attempt OTP verification
alt Verification Successful
AuthRoute->>RateLimiter: Reset rate limit
AuthRoute->>User: Redirect to success URL
else Verification Fails
AuthRoute->>AuthErrorHandler: Handle OTP error
AuthRoute->>User: Redirect to error page
end
else Blocked
AuthRoute->>User: Redirect indicating rate limit exceeded
end
Possibly related PRs
Suggested reviewers
Poem
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 (
|
evgongora
changed the title
coderabbitai
bot
added
documentation
![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
🧹 Nitpick comments (9)
apps/web/app/auth/confirm/route.ts (5)
26-33: Consider returning a more descriptive error code or path.
We could enhance the end-user experience by including a helpful message in the error page or by redirecting to a dedicated help page. This approach can reduce confusion and guide users if critical parameters are missing.
39-54: Refactor the rate limit redirection logic for clarity.
The current rate limiting flow is understandable. We can further simplify it by moving the building of theerrorUrlinto a small helper function. This would make the flow more readable and align with the single-responsibility principle.
56-62: Provide specific handling for Supabase connection errors.
Consider capturing potential connection or response failures from Supabase. This would help in proactively logging or retrying if the service becomes temporarily unavailable, improving overall robustness.
63-79: Maintain consistent error responses in logs and redirects.
We could enhance this by standardizing the message or reason for failures. For instance, logging the same code in both the console log and the redirect URL ensures better traceability across logs and user-facing errors.
90-98: Unify unexpected error handling.
We could enhance this by integrating an overarching error handling system that standardizes the logging, messaging, and redirection for unforeseen exceptions. This would reduce duplication, improve clarity, and make troubleshooting easier.apps/web/test/auth-rate-limit.test.mjs (1)
13-37: Extract repeated request logic into a helper utility.
We could enhance maintainability by moving themakeRequestfunction to a shared utility or a reusable helper within tests. This way, multiple test suites can leverage the same function when carrying out rate limit checks or OTP verifications.apps/web/test/check-redis.mjs (2)
1-9: Validate environment variable existence.
We could enhance stability by confirming thatUPSTASH_REDIS_REST_URLandUPSTASH_REDIS_REST_TOKENare defined before instantiating Redis. This prevents runtime exceptions and aids troubleshooting.
27-40: Add more context when displaying block status.
We could include additional information, such as how long an IP has been blocked, or enforce a system to measure partial windows. This can help in diagnosing scenarios where repeated blocks occur.apps/web/components/icons/illustrations.tsx (1)
9-28: Consider standardizing className handling across components.The
WaveIllustrationandExploreProjectsIconcomponents handle className differently:
WaveIllustrationuses template literals with a default empty stringExploreProjectsIconpasses className directlyLet's standardize this across all components for better consistency.
Apply this pattern to all icon components:
-className={`w-48 h-48 ${className || ''}`} +className={className}And update the component usage to include default classes where needed:
<ExploreProjectsIcon className="w-48 h-48" />Also applies to: 538-561
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/contract/contracts/nft/test_snapshots/test/test_admin_access_control.1.json(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_basic_flow.1.json(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_error_handling.1.json(1 hunks)apps/contract/contracts/nft/test_snapshots/test/test_transfer_ownership.1.json(1 hunks)apps/web/.env.sample(1 hunks)apps/web/app/auth/confirm/route.ts(2 hunks)apps/web/components/icons/illustrations.tsx(9 hunks)apps/web/package.json(2 hunks)apps/web/test/auth-rate-limit.test.mjs(1 hunks)apps/web/test/check-redis.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- apps/contract/contracts/nft/test_snapshots/test/test_error_handling.1.json
- apps/contract/contracts/nft/test_snapshots/test/test_basic_flow.1.json
- apps/contract/contracts/nft/test_snapshots/test/test_transfer_ownership.1.json
- apps/contract/contracts/nft/test_snapshots/test/test_admin_access_control.1.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: Review TypeScript files ensuring type safety with...
**/*.ts: Review TypeScript files ensuring type safety with no 'any' usage unless justified. Verify named exports, proper error handling with typed errors, and pure functions. Check module encapsulation and consistent naming using camelCase for functions and PascalCase for types. Validate unit test coverage for utilities if required.
apps/web/app/auth/confirm/route.ts
🔇 Additional comments (11)
apps/web/app/auth/confirm/route.ts (2)
35-37: Verify handling of unknown IP addresses.
We could enhance this by assigning a canonical fallback IP (e.g.,0.0.0.0) or storing logs more explicitly when IP detection fails. This helps avoid potential collisions or ambiguity in rate-limiting logic for multiple users flagged as "unknown."
81-83: Reset logic looks good.
The explicit reset of rate limits upon successful verification helps maintain proper states and provides a good user experience for valid attempts. Great job!apps/web/test/auth-rate-limit.test.mjs (3)
4-9: Good use of descriptive parameters.
Providing a realistic token hash and type helps verify real-world edge cases. This approach ensures that the test logic aligns more closely with production scenarios.
39-45: Verify timed delays are sufficient to simulate real conditions.
A one-second delay is a great start. We could fine-tune these delays or introduce configurable pause durations to ensure the test suite covers a broader spectrum of real-world request intervals.
47-48: Clean and direct error handling.
Catching and logging errors at this top level prevents silent failures and helps maintain clarity when test scripts misbehave. Nicely done!apps/web/test/check-redis.mjs (2)
16-23: Segmenting keys by type is clear.
Filtering and categorizing the keys into block, attempt, and internal sets is a logical approach that simplifies debugging. The code is easy to follow and well-structured. Good job!
72-74: Great user feedback for empty sets.
Letting the user know "No rate limit keys found" provides quick clarity during diagnostics. This is a nice, user-friendly touch.apps/web/components/icons/illustrations.tsx (2)
5-7: LGTM! Well-structured interface definition.The
IllustrationPropsinterface extendsSVGProps<SVGSVGElement>and adds an optional className prop, providing good type safety and flexibility for SVG components.
538-561:✅ Verification successful
Verify the simplified ExploreProjectsIcon implementation.
The new
ExploreProjectsIconappears significantly simpler compared to other icon components. Please confirm if this is intentional or if some visual elements are missing.
🏁 Script executed:
#!/bin/bash # Search for ExploreProjectsIcon usage to verify if the simplified version meets all use cases rg -A 5 "ExploreProjectsIcon"Length of output: 448
Simplified ExploreProjectsIcon Implementation Looks Intentional
After verifying the usage context, it appears that the current, minimal implementation in
apps/web/components/icons/illustrations.tsxis indeed intentional. The search output confirms that no additional visual elements have been omitted, aligning with a deliberate design choice to keep this icon simple.
- The component's usage across the codebase doesn’t indicate any missing functionality.
- Consider reviewing your overall icon design guidelines to ensure that the simplicity of this icon is consistent with your visual language.
apps/web/.env.sample (1)
10-12: LGTM! Well-documented Redis configuration.The Redis configuration section is clearly commented and includes the necessary variables for Upstash Redis integration, aligning with the rate limiting requirements.
apps/web/package.json (1)
36-36: LGTM! Appropriate Redis dependency.The addition of
@upstash/redisaligns well with the rate limiting requirements mentioned in the PR objectives.
There was a problem hiding this comment.
Very good implementation! Good measure of request rate limits, this fit good for server attacks on account recovery requests. I only found a few observations and left the @coderabbitai observations to take.
After those, it will be ready for a merge!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/web/lib/auth/rate-limiter.ts (1)
10-23: Well-structured class with clear key-generation methods!We could enhance robustness by handling potential Redis connection or command errors (e.g., surrounding
redis.existswith a try-catch) to ensure failures are handled gracefully.apps/web/test/auth-rate-limit.test.mjs (3)
1-31: Comprehensive setup and request helper!We could enhance these tests by verifying behavior across repeated attempts after the block duration ends, ensuring the rate limit properly resets.
56-71: Excellent handling of block scenarios.Consider adding an assertion that checks the response status code, ensuring we catch any unexpected response format changes.
73-96: Neat check for independent rate limits between clients.We could enhance this further by verifying that once client A is blocked, unblocking it (via a reset or waiting for TTL) indeed allows subsequent requests.
apps/web/test/check-redis.test.mjs (2)
55-72: Thorough check for rate limit blocks.We could enhance this by confirming that a block key cannot be overridden prematurely, adding confidence to your blocking strategy.
92-116: Separate client counters test is well structured.It might be helpful to confirm that clients also remain isolated when they happen to share partial IP segments, to rule out any unexpected collisions in the naming scheme.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/lib/auth/rate-limiter.ts(1 hunks)apps/web/package.json(3 hunks)apps/web/test/auth-rate-limit.test.mjs(1 hunks)apps/web/test/check-redis.test.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: Review TypeScript files ensuring type safety with...
**/*.ts: Review TypeScript files ensuring type safety with no 'any' usage unless justified. Verify named exports, proper error handling with typed errors, and pure functions. Check module encapsulation and consistent naming using camelCase for functions and PascalCase for types. Validate unit test coverage for utilities if required.
apps/web/lib/auth/rate-limiter.ts
🔇 Additional comments (5)
apps/web/lib/auth/rate-limiter.ts (2)
6-8: Consider aligning the rate limit window with your PR objectives.Your PR description mentions a 5-minute window, yet this code sets the window to 15 minutes. Let’s verify if this discrepancy is intentional or if it needs an update.
25-70: Nicely implemented incremental rate-limiting logic.We might expand test coverage to confirm that blocking and unblocking flow works as intended and that error states are correctly handled in edge cases (e.g., Redis being temporarily unavailable).
apps/web/test/auth-rate-limit.test.mjs (1)
41-55: Great scenario testing under the threshold.Everything appears solid here. The loop approach is straightforward and helps confirm multiple attempts. This is a nice demonstration of your rate-limiting boundary.
apps/web/test/check-redis.test.mjs (2)
15-34: Straightforward Redis connection test confirms environment setup.This is a clear way to ensure that the credentials are correct and the environment variables are properly loaded. Nicely done!
35-53: Good practice verifying counters and TTL together.We can reinforce resilience by testing how the counter behaves when Redis is at capacity or experiences network hiccups, ensuring the code recovers or retries gracefully.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/package.json (2)
5-12: New Scripts for Testing and PostinstallThe new
"test": "bun test/**.test.{ts,js,mjs}"and"postinstall": "bun test"scripts are a nice addition to streamline testing with Bun. Consider verifying that automatically running tests on postinstall is intended behavior, as this may introduce delays or unexpected failures in certain deployment environments.
66-77: DevDependency Updates and Type Definitions ReviewIncluding
"@types/node-fetch": "^2.6.12"is a solid enhancement for type safety. However, the AI summary mentioned updating"@types/react"and"@types/react-dom"to more specific versions (^19.0.10and^19.0.4respectively), while the file still uses"^19". Please confirm if this is intentional or if you’d like to update these for tighter version control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/package.json(1 hunks)
🔇 Additional comments (2)
apps/web/package.json (2)
2-4: Project Metadata is Clean and ClearThe basic properties ("name", "version", and "private") are straightforward and well-defined.
63-64: New Dependencies for Rate Limiting and FetchingThe addition of
"@upstash/redis": "^1.34.4"appropriately supports Redis rate limiting for OTP verification, and using"node-fetch": "^2.7.0"aligns with the need to work with type definitions from"@types/node-fetch": "^2.6.12". Please ensure that the selected version of node-fetch remains compatible with the dev dependency.
|
@AndlerRL PR is ready, I made a few changes to enhance the tests and changed the instructions for testing. You can now do bun test and will run correctly. 
|
AndlerRL
dismissed stale reviews from coderabbitai[bot] and themself
via
0c5e640
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.husky/pre-commit (1)
4-5: Great addition of test automation! Consider enhancing error handling.The addition of automated testing in the pre-commit hook is excellent, and the echo message provides good user guidance. We could enhance this by adding error handling and environment validation.
Consider this improved version:
-echo "Running tests for the web application. Ensure to have no ENV var missing. Check test errors if any." -cd apps/web && bun test +echo "🧪 Running tests for the web application..." +if [ ! -d "apps/web" ]; then + echo "❌ Error: apps/web directory not found" + exit 1 +fi +cd apps/web +if [ ! -f ".env" ]; then + echo "⚠️ Warning: .env file not found in apps/web" + echo "Required environment variables for tests:" + echo "- UPSTASH_REDIS_REST_URL" + echo "- UPSTASH_REDIS_REST_TOKEN" +fi +if ! bun test; then + echo "❌ Tests failed. Please fix the errors before committing." + exit 1 +fi +echo "✅ Tests passed successfully!"This enhancement:
- Validates the directory exists before cd
- Checks for .env file and lists required variables
- Provides clearer error messages with emojis
- Ensures the commit fails if tests fail
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.husky/pre-commit(1 hunks)apps/web/package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/package.json
|
@evgongora I had to remove the
|
Rate Limiting Implementation for OTP Verification
closes #125
Changes Made
Implementation Details
Rate Limiting Logic
Redis Implementation
Test Results
Rate Limit Testing
Initial attempts (1-5):
Rate limit exceeded (6th attempt onwards):
Redis Verification
Logging Implementation
Testing Instructions
1.Setup Redis credentials in
apps/web/.env:Run the server:
Execute tests:
bun testSecurity Considerations
Demo Results
Monitoring Tools
check-redis.mjs)Next Steps
Summary by CodeRabbit