Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions src/Antiforgery/src/AntiforgeryMiddleware.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Http.Abstractions.Metadata;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Antiforgery;

internal sealed partial class AntiforgeryMiddleware
{
private readonly IAntiforgery _antiforgery;
private readonly RequestDelegate _next;
private readonly ILogger<AntiforgeryMiddleware> _logger;

public AntiforgeryMiddleware(IAntiforgery antiforgery, RequestDelegate next, ILogger<AntiforgeryMiddleware> logger)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this middleware important enough to be included in security check in EndpointMiddleware?

if (endpoint.Metadata.GetMetadata<IAuthorizeData>() != null &&
!httpContext.Items.ContainsKey(AuthorizationMiddlewareInvokedKey))
{
ThrowMissingAuthMiddlewareException(endpoint);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's on my list of things to do. We also need to add a StartupAnalyzer that suggests ordering this after Routing and AuthZ middlewares.

{
_antiforgery = antiforgery;
_next = next;
_logger = logger;
}

public Task Invoke(HttpContext context)
{
var endpoint = context.GetEndpoint();
if (endpoint is null)
{
return _next(context);
}

var antiforgeryMetadata = endpoint.Metadata.GetMetadata<IAntiforgeryMetadata>();
if (antiforgeryMetadata is null)
{
Log.NoAntiforgeryMetadataFound(_logger);
return _next(context);
}

if (antiforgeryMetadata is not IValidateAntiforgeryMetadata validateAntiforgeryMetadata)
{
Log.IgnoreAntiforgeryMetadataFound(_logger);
return _next(context);
}

if (_antiforgery is DefaultAntiforgery defaultAntiforgery)
{
var valueTask = defaultAntiforgery.TryValidateAsync(context, validateAntiforgeryMetadata.ValidateIdempotentRequests);
if (valueTask.IsCompletedSuccessfully)
{
var (success, message) = valueTask.GetAwaiter().GetResult();
if (success)
{
Log.AntiforgeryValidationSucceeded(_logger);
return _next(context);
}
else
{
Log.AntiforgeryValidationFailed(_logger, message);
return WriteAntiforgeryInvalidResponseAsync(context, message);
}
}

return TryValidateAsyncAwaited(context, valueTask);
}
else
{
return ValidateNonDefaultAntiforgery(context);
}
}

private async Task TryValidateAsyncAwaited(HttpContext context, ValueTask<(bool success, string? message)> tryValidateTask)
{
var (success, message) = await tryValidateTask;
if (success)
{
Log.AntiforgeryValidationSucceeded(_logger);
await _next(context);
}
else
{
Log.AntiforgeryValidationFailed(_logger, message);
await context.Response.WriteAsJsonAsync(new ProblemDetails
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this doesn't set the status code to 400 or call WriteAntiforgeryInvalidResponseAsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops

{
Status = StatusCodes.Status400BadRequest,
Title = "Antiforgery validation failed",
Detail = message,
});
}
}

private async Task ValidateNonDefaultAntiforgery(HttpContext context)
{
if (await _antiforgery.IsRequestValidAsync(context))
{
Log.AntiforgeryValidationSucceeded(_logger);
await _next(context);
}
else
{
Log.AntiforgeryValidationFailed(_logger, message: null);
await WriteAntiforgeryInvalidResponseAsync(context, message: null);
}
}

private static Task WriteAntiforgeryInvalidResponseAsync(HttpContext context, string? message)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed like doing a ProblemDetails would be a possible thing to do here. The current filter in MVC simply returns a status code but it felt like we could offer more details.

The interesting bit to note is that these requests should have primarily been triggered by a browser POST so returning JSON is a bit unusual.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems wrong to be forcibly returning JSON here. Why wouldn't we just let it throw or throw BadHttpRequestException or something else to enable the app to handle how this responds to the client?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or make it an event.

{
context.Response.StatusCode = StatusCodes.Status400BadRequest;
return context.Response.WriteAsJsonAsync(new ProblemDetails
{
Status = StatusCodes.Status400BadRequest,
Title = "Antiforgery validation failed",
Detail = message,
});
}

private static partial class Log
{
[LoggerMessage(1, LogLevel.Debug, "No antiforgery metadata found on the endpoint.", EventName = "NoAntiforgeryMetadataFound")]
public static partial void NoAntiforgeryMetadataFound(ILogger logger);

[LoggerMessage(2, LogLevel.Debug, $"Antiforgery validation suppressed on endpoint because {nameof(IValidateAntiforgeryMetadata)} was not found.", EventName = "IgnoreAntiforgeryMetadataFound")]
public static partial void IgnoreAntiforgeryMetadataFound(ILogger logger);

[LoggerMessage(3, LogLevel.Debug, "Antiforgery validation completed successfully.", EventName = "AntiforgeryValidationSucceeded")]
public static partial void AntiforgeryValidationSucceeded(ILogger logger);

[LoggerMessage(4, LogLevel.Debug, "Antiforgery validation failed with message '{message}'.", EventName = "AntiforgeryValidationFailed")]
public static partial void AntiforgeryValidationFailed(ILogger logger, string? message);
}
}
22 changes: 22 additions & 0 deletions src/Antiforgery/src/AntiforgeryMiddlewareExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using Microsoft.AspNetCore.Antiforgery;
using Microsoft.AspNetCore.Cors.Infrastructure;

namespace Microsoft.AspNetCore.Builder;

/// <summary>
/// The <see cref="IApplicationBuilder"/> extensions for adding Antiforgery middleware support.
/// </summary>
public static class AntiforgeryMiddlewareExtensions
{
/// <summary>
/// Adds the Antiforgery middleware to the middleware pipeline.
/// </summary>
/// <param name="app">The <see cref="IApplicationBuilder"/>.</param>
/// <returns>A reference to the <paramref name="app"/> after the operation has completed.</returns>
public static IApplicationBuilder UseAntiforgery(this IApplicationBuilder app)
=> app.UseMiddleware<AntiforgeryMiddleware>();
}
24 changes: 15 additions & 9 deletions src/Antiforgery/src/Internal/DefaultAntiforgery.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
Expand Down Expand Up @@ -100,35 +98,43 @@ public async Task<bool> IsRequestValidAsync(HttpContext httpContext)
throw new ArgumentNullException(nameof(httpContext));
}

