feat(webapi): Limit Content-Length / request body size #1416
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple classes, primarily focusing on error handling improvements. Changes include the implementation of a new response handling method, Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/UserTypeValidationMiddleware.cs (2)
Line range hint
24-31: Consider adding logging for security monitoring.Since this middleware handles authentication validation failures, it would be valuable to log these events for security monitoring and auditing purposes.
if (userType == UserIdType.Unknown) { + _logger.LogWarning("Authentication failed: Unable to determine valid user type for {User}", + context.User.Identity?.Name); context.Response.StatusCode = StatusCodes.Status403Forbidden; await context.Response.WriteAsJsonAsync(context.GetResponseOrDefault(
Line range hint
24-31: Consider externalizing error messages.The error message is currently hardcoded. Consider moving it to a resource file to support internationalization and maintain messages in a central location.
await context.Response.WriteAsJsonAsync(context.GetResponseOrDefault( context.Response.StatusCode, [ new("Type", - "The request was authenticated, but we were unable to determine valid user type (person or system user) in order to authorize the request.") + ErrorMessages.UnknownUserType) ]src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/ServiceOwnerOnBehalfOfPersonMiddleware.cs (1)
Line range hint
1-49: Consider adding logging for security events.Since this middleware handles authentication and authorization, it would be beneficial to add logging when:
- Authentication fails
- Required scope is missing
- Invalid endUserId is provided
Here's a suggested implementation:
+ private readonly ILogger<ServiceOwnerOnBehalfOfPersonMiddleware> _logger; - public ServiceOwnerOnBehalfOfPersonMiddleware(RequestDelegate next) + public ServiceOwnerOnBehalfOfPersonMiddleware( + RequestDelegate next, + ILogger<ServiceOwnerOnBehalfOfPersonMiddleware> logger) { _next = next; + _logger = logger; } public Task InvokeAsync(HttpContext context) { if (context.User.Identity is not { IsAuthenticated: true }) { + _logger.LogWarning("Unauthenticated request received"); return _next(context); } if (!context.User.HasScope(AuthorizationScope.ServiceProvider)) { + _logger.LogWarning("Request missing required scope: {Scope}", + AuthorizationScope.ServiceProvider); return _next(context); } // ... rest of the method }src/Digdir.Domain.Dialogporten.WebApi/Common/Constants.cs (1)
8-8: Add XML documentation for the constant.Consider adding XML documentation to explain:
- The purpose of this limit
- Why this specific value was chosen
- The impact on API consumers
+ /// <summary> + /// Maximum allowed size for HTTP request body in bytes. + /// This limit helps prevent denial of service attacks while allowing legitimate requests. + /// </summary> internal const int MaxRequestBodySize = 100_000;src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/PatchDialogsController.cs (2)
91-96: Consider improving readability with line breaks.The error handling implementation looks good, but the readability could be improved by adding line breaks between different error cases.
Consider this format for better readability:
return result.Match( success => (IActionResult)NoContent(), - notFound => NotFound(HttpContext.GetResponseOrDefault(StatusCodes.Status404NotFound, notFound.ToValidationResults())), - badRequest => BadRequest(HttpContext.GetResponseOrDefault(StatusCodes.Status400BadRequest, badRequest.ToValidationResults())), - validationFailed => BadRequest(HttpContext.GetResponseOrDefault(StatusCodes.Status400BadRequest, validationFailed.Errors.ToList())), - forbidden => new ObjectResult(HttpContext.GetResponseOrDefault(StatusCodes.Status403Forbidden, forbidden.ToValidationResults())), - domainError => UnprocessableEntity(HttpContext.GetResponseOrDefault(StatusCodes.Status422UnprocessableEntity, domainError.ToValidationResults())), - concurrencyError => new ObjectResult(HttpContext.GetResponseOrDefault(StatusCodes.Status412PreconditionFailed)) { StatusCode = StatusCodes.Status412PreconditionFailed } + notFound => NotFound(HttpContext.GetResponseOrDefault( + StatusCodes.Status404NotFound, notFound.ToValidationResults())), + + badRequest => BadRequest(HttpContext.GetResponseOrDefault( + StatusCodes.Status400BadRequest, badRequest.ToValidationResults())), + + validationFailed => BadRequest(HttpContext.GetResponseOrDefault( + StatusCodes.Status400BadRequest, validationFailed.Errors.ToList())), + + forbidden => new ObjectResult(HttpContext.GetResponseOrDefault( + StatusCodes.Status403Forbidden, forbidden.ToValidationResults())), + + domainError => UnprocessableEntity(HttpContext.GetResponseOrDefault( + StatusCodes.Status422UnprocessableEntity, domainError.ToValidationResults())), + + concurrencyError => new ObjectResult(HttpContext.GetResponseOrDefault( + StatusCodes.Status412PreconditionFailed)) + { + StatusCode = StatusCodes.Status412PreconditionFailed + } );
Line range hint
64-98: Missing implementation of request body size limit.The PR's objective is to limit Content-Length/request body size, but the current implementation doesn't address this requirement. Consider:
- Adding request size validation in the controller or middleware.
- Implementing a maximum size limit for the patch document.
- Adding appropriate error responses for requests exceeding the size limit.
Would you like assistance in implementing the request size validation?
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/GlobalExceptionHandler.cs (2)
24-28: Improve log message formatting for better readabilityThe current log message format string
"{@Http}{@Type}{@Reason}"concatenates placeholders without separators, which may make the logs hard to read. Consider adding separators or labels for better clarity.Apply this diff to improve the log message:
-logger.LogError(exception, "{@Http}{@Type}{@Reason}", http, type, error); +logger.LogError(exception, "Request: {@Http}, ExceptionType: {@Type}, Message: {@Reason}", http, type, error);
24-24: Include full request URL in error logsConsider including the full request URL in the error logs to enhance traceability and debugging effectiveness.
Apply this diff to include the full URL:
-var http = $"{ctx.Request.Scheme}: {ctx.Request.Method} {ctx.Request.Path}"; +var http = $"{ctx.Request.Scheme}://{ctx.Request.Host}{ctx.Request.Path}{ctx.Request.QueryString}";src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs (1)
36-43: Ensure consistent use ofProblemDetailsandValidationProblemDetailsWhile handling
StatusCodes.Status413PayloadTooLarge, consider whetherValidationProblemDetailsis more appropriate if validation errors are involved, to maintain consistency with other status codes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/ServiceOwnerOnBehalfOfPersonMiddleware.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/UserTypeValidationMiddleware.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Constants.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/GlobalExceptionHandler.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/PatchDialogsController.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Program.cs(1 hunks)
🔇 Additional comments (9)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/UserTypeValidationMiddleware.cs (1)
Line range hint 24-31: LGTM! Error handling improvements look good.
The switch to GetResponseOrDefault maintains consistent error handling across the application while providing a clear error message.
src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/ServiceOwnerOnBehalfOfPersonMiddleware.cs (2)
Line range hint 40-49: LGTM! Error handling improvement looks good.
The change to use GetResponseOrDefault aligns with the standardized error handling approach. The error message is clear and provides helpful guidance for API consumers.
Line range hint 40-49: Consider adding request body size validation.
Given that this PR's objective is to limit request body size, and this middleware handles incoming requests, it would be good to add size validation here as well.
Let's check if request size validation is implemented elsewhere:
src/Digdir.Domain.Dialogporten.WebApi/Common/Constants.cs (1)
8-8: Verify if 100KB is sufficient for your use cases.
The current limit of 100,000 bytes (~97.7 KB) seems quite restrictive for a web API. While setting a request size limit is good security practice, this value might be too low for common scenarios like file uploads or large JSON payloads. Consider:
- What are the typical payload sizes in your API?
- Are there any file upload endpoints?
- What's the maximum size of your largest expected JSON payload?
Industry standard limits are typically in megabytes range (e.g., 10MB-50MB) unless there's a specific reason for a stricter limit.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Patch/PatchDialogsController.cs (1)
73-76: LGTM, but consider adding request size validation.
The error handling changes look good, standardizing the response generation. However, given the PR's objective to limit request body size, consider adding validation for the patch document size.
Let's check if size limits are implemented elsewhere:
src/Digdir.Domain.Dialogporten.WebApi/Program.cs (1)
55-58: 🛠️ Refactor suggestion
Verify if 100KB request size limit is sufficient for all use cases.
The implementation correctly adds a request body size limit, but 100KB might be too restrictive for a modern web API. Consider the following recommendations:
- Make the limit configurable per environment
- Document the limit in API documentation
- Consider customizing the error response when limit is exceeded
Let's verify the existing request patterns:
Consider applying these improvements:
builder.WebHost.ConfigureKestrel(kestrelOptions =>
{
- kestrelOptions.Limits.MaxRequestBodySize = Constants.MaxRequestBodySize;
+ // Configure request size limit based on environment
+ var maxRequestSize = builder.Environment.IsDevelopment()
+ ? 1024 * 1024 // 1MB for development
+ : Constants.MaxRequestBodySize;
+ kestrelOptions.Limits.MaxRequestBodySize = maxRequestSize;
});Additionally:
- Add XML documentation to
Constants.MaxRequestBodySizeexplaining the limit - Consider implementing custom middleware to handle
BadHttpRequestExceptionwith status code 413 (Payload Too Large) - Update API documentation to mention the request size limits
✅ Verification successful
100KB request size limit is appropriate and properly handled
The implementation of the request size limit is well-designed and sufficient for the following reasons:
- The API already has proper error handling for payload size limits, returning a standard RFC-compliant 413 response with a clear error message
- The 100KB limit (Constants.MaxRequestBodySize = 100_000) is reasonable for this API's endpoints which appear to be primarily focused on metadata operations (creating/updating dialogs, activities, and transmissions)
- The error response includes helpful information: the actual size limit, a link to the RFC specification, and the specific endpoint that was called
No changes are needed as the current implementation follows best practices for request size limiting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for POST/PUT endpoints to understand payload requirements
ast-grep --pattern 'public class $_ : EndpointWithMapping<$_, $_>
{
public override void Configure()
{
$$$
Post($$$);
$$$
}
}'
ast-grep --pattern 'public class $_ : EndpointWithMapping<$_, $_>
{
public override void Configure()
{
$$$
Put($$$);
$$$
}
}'
Length of output: 292
Script:
#!/bin/bash
# Let's try a different approach to find endpoints and their request models
# First, let's find all endpoint files
fd "Endpoint.cs$"
# Then search for POST/PUT route configurations with context
rg -A 5 "Post\(|Put\(" --type cs
# Let's also check the Constants file to see the actual size limit
rg "MaxRequestBodySize" --type cs
Length of output: 10311
Script:
#!/bin/bash
# Let's examine the request models for POST/PUT endpoints to understand payload requirements
cat src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogDto.cs
cat src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogDto.cs
cat src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityDto.cs
cat src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionDto.cs
# Check if there's any custom error handling for request size limit
rg -A 5 "413|PayloadTooLarge|RequestSizeLimit" --type cs
Length of output: 1983
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/GlobalExceptionHandler.cs (1)
12-17: Efficient exception-to-status code mapping using switch expression
The use of a switch expression to map exceptions to HTTP status codes enhances code clarity and maintainability.
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs (2)
9-17: LGTM: Correct implementation of DefaultResponse method
The DefaultResponse method is correctly implemented, providing a standardized default error response with all necessary details.
19-21: LGTM: Appropriate fallback mechanism in GetResponseOrDefault method
The GetResponseOrDefault method effectively provides a fallback to DefaultResponse when ResponseBuilder returns null, ensuring consistent error handling.
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Extensions/ErrorResponseBuilderExtensions.cs
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [1.30.0](v1.29.0...v1.30.0) (2024-11-08) ### Features * **performance:** Performance/create serviceowner search ([#1413](#1413)) ([f1096a4](f1096a4)) * **webapi:** Combine actorDtos ([#1374](#1374)) ([ca18a99](ca18a99)) * **webapi:** Limit Content-Length / request body size ([#1416](#1416)) ([44be20a](44be20a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Large request bodies can be used as a form of DDOS, especially when it comes to transmissions because they have a more complex hierarchy validation
Limiting the body size on requests to 100 kB
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
endUserIdand unknown user types, providing more informative responses.PatchDialogsController.Documentation
Refactor