Add IExceptionHandler and use it to warn about old localtest #998
Conversation
* New RequestExceptionHandler: IExceptionHandler to create ProblemDetails * New AltinnAppException with the fields of ProblemDetails * Throw exception for old localtest with text for ProblemDetails about upgrading * Modify callstack to let the exception bubble through.
ivarne
added
the
feature
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| internal class RequestExceptionHandler : IExceptionHandler | ||
| { | ||
| private readonly IProblemDetailsService _problemDetailsService; | ||
| private ILogger<RequestExceptionHandler> _logger; |
Check notice
Code scanning / CodeQL
Missed 'readonly' opportunity Note
| } | ||
|
|
||
| _logger.LogError($"Unable to update instance process with instance id {instance.Id}"); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 1 month ago
To fix the problem, we need to sanitize the instance.Id before logging it. This can be done by removing any newline characters from the instance.Id to prevent log forging. We will use the String.Replace method to achieve this.
- Identify the line where the
instance.Idis logged. - Sanitize the
instance.Idby removing newline characters before logging it. - Ensure that the fix does not change the existing functionality of the code.
| @@ -206,3 +206,3 @@ | ||
|
|
||
| _logger.LogError($"Unable to update instance process with instance id {instance.Id}"); | ||
| _logger.LogError($"Unable to update instance process with instance id {instance.Id.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", "")}"); | ||
| throw await PlatformHttpException.CreateAsync(response); |
| public bool IsForbidden | ||
| { | ||
| // ReSharper disable once ValueParameterNotUsed | ||
| init { StatusCode = StatusCodes.Status403Forbidden; } |
Check warning
Code scanning / CodeQL
Property value is not used when setting a property Warning
| public bool IsBadRequest | ||
| { | ||
| // ReSharper disable once ValueParameterNotUsed | ||
| init { StatusCode = StatusCodes.Status400BadRequest; } |
Check warning
Code scanning / CodeQL
Property value is not used when setting a property Warning
| @@ -369,6 +370,12 @@ await _instanceClient.DeleteInstance( | |||
| _logger.LogInformation("Events sent to process engine: {Events}", change?.Events); | |||
| await _processEngine.HandleEventsAndUpdateStorage(instance, null, change?.Events); | |||
| } | |||
| // Let the AltinnAppException through as it is handled in IExceptionHandler | |||
| catch (AltinnAppException) | |||
There was a problem hiding this comment.
The part I don't like about this is the kind of coupling we get that isn't expressed anywhere (at least not cohesively). IMO it is hard to trace and maintain over time, so I think it's not a good solution for something to be used application-wide. In this particular case, the caller and callee have to be kept in sync, but the compiler doesn't help us :(
Since this is all internal though, we could let this through as long as we think of it as a temporary solution to this specific problem.
There was a problem hiding this comment.
Do you object to this specific case of letting an exception through (which I totally agree, but wanted to preserve behavior). Or the general concept of throwing exceptions with explicit problem details info and a central catch middleware?
If this approach is accepted it would obviously cause a bigger change to how we handle errors.
There was a problem hiding this comment.
I have also purposefully limited the available status codes that can be returned, to reduce surprises for api documentation.
There was a problem hiding this comment.
The latter, exception based error handling, using IExceptionHandler in this case. IExceptionHandler is one of those global mechanisms that might create surprises for app devs in the same way we encountered with JsonOptions. So in that sense I like local try/catch better
| @@ -16,10 +16,7 @@ public static class WebApplicationBuilderExtensions | |||
| public static IApplicationBuilder UseAltinnAppCommonConfiguration(this IApplicationBuilder app) | |||
| { | |||
| var appId = StartupHelper.GetApplicationId(); | |||
| if (app is WebApplication webApp && webApp.Environment.IsDevelopment()) | |||
| { | |||
| app.UseDeveloperExceptionPage(); | |||
There was a problem hiding this comment.
Note that develperexeptionpage gets added by default in net6 and forwads
|
This PR introduces a new concept
IExceptionHandlerthat currently only handles the newAltinnAppExceptionand customises theProblemDetailsresponse to include the data from the exceptionThe use case is to warn about old localtest, but in the future I think this Is a richer way to both ensure that exceptions gets logged, and that api users gets more helpful error messages.
New stuff
Verification
Documentation