Allow user to specify antiforgery request token source #51962

MaceWindu · 2023-11-09T13:15:55Z

Allow user to specify antiforgery request token source

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Currently default antiforgery implementation looks for antiforgery request token in header and when it is not found looks for token in request body (only for form requests).

This PR adds global and endpoint-level option to specify where to look for token. This is especialy useful when you want to prevent default antiforgery from reading request body as it could lead to:

performance issues on large requests (forms with file uploads)
request body stream cannot be read twice and will result in failure from custom request parser

Fixes #51912

Note that I've added both global and endpoint-level options. If we decide to keep only one, I will remove second.

ghost · 2023-11-09T13:16:02Z

Thanks for your PR, @MaceWindu. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

MaceWindu · 2023-11-09T13:18:04Z

src/Antiforgery/test/DefaultAntiforgeryTokenStoreTest.cs

@@ -237,6 +329,7 @@ public async Task GetRequestTokens_ReadFormAsyncThrowsIOException_ThrowsAntiforg
        var ioException = new IOException();
        var httpContext = new Mock<HttpContext>();

+        httpContext.Setup(r => r.Features).Returns(new FeatureCollection());


Mock context doesn't have Features initialized. It is now needed to endpoint-level source configuration. ~~Some mocked tests in other projects could probably fail for same reason as I run tests only for Antiforgery project~~

MaceWindu · 2023-11-09T13:19:39Z