var (result, _) = await TryValidateAsync(httpContext, validateIdempotentRequests: false);
return result;
}

internal async ValueTask<(bool success, string? errorMessage)> TryValidateAsync(HttpContext httpContext, bool validateIdempotentRequests)
{
CheckSSLConfig(httpContext);

var method = httpContext.Request.Method;
if (HttpMethods.IsGet(method) ||
if (
!validateIdempotentRequests &&
(HttpMethods.IsGet(method) ||
HttpMethods.IsHead(method) ||
HttpMethods.IsOptions(method) ||
HttpMethods.IsTrace(method))
HttpMethods.IsTrace(method)))
{
// Validation not needed for these request types.
return true;
return (true, null);
}

var tokens = await _tokenStore.GetRequestTokensAsync(httpContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something should catch the AntiforgeryValidationException and convert it to a 400.

try
{
form = await httpContext.Request.ReadFormAsync();
}
catch (InvalidDataException ex)
{
// ReadFormAsync can throw InvalidDataException if the form content is malformed.
// Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure.
throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex);
}
catch (IOException ex)
{
// Reading the request body (which happens as part of ReadFromAsync) may throw an exception if a client disconnects.
// Wrap it in an AntiforgeryValidationException and allow the caller to handle it as just another antiforgery failure.
throw new AntiforgeryValidationException(Resources.AntiforgeryToken_UnableToReadRequest, ex);
}

if (tokens.CookieToken == null)
{
_logger.MissingCookieToken(_options.Cookie.Name);
return false;
return (false, "Missing cookie token");
}

if (tokens.RequestToken == null)
{
_logger.MissingRequestToken(_options.FormFieldName, _options.HeaderName);
return false;
return (false, "Antiforgery token could not be found in the HTTP request.");
}

// Extract cookie & request tokens
if (!TryDeserializeTokens(httpContext, tokens, out var deserializedCookieToken, out var deserializedRequestToken))
{
return false;
return (false, "Unable to deserialize antiforgery tokens");
}

// Validate
Expand All @@ -147,7 +153,7 @@ public async Task<bool> IsRequestValidAsync(HttpContext httpContext)
_logger.ValidationFailed(message!);
}

return result;
return (result, message);
}

/// <inheritdoc />
Expand Down
2 changes: 2 additions & 0 deletions src/Antiforgery/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
#nullable enable
Microsoft.AspNetCore.Builder.AntiforgeryMiddlewareExtensions
static Microsoft.AspNetCore.Builder.AntiforgeryMiddlewareExtensions.UseAntiforgery(this Microsoft.AspNetCore.Builder.IApplicationBuilder! app) -> Microsoft.AspNetCore.Builder.IApplicationBuilder!
11 changes: 11 additions & 0 deletions src/Http/Http.Abstractions/src/Metadata/IAntiforgeryMetadata.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Http.Abstractions.Metadata;

/// <summary>
/// A marker interface which can be used to identify Antiforgery metadata.
/// </summary>
public interface IAntiforgeryMetadata
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

namespace Microsoft.AspNetCore.Http.Abstractions.Metadata;

