Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add metadata types for form reading and mapping options #49844

Closed
captainsafia opened this issue Aug 3, 2023 · 2 comments · Fixed by #49935
Closed

Add metadata types for form reading and mapping options #49844

captainsafia opened this issue Aug 3, 2023 · 2 comments · Fixed by #49935
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Aug 3, 2023

Background and Motivation

To support full parity between the existing anti-forgery support in MVC and the new anti-forgery middleware, we need to support a metadata-based approach for configuring limits to be used when reading from a form. Also, we need to introduce a mapping options type to support configuring the binding experience for forms.

Proposed API

// Assembly: Microsoft.AspNetCore.Http.Abstractions;
namespace Microsoft.AspNetCore.Http.Metadata;

public class FormMappingOptionsMetadata(int maxCollectionSize, int maxRecursionDepth, int maxKeySize)
{
    public int MaxCollectionSize { get; } = maxCollectionSize;
    public int MaxRecursionDepth { get; } = maxRecursionDepth;
    public int MaxKeyBufferSize { get; } = maxKeySize;
}
// Assembly: Microsoft.AspNetCore.Http.Abstractions;
namespace Microsoft.AspNetCore.Http.Metadata;

public interface IRequestFormLimitsMetadata
{
    bool BufferBody { get; set; }
    int MemoryBufferThreshold { get; set; }
    long BufferBodyLengthLimit { get; set; }
    int ValueCountLimit { get; set; }
    int KeyLengthLimit { get; set; }
    int ValueLengthLimit { get; set; }
    int MultipartBoundaryLengthLimit { get; set; }
    int MultipartHeadersCountLimit { get; set; }
    int MultipartHeadersLengthLimit { get; set; }
    long MultipartBodyLengthLimit { get; set; }
}
// Assembly: Microsoft.AspNetCore.Routing;
namespace Microsoft.AspNetCore.Builder;

public static class RoutingEndpointConventionBuilderExtensions
{
  public static TBuilder WithFormMappingOptions<TBuilder>(this TBuilder builder,
    int maxCollectionSize = FormDataReader.DefaultValueCountLimit, // 1024
    int maxRecursionDepth = 64, // Same as STJ default
    int maxKeySize = FormReader.DefaultKeyLengthLimit /* 1024 * 2 */) where TBuilder : IEndpointConventionBuilder { }

  // Defaults map to pre-existing values throughout framework
  public static TBuilder WithRequestFormLimits<TBuilder>(this TBuilder builder,
    bool bufferBody = false,
    int memoryBufferThreshold = DefaultMemoryBufferThreshold, // 1024 * 64
    long bufferBodyLengthLimit = DefaultBufferBodyLengthLimit, // 1024 * 1024 * 128
    int valueCountLimit = FormReader.DefaultValueCountLimit, // 1024
    int keyLengthLimit = FormReader.DefaultKeyLengthLimit, // 1024 * 2
    int valueLengthLimit = FormReader.DefaultValueLengthLimit, // 1024 * 1024 * 4,
    int multipartBoundaryLengthLimit = DefaultMultipartBoundaryLengthLimit, // 128
    int multipartHeadersCountLimit = MultipartReader.DefaultHeadersCountLimit, // 16
    int multipartHeaderLengthLimit = MultipartReader.DefaultHeadersLengthLimit, // 1024 * 16,
    int multipartBodyLengthLimit = DefaultMultipartBodyLengthLimit, /* 1024 * 1024 * 128 */) where TBuilder : IEndpointConventionBuilder { }
}
// Assembly: Microsoft.AspNetCore.Mvc.Core;
namespace Microsoft.AspNetCore.Mvc;

- public class RequestFormLimitsAttribute : Attribute, IFilterFactory, IOrderedFilter {}
+ public class RequestFormLimitsAttribute : Attribute, IFilterFactory, IOrderedFilter, IRequestFormLimitsMetadata {}

Usage Examples

var app = WebApplication.Create();

app.MapPost("/todo-1", ([FromForm] Todo todo) => todo)
  .WithFormMappingOptions(10, 10, 2);

app.MapPost("/todo-2", ([FromForm] Todo todo) => todo)
  .WithFormMappingOptions(maxRecursionDepth: 5);

app.MapPost("/todo-3", ([FromForm] Todo todo) => todo)
  .WithFormMappingOptions(maxRecursionDepth: 5)
  .WithRequestFormLimits(valueCountLimit: 4);

var todos = app.MapGroup("/todos").WithRequestFormLimits(bufferBody: true);

todos.MapPost("/4", ([FromForm] Todo todo) => todo)
  .WithRequestFormLimits(valueLengthLimit: 1024, multipartHeadersCountLimit: 4)

app.Run();

Alternative Designs

