Rerun PostRoutingPipeline on Rerouting#67618
Conversation
…nmiddleware + extensive rerouting test suite
There was a problem hiding this comment.
Pull request overview
This PR adjusts how the framework’s deferred “post-routing” pipeline (implicit auth/authz/CSRF middleware) is applied so it also runs correctly when the request pipeline is re-executed via rerouting (e.g., UseStatusCodePagesWithReExecute’s reroute branch). It also tightens CSRF marker stamping to only occur when an endpoint opts into antiforgery metadata, and expands integration test coverage for rerouting-related scenarios.
Changes:
- Stop consuming/removing the
__Internal_PostRoutingPipelineapplication property so reroute-built routing branches can still observe and apply the framework’s post-routing pipeline. - Update
CsrfProtectionMiddlewareto early-return unless the matched endpoint hasIAntiforgeryMetadata, reducingHttpContext.Itemsallocations on the non-antiforgery path. - Add routing/rerouting-focused CSRF integration tests (and required Rewrite assembly reference).
Show a summary per file
| File | Description |
|---|---|
| src/Http/Routing/src/EndpointRoutingMiddleware.cs | Keeps the framework post-routing pipeline available so it can be applied again in reroute-created routing branches. |
| src/DefaultBuilder/src/Internal/CsrfProtectionMiddleware.cs | Only stamps the CSRF sentinel when the matched endpoint has antiforgery metadata; avoids work on endpoint-less / non-antiforgery paths. |
| src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/Microsoft.AspNetCore.Tests.csproj | Adds Microsoft.AspNetCore.Rewrite reference for new test coverage using UseRewriter. |
| src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/CsrfProtectionIntegrationTests.cs | Adds integration tests for CSRF marker/endpoint behavior across rerouting and pipeline variants. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 2
| } No newline at end of file | ||
|
|
||
| [Fact] | ||
| public async Task Repro_BlazorTemplatePipeline_CsrfMarkerIsStampedBeforeEndpoint_OnReroute() |
There was a problem hiding this comment.
This doesn't look like it reroutes. The request to / returns 200, so StatusCodePages never re-executes /not-found.
There was a problem hiding this comment.
it does not "reroute", but calls app.UseStatusCodePagesWithReExecute() which essentially adds the following to the pipeline:
return app.Use(next =>
{
var newNext = RerouteHelper.Reroute(app, routeBuilder, next);
return new StatusCodePagesMiddleware(next,
Options.Create(new StatusCodePagesOptions()
{
HandleAsync = CreateHandler(pathFormat, queryFormat, newNext),
CreateScopeForStatusCodePages = createScopeForStatusCodePages,
PathFormat = pathFormat
})).Invoke;
});and in the previous code was causing problems - it happens after routing always, and has no PostRoutingPipeline in the applicationBuilder.Properties, meaning that it was not invoking CSRF on "reroute". I am sure this test must exist - do you think we should change it anyhow?
renamed to Repro_BlazorTemplatePipeline_CsrfMarkerIsStampedBeforeEndpoint_WithStatusCodePagesMiddleware to not bring confusion.
| } | ||
|
|
||
| [Fact] | ||
| public async Task CsrfProtection_UseStatusCodePagesWithReExecute_CsrfMarkerStampedOnReroutedRequest() |
There was a problem hiding this comment.
Can this cover the CORS metadata case too? The original concern was ValidateAsync running before the re-executed endpoint's RequireCors metadata was available, and this only checks antiforgery metadata.
There was a problem hiding this comment.
Added test which touches CORS and verifies it runs. The flow is the following:
-
EndpointRoutingMiddlewareruns, does not find match and setscontext.Endpoint = null -
PostRoutingPipelinewith CSRF just skips CSRF run -
EndpointMiddlewaredoes not see endpoint either (null), and skips -
StatusCodePagesMiddlewarestarts reroute because it sees 404- Path is set to
not-foundnow, so it matches onEndpointRoutingMiddlewarenow - It triggers
PostRoutingPipeline, which callsCsrfProtectionMiddlewarewhich now sees endpoint, and calls CORS policy resolve - It resolves
ICorsMetadata, and which callsICorsPolicyProviderand it resolves the policy
- Path is set to
CORS knows how to match if the context.GetEndpoint() is not null. Since we now skip if (endpoint is null) { return ... } in CSRF, then its safe.
As found out in #67588 and #67589, there are blazor template test which repros the bug with CsrfProtectionMiddleware used with
UseStatusCodePagesWithReExecuteon the same app. I reproduced it inCsrfProtectionIntegrationTestsand am fixing in current PR.The major change is to start reruning PostRoutingPipeline on each reroute - technically, that is a breaking change, but is safer and more explanative.
Related #67588