Implement JWT authentication improvements and enhance error handling #1
Conversation
- Added JwtOptions class for structured JWT configuration with validation. - Updated Program.cs to bind and validate JWT options from configuration. - Enhanced token generation logic in JwtTokenGenerator to utilize new options. - Implemented global exception handling with ProblemDetails for better error reporting. - Added health checks and improved documentation generation in Swagger.
|
Warning Rate limit exceeded@refactor-gremlin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a strongly typed JwtOptions with validation, updates JwtTokenGenerator to use IOptions (including base64 key support and lifetime), and updates Program startup to bind/validate options, configure JWT validation, add health checks, ProblemDetails error handling, and conditional Swagger XML comments; enables XML docs in the csproj. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as Host
participant Conf as Configuration
participant Binder as OptionsBinder
participant Validator as DataAnnotationsValidator
participant Auth as JwtBearerMiddleware
participant Swagger as SwaggerConfig
participant HC as HealthChecks
Note over Host,Conf: Startup binding & validation
Host->>Conf: Read "Jwt" section
Host->>Binder: Bind to JwtOptions
Binder->>Validator: ValidateDataAnnotations + custom key length
Validator-->>Host: Validated JwtOptions (or fail startup)
Note over Host,Auth: Configure auth
Host->>Auth: Configure TokenValidationParameters using JwtOptions (Issuer, Audience, Key)
Host->>Swagger: Include XML comments if file exists
Host->>HC: AddHealthChecks & Map "/health"
Host->>Host: UseExceptionHandler -> ProblemDetails responses
sequenceDiagram
autonumber
participant Controller as API Controller
participant Generator as JwtTokenGenerator
participant Options as IOptions<JwtOptions>
participant Handler as JwtSecurityTokenHandler
Controller->>Generator: Request token (user)
Generator->>Options: Read Key, Issuer, Audience, AccessTokenLifetimeHours
Generator->>Generator: Decode base64 key if prefixed / get key bytes
Generator->>Handler: Create token (claims, iat, sub, expiry, signing key)
Handler-->>Generator: Encoded JWT
Generator-->>Controller: Return token string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances JWT authentication configuration and error handling by introducing structured configuration validation and global exception handling middleware. The changes improve security and maintainability by replacing hardcoded JWT configuration values with a validated options pattern.
- Added JwtOptions class with data validation annotations for structured JWT configuration
- Implemented global exception handling middleware using ProblemDetails for standardized error responses
- Enhanced documentation generation and added health checks
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| MySvelteApp.Server/Program.cs | Updated JWT configuration to use options pattern with validation and added global exception handling |
| MySvelteApp.Server/MySvelteApp.Server.csproj | Enabled XML documentation generation for Swagger |
| MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs | Refactored to use JwtOptions instead of direct configuration access |
| MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs | New configuration class with validation attributes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| { | ||
| policy | ||
| .WithOrigins("http://localhost:5173", "http://localhost:3000", "http://web:3000", "http://localhost:5173") | ||
| .WithOrigins("http://localhost:5173", "http://localhost:3000", "http://web:3000") |
There was a problem hiding this comment.
Duplicate origin 'http://localhost:5173' was removed, but this may indicate a configuration issue. Consider using a configuration-based approach for CORS origins to avoid hardcoded values and potential duplicates.
MySvelteApp.Server/Program.cs
Outdated
| builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) | ||
| .AddJwtBearer(options => | ||
| { | ||
| var jwtOptions = builder.Configuration.GetSection("Jwt").Get<JwtOptions>() ?? new JwtOptions(); |
There was a problem hiding this comment.
Using a fallback to 'new JwtOptions()' when configuration binding fails will result in empty/default values that will cause JWT authentication to fail. Since validation is configured above, this should throw an exception instead of silently failing.
| var jwtOptions = builder.Configuration.GetSection("Jwt").Get<JwtOptions>() ?? new JwtOptions(); | |
| var jwtOptions = builder.Configuration.GetSection("Jwt").Get<JwtOptions>(); | |
| if (jwtOptions == null) | |
| { | |
| throw new InvalidOperationException("Failed to bind JwtOptions from configuration. Please check your 'Jwt' settings."); | |
| } |
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Extra blank line at the end of the file should be removed to maintain consistent formatting.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
MySvelteApp.Server/Program.cs
Outdated
| IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(builder.Configuration["Jwt:Key"] ?? "your-secret-key-here")) | ||
| ValidIssuer = jwtOptions.Issuer, | ||
| ValidAudience = jwtOptions.Audience, | ||
| IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(jwtOptions.Key)) |
There was a problem hiding this comment.
Bug: JWT Configuration Bypass Causes Runtime Failures
The JWT Bearer authentication setup bypasses the JwtOptions validation. It directly retrieves configuration, falling back to an unvalidated JwtOptions with empty Key, Issuer, and Audience if the 'Jwt' section is missing or invalid. This configures authentication with invalid values before ValidateOnStart() can detect the issue, leading to runtime failures.
![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)
MySvelteApp.Server/Program.cs (1)
66-70: Make /health anonymous; Swagger is already served before authFallback RequireAuthenticatedUser will protect endpoints without AllowAnonymous — MapHealthChecks("/health") is mapped after UseAuthorization and currently has no AllowAnonymous. Add .AllowAnonymous() to the health mapping.
builder.Services.AddAuthorizationBuilder() .SetFallbackPolicy(new AuthorizationPolicyBuilder() .RequireAuthenticatedUser() .Build()); @@ -app.MapHealthChecks("/health"); +app.MapHealthChecks("/health").AllowAnonymous();
🧹 Nitpick comments (10)
MySvelteApp.Server/MySvelteApp.Server.csproj (1)
9-10: Scope XML-doc build and CS1591 suppression to Release only.Avoid hiding missing XML docs during local dev and speed up Debug builds.
- <GenerateDocumentationFile>true</GenerateDocumentationFile> - <NoWarn>$(NoWarn);1591</NoWarn> + <!-- Enable docs + suppress "missing XML comment" only for Release --> + <GenerateDocumentationFile>false</GenerateDocumentationFile> + <NoWarn>$(NoWarn)</NoWarn> + </PropertyGroup> + <PropertyGroup Condition="'$(Configuration)' == 'Release'"> + <GenerateDocumentationFile>true</GenerateDocumentationFile> + <NoWarn>$(NoWarn);1591</NoWarn>MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (3)
7-9: Clarify/strengthen key requirements.MinLength(32) is good for HMAC-SHA256. If you expect base64/hex secrets, either document that Key is raw text (UTF‑8) or add support for base64/hex decode at usage to avoid accidental weak keys due to encoding assumptions.
11-15: Prevent whitespace-only Issuer/Audience.[Required] rejects empty strings but allows whitespace. Add explicit validation (either here via a custom attribute or in Program.cs) to ensure non‑whitespace values.
5-6: Consider narrowing visibility.If JwtOptions isn’t consumed outside this assembly, make it internal to reduce public surface.
-public sealed class JwtOptions +internal sealed class JwtOptionsMySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (2)
22-22: OK on symmetric key derivation, but support base64 if needed.If operators provide a base64 secret, consider decoding when Key starts with a known prefix (e.g., "base64:") to avoid using the base64 text bytes as the key.
- var securityKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(_jwtOptions.Key)); + var key = _jwtOptions.Key; + var keyBytes = key.StartsWith("base64:", StringComparison.Ordinal) + ? Convert.FromBase64String(key["base64:".Length..]) + : Encoding.UTF8.GetBytes(key); + var securityKey = new SymmetricSecurityKey(keyBytes);
25-31: Add standard 'sub' and 'iat' claims; avoid culture-sensitive ToString.Improves interoperability and ensures stable formatting.
+using System.Globalization; +using System.IdentityModel.Tokens.Jwt; @@ - var claims = new[] + var claims = new[] { - new Claim(ClaimTypes.NameIdentifier, user.Id.ToString()), + new Claim(ClaimTypes.NameIdentifier, user.Id.ToString(CultureInfo.InvariantCulture)), + new Claim(JwtRegisteredClaimNames.Sub, user.Id.ToString(CultureInfo.InvariantCulture)), new Claim(ClaimTypes.Name, user.Username), - new Claim(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString()) + new Claim(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString()), + new Claim(JwtRegisteredClaimNames.Iat, EpochTime.GetIntDate(DateTime.UtcNow).ToString(CultureInfo.InvariantCulture), ClaimValueTypes.Integer64) };MySvelteApp.Server/Program.cs (4)
43-49: Unify validation and cover whitespace for Issuer/Audience; remove duplicate length check.You already have [MinLength(32)] on Key. Keep the custom check for whitespace, and add similar checks for Issuer/Audience.
builder.Services.AddOptions<JwtOptions>() .Bind(builder.Configuration.GetSection("Jwt")) .ValidateDataAnnotations() - .Validate(options => !string.IsNullOrWhiteSpace(options.Key) && options.Key.Length >= 32, "Jwt:Key must be at least 32 characters long.") + .Validate(o => !string.IsNullOrWhiteSpace(o.Key), "Jwt:Key cannot be blank/whitespace.") + .Validate(o => !string.IsNullOrWhiteSpace(o.Issuer), "Jwt:Issuer cannot be blank/whitespace.") + .Validate(o => !string.IsNullOrWhiteSpace(o.Audience), "Jwt:Audience cannot be blank/whitespace.") .ValidateOnStart();
53-64: Avoid re-binding configuration; source JwtBearer from validated options.Using Get() duplicates binding and can drift from validated options. Configure JwtBearerOptions via DI using IOptions.
-builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) - .AddJwtBearer(options => - { - var jwtOptions = builder.Configuration.GetSection("Jwt").Get<JwtOptions>() ?? new JwtOptions(); - options.TokenValidationParameters = new TokenValidationParameters - { - ValidateIssuer = true, - ValidateAudience = true, - ValidateLifetime = true, - ValidateIssuerSigningKey = true, - ValidIssuer = jwtOptions.Issuer, - ValidAudience = jwtOptions.Audience, - IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(jwtOptions.Key)) - }; - }); +builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) + .AddJwtBearer(); + +builder.Services + .AddOptions<JwtBearerOptions>(JwtBearerDefaults.AuthenticationScheme) + .Configure<IOptions<JwtOptions>>((options, jwt) => + { + var o = jwt.Value; + options.TokenValidationParameters = new TokenValidationParameters + { + ValidateIssuer = true, + ValidateAudience = true, + ValidateLifetime = true, + ValidateIssuerSigningKey = true, + ValidIssuer = o.Issuer, + ValidAudience = o.Audience, + IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(o.Key)), + // Optional hardening: + // ClockSkew = TimeSpan.Zero + }; + });
72-74: ProblemDetails registered — consider exposing exception details only in Development.Tweak options if desired.
-builder.Services.AddProblemDetails(); +builder.Services.AddProblemDetails(o => +{ + o.IncludeExceptionDetails = (ctx, _) => + builder.Environment.IsDevelopment(); +});
60-63: Optional: tighten lifetime skew.If clients and server share reliable time, set ClockSkew = TimeSpan.Zero for stricter expiry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs(1 hunks)MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs(3 hunks)MySvelteApp.Server/MySvelteApp.Server.csproj(1 hunks)MySvelteApp.Server/Program.cs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (3)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(5-22)MySvelteApp.Server/Application/Common/Interfaces/IJwtTokenGenerator.cs (1)
GenerateToken(7-7)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
MySvelteApp.Server/Program.cs (1)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(5-22)
⏰ 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). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (2)
15-18: Good switch to typed options.
33-37: Issuer/Audience/expiry usage looks correct.MySvelteApp.Server/Program.cs (1)
104-110: XML comments inclusion looks good.
- Updated JwtOptions class to include validation for Issuer and Audience fields. - Improved token generation logic in JwtTokenGenerator to support base64-encoded keys. - Refined validation messages for JWT options in Program.cs. - Adjusted project file to enable documentation generation only in Release mode. - Enhanced health check endpoint to allow anonymous access.
…accessibility - Introduced JwtOptionsValidator class for enhanced validation of Issuer and Audience fields. - Updated JwtOptions class to utilize the new validator for better data integrity. - Changed JwtTokenGenerator class to public for broader accessibility within the application.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
45-45: Trailing blank line.Remove for consistency.
🧹 Nitpick comments (3)
MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (1)
23-29: Key derivation: avoid duplication; extract helper.Program.cs currently derives the validation key differently (see separate comment). Centralize derivation to prevent drift.
Apply this minimal change here and call a shared helper:
- var key = _jwtOptions.Key; - var keyBytes = key.StartsWith("base64:", StringComparison.Ordinal) - ? Convert.FromBase64String(key["base64:".Length..]) - : Encoding.UTF8.GetBytes(key); + var keyBytes = DeriveKeyBytes(_jwtOptions.Key); var securityKey = new SymmetricSecurityKey(keyBytes);Add (outside the shown range) a reusable helper in this class or a small internal utility:
private static byte[] DeriveKeyBytes(string key) => key.StartsWith("base64:", StringComparison.Ordinal) ? Convert.FromBase64String(key["base64:".Length..]) : Encoding.UTF8.GetBytes(key);MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
26-28: Strength-check the key in bytes, not characters.MinLength(32) on the string doesn’t guarantee a 256‑bit key (esp. with base64:). Add a custom validator to ensure ≥32 bytes after derivation.
Apply:
[Required] - [MinLength(32, ErrorMessage = "Jwt:Key must be at least 32 characters long.")] + [MinLength(32, ErrorMessage = "Jwt:Key must be at least 32 characters long.")] + [CustomValidation(typeof(JwtOptionsValidator), nameof(JwtOptionsValidator.ValidateKeyStrength))] public string Key { get; set; } = string.Empty;And add (outside this range, e.g., below ValidateNotWhitespace):
public static ValidationResult ValidateKeyStrength(object? value, ValidationContext context) { var s = value as string ?? string.Empty; byte[] bytes = s.StartsWith("base64:", StringComparison.Ordinal) ? Convert.FromBase64String(s["base64:".Length..]) : Encoding.UTF8.GetBytes(s); return bytes.Length >= 32 ? ValidationResult.Success! : new ValidationResult("Jwt:Key must be at least 32 bytes (256-bit) after decoding."); }MySvelteApp.Server/Program.cs (1)
52-54: Minor: configure JwtBearer in one place.You can inline options into AddJwtBearer to avoid split configuration.
Example:
-builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) - .AddJwtBearer(); +builder.Services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme) + .AddJwtBearer(); // keep or inline with Configure below; either is fineKeeping the current pattern is acceptable; just a style note.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs(1 hunks)MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs(2 hunks)MySvelteApp.Server/MySvelteApp.Server.csproj(1 hunks)MySvelteApp.Server/Program.cs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (3)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(24-43)MySvelteApp.Server/Application/Common/Interfaces/IJwtTokenGenerator.cs (1)
GenerateToken(7-7)MySvelteApp.Server/Domain/Entities/User.cs (1)
User(3-10)
MySvelteApp.Server/Program.cs (1)
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (1)
JwtOptions(24-43)
🔇 Additional comments (11)
MySvelteApp.Server/MySvelteApp.Server.csproj (1)
9-15: Release-only XML docs wiring looks correct.Matches Program.cs conditional XML include; no action needed.
MySvelteApp.Server/Infrastructure/Authentication/JwtTokenGenerator.cs (2)
31-38: Claims set is sensible.Sub, jti, iat are good additions; invariant culture usage is correct.
41-45: Expiry sourcing from options LGTM.Using AccessTokenLifetimeHours improves configurability.
MySvelteApp.Server/Infrastructure/Authentication/JwtOptions.cs (2)
34-36: Validator attribute will work once signature is fixed.Redundant with extra
.Validate(...)in Program.cs, but harmless.
41-42: Reasonable lifetime bounds.1–168 hours range is sane.
MySvelteApp.Server/Program.cs (6)
36-39: CORS with explicit origins + credentials is fine.No wildcard with credentials; good.
43-51: Typed options binding + startup validation LGTM.Once JwtOptions custom validator is fixed, this will fail fast correctly.
Please run the app once with an intentionally bad Jwt section to confirm
ValidateOnStart()blocks startup as expected.
80-82: ProblemDetails + HealthChecks registration looks good.Consistent with middleware below.
112-117: XML comments inclusion is safe and conditional.Matches csproj Release-only XML generation.
166-187: Exception handler is correctly first in the pipeline.Will catch upstream middleware exceptions; DEV-only detail is appropriate.
202-202: Health endpoint explicitly anonymous.Correct with fallback auth policy.
- Removed outdated gcdump file. - Updated Program.cs to derive key bytes from the JWT key for improved security. - Enhanced JwtOptionsValidator to include key strength validation. - Refactored JwtTokenGenerator to utilize the new key derivation method, ensuring consistent key handling across the application.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores