Skip to content

Commit ad35fbf

Browse files
captainsafiacaptainsafia
committed
Address more feedback
1 parent afba012 commit ad35fbf

13 files changed

+97
-89
lines changed

‎src/Antiforgery/src/AntiforgeryMiddleware.cs‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -28,26 +28,25 @@ public Task Invoke(HttpContext context)
2828
return _next(context);
2929
}
3030

31-
return InvokeAwaited(context, endpoint);
31+
if (endpoint?.Metadata.GetMetadata<IAntiforgeryMetadata>() is { RequiresValidation: true })
32+
{
33+
return InvokeAwaited(context);
34+
}
35+
36+
return _next(context);
3237
}
3338

34-
public async Task InvokeAwaited(HttpContext context, Endpoint? endpoint)
39+
public async Task InvokeAwaited(HttpContext context)
3540
{
36-
if (endpoint?.Metadata.GetMetadata<IAntiforgeryMetadata>() is { RequiresValidation: true })
41+
try
3742
{
38-
try
39-
{
40-
await _antiforgery.ValidateRequestAsync(context);
41-
}
42-
catch (AntiforgeryValidationException e)
43-
{
44-
context.Features.Set<IAntiforgeryValidationFeature>(new AntiforgeryValidationFeature(false, e));
45-
await _next(context);
46-
return;
47-
}
43+
await _antiforgery.ValidateRequestAsync(context);
4844
context.Features.Set(AntiforgeryValidationFeature.Valid);
4945
}
50-
46+
catch (AntiforgeryValidationException e)
47+
{
48+
context.Features.Set<IAntiforgeryValidationFeature>(new AntiforgeryValidationFeature(false, e));
49+
}
5150
await _next(context);
5251
}
5352
}

‎src/Antiforgery/src/Internal/AntiforgeryValidationFeature.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33
namespace Microsoft.AspNetCore.Antiforgery;
44

5-
internal class AntiforgeryValidationFeature(bool isValid, AntiforgeryValidationException? exception) : IAntiforgeryValidationFeature
5+
internal sealed class AntiforgeryValidationFeature(bool isValid, AntiforgeryValidationException? exception) : IAntiforgeryValidationFeature
66
{
77
public static readonly IAntiforgeryValidationFeature Valid = new AntiforgeryValidationFeature(true, null);
88

‎src/Http/Http.Extensions/src/RequestDelegateFactory.cs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1429,7 +1429,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall,
14291429

14301430
if (feature?.CanHaveBody == true)
14311431
{
1432-
if (httpContext.Features.Get<IAntiforgeryValidationFeature>() is { IsValid: false } antiforgeryValidationFeature)
1432+
if (httpContext.Features.Get<IAntiforgeryValidationFeature>() is { IsValid: false } antiforgeryValidationFeature)
14331433
{
14341434
Log.InvalidAntiforgeryToken(httpContext, parameterTypeName, parameterName, antiforgeryValidationFeature.Error!, throwOnBadRequest);
14351435
httpContext.Response.StatusCode = StatusCodes.Status400BadRequest;
@@ -1995,13 +1995,12 @@ private static Expression BindComplexParameterFromFormItem(
19951995
// var name_reader;
19961996
// var form_dict;
19971997
// var form_buffer;
1998-
// var form_max_key_length;
19991998
var formArgument = Expression.Variable(parameter.ParameterType, $"{parameter.Name}_local");
20001999
var formReader = Expression.Variable(typeof(FormDataReader), $"{parameter.Name}_reader");
20012000
var formDict = Expression.Variable(typeof(IReadOnlyDictionary<FormKey, StringValues>), "form_dict");
20022001
var formBuffer = Expression.Variable(typeof(char[]), "form_buffer");
20032002

2004-
// ProcessForm(context.Request.Form, form_dict, form_max_key_length, form_buffer);
2003+
// ProcessForm(context.Request.Form, form_dict, form_buffer);
20052004
var processFormExpr = Expression.Call(ProcessFormMethod, FormExpr, formDict, formBuffer);
20062005
// name_reader = new FormDataReader(form_dict, CultureInfo.InvariantCulture, form_buffer.AsMemory(0, FormDataMapperOptions.MaxKeyBufferSize));
20072006
var initializeReaderExpr = Expression.Assign(

‎src/Http/Http.Features/src/IAntiforgeryValidationFeature.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
3-
using System.Diagnostics.CodeAnalysis;
3+
44
namespace Microsoft.AspNetCore.Antiforgery;
55

66
/// <summary>
@@ -16,6 +16,5 @@ public interface IAntiforgeryValidationFeature
1616
/// <summary>
1717
/// Gets the <see cref="Exception"/> that occurred when validating the anti-forgery token.
1818
/// </summary>
19-
[MemberNotNullWhen(false, nameof(IsValid))]
2019
Exception? Error { get; }
2120
}

‎src/Http/Routing/src/EndpointMiddleware.cs‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,7 @@ public Task Invoke(HttpContext httpContext)
5151
}
5252

5353
if (endpoint.Metadata.GetMetadata<IAntiforgeryMetadata>() is { RequiresValidation: true } &&
54-
!httpContext.Items.ContainsKey(AntiforgeryMiddlewareWithEndpointInvokedKey) &&
55-
httpContext.Request.Method is {} method &&
56-
HttpMethodExtensions.IsValidHttpMethodForForm(method))
54+
!httpContext.Items.ContainsKey(AntiforgeryMiddlewareWithEndpointInvokedKey))
5755
{
5856
ThrowMissingAntiforgeryMiddlewareException(endpoint);
5957
}

‎src/Http/Routing/src/EndpointRoutingMiddleware.cs‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,10 +136,6 @@ private Task SetRoutingAndContinue(HttpContext httpContext)
136136
// We do this during endpoint routing to ensure that successive middlewares in the pipeline
137137
// can access the feature with the correct value.
138138
SetMaxRequestBodySize(httpContext);
139-
if (httpContext.Features.Get<IHttpMaxRequestBodySizeFeature>() is { } httpMaxRequestBodySizeFeature)
140-
{
141-
httpContext.Request.Body = new SizeLimitedStream(httpContext.Request.Body, httpMaxRequestBodySizeFeature.MaxRequestBodySize);
142-
}
143139

144140
var shortCircuitMetadata = endpoint.Metadata.GetMetadata<ShortCircuitMetadata>();
145141
if (shortCircuitMetadata is not null)

‎src/Http/Routing/src/Microsoft.AspNetCore.Routing.csproj‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
<Compile Include="$(SharedSourceRoot)Debugger\DebuggerHelpers.cs" LinkBase="Shared" />
3838
<Compile Include="$(SharedSourceRoot)AntiforgeryMetadata.cs" LinkBase="Shared" />
3939
<Compile Include="$(SharedSourceRoot)HttpMethodExtensions.cs" LinkBase="Shared" />
40-
<Compile Include="$(SharedSourceRoot)SizeLimitedStream.cs" LinkBase="Shared" />
4140
</ItemGroup>
4241

4342
<ItemGroup>

‎src/Http/Routing/test/FunctionalTests/AntiforgeryTests.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,6 +398,11 @@ public async Task MapPost_WithForm_ValidToken_RequestSizeLimit_Works(bool hasLim
398398
return next(context);
399399
});
400400
app.UseRouting();
401+
app.Use((context, next) =>
402+
{
403+
context.Request.Body = new SizeLimitedStream(context.Request.Body, context.Features.Get<IHttpMaxRequestBodySizeFeature>()?.MaxRequestBodySize);
404+
return next(context);
405+
});
401406
app.UseAntiforgery();
402407
app.UseEndpoints(b =>
403408
b.MapPost("/todo", ([FromForm] Todo todo) => todo).WithMetadata(new RequestSizeLimitMetadata(hasLimit ? 2 : null)));

‎src/Http/Routing/test/FunctionalTests/Microsoft.AspNetCore.Routing.FunctionalTests.csproj‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@
1616
<Reference Include="Microsoft.AspNetCore.Routing" />
1717
<Reference Include="Microsoft.AspNetCore.TestHost" />
1818
</ItemGroup>
19-
19+
2020
<ItemGroup>
2121
<Compile Include="$(SharedSourceRoot)AntiforgeryMetadata.cs" LinkBase="Shared" />
22+
<Compile Include="$(SharedSourceRoot)SizeLimitedStream.cs" LinkBase="Shared" />
2223
</ItemGroup>
2324

2425
</Project>

‎src/Http/Routing/test/UnitTests/EndpointMiddlewareTest.cs‎

Lines changed: 2 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -315,11 +315,8 @@ public async Task Invoke_WithEndpoint_DoesNotThrowIfUnhandledCorsAttributesWereF
315315
Assert.True(calledEndpoint);
316316
}
317317

318-
[Theory]
319-
[InlineData("POST")]
320-
[InlineData("PUT")]
321-
[InlineData("PATCH")]
322-
public async Task Invoke_WithEndpoint_ThrowsIfAntiforgeryMetadataWasFound_ButAntiforgeryMiddlewareNotInvoked_ForValidHttpMethods(string method)
318+
[Fact]
319+
public async Task Invoke_WithEndpoint_ThrowsIfAntiforgeryMetadataWasFound_ButAntiforgeryMiddlewareNotInvoked()
323320
{
324321
// Arrange
325322
var expected = "Endpoint Test contains anti-forgery metadata, but a middleware was not found that supports anti-forgery." +
@@ -338,7 +335,6 @@ public async Task Invoke_WithEndpoint_ThrowsIfAntiforgeryMetadataWasFound_ButAnt
338335
};
339336

340337
httpContext.SetEndpoint(new Endpoint(throwIfCalled, new EndpointMetadataCollection(AntiforgeryMetadata.ValidationRequired), "Test"));
341-
httpContext.Request.Method = method;
342338

343339
var middleware = new EndpointMiddleware(NullLogger<EndpointMiddleware>.Instance, throwIfCalled, RouteOptions);
344340

@@ -349,39 +345,6 @@ public async Task Invoke_WithEndpoint_ThrowsIfAntiforgeryMetadataWasFound_ButAnt
349345
Assert.Equal(expected, ex.Message);
350346
}
351347

352-
[Theory]
353-
[InlineData("GET")]
354-
[InlineData("TRACE")]
355-
[InlineData("HEAD")]
356-
[InlineData("OPTIONS")]
357-
[InlineData("DELETE")]
358-
[InlineData("CONNECT")]
359-
public async Task Invoke_WithEndpoint_WorksIfAntiforgeryMetadataWasFound_ButAntiforgeryMiddlewareNotInvoked_ForInvalidHttpMethods(string method)
360-
{
361-
// Arrange
362-
var message = "Should be called";
363-
var httpContext = new DefaultHttpContext
364-
{
365-
RequestServices = new ServiceProvider()
366-
};
367-
368-
RequestDelegate throwIfCalled = (c) =>
369-
{
370-
throw new InvalidTimeZoneException(message);
371-
};
372-
373-
httpContext.SetEndpoint(new Endpoint(throwIfCalled, new EndpointMetadataCollection(AntiforgeryMetadata.ValidationRequired), "Test"));
374-
httpContext.Request.Method = method;
375-
376-
var middleware = new EndpointMiddleware(NullLogger<EndpointMiddleware>.Instance, throwIfCalled, RouteOptions);
377-
378-
// Act & Assert
379-
var ex = await Assert.ThrowsAsync<InvalidTimeZoneException>(() => middleware.Invoke(httpContext));
380-
381-
// Assert
382-
Assert.Equal(message, ex.Message);
383-
}
384-
385348
[Fact]
386349
public async Task Invoke_WithEndpoint_WorksIfAntiforgeryMetadataWasFound_AndAntiforgeryMiddlewareInvoked()
387350
{

0 commit comments

Comments
 (0)