For .NET 8, we're intentionally moving with the model of having separate form mapping options types for Blazor and minimal APIs, even though they have the same form mapping infrastructure. The goal is to unify these options types in .NET 9 into a new assembly, but for now, the FormMappingOptionsMetadata is defined only as a concrete implementation and not an interface since we don't anticipate evolving it further in .NET 8 before we unify the two types.

Risks

There are risks and costs to action.

@captainsafia captainsafia added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 3, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Aug 3, 2023
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Aug 3, 2023
@ghost
Copy link

ghost commented Aug 3, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@captainsafia captainsafia self-assigned this Aug 3, 2023
@captainsafia captainsafia added this to the 8.0-rc1 milestone Aug 3, 2023
@halter73
Copy link
Member

halter73 commented Aug 7, 2023

API Review Notes:

  • Why is MaxFormMappingErrorCount missing from FormMappingOptionsMetadata even though it's in RazorComponentOptions?
    • We always throw for the first error rather than display each error like in Blazor.
  • What about the name MaxKeyBufferSize?
    • FormOptions uses KeyLengthLimit. To support a large enough form key you need to configure both, but these refer to different keys. The former is in the form itself. The latter is in the collection that the form was mapped to.
    • Let's rename MaxKeyBufferSize to MaxKeySize for consistency with BlazorComponentsOptions.
  • Do we prefer ctor parameters or init; for FormMappingOptionsMetadata?
    • We haven't used init; too much for metadata yet (if at all)
    • Let's use the constructor like we do for things like EndpointNameMetadata.
  • Is FormMappingOptionsMetadata actually endpoint metadata?
    • Yes.
  • Does IRequestFormLimitsMetadata need setters?
    • No. It can get getter-only like IFromBodyMetadata and similar.
    • It's a setter so you can use [RequestFormLimits] with minimal route handlers.
  • What if you want to override only some values using WithRequestFormLimits?
    • Let's make the limits nullable everywhere.
  • IRequestFormLimitsMetadata -> IFormOptionsMetadata
    • WithRequestFormLimits -> WithFormOptions
      • WithFormParsingOptions might make it clearer what's different from WithFormMappingOptions, but we don't think it's necessary.
  • What if someone tries changing HttpContext.SetEndpoint(endpoint)? Anything applied inside routing middleware will not have a chance to rerun.
    • We'll have to figure this out. We don't think this happens often or at all today. At least not to anything we'd enforce in the routing middleware like request body size limits.

API Approved!

// Assembly: Microsoft.AspNetCore.Http.Abstractions;
namespace Microsoft.AspNetCore.Http.Metadata;

public class FormMappingOptionsMetadata(int? maxCollectionSize = null, int? maxRecursionDepth = null, int? maxKeySize = null)
{
    public int? MaxCollectionSize { get; } = maxCollectionSize;
    public int? MaxRecursionDepth { get; } = maxRecursionDepth;
    public int? MaxKeySize { get; } = maxKeySize;
}

public interface IFormOptionsMetadata
{
    bool? BufferBody { get; }
    int? MemoryBufferThreshold { get; }
    long? BufferBodyLengthLimit { get; }
    int? ValueCountLimit { get; }
    int? KeyLengthLimit { get; }
    int? ValueLengthLimit { get; }
    int? MultipartBoundaryLengthLimit { get; }
    int? MultipartHeadersCountLimit { get; }
    int? MultipartHeadersLengthLimit { get; }
    long? MultipartBodyLengthLimit { get; }
}

// Assembly: Microsoft.AspNetCore.Routing;
namespace Microsoft.AspNetCore.Builder;

public static class RoutingEndpointConventionBuilderExtensions
{
  public static TBuilder WithFormMappingOptions<TBuilder>(this TBuilder builder,
    int? maxCollectionSize = null,
    int? maxRecursionDepth = null,
    int? maxKeySize = null) where TBuilder : IEndpointConventionBuilder { }

  // Defaults map to pre-existing values throughout framework
  public static TBuilder WithFormOptions<TBuilder>(this TBuilder builder,
    bool? bufferBody = null,
    int? memoryBufferThreshold = null,
    long? bufferBodyLengthLimit = null,
    int? valueCountLimit = null,
    int? keyLengthLimit = null,
    int? valueLengthLimit =  = null,
    int? multipartBoundaryLengthLimit = null,
    int? multipartHeadersCountLimit = null,
    int? multipartHeaderLengthLimit = null,
    int? multipartBodyLengthLimit = null) where TBuilder : IEndpointConventionBuilder { }
}
// Assembly: Microsoft.AspNetCore.Mvc.Core;
namespace Microsoft.AspNetCore.Mvc;

- public class RequestFormLimitsAttribute : Attribute, IFilterFactory, IOrderedFilter {}
+ public class RequestFormLimitsAttribute : Attribute, IFilterFactory, IOrderedFilter, IFormOptionsMetadata {}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 7, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants