dotnet / dotnet/aspnetcore

CORS middleware applies a stale policy decision after pipeline re-execution

Open
#68,614 0 comments 0 reactions 0 assignees View on GitHub
area-middleware feature-cors
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.