Align Fetch-Metadata CSRF trust with CORS credentials intent - #69345
DeagleGross wants to merge 1 commit into
Conversation
DeagleGross
changed the title
DeagleGross
added
the
Servicing-consider
|
Hi @DeagleGross. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Existing integration tests must be updated to match the new credential requirement.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 1
Open (1)
What changed in this PR
This pull request restricts CORS-derived CSRF trust to policies that explicitly allow credentials.
Changes:
- Requires
SupportsCredentialsfor CORS-based CSRF trust. - Adds credentialed and non-credentialed policy test coverage.
| File | Summary |
|---|---|
src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/DefaultCsrfProtectionTests.cs |
Expands coverage across CORS policy configurations. |
src/DefaultBuilder/src/Internal/DefaultCsrfProtection.cs |
Requires credential support when deriving CSRF trust from CORS. Existing integration tests still expect non-credentialed policies to be trusted. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
1b006a5 to
4685f52
Compare
Per feedback: the AllowCredentials() requirement for CORS-derived CSRF trust is unreleased (dotnet/aspnetcore#69345 is still open), so it shouldn't be documented as a breaking change or a 10->11 migration concern yet. - Removed breaking-changes/11/csrf-cors-allowcredentials-required.md - Reverted the registration in breaking-changes/11/overview.md and toc.yml - Reverted the migration/100-to-110/includes/security.md CORS bullet - Removed the breaking-change cross-link from anti-request-forgery.md, keeping the AllowCredentials() requirement description, updated CORS code sample, and AllowAnyOrigin note Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
I think the functional change is sound, but we should probably add more testing and update some more existing tests to include AllowCredentials().
| builder.WebHost.UseTestServer(); | ||
| builder.Services.AddCors(options => | ||
| options.AddDefaultPolicy(policy => policy.WithOrigins("https://trusted.example.com"))); | ||
| options.AddDefaultPolicy(policy => policy.WithOrigins("https://trusted.example.com").AllowCredentials())); |
There was a problem hiding this comment.
I think we need to addAllowCredentials() to a few more tests to ensure they still cover what they're supposed to. CsrfProtection_PerEndpointEnableCors_OverridesDefaultPolicy now passes even if the default policy is incorrectly selected, and CsrfProtection_PerEndpointDisableCors_FallsThroughToSecFetchSite passes even if [DisableCors] is ignored.
| // AllowCredentials() is required, so request is not allowed | ||
| var services = BuildCorsServices(o => o.AddDefaultPolicy(p => p.WithOrigins("https://trusted.com"))); | ||
| var context = CreateContext(origin: "https://trusted.com", secFetchSite: "cross-site", services: services); | ||
| Assert.False((await _validator.ValidateAsync(context)).IsAllowed); |
There was a problem hiding this comment.
I think we should cover this through a [FromForm] endpoint in an integration test too: a matching cross-origin request, with and without AllowCredentials(), for both anonymous and authenticated users (which should receive the same CSRF validation result). We should probably also cover an endpoint policy without credentials overriding a credential-enabled default policy for the same origin, so we don't accidentally inherit the default policy's trust.
DefaultCsrfProtectiontreats a matched CORS policy's allowed origin as a trust signal for Fetch-Metadata-based CSRF checks. Today it does this whenever!policy.AllowAnyOrigin && policy.IsOriginAllowed(origin)is true, without checkingpolicy.SupportsCredentials(the property backing.AllowCredentials()).This change adds that check, so a CORS policy only expands CSRF trust when it was also configured with
.AllowCredentials().Why is requiring
AllowCredentials()fine?CORS's
AllowCredentials()(which setspolicy.SupportsCredentials) is the one explicit flag a developer sets when they mean "this cross-origin caller is allowed to send/receive the user's cookies." If a developer did not set it, they were saying "this origin can call me anonymously/publicly," not "this origin can act as the logged-in user."Since CSRF protection is entirely about authenticated, cookie-bearing actions being triggered from elsewhere, only the credentialed case is actually claiming the trust relationship CSRF protection cares about. Requiring
SupportsCredentialsbefore letting a CORS policy grant CSRF trust makes the code check what the developer actually intended, instead of trusting origins that were only ever configured for anonymous/public access.Docs change if this is merged: dotnet/AspNetCore.Docs#37660