AntiforgeryMiddleware needs an escape-hatch for troubleshooting and logging, and customizing the final response
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 10h
- Merged PRs (30d)
- 281
Description
### Is there an existing issue for this?
- [x] I have searched the existing issues
Searching existing issues found these which are somewhat relevant, but ultimately unhelpful for this specific problem:
* https://github.com/dotnet/aspnetcore/pull/66772
* https://github.com/dotnet/aspnetcore/pull/51962
### Is your feature request related to a problem? Please describe the problem.
When `AntiforgeryMiddleware` is active, it becomes impossible to inspect `HttpRequest.Form` for logging or troubleshooting purposes, which frustrates attempts to resolve (non-malicious) inadvertent CSRF failures. Additionally, there are plenty of locations within AspNetCore itself where `HttpRequest.Form` or `HasFormContentType` is used such that `HandleUncheckedAntiforgeryValidationFeature()` will throw.
Relatedly, I noticed that there doesn't seem to be a way to return a user-friendly error-page or non-500 response when there is a CSRF failure.
For example, because `Microsoft.AspNetCore.Mvc.ModelBinding.FormValueProviderFactory` accesses `HttpRequest.HasFormContentType` it will throw-up the `InvalidOperationException` caused by the `AntiforgeryMiddleware`:
```
[12:56:19 Error] Microsoft.AspNetCore.Server.Kestrel
Connection id "0HNNC321O3812", Request id "0HNNC321O3812:00000035": An unhandled exception was thrown by the application.
System.InvalidOperationException: This form is being accessed with an invalid anti-forgery token. Validate the `IAntiforgeryValidationFeature` on the request before reading from the form.
at Microsoft.AspNetCore.Http.Features.FormFeature.HandleUncheckedAntiforgeryValidationFeature()
at Microsoft.AspNetCore.Http.Features.FormFeature.get_HasFormContentType()
at Microsoft.AspNetCore.Mvc.ModelBinding.FormValueProviderFactory.CreateValueProviderAsync(ValueProviderFactoryContext context)
at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.CreateAsync(ActionContext actionContext, IList`1 factories)
at Microsoft.AspNetCore.Mvc.ModelBinding.CompositeValueProvider.TryCreateAsync(ActionContext actionContext, IList`1 factories)
at Microsoft.AspNetCore.Mvc.Controllers.ControllerBinderDelegateProvider.<>c__DisplayClass0_0.<g__Bind|0>d.MoveNext()
--- End of stack trace from previous location ---
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()
--- End of stack trace from previous location ---
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
```
Furthermore, because it throws the generic exception-type, `InvalidOperationException` with no specific information carried with it, it's impractical to intercept it in middleware and return a more appropriate response - and as said, it's impossible to log the _actual_ parsed `Request.Form` values (if any) when they may aid troubleshooting.
### Describe the solution you'd like
`IAntiforgeryValidationFeature` should expose `RequestHasFormContentType` and the `IFormCollection` that `HttpRequest.Form` would otherwise return without throwing `InvalidOperationException`, for use by logging/debugging/troubleshooters.
### Workaround
This is what I'm currently doing as a hack workaround to enable logging of `Request.Form`, though it uses an undocumented `HttpContext.Items` key - and I'm unsure how robust or reliable it is under other CSRF scenarios:
```
private static String GetRequestFormForLogScope( HttpRequest req )
{
if( !"POST".EqualsIns( req.Method ) ) return "(Non-POST request)";
//
// AspNetCore's AntiForgery feature blocks access to `HttpRequest.Form`, including even checking `HasFormContentType` - which means we can't log those details (grrr).
// I filed this issue concerning this problem: https://github.com/dotnet/aspnetcore/issues/68054
IAntiforgeryValidationFeature? csrfFeature = req.HttpContext.Features.Get();
if( csrfFeature is null || csrfFeature.IsValid )
{
if( req.HasFormContentType ) // <-- Accessing this property will throw if `csrfFeature.IsValid == false`
{
return StringBuilderLoggingExtensions.FlattenRequestForm( req.Form );
}
else
{
return KnownSerilogProperties.AspNetRequestForm_InvalidContentType;
}
}
else
{
HttpContext httpContext = req.HttpContext;
// HACK:
const String FLAG_ITEM_KEY = @"__AntiforgeryMiddlewareWithEndpointInvoked";
if( httpContext.Items.ContainsKey( FLAG_ITEM_KEY ) )
{
Object? item = httpContext.Items[ FLAG_ITEM_KEY ];
_ = httpContext.Items.Remove( FLAG_ITEM_KEY );
try
{
if( req.HasFormContentType )
{
return StringBuilderLoggingExtensions.FlattenRequestForm( req.Form );
}
else
{
return KnownSerilogProperties.AspNetRequestForm_InvalidContentType;
}
}
finally
{
httpContext.Items[ FLAG_ITEM_KEY ] = item;
}
}
else
{
return "CSRF Antiforgery error: " + csrfFeature.Error?.ToString();
}
}
}
```
### Additional context
_No response_
Contributor guide
Research direction
Start by reading AntiforgeryMiddleware, IAntiforgeryValidationFeature, and FormFeature, then review the related pull requests linked in the issue. Define the API and behavior for inspecting form data after validation failure and customizing the resulting response. Done should include coverage for invalid tokens, form-content checks, and avoiding the unhandled InvalidOperationException path.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- api, backend, security
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100