[WIP] feat: Sec-Fetch-* based Antiforgery#65887
[WIP] feat: Sec-Fetch-* based Antiforgery#65887DeagleGross wants to merge 6 commits intodotnet:mainfrom
Conversation
…Antiforgery/UseTokenBasedAntiforgery
|
Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at. |
# Conflicts: # src/Antiforgery/src/AntiforgeryServiceCollectionExtensions.cs # src/Antiforgery/src/Microsoft.AspNetCore.Antiforgery.csproj
There was a problem hiding this comment.
Pull request overview
Implements an in-design update to ASP.NET Core Antiforgery to incorporate Fetch Metadata–based cross-origin validation (Sec-Fetch-Site / Origin) and introduces middleware/service registration for cross-origin-only, token-only, and combined (cross-origin first, token fallback) validation.
Changes:
- Add
ICrossOriginAntiforgery+ options/result types and an internalCrossOriginRequestValidatorimplementation. - Update the default
AntiforgeryMiddlewareto run cross-origin validation first and fall back to token validation when inconclusive. - Add
UseTokenBasedAntiforgery/UseCrossOriginAntiforgerypipeline options and expand test coverage for the new behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Antiforgery/src/AntiforgeryMiddleware.cs | Default middleware now does cross-origin validation before token fallback. |
| src/Antiforgery/src/Internal/CrossOriginRequestValidator.cs | Implements the Sec-Fetch-Site / Origin-based validation algorithm. |
| src/Antiforgery/src/CrossOrigin/ICrossOriginAntiforgery.cs | New public interface for cross-origin validation. |
| src/Antiforgery/src/CrossOrigin/CrossOriginValidationResult.cs | New public enum describing allowed/denied/unknown outcomes. |
| src/Antiforgery/src/CrossOrigin/CrossOriginAntiforgeryOptions.cs | New public options container for trusted origins. |
| src/Antiforgery/src/CrossOriginAntiforgeryMiddleware.cs | New “cross-origin-only (strict)” middleware. |
| src/Antiforgery/src/TokenBasedAntiforgeryMiddleware.cs | New “token-only” middleware for legacy behavior. |
| src/Antiforgery/src/AntiforgeryApplicationBuilderExtensions.cs | Adds UseTokenBasedAntiforgery and UseCrossOriginAntiforgery. |
| src/Antiforgery/src/AntiforgeryServiceCollectionExtensions.cs | Adds cross-origin registrations and new overloads. |
| src/Antiforgery/src/PublicAPI.Unshipped.txt | Declares the newly introduced public surface area. |
| src/Antiforgery/src/Microsoft.AspNetCore.Antiforgery.csproj | Minor formatting-only change. |
| src/Antiforgery/test/AntiforgeryMiddlewareTest.cs | Updates tests for new constructor + adds cross-origin behavior tests. |
| src/Antiforgery/test/CrossOriginAntiforgeryMiddlewareTest.cs | New tests for cross-origin-only middleware behavior. |
| src/Antiforgery/test/AntiforgeryApplicationBuilderExtensionsTest.cs | Updates pipeline tests to include cross-origin dependency. |
| public static IServiceCollection AddCrossOriginAntiforgery(this IServiceCollection services) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(services); | ||
|
|
There was a problem hiding this comment.
AddCrossOriginAntiforgery() registers CrossOriginRequestValidator, which requires IOptions. If a consumer only calls AddCrossOriginAntiforgery() (without other features that add Options), DI may fail to resolve IOptions<...>; consider ensuring Options services are added here (e.g., services.AddOptions()) to make this registration self-contained.
| services.AddOptions(); |
|
|
||
| public CrossOriginValidationResult Validate(HttpContext context) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(context); | ||
|
|
||
| var request = context.Request; |
There was a problem hiding this comment.
ICrossOriginAntiforgery is a public API, but CrossOriginRequestValidator.Validate() does not account for safe methods (GET/HEAD/OPTIONS/TRACE) and can return Denied purely based on headers. Either incorporate the safe-method allowance into Validate() or clarify in the interface/docs that the caller is expected to pre-filter to only the methods/endpoints that require antiforgery validation.
| public CrossOriginValidationResult Validate(HttpContext context) | |
| { | |
| ArgumentNullException.ThrowIfNull(context); | |
| var request = context.Request; | |
| /// <summary> | |
| /// Validates the specified HTTP context for potential cross-origin attacks. | |
| /// </summary> | |
| /// <param name="context">The <see cref="HttpContext"/> associated with the current request.</param> | |
| /// <returns> | |
| /// A <see cref="CrossOriginValidationResult"/> value indicating whether the request is allowed, | |
| /// denied, or requires additional validation. | |
| /// </returns> | |
| /// <remarks> | |
| /// This method always treats HTTP safe methods (<c>GET</c>, <c>HEAD</c>, <c>OPTIONS</c>, and <c>TRACE</c>) | |
| /// as allowed and does not apply cross-origin antiforgery checks to them. | |
| /// </remarks> | |
| /// <example> | |
| /// <code> | |
| /// var result = crossOriginAntiforgery.Validate(httpContext); | |
| /// if (result == CrossOriginValidationResult.Denied) | |
| /// { | |
| /// // Handle denied cross-origin request | |
| /// } | |
| /// </code> | |
| /// </example> | |
| public CrossOriginValidationResult Validate(HttpContext context) | |
| { | |
| ArgumentNullException.ThrowIfNull(context); | |
| var request = context.Request; | |
| if (HttpMethods.IsGet(request.Method) || | |
| HttpMethods.IsHead(request.Method) || | |
| HttpMethods.IsOptions(request.Method) || | |
| HttpMethods.IsTrace(request.Method)) | |
| { | |
| return CrossOriginValidationResult.Allowed; | |
| } |
| public async Task SkipsValidation_WhenEndpointHasIgnoreMetadata() | ||
| { | ||
| var crossOrigin = new Mock<ICrossOriginAntiforgery>(); | ||
| var middleware = new CrossOriginAntiforgeryMiddleware(crossOrigin.Object, hc => Task.CompletedTask); | ||
| var httpContext = GetHttpContext(hasIgnoreMetadata: true); |
There was a problem hiding this comment.
SkipsValidation_WhenEndpointHasIgnoreMetadata doesn't actually configure an endpoint with IAntiforgeryMetadata where RequiresValidation=false (GetHttpContext() uses an empty EndpointMetadataCollection when hasIgnoreMetadata=true). This test currently only verifies the 'no antiforgery metadata' case; consider adding AntiforgeryMetadata(false) so the ignore-metadata behavior is exercised.
| services.AddSingleton(antiforgeryService); | ||
| var crossOriginMock = new Mock<ICrossOriginAntiforgery>(); | ||
| crossOriginMock.Setup(c => c.Validate(It.IsAny<HttpContext>())).Returns(CrossOriginValidationResult.Unknown); | ||
| services.AddSingleton(crossOriginMock.Object); | ||
| } |
There was a problem hiding this comment.
CreateServices() always registers an ICrossOriginAntiforgery when IAntiforgery is present, which can mask the current behavior where UseAntiforgery() doesn't validate that ICrossOriginAntiforgery is registered. Adding a test case with IAntiforgery registered but without ICrossOriginAntiforgery would catch the runtime DI failure (or verify the improved guard if added).
| services.TryAddSingleton<IAntiforgery, DefaultAntiforgery>(); | ||
| services.TryAddSingleton<IAntiforgeryTokenGenerator, DefaultAntiforgeryTokenGenerator>(); | ||
| services.TryAddSingleton<IAntiforgeryTokenSerializer, DefaultAntiforgeryTokenSerializer>(); | ||
| services.TryAddSingleton<IAntiforgeryTokenStore, DefaultAntiforgeryTokenStore>(); | ||
| services.TryAddSingleton<IClaimUidExtractor, DefaultClaimUidExtractor>(); |
There was a problem hiding this comment.
AddAntiforgery(Action) no longer registers the ObjectPool services needed by DefaultAntiforgeryTokenSerializer and DefaultClaimUidExtractor (both depend on ObjectPool). This will cause DI activation failures at runtime; re-add the ObjectPoolProvider and the ObjectPool registrations (as previously done) when registering token-based antiforgery services.
| public static IServiceCollection AddAntiforgery(this IServiceCollection services, Action<AntiforgeryOptions> setupAction) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(services); | ||
| ArgumentNullException.ThrowIfNull(setupAction); | ||
|
|
||
| services.AddDataProtection(); | ||
|
|
||
| // Don't overwrite any options setups that a user may have added. | ||
| services.TryAddEnumerable( |
There was a problem hiding this comment.
The common overload AddAntiforgery(Action) does not register ICrossOriginAntiforgery, but AntiforgeryMiddleware now requires it in the constructor. Apps using the existing AddAntiforgery(options => ...) + UseAntiforgery() pattern will fail at runtime; this overload should also register the cross-origin services (or UseAntiforgery should fall back to a token-only middleware when cross-origin services are absent).
| public static IApplicationBuilder UseTokenBasedAntiforgery(this IApplicationBuilder builder) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(builder); | ||
| builder.VerifyAntiforgeryServicesAreRegistered(); | ||
|
|
There was a problem hiding this comment.
UseAntiforgery() still only checks for IAntiforgery, but AntiforgeryMiddleware now also depends on ICrossOriginAntiforgery. Consider updating the UseAntiforgery() verification logic (similar to VerifyCrossOriginAntiforgeryServicesAreRegistered) so the error is thrown at pipeline setup time with an actionable message.
| static Microsoft.Extensions.DependencyInjection.AntiforgeryServiceCollectionExtensions.AddAntiforgery(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services, System.Action<Microsoft.AspNetCore.Antiforgery.AntiforgeryOptions!>! setupAction, System.Action<Microsoft.AspNetCore.Antiforgery.CrossOrigin.CrossOriginAntiforgeryOptions!>! crossOriginSetupAction) -> Microsoft.Extensions.DependencyInjection.IServiceCollection! | ||
| static Microsoft.Extensions.DependencyInjection.AntiforgeryServiceCollectionExtensions.AddCrossOriginAntiforgery(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection! |
There was a problem hiding this comment.
PublicAPI.Unshipped.txt lists an AddAntiforgery(IServiceCollection, Action) overload, but the code adds AddCrossOriginAntiforgery(...) instead and no such AddAntiforgery overload exists. Update the public API baseline to match the implemented method signature(s) (or add the missing overload if intended).
[IN DESIGN]
Fixes #65127