feat: rate limiting configuration with IP-based partitioning #269
Conversation
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
nanotaboada
force-pushed
the
feature/rate-limiting
branch
from
ef85056 to
98f133c
Compare
nanotaboada
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis change introduces a utility for extracting client IP addresses from HTTP requests, adds a global IP-based fixed window rate limiter, and updates CORS policy logic to apply only in development. It also refactors the service registration and middleware order in the ASP.NET Core Web API setup to accommodate these new features. Additionally, it adds a new configuration class for rate limiter settings and updates configuration files to include rate limiting parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebServer
participant ServiceCollectionExtensions
participant HttpContextUtilities
Client->>WebServer: HTTP Request
WebServer->>ServiceCollectionExtensions: AddFixedWindowRateLimiter()
ServiceCollectionExtensions->>HttpContextUtilities: ExtractIpAddress(HttpContext)
HttpContextUtilities-->>ServiceCollectionExtensions: Return client IP or fallback
ServiceCollectionExtensions-->>WebServer: Configure rate limiter partitioned by IP
WebServer-->>Client: Response (429 if limit exceeded)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Dotnet.Samples.AspNetCore.WebApi/Program.cs (1)
58-61: Middleware ordering correctly addresses health check concerns.The current middleware order places health checks (
MapHealthChecks("/health")) before rate limiting (UseRateLimiter()), which ensures monitoring endpoints remain accessible even when rate limits are exceeded. This addresses the concern raised in previous reviews about health check availability.
🧹 Nitpick comments (5)
src/Dotnet.Samples.AspNetCore.WebApi/appsettings.Development.json (1)
37-41: Consider development-specific rate limiting configuration.The identical rate limiting settings across environments ensure consistent behavior. However, you might consider more lenient limits in development (e.g., higher PermitLimit) to facilitate testing and debugging activities.
src/Dotnet.Samples.AspNetCore.WebApi/Configurations/RateLimiterConfiguration.cs (1)
6-25: Consider adding validation attributes for configuration robustness.The configuration class looks well-structured with reasonable defaults. However, consider adding validation attributes to prevent invalid configurations that could cause runtime issues.
+using System.ComponentModel.DataAnnotations; + /// <summary> /// Configuration options for the Fixed Window Rate Limiter. /// </summary> public class RateLimiterConfiguration { /// <summary> /// Gets or sets the maximum number of permits that can be leased per window. /// Default value is 60 requests. /// </summary> + [Range(1, int.MaxValue, ErrorMessage = "PermitLimit must be greater than 0")] public int PermitLimit { get; set; } = 60; /// <summary> /// Gets or sets the time window in seconds for rate limiting. /// Default value is 60 seconds (1 minute). /// </summary> + [Range(1, 3600, ErrorMessage = "WindowSeconds must be between 1 and 3600 seconds")] public int WindowSeconds { get; set; } = 60; /// <summary> /// Gets or sets the maximum number of requests that can be queued when the permit limit is exceeded. /// A value of 0 means no queuing (default). /// </summary> + [Range(0, 1000, ErrorMessage = "QueueLimit must be between 0 and 1000")] public int QueueLimit { get; set; } = 0; }src/Dotnet.Samples.AspNetCore.WebApi/Utilities/HttpContextUtilities.cs (2)
26-35: Consider validating extracted IP addresses for security.The X-Forwarded-For header parsing accepts the first valid IP without additional validation. In production environments behind trusted proxies, this is generally safe, but consider documenting the trust assumptions.
if (headers.TryGetValue("X-Forwarded-For", out var xForwardedFor)) { var clientIp = xForwardedFor .ToString() .Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries) .FirstOrDefault(); - if (!string.IsNullOrWhiteSpace(clientIp) && IPAddress.TryParse(clientIp, out ipAddress)) + if (!string.IsNullOrWhiteSpace(clientIp) && + IPAddress.TryParse(clientIp, out ipAddress) && + !IPAddress.IsLoopback(ipAddress) && + !ipAddress.Equals(IPAddress.Any) && + !ipAddress.Equals(IPAddress.IPv6Any)) return ipAddress.ToString(); }
10-18: Document security assumptions for proxy header trust.The method trusts proxy headers (X-Forwarded-For, X-Real-IP) without validation. Consider adding documentation about when this is safe to use and potential security implications.
/// <summary> + /// Extracts the client IP address from the HTTP context. + /// + /// SECURITY NOTE: This method trusts proxy headers (X-Forwarded-For, X-Real-IP). + /// Only use in environments where you trust the proxy infrastructure, or consider + /// implementing additional validation for untrusted proxy scenarios. + /// /// This method checks for the "X-Forwarded-For" and "X-Real-IP" headers, /// which are commonly used by proxies to forward the original client IP address. /// If these headers are not present or the IP address cannot be parsed, /// it falls back to the remote IP address from the connection. /// If no valid IP address can be determined, it returns "unknown". /// </summary>src/Dotnet.Samples.AspNetCore.WebApi/Extensions/ServiceCollectionExtensions.cs (1)
171-173: Consider validation for rate limiter configuration.The configuration binding uses null-coalescing to provide defaults, but there's no validation of the loaded configuration values. Invalid values could cause runtime issues.
var settings = configuration.GetSection("RateLimiter").Get<RateLimiterConfiguration>() ?? new RateLimiterConfiguration(); + + // Validate configuration values + if (settings.PermitLimit <= 0) + throw new InvalidOperationException("RateLimiter:PermitLimit must be greater than 0"); + if (settings.WindowSeconds <= 0) + throw new InvalidOperationException("RateLimiter:WindowSeconds must be greater than 0"); + if (settings.QueueLimit < 0) + throw new InvalidOperationException("RateLimiter:QueueLimit must be >= 0");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Dotnet.Samples.AspNetCore.WebApi/Configurations/RateLimiterConfiguration.cs(1 hunks)src/Dotnet.Samples.AspNetCore.WebApi/Dotnet.Samples.AspNetCore.WebApi.csproj(2 hunks)src/Dotnet.Samples.AspNetCore.WebApi/Extensions/ServiceCollectionExtensions.cs(4 hunks)src/Dotnet.Samples.AspNetCore.WebApi/Program.cs(2 hunks)src/Dotnet.Samples.AspNetCore.WebApi/Utilities/HttpContextUtilities.cs(1 hunks)src/Dotnet.Samples.AspNetCore.WebApi/appsettings.Development.json(1 hunks)src/Dotnet.Samples.AspNetCore.WebApi/appsettings.json(1 hunks)test/Dotnet.Samples.AspNetCore.WebApi.Tests/Dotnet.Samples.AspNetCore.WebApi.Tests.csproj(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Dotnet.Samples.AspNetCore.WebApi/Extensions/ServiceCollectionExtensions.cs (2)
src/Dotnet.Samples.AspNetCore.WebApi/Configurations/RateLimiterConfiguration.cs (1)
RateLimiterConfiguration(6-25)src/Dotnet.Samples.AspNetCore.WebApi/Utilities/HttpContextUtilities.cs (2)
HttpContextUtilities(8-47)ExtractIpAddress(19-46)
🔇 Additional comments (11)
test/Dotnet.Samples.AspNetCore.WebApi.Tests/Dotnet.Samples.AspNetCore.WebApi.Tests.csproj (1)
12-17: LGTM! Clean formatting improvement.The consolidation of PackageReference elements from multiline to single-line format with inline PrivateAssets attributes improves readability and consistency without affecting functionality.
src/Dotnet.Samples.AspNetCore.WebApi/appsettings.json (1)
37-41: Rate limiting configuration looks reasonable.The settings provide a balanced approach:
- 60 requests per minute is moderate and suitable for most API scenarios
- Zero queue limit ensures immediate rejection when limits are exceeded, preventing request delays
Consider if these limits align with your expected API usage patterns and whether you might need different limits for authenticated vs. anonymous users in the future.
src/Dotnet.Samples.AspNetCore.WebApi/Dotnet.Samples.AspNetCore.WebApi.csproj (2)
21-21: Correct dependency categorization.Moving FluentValidation.DependencyInjectionExtensions to runtime dependencies is appropriate since dependency injection extensions are needed at runtime for service registration, not just during development.
11-12: LGTM! Consistent formatting improvement.The inline PrivateAssets attributes maintain consistency with the formatting improvements made across the project files.
src/Dotnet.Samples.AspNetCore.WebApi/Program.cs (2)
25-29: Service registration improvements look good.The reordering of service registrations and the addition of the rate limiter service are well-structured. The environment parameter passed to
AddCorsDefaultPolicyenables environment-specific CORS configuration.
65-65: CORS properly scoped to development environment.Moving CORS middleware inside the development environment block and placing it first ensures it only applies when needed, improving security in production environments.
src/Dotnet.Samples.AspNetCore.WebApi/Extensions/ServiceCollectionExtensions.cs (5)
17-19: LGTM!The class declaration changes are appropriate. Making the class
partialallows for better organization of extension methods, and the updated XML documentation correctly specifies the target interface.
57-78: LGTM! Addresses previous security concerns.The CORS policy has been improved to only apply the permissive configuration in development environments, which addresses the security concerns raised in previous reviews. This is a much safer approach for production deployments.
108-110: LGTM!Extracting the OpenApiInfo configuration into a variable improves readability and follows good practices for configuration binding.
177-193: LGTM! Well-implemented IP-based rate limiting.The rate limiter implementation is well-structured:
- Uses IP-based partitioning for fair limiting across clients
- Configurable parameters from settings
- Proper use of FixedWindowRateLimiterOptions
- Appropriate queue processing order (OldestFirst)
The integration with
HttpContextUtilities.ExtractIpAddressprovides proper IP extraction handling proxy scenarios.
195-195: LGTM!Setting the rejection status code to 429 (Too Many Requests) is the correct HTTP status code for rate limiting scenarios.
src/Dotnet.Samples.AspNetCore.WebApi/Utilities/HttpContextUtilities.cs
Outdated
Show resolved
Hide resolved
|
Summary by CodeRabbit
New Features
Improvements