src/Antiforgery/src/RequireAntiforgeryTokenAttribute.cs

 [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
-public class RequireAntiforgeryTokenAttribute(bool required = true) : Attribute, IAntiforgeryMetadata
+public class RequireAntiforgeryTokenAttribute(bool required = true, AntiforgeryRequestTokenSource? requestTokenSource = null) : Attribute, IAntiforgeryMetadata
 {
    /// <summary>
    /// Gets or sets a value indicating whether the antiforgery token should be validated.


BTW, it looks like documentation bug to me as there is no property setter to make it "Gets or sets"

mkArtakMSFT · 2023-11-13T19:29:23Z

Thanks for your PR, @MaceWindu.
@halter73 can you please review this? Thanks!

src/Antiforgery/src/RequireAntiforgeryTokenAttribute.cs

javiercn

Overall the change looks good, but needs more integration tests. It is hard to asses the correctness of the change based on the tests in DefaultAntiforgeryTokenStore and this kind of change warrants some tests here

We need to make sure we cover the following combos:

HeaderOrBody (Token present / Token not present) -> (should already be covered by existing tests, point them on the PR if possible).
HeaderOnly (Token present / Token not present) -> This is the new behavior, so we need to get a clear understanding that the implementation matches our expectations.
- We should only check the header during validation, but nothing should change for generation.
Body only (Token present / Token not present) -> This should match the current semantics and skip the header altogether.

This also needs to go through API review @halter73.

MaceWindu · 2023-11-14T11:36:02Z

It is hard to asses the correctness of the change based on the tests in DefaultAntiforgeryTokenStore and this kind of change warrants some tests here

Could you provide more details on expected tests? I don't see how AntiforgeryMiddlewareTest related to this change as it tests how AntiforgeryMiddleware interacts with IAntiforgery interface using mocks and don't use any code from this PR. All tests for affected functionality as I can see located in DefaultAntiforgeryTokenStoreTest

HeaderOrBody (Token present / Token not present) -> (should already be covered by existing tests, point them on the PR if possible).

with token: GetRequestTokens_HeaderTokenTakensPriority_OverFormToken
without: GetRequestTokens_BothHeaderValueAndFormFieldsEmpty_ReturnsNullTokens

HeaderOnly (Token present / Token not present) -> This is the new behavior, so we need to get a clear understanding that the implementation matches our expectations.

with token: added GetRequestTokens_FormTokenDisabled_ReturnsHeaderToken
without: GetRequestTokens_FormTokenDisabled_ReturnsNullToken

We should only check the header during validation, but nothing should change for generation.

what kind of test you expect here?

Body only (Token present / Token not present) -> This should match the current semantics and skip the header altogether.
added GetRequestTokens_FormTokenDisabled_ReturnsHeaderToken test

with token: GetRequestTokens_HeaderTokenDisabled_FallsBackToFormToken, added GetRequestTokens_HeaderTokenDisabled_ReturnsFormToken
without: added GetRequestTokens_HeaderTokenDisabled_ReturnsNull

ghost · 2023-11-21T15:00:31Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

halter73 · 2023-11-21T17:27:36Z

This also needs to go through API review @halter73.

@MaceWindu do you mind filling out and submitting the API proposal issue template for the new public AntiforgeryRequestTokenSource flags enum and the corresponding RequestTokenSource properties on AntiforgeryOptions and RequireAntiforgeryTokenAttribute? You can look at api-approved labeled issues for more examples.

halter73 · 2023-11-21T17:28:17Z

src/Antiforgery/src/RequireAntiforgeryTokenAttribute.cs

+    /// Gets or sets a value indicating the locations to look for the antiforgery token in the request.
+    /// If not set, <see cref="AntiforgeryOptions.RequestTokenSource"/> value used.
+    /// </summary>
+    public AntiforgeryRequestTokenSource RequestTokenSource


Should we just make this nullable so anyone can determine the difference between unconfigured and explicitly HeaderOrFormBody? Then we can go back to just using auto properties.

That was initial design, but such types are not supported by metadata

MaceWindu · 2023-11-22T08:46:18Z

@halter73 done #52281

halter73 · 2023-11-30T01:14:41Z

src/Antiforgery/src/Internal/DefaultAntiforgeryTokenStore.cs

@@ -39,16 +39,19 @@ public async Task<AntiforgeryTokenSet> GetRequestTokensAsync(HttpContext httpCon

        var cookieToken = httpContext.Request.Cookies[_options.Cookie.Name!];

+        // endpoint could override global request token source option
+        var tokenSource = httpContext.GetEndpoint()?.Metadata.GetMetadata<RequireAntiforgeryTokenAttribute>()?.RequestTokenSourceValue ?? _options.RequestTokenSource;


What if you have something like this?

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

Based on this line, it looks like it will resolve the most specific RequireAntiforgeryTokenAttribute which would be the one on the Post() method and see that the RequestTokenSourceValue == null and fall back to _options.RequestTokenSource which is HeaderOrFormBody when it ought to be using Header as specified by the attribute on MyController.

I will look into this scenario

MaceWindu · 2024-01-01T20:21:25Z

@halter73, code updated to latest API design

halter73

Thank you @MaceWindu!

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 9, 2023

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 Nov 9, 2023

MaceWindu mentioned this pull request Nov 9, 2023

Add option to disable form field as CSRF token source from AntiforgeryOptions #51912

Closed

1 task

MaceWindu commented Nov 9, 2023

View reviewed changes

martincostello added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-mvc-antiforgery and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Nov 9, 2023

mkArtakMSFT assigned javiercn Nov 13, 2023

mkArtakMSFT added area-ui-rendering Includes: MVC Views/Pages, Razor Views/Pages and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Nov 13, 2023

mkArtakMSFT assigned halter73 and unassigned javiercn Nov 13, 2023

javiercn reviewed Nov 14, 2023

View reviewed changes

src/Antiforgery/src/RequireAntiforgeryTokenAttribute.cs Outdated Show resolved Hide resolved

javiercn reviewed Nov 14, 2023

View reviewed changes

src/Antiforgery/src/RequireAntiforgeryTokenAttribute.cs Outdated Show resolved Hide resolved

javiercn reviewed Nov 14, 2023

View reviewed changes

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Nov 21, 2023

halter73 reviewed Nov 21, 2023

View reviewed changes

halter73 mentioned this pull request Nov 30, 2023

Add API to specify antiforgery request token source #52281

Closed

halter73 reviewed Nov 30, 2023

View reviewed changes

Add token from body suppression option

086de76

halter73 approved these changes Jan 8, 2024

View reviewed changes

halter73 enabled auto-merge (squash) January 8, 2024 23:30

halter73 merged commit dca5b04 into dotnet:main Jan 9, 2024
26 checks passed

ghost added this to the 9.0-preview1 milestone Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow user to specify antiforgery request token source #51962

Allow user to specify antiforgery request token source #51962

MaceWindu commented Nov 9, 2023

ghost commented Nov 9, 2023

MaceWindu Nov 9, 2023 •

edited

Loading

MaceWindu Nov 9, 2023

mkArtakMSFT commented Nov 13, 2023

javiercn left a comment •

edited

Loading

MaceWindu commented Nov 14, 2023

ghost commented Nov 21, 2023

halter73 commented Nov 21, 2023

halter73 Nov 21, 2023

MaceWindu Nov 22, 2023

MaceWindu commented Nov 22, 2023

halter73 Nov 30, 2023

MaceWindu Nov 30, 2023

MaceWindu commented Jan 1, 2024

halter73 left a comment

Allow user to specify antiforgery request token source #51962

Allow user to specify antiforgery request token source #51962

Conversation

MaceWindu commented Nov 9, 2023

Allow user to specify antiforgery request token source

Description

ghost commented Nov 9, 2023

MaceWindu Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

MaceWindu Nov 9, 2023

Choose a reason for hiding this comment

mkArtakMSFT commented Nov 13, 2023

javiercn left a comment • edited Loading

Choose a reason for hiding this comment

MaceWindu commented Nov 14, 2023

ghost commented Nov 21, 2023

halter73 commented Nov 21, 2023

halter73 Nov 21, 2023

Choose a reason for hiding this comment

MaceWindu Nov 22, 2023

Choose a reason for hiding this comment

MaceWindu commented Nov 22, 2023

halter73 Nov 30, 2023

Choose a reason for hiding this comment

MaceWindu Nov 30, 2023

Choose a reason for hiding this comment

MaceWindu commented Jan 1, 2024

halter73 left a comment

Choose a reason for hiding this comment

MaceWindu Nov 9, 2023 •

edited

Loading

javiercn left a comment •

edited

Loading