StatusCodePages middleware returns both JSON Problem Details & plaintext content for responses to requests from browsers
DamianEdwards opened this issue · 20 comments
Is there an existing issue for this?
- I have searched the existing issues
Describe the bug
The StatusCodePagesMiddleware is returning both JSON Problem Details and plain text content (concatenated together) when the request's Accept header contains certain combinations of media types. This can be easily seen by making a request from a browser (reproduced using Edge latest on Windows).
Expected Behavior
The response should only contain either plain text or JSON Problem Details content, depending on the value of the Accept header.
Steps To Reproduce
- Create a new ASP.NET Core Web API application (minimal)
- In
Program.csadd a call tobuilder.Services.AddProblemDetails()to enable theIProblemDetailsService - In
Program.csadd a call toapp.UseStatusCodePages()to add theStatusCodePagesMiddlewareto the pipeline - Run the application
- Make a request from a browser to "/test" to force a 404 response
Example Program.cs:
var builder = WebApplication.CreateBuilder(args);
// Add services to the container.
// Learn more about configuring Swagger/OpenAPI at https://aka.ms/aspnetcore/swashbuckle
builder.Services.AddEndpointsApiExplorer();
builder.Services.AddSwaggerGen();
builder.Services.AddProblemDetails();
var app = builder.Build();
app.UseStatusCodePages();
// Configure the HTTP request pipeline.
if (app.Environment.IsDevelopment())
{
app.UseSwagger();
app.UseSwaggerUI();
}
app.UseHttpsRedirection();
app.MapGet("/", () => new { hello = "World" });
app.Run();Result:
Accet header content for request from browser:
text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9
Exceptions (if any)
No response
.NET Version
7.0.200-preview.22605.5
Anything else?
No response
This check should have prevented that:
The response must be buffered?
The easy fix is to make that line an else.
Ah interesting, I only tried reproducing by launching from VS which would inject the BrowserLink/Refresh middleware so almost certainly buffered.
Confirmed it doesn't repro when launching from the cmd line with dotnet run
Triage: We think it might be related to the change in behavior in the BrowserLink middleware that is documented here (#45037).
@MackinnonBuck Do you know if the PR to fix this in .NET 7 has been approved?
We can also do what Chris mentioned here (#45678 (comment)).
cc: @brunolins16
Thanks for contacting us.
We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
@captainsafia The PR has been approved. I just left a comment asking if we're clear to merge it (I'm not sure what the rules are regarding when merging into servicing release branches is allowed).
@MackinnonBuck link?
Wait for the build team to merge servicing PRs.
Oh, I thought you were referring to a fix for this issue. This issue should still be fixed regardless.
The response must be buffered?
The easy fix is to make that line an
else.
@Tratcher I wasn't able to reproduce (buffered or not) and a quick look seems move to an else might not be possible since this can skip all Writers and complete without writing anything.
I am planning to change the check to something like this, that might be good enough:
Thoughts?
Background and Motivation
I have been talking with @Tratcher and we believe that we are trying to work around a wrong decision about the IProblemDetailsService design (I can blame myself 😅) where the WriteAsync might or might not write the ProblemDetails, however, it doesn't report this to the caller.
Just for context, a ProblemDetails will not be written when all of the registered IProblemDetailsWriter, including the DefaultProblemDetailsWriter, returns false for a call to CanWrite.
As an example, the DefaultProblemDetailsWriter will not be able to write when the request contains a Accept header without application/json or application/problem+json.
Proposed API
namespace Microsoft.AspNetCore.Http;
public interface IProblemDetailsService
{
ValueTask WriteAsync(ProblemDetailsContext context);
+ async ValueTask<bool> TryWriteAsync(ProblemDetailsContext context)
+ {
+ await WriteAsync(context);
+ return true;
+ }
}Usage Examples
var problemDetailsService = httpContext.RequestServices.GetService<IProblemDetailsService>();
if (problemDetailsService == null ||
!await problemDetailsService.TryWriteAsync(new() { HttpContext = httpContext }))
{
// Your fallback behavior, since problem details was not able to be written.
}Alternative Designs
public interface IProblemDetailsService
{
ValueTask WriteAsync(ProblemDetailsContext context);
+ bool CanWrite(ProblemDetailsContext context);
}Example
var problemDetailsService = httpContext.RequestServices.GetService<IProblemDetailsService>();
var context = new ProblemDetailsContext(){ HttpContext = httpContext };
if (problemDetailsService != null && problemDetailsService.CanWrite(context))
{
await problemDetailsService.WriteAsync(context)
}Risks
To be consistent with the Parse/TryParse behavior, as part of this proposal we should change our DefaultProblemDetailsWriter to throw in WriteAsync when not able to write. If this behavior change is too bad, maybe we could introduce some flag to the ProblemDetailsOptions where users can tweak the behavior.
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
- The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
- The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
- Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.
- ValueTask TryWriteAsync(ProblemDetailsContext context, out bool result)
Async APIs can't have out parameters.
One other alternative: IProblemDetailsService.CanWrite(ProblemDetailsContext ) that mirrors IProblemDetailsWriter
Async APIs can't have out parameters.
I proposed it while writing the API suggestion and I completely missed this. Thanks
API Review Notes:
- We should
return context.HttpContext.Response.HasStarted;by default. - Do we prefer
CanWrite()orTryWrite()? There's no good default implementationCanWrite(). However, we don't think there are many third-party implementations yet, but @DamianEdwards wrote one and filed the bug 😆
The TryWrite API looks good!
namespace Microsoft.AspNetCore.Http;
public interface IProblemDetailsService
{
ValueTask WriteAsync(ProblemDetailsContext context);
+ async ValueTask<bool> TryWriteAsync(ProblemDetailsContext context)
+ {
+ await WriteAsync(context);
+ return context.HttpContext.Response.HasStarted;
+ }
}Is there a way to fix this behavior in .NET 7?
Is there a way to fix this behavior in .NET 7?
We don't typically backport bug fixes that include a new API. @DamianEdwards was there a workaround for this other than running the app from the CLI when you ran into it?
The behavior in the browser refresh middleware injected by Visual Studio was fixed in 7.0.2xx back in November 2022 so I'd ensure you're running the latest version of Visual Studio and confirm you have the 7.0.202 SDK installed. If the behavior is still observed it could be due to another middleware in the pipeline doing "bad things".
We're definitely not backporting new API, but it should be possible to work around this issue.
The issue happens when HttpResponse.Body is buffered and not flushed before UseStatusCodePages tries to look at it. That's what was happening with the hot reload script injection logic which originally triggered this issue before it was reverted. E.g.:
// StatusCodePages would work here because the original stream was put back
// before it attempts to write and chedk context.Resposne.HasStarted.
//app.UseStatusCodePages();
// Example buffering middleware. Thanks Bing Chat!
app.Use(async (context, next) =>
{
var bodyStream = context.Response.Body;
try
{
using var bufferStream = new MemoryStream();
context.Response.Body = bufferStream;
await next();
bufferStream.Seek(0, SeekOrigin.Begin);
await bufferStream.CopyToAsync(bodyStream);
}
finally
{
context.Response.Body = bodyStream;
}
});
// StatusCodePages breaks here because context.Response.Body is a MemoryStream
// that does not pass through writes to the response until after UseSatatCodePages exits.
app.UseStatusCodePages();The fix is to call app.UseStatusCodePages() before adding the buffering middleware if you don't need the status code page to be buffered as well.
If you just need to log the status code pages, but not delay the response, you can pass through the writes like our HttpLogging ResponseBufferingStream does rather than waiting for middleware to call CopyToAsync(). Or maybe just use that middleware since it's available in .NET 7. https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-logging/?view=aspnetcore-7.0
Worst case scenario, you could override IHttpResponseFeature.HasStarted, but that would be a lot more involved.
Moving UseStatusCodePages() and UseExceptionHandler() to the top was the first thing I have tried. But it did not work until I updated Visual Studio (I was on the latest minor but not the patch version) and the SDK. Now everything works correctly.
Thanks. Both answers were very helpful.