CORS middleware applies a stale policy decision after pipeline re-execution
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 276
Description
## Summary
`CorsMiddleware` binds its CORS decision to the endpoint observed at evaluation time, but applies that decision at response-start. When the pipeline re-executes against a different endpoint on the same `HttpResponse`, the earlier endpoint's decision can be the one written to the response. The invariant that a response's `Access-Control-*` headers reflect the endpoint that produced it does not hold.
## What is wrong
**Broken invariant:** the `Access-Control-*` headers on a response must reflect the CORS configuration of the endpoint that produced that response. Today they can reflect a different endpoint's configuration.
The behavior emerges from three interacting mechanisms, none of which is wrong on its own:
1. `CorsMiddleware` defers header application to `HttpResponse.OnStarting`, capturing an immutable `CorsResult` in the callback state. This deferral is deliberate — it was introduced to resolve #2378 so that CORS headers survive an error handler calling `Response.Clear()`.
2. `HttpResponse.OnStarting` callbacks run last-registered-first. This is a documented contract, implemented consistently by Kestrel, IIS in-process, HTTP.sys, and `TestServer`.
3. Diagnostics re-execution (`UseStatusCodePagesWithReExecute`, `UseExceptionHandler`) clears the endpoint and re-invokes the pipeline on the same response. It does not unregister previously registered `OnStarting` callbacks — and cannot, since `IHttpResponseFeature` exposes no removal API.
Because `CorsMiddleware` has no re-entrancy guard, it registers one callback per evaluation pass. Under the documented ordering, the first pass's callback is applied last and is therefore the final writer of `Access-Control-Allow-Origin`.
Two related properties compound this:
* Applying a **denying** result is a no-op. `CorsService.ApplyResult` returns before touching `response.Headers`, so a later, more restrictive evaluation cannot correct an earlier permissive one. The net semantics are a union of both passes rather than an intersection.
* `Vary` is **appended** rather than assigned, so it is emitted twice when two passes both allow and the policy sets `VaryByOrigin`.
**Scope note — what is intentionally not changing:** when the re-execution target declares no CORS policy at all, headers continue to be inherited from the earlier evaluation. That is the deliberate behavior from #2378 and must be preserved.
## Why it matters (defense in depth)
* **Correctness, independent of any attacker.** `RequireCors(...)` and `[DisableCors]` on a re-execution target are not honored on the re-executed response. An endpoint explicitly annotated `[DisableCors]` can still emit another endpoint's `Access-Control-Allow-Origin` and `Access-Control-Allow-Credentials`. Configuration that a developer deliberately wrote is silently inert on this path.
* **Hardening.** This strengthens per-endpoint CORS scoping so that a grant computed for one endpoint cannot widen the reachability of a different endpoint's response. The framework should not emit headers that contradict the declared policy of the endpoint that produced the body.
* **Cache correctness.** A duplicated `Vary: Origin` is undesirable for intermediaries and for response/output caching.
* **No coverage today.** Neither the CORS nor the Diagnostics test suite exercises CORS across pipeline re-execution.
## Affected code
* `src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs:168-191` — `EvaluateAndApplyPolicy`; registers an `OnStarting` callback per evaluation at line 188
* `src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs:193-206` — `OnResponseStarting`; applies the captured `CorsResult` without re-reading the endpoint
* `src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs:116-130` — `IDisableCorsAttribute` branch; returns without registering a callback
* `src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs:170-174` — no-policy branch; returns without registering a callback (the #2378 case)
* `src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs:176-185` — preflight path; applies synchronously and short-circuits, so it is unaffected
* `src/Middleware/CORS/src/Infrastructure/CorsService.cs:157-162` — denial returns before writing headers and removes nothing
* `src/Middleware/CORS/src/Infrastructure/CorsService.cs:203-206` — `Vary` is appended rather than assigned
* `src/Http/Http.Abstractions/src/HttpResponse.cs:80-91` — documents the last-registered-first ordering contract
* `src/Http/Http.Features/src/IHttpResponseFeature.cs:9-69` — no API to unregister a callback; constrains the fix design
* `src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesExtensions.cs:215-281` — re-execution path; does not call `Response.Clear()`
* `src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddlewareImpl.cs:160-175` and `:285-300` — re-execution path; registers its own `OnStarting` after `Response.Clear()`
* `src/Shared/HttpExtensions.cs:49-62` — `ClearEndpoint` clears only the endpoint and route values
* `src/Middleware/CORS/test/UnitTests/CorsMiddlewareTests.cs:567` — existing `CorsRequest_SetsResponseHeader_IfExceptionHandlerClearsResponse`, which locks in #2378
## Recommended fix
**Selected approach — one `OnStarting` registration per request, backed by a mutable per-request holder, with last evaluation winning.**
`EvaluateAndApplyPolicy` stores the computed `CorsResult` in a small mutable holder kept in `HttpContext.Items` under a private `__`-prefixed key, matching the convention already used in this file for `CorsMiddlewareWithEndpointInvokedKey`. `Response.OnStarting` is registered only when the holder is created; subsequent passes overwrite the stored result. `OnResponseStarting` reads the holder's current value and no-ops when it is null. This collapses N registrations to one, which addresses both the stale-decision problem and the duplicated `Vary` at their common root, and it aligns the middleware with `CorsAuthorizationFilter`, which already applies results synchronously so that the last pass wins.
Two non-evaluating exits must be handled by a mutate-if-present helper that never creates a holder and never registers a callback:
* The `IDisableCorsAttribute` branch **clears** the stored result. This asymmetry is load-bearing, not an optimization: that branch never produces a `CorsResult` to overwrite, so a plain last-wins holder would still leave `[DisableCors]` targets emitting the earlier pass's headers.
* The `corsPolicy == null` branch **must not clear** — that is precisely the #2378 case.
The helper must not call `GetPolicyAsync` or `EvaluatePolicy`; `Invoke_HasEndpointWithEnableMetadata_HasSignificantDisableCors_ExecutesNextMiddleware` asserts both are never invoked. The preflight path is unchanged.
**Alternatives considered and rejected:**
* *Re-resolve the policy inside `OnResponseStarting`.* Both re-execution paths dispose an `AsyncServiceScope` and restore `RequestServices` in a `finally`, so `ICorsPolicyProvider.GetPolicyAsync` is not safe to call at response-start. It also re-enters user code at header-flush time.
* *Make a denial actively remove previously written headers.* Ineffective on its own: under last-registered-first ordering the earlier allowing callback runs afterwards and re-adds them. `ApplyResult` is also `public virtual` on `CorsService` and declared on `ICorsService`, so changing its semantics is a behavior break for derived implementations.
* *Have the diagnostics middlewares signal re-execution to CORS.* `IStatusCodeReExecuteFeature` is cleared in a `finally` while `IExceptionHandlerFeature` is not, so the two flows behave inconsistently, and any third-party re-executing middleware would be missed entirely.
* *Change server callback ordering.* The ordering is a documented contract on `HttpResponse.OnStarting` and is implemented consistently across all servers; changing one server would break the contract and still leave the others unchanged.
**Compatibility, migration, and versioning:**
* No public API change. `PublicAPI.Unshipped.txt` is untouched, so no API review or baseline update is required.
* The behavior change is confined to requests where the pipeline re-executes *and* the two passes resolve different CORS outcomes. Applications whose re-execution target declares no CORS policy see no change, preserving #2378.
* Targets `main` (11.0) only. **No servicing backport is planned.**
* Worth a short note in the CORS documentation, which currently does not mention error handling or pipeline re-execution anywhere.
## Acceptance criteria
* [ ] The `Access-Control-*` headers on a re-executed response equal those on a direct request to the re-execution target, except where the target declares no CORS policy.
* [ ] A re-execution target annotated `[DisableCors]` emits no `Access-Control-*` headers.
* [ ] A re-execution target whose policy does not allow the request origin emits no `Access-Control-*` headers.
* [ ] A re-execution target with no CORS policy continues to inherit the earlier evaluation's headers, and `CorsRequest_SetsResponseHeader_IfExceptionHandlerClearsResponse` still passes.
* [ ] `Vary: Origin` is emitted at most once.
* [ ] Exactly one `OnStarting` callback is registered per request regardless of how many evaluation passes occur.
* [ ] Coverage for both `UseStatusCodePagesWithReExecute` and `UseExceptionHandler`.
* [ ] Preflight behavior is unchanged.
* [ ] Compatibility expectation documented: header inheritance is retained when the target declares no policy.
**Test-authoring notes for the implementer:**
* Assert the **absence** of `Access-Control-Allow-Origin` and `Access-Control-Allow-Credentials`, not merely that a value changed. Because applying a denying result is a no-op, a "value changed" assertion would pass on a broken build.
* Any `Vary` assertion must use a policy that actually sets `VaryByOrigin` (`Origins.Count > 1 || !IsDefaultIsOriginAllowed`). A single-origin policy emits no `Vary` at all, which would make such a test vacuous.
* `Access-Control-Expose-Headers` is a useful ordering probe: it is assigned rather than appended, so two allowing policies exposing different headers make the winning callback directly observable.
* `Microsoft.AspNetCore.Cors.Test.csproj` references neither `Microsoft.AspNetCore.Diagnostics` nor `Microsoft.AspNetCore.Routing`. Either add the references or simulate re-execution with plain `app.Use`, which is what the existing #2378 test does.
* `DefaultHttpContext`'s response feature treats `OnStarting` as a no-op, so counting registrations requires a purpose-built `IHttpResponseFeature` test double.
* Existing `TestServer` tests in this file use strict `Assert.Single(response.Headers)` / `Assert.Collection(...)` assertions that will fail on any extra or duplicated header.
Contributor guide
Research direction
Start with EvaluateAndApplyPolicy and OnResponseStarting in src/Middleware/CORS/src/Infrastructure/CorsMiddleware.cs, then read the existing #2378 test in src/Middleware/CORS/test/UnitTests/CorsMiddlewareTests.cs. Run the CORS middleware tests and add coverage for StatusCodePagesWithReExecute and ExceptionHandler re-execution. Done means the final headers match the re-execution target, no-policy inheritance remains, Vary is not duplicated, and preflight behavior is unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 52/100