-
Notifications
You must be signed in to change notification settings - Fork 10.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change DefaultFiles/DirectoryBrowser/StaticFileMiddleware to only no-op if there's an active endpoint with a non-null RequestDelegate #42413
Comments
@Tratcher @davidfowl @JamesNK any thoughts if this would cause any issues? |
Could RequestDelegate ever be null before? |
|
Sure, but in practice where there any situations where it was null that we would now handle differently? |
Not that I'm aware of, but that's impossible to prove absolutely. Is your question basically "Are there cases where the middleware would now start handling the request where before it didn't?" |
Right. If a null RequestDelegate was common for some situation before, those requests could now collide with StaticFiles. If this wasn't a pattern we saw in use before then the change is harmless. |
I'll let the others chime in on that then, given I only found out today that |
I am supportive of this change. It opens up something we're trying to do with endpoints and policies. An endpoint without a request delegate is pretty rare which is why I feel comfortable taking this one. It is a breaking change though, so maybe its worth documenting anyways. |
Do we feel it warrants an appcontext switch (I'm guessing... no)? |
Triage - we don't think it needs one. If we can get this in for preview7 without breaking anybody, we say go ahead |
…est delegate (#42458) * Allow file middlewares to run if there's an endpoint with a null request delegate #42413 * Ensure EndpointMiddleware checks AuthZ/CORS metadata even if endpoint.RequestDelegate is null * Update null equality check to use "is" * Added tests for StaticFileMiddleware changes * Add tests for DefaultFiles/DirectoryBrowser middleware * Add endpoint middleware test
The
DefaultFilesMiddleware
,DirectoryBrowserMiddleware
, andStaticFileMiddleware
currently no-op (fall through to the next middleware in the pipeline) if they detect the request has an active endpoint (see code here).This prevents the middleware from running even in cases where there's an endpoint with a
null
RequestDelegate
. This prevents the ability to have an active endpoint for the purposes of communicating metadata to middleware while allowing middleware like theStaticFileMiddleware
to keep performing its function, e.g. have a "dummy" endpoint to carryIAuthorizeData
metadata so that theAuthorizationMiddleware
performs authorization on a request before theStaticFileMiddleware
runs (example here).We should consider updating the check in these middleware to instead only no-op if the active endpoint's request delegate is null, i.e.:
If there are other middleware with similar behavior we should consider updating those in a similar fashion.
The text was updated successfully, but these errors were encountered: