dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.19k stars 9.93k forks source link

Response caching and output caching do not cache headers modified in OnStarting handler #39310

Open lawrencek76 opened 2 years ago

lawrencek76 commented 2 years ago

Is there an existing issue for this?

Describe the bug

Response caching does not cache headers modified in the OnStarting delegate in middleware loaded after responsecaching middleware.

context.Response.OnStarting(CheckIfHeaderNeeded, context);

private Task CheckIfHeaderNeeded(object state)
    {
        var context = (HttpContext)state;
        context.Response.Headers.Remove("removeme");
        return Task.CompletedTask;
    }

revmoveme header will only be removed on the initial request

Expected Behavior

Response caching should cache the actual final headers sent to the client. It cannot cache only headers created before the await(next) in middleware.

This will also fix #23218 if I understand the issue correctly.

@Tratcher mentioned one possible fix is

One fix would be for the caching middleware to shim OnStarting and run those callbacks before it captured the headers. (Complex)

That seems to be the correct behavior

Steps To Reproduce

https://github.com/lawrencek76/header-caching-issue

Exceptions (if any)

No response

.NET Version

6.0.101

Anything else?

Initially ran into this trying to cache security headers with a nonce in another middleware. https://github.com/andrewlock/NetEscapades.AspNetCore.SecurityHeaders/issues/117

After attempting to make a pr to fix this in the other middleware I realized there is really no way I can find to really do this correctly outside of the caching middleware itself.

https://github.com/andrewlock/NetEscapades.AspNetCore.SecurityHeaders/pull/118

Tratcher commented 2 years ago

One challenge with changing this is that response caching reads the current response headers to decide if it should cache, and then starts intercepting writes (or not). This will mean also delaying the decision to cache (or not) until after the first write completes.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 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.

ghost commented 1 year ago

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.

adityamandaleeka commented 1 year ago

Triage: may be interesting to fix this in output caching. Consider capturing headers after the first write rather than during (or maybe in middleware unwind).