search

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.49k stars 10.04k forks source link

Configurable ClearCacheHeaders() method on ExceptionHandlerMiddleware.cs #18683

Closed gyphysics closed 4 years ago

gyphysics commented 4 years ago

I've been trying to move from some custom middleware for exception handling to using the handler defined by the framework [https://github.com/dotnet/aspnetcore/blob/master/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs]

Unfortunately I've had to abandon this effort due to the way the handler alters the response headers before sending. The application is designed to handle data inside PCI-DSS scope and so we are mandated to have Cache-Control headers, which include among other things "no-store". Although I set these headers using OnStarting() the middleware class overrides it.

Can this set of headers be made configurable? I want to specify my on cache-control values.

Tratcher commented 4 years ago

We should probably just add the no-store value here. @gyphysics are there any other values you'd expect to set here? https://github.com/dotnet/aspnetcore/blob/6f285dce1dba363cf108abb179a54638153c5fc6/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs#L156

While we're at it, don't use -1 for expires, use the epoc date. https://github.com/dotnet/aspnetcore/blob/6f285dce1dba363cf108abb179a54638153c5fc6/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs#L158

gyphysics commented 4 years ago

@Tratcher The specific unit test asserts that this project is performing are these: Cache-Control - "no-store", "no-cache", "max-age=0", "must-revalidate" Pragma - "no-cache" Expires - "0"

I'm not sure if updating the settings so that they are designed to work with my specific project is the best thing to do for a shared library though - these cache settings could cause problems for other consumers, there doesn't seem to be a consensus on the value for Expires, for example, and new security headers may be required in the future.

The fact that the library wiped all of the cache-control headers no matter what I did seemed to breach the POLA and allowing the consumer some control/configurability on this may be preferable.

analogrelay commented 4 years ago

Two proposals here:

gyphysics commented 4 years ago 
* Allow the content of the `Cache-Control` header to be set as a string property on `ExceptionHandlerOptions`.

I would also want to be able to control the value set in other headers, e.g. Pragma and Expires

Tratcher commented 4 years ago

I would also want to be able to control the value set in other headers, e.g. Pragma and Expires

Set to what? The point is that error pages aren't supposed to be cachable. I don't see any other defined values for Pragma, and it seems like the only point in changing Expires would be to enable caching.

gyphysics commented 4 years ago

Set to whatever I have directed the values to be set to within the response's OnStarting() event.

Currently Expires is set to -1, and as part of this change it is proposed to use an epoch date.

Our existing code sets Expires to 0 for all responses that we deem to be non-cachable - including errors - and that is what our integration tests look for on relevant response outputs. I don't want to be trying to decide which of the 3 values will be returned in tests and having values I set in OnStarting() overridden is frustrating.

JunTaoLuo commented 4 years ago

We set the Cache-Control header to no-cache,no-store now. As for the other proposals here, I don't see why they would be needed given that we don't want to cache the error page. no-cache,no-store should be sufficient for that purpose.