/// <summary>
/// A marker interface which can be used to identify a resource with Antiforgery validation enabled.
/// </summary>
public interface IValidateAntiforgeryMetadata : IAntiforgeryMetadata
{
/// <summary>
/// Gets a value that determines if idempotent HTTP methods (<c>GET</c>, <c>HEAD</c>, <c>OPTIONS</c> and <c>TRACE</c>) are validated.
/// </summary>
bool ValidateIdempotentRequests { get; }
}
3 changes: 3 additions & 0 deletions src/Http/Http.Abstractions/src/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
#nullable enable
*REMOVED*abstract Microsoft.AspNetCore.Http.HttpResponse.ContentType.get -> string!
Microsoft.AspNetCore.Http.Abstractions.Metadata.IAntiforgeryMetadata
Microsoft.AspNetCore.Http.Abstractions.Metadata.IValidateAntiforgeryMetadata
Microsoft.AspNetCore.Http.Abstractions.Metadata.IValidateAntiforgeryMetadata.ValidateIdempotentRequests.get -> bool
abstract Microsoft.AspNetCore.Http.HttpResponse.ContentType.get -> string?
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Linq;
using Microsoft.AspNetCore.Http.Abstractions.Metadata;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Mvc.ApplicationModels;

/// <summary>
/// An <see cref="IApplicationModelProvider"/> that removes antiforgery filters that appears as endpoint metadata.
/// </summary>
internal sealed class AntiforgeryApplicationModelProvider : IApplicationModelProvider
{
private readonly MvcOptions _mvcOptions;

public AntiforgeryApplicationModelProvider(IOptions<MvcOptions> mvcOptions)
{
_mvcOptions = mvcOptions.Value;
}

// Run late in the pipeline so that we can pick up user configured AntiforgeryTokens.
public int Order { get; } = 1000;

public void OnProvidersExecuted(ApplicationModelProviderContext context)
{
}

public void OnProvidersExecuting(ApplicationModelProviderContext context)
{
if (!_mvcOptions.EnableEndpointRouting)
{
return;
}

foreach (var controller in context.Result.Controllers)
{
RemoveAntiforgeryFilters(controller.Filters, controller.Selectors);

foreach (var action in controller.Actions)
{
RemoveAntiforgeryFilters(action.Filters, action.Selectors);
}
}
}

private static void RemoveAntiforgeryFilters(IList<IFilterMetadata> filters, IList<SelectorModel> selectorModels)
{
for (var i = filters.Count - 1; i >= 0; i--)
{
if (filters[i] is IAntiforgeryMetadata antiforgeryMetadata &&
selectorModels.All(s => s.EndpointMetadata.Contains(antiforgeryMetadata)))
{
filters.RemoveAt(i);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ internal static void AddMvcCoreServices(IServiceCollection services)
services.TryAddSingleton<ApplicationModelFactory>();
services.TryAddEnumerable(
ServiceDescriptor.Transient<IApplicationModelProvider, DefaultApplicationModelProvider>());
services.TryAddEnumerable(
ServiceDescriptor.Transient<IApplicationModelProvider, AntiforgeryApplicationModelProvider>());
services.TryAddEnumerable(
ServiceDescriptor.Transient<IApplicationModelProvider, ApiBehaviorApplicationModelProvider>());
services.TryAddEnumerable(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#nullable enable

using System;
using Microsoft.AspNetCore.Http.Abstractions.Metadata;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc.ViewFeatures.Filters;
using Microsoft.Extensions.DependencyInjection;
Expand All @@ -21,7 +22,7 @@ namespace Microsoft.AspNetCore.Mvc;
/// a controller or action.
/// </remarks>
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
public class AutoValidateAntiforgeryTokenAttribute : Attribute, IFilterFactory, IOrderedFilter
public class AutoValidateAntiforgeryTokenAttribute : Attribute, IFilterFactory, IOrderedFilter, IValidateAntiforgeryMetadata
{
/// <summary>
/// Gets the order value for determining the order of execution of filters. Filters execute in
Expand All @@ -44,6 +45,8 @@ public class AutoValidateAntiforgeryTokenAttribute : Attribute, IFilterFactory,
/// <inheritdoc />
public bool IsReusable => true;

bool IValidateAntiforgeryMetadata.ValidateIdempotentRequests => false;

/// <inheritdoc />
public IFilterMetadata CreateInstance(IServiceProvider serviceProvider)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;

namespace Microsoft.AspNetCore.Mvc.ViewFeatures.Filters;

internal class AutoValidateAntiforgeryTokenAuthorizationFilter : ValidateAntiforgeryTokenAuthorizationFilter
{
public AutoValidateAntiforgeryTokenAuthorizationFilter(IAntiforgery antiforgery, ILoggerFactory loggerFactory)
: base(antiforgery, loggerFactory)
public AutoValidateAntiforgeryTokenAuthorizationFilter(IAntiforgery antiforgery, ILoggerFactory loggerFactory, IOptions<MvcOptions> mvcOptions)
: base(antiforgery, loggerFactory, mvcOptions)
{
}

Expand Down
Loading