Add API to specify antiforgery request token source

[MaceWindu]

Background and Motivation

#51912 (comment)

Proposed API

namespace Microsoft.AspNetCore.Antiforgery;

public class AntiforgeryOptions
{
+        public AntiforgeryRequestTokenSource RequestTokenSource { get; set; }
}

+[Flags]
+public enum AntiforgeryRequestTokenSource
+{
+        Header = 1,
+        FormBody = 2,
+        HeaderOrFormBody = Header | FormBody
+}

public class RequireAntiforgeryTokenAttribute : Attribute, IAntiforgeryMetadata
{
+        public AntiforgeryRequestTokenSource RequestTokenSource { get; init; }
}

Usage Examples

services.AddAntiforgery(static options =>
{
    options.RequestTokenSource = AntiforgeryRequestTokenSource.Header
});

[RequireAntiforgeryToken(RequestTokenSource = AntiforgeryRequestTokenSource.Header)]
public class MyController
{
    [HttpPost]
    public IActionResult Post() { ... }

    [HttpPost]
    [RequireAntiforgeryToken(RequestTokenSource = AntiforgeryRequestTokenSource.FormBody)]
    public IActionResult Post2() { ... }
}

Alternative Designs

Risks

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.

[halter73]

API Review Notes:

• Should a flags enum have a None member? That will be the default value regardless.
• Is AntiforgeryRequestTokenSource.FormBody useful? Or do people just want to suppress looking in the body?
  • The upside of something like bool SuppressFormBody is that people will be less likely to misconfigure it.
  • The upside of the flags enum is that the API itself documents what all the antiforgery sources are, and is more extensible.
  • We could argue this is a power user API that not many people will see.
  • The primary scenario is not reading potentially large forms with file uploads when you know the token will not be in the body.
• We like the idea of just disabling with a bool. What should we name it?
  • SuppressFormBodySource
  • SuppressReadingTokenFromFormBody
  • OnlyReadTokenFromHeader
  • SuppressReadingRequestTokenFromFormBody
  • We decided on SuppressReadingTokenFromFormBody
• Do we need to update IAntiforgeryMetadata? Yes.
• Do we like init; over set; for the attribute? Yes.

namespace Microsoft.AspNetCore.Antiforgery;

public class AntiforgeryOptions
{
+        public bool SuppressReadingTokenFromFormBody { get; set; }
}

public class RequireAntiforgeryTokenAttribute : Attribute, IAntiforgeryMetadata
{
+        // Unset means use AntiforgeryOptions.SuppressReadingTokenFromFormBody
+        public bool? SuppressReadingTokenFromFormBody { get; init; }
}

public interface IAntiforgeryMetadata
{
+        public bool? SuppressReadingTokenFromFormBody => null;
}

[MaceWindu]

I like this change, but bool? type is not supported by attributes

[MaceWindu]

Also I'm not sure IAntiforgeryMetadata needs this information

[halter73]

I like this change, but bool? type is not supported by attributes

Oops. @BrennanConroy pointed out we made this mistake before when we were designing HttpLoggingAttribute.ResponseBodyLogLimit . We had to update

public int? ResponseBodyLogLimit { get; }

to be

public bool IsResponseBodyLogLimitSet { get; }
public int ResponseBodyLogLimit { get; set; }

The equivalent here would be:

public class RequireAntiforgeryTokenAttribute : Attribute, IAntiforgeryMetadata
{
+       // Unset means use AntiforgeryOptions.SuppressReadingTokenFromFormBody
+       public bool IsSuppressReadingTokenFromFormBodySet { get; }
+       public bool SuppressReadingTokenFromFormBody { get; init; }
}

The interface property could remain nullable, and the implementation in the attribute would be explicit.

IAntiforgeryMetadata.SuppressReadingTokenFromFormBody =>
    IsSuppressReadingTokenFromFormBodySet
    ? SuppressReadingTokenFromFormBody
    : null;

The problem with just leaving it as bool SuppressReadingTokenFromFormBody is that there would be no way to tell the difference between [RequireAntiforgeryToken(SuppressReadingTokenFromFormBody = false)] and [RequireAntiforgeryToken] using public API. I think developers will expect the local attribute to override a global AntiforgeryOptions.SuppressReadingTokenFromFormBody = true configuration if the property is set, but certainly not [RequireAntiforgeryToken].

If we use a flags enum like the originally proposed AntiforgeryRequestTokenSource, we could make Default = 0 and basically treat that as null or unconfigured. It's more natural to make None = 0 in most flags enums, but since having no token source configured is nonsensical, Default = 0 might make more sense. And it does get rid of the ugly extra IsSuppressReadingTokenFromFormBodySet property.

The current approach in #51962 of having the DefaultAntiforgeryTokenStore use internal APIs to determine whether or not the property is set is undesirable because it means no one could write a fully custom implementation that correctly handles the metadata.

Also I'm not sure IAntiforgeryMetadata needs this information

While I agree it's not strictly required, it seems natural this would be included with the metadata already present on the same attribute indicating whether validation is required or not. Why shouldn't it be included in IAntiforgeryMetadata?

@javiercn Do you have any opinions about this API?

This will need to go back to API review to discuss which approach we want to take. We should get back to you after our next meeting on December 7th. Thanks for your patience.

P.S. If we stick with a bool instead of a flags enum, I wonder if we should use "Disable" instead of "Suppress" in the property name(s).