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 antiforgery support #509

Merged
merged 25 commits into from
Oct 21, 2023
Merged

Add antiforgery support #509

merged 25 commits into from
Oct 21, 2023

Conversation

vipwan
Copy link
Contributor

@vipwan vipwan commented Oct 20, 2023

@@ -60,6 +63,9 @@ public static IApplicationBuilder UseFastEndpoints(this IApplicationBuilder app,

public static IEndpointRouteBuilder MapFastEndpoints(this IEndpointRouteBuilder app, Action<Config>? configAction = null)
{
//use AntiforgeryMiddleware middleware
(app as WebApplication)?.UseMiddleware<Middleware.AntiforgeryMiddleware>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe only use middleware if antiforgery is used?

Could be done by checking if its turned "on" on any endpoint, or by some other configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe only use middleware if antiforgery is used?

Could be done by checking if its turned "on" on any endpoint, or by some other configuration

the Middlewire is really Simple, just when set EndpointDefinition.IsEnlableAntiforgery = true the validation logic is executed

(Default:false) it has no effect :)

    public async Task Invoke(HttpContext context)
    {
        //GET请求不需要防伪验证
        if (context.Request.Method == HttpMethods.Get ||
            context.Request.Method == HttpMethods.Trace ||
            context.Request.Method == HttpMethods.Options ||
            context.Request.Method == HttpMethods.Head)
        {
            await _next(context);
            return;
        }
        var endpointDefinition = context.GetEndpoint()?.Metadata.GetMetadata<EndpointDefinition>();
        if (endpointDefinition?.IsEnlableAntiforgery is true)
        {
            try
            {
                await _antiforgery.ValidateRequestAsync(context);
            }
            catch (AntiforgeryValidationException)
            {
                context.Response.StatusCode = StatusCodes.Status400BadRequest;
                await context.Response.WriteAsync("Invalid anti-forgery token");
                return;
            }
        }
        await _next(context);
    }

Copy link
Member

Choose a reason for hiding this comment

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

haven't checked the code properly yet. but ideally this middleware should only be added to the pipeline if user explicitly asks to enable it during startup, something like this:

app.UseFastEndpoints(c => c.Security.EnableAntiForgeryTokens = true);

if this value is false, endpoint level EnlableAntiforgery() call should throw an exception during startup.

@vipwan think you can make that happen?

Copy link
Member

Choose a reason for hiding this comment

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

actually scratch that.... lets make the user do two calls to configure antiforgery.

builder.Services.AddAntiForgery()

app.UseAntiForgery() // this will be a custom extensionmethod that registers the middleware.

and during startup if endpoints say EnableAntiForgery() and the user hasn't called AddAntiForgery() an exception should be thrown.

how about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haven't checked the code properly yet. but ideally this middleware should only be added to the pipeline if user explicitly asks to enable it during startup, something like this:

app.UseFastEndpoints(c => c.Security.EnableAntiForgeryTokens = true);

if this value is false, endpoint level EnlableAntiforgery() call should throw an exception during startup.

@vipwan think you can make that happen?

its a good Idea

app.UseFastEndpoints(c => c.Security.EnableAntiForgeryTokens = true);

Copy link
Member

Choose a reason for hiding this comment

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

i think this should be the way:

builder.Services.AddAntiForgery()

app.UseAntiForgery() // this will be a custom extensionmethod that registers the middleware.

@dj-nitehawk
Copy link
Member

dj-nitehawk commented Oct 21, 2023

usage changed to this:

//register services
builder.Services.AddAntiForgery()

//enable middleware
app.UseAntiForgery()

//endpoint config
public override void Configure()
{
    EnableAntiforgery();
}

also changed the 400 response by middleware to use the error response builder method instead of just returning a string message.

this way it won't have any conflicts with the antiforgery support coming in .net 8 to minimal apis.

@dj-nitehawk dj-nitehawk merged commit 2b4d4eb into FastEndpoints:main Oct 21, 2023
1 check was pending
@dj-nitehawk
Copy link
Member

@vipwan is this the correct way to spell your name in english?

Thank you Wàn Yǎhǔ for the [contribution](https://github.com/FastEndpoints/FastEndpoints/pull/500).

@vipwan
Copy link
Contributor Author

vipwan commented Oct 21, 2023

@vipwan is this the correct way to spell your name in english?

Thank you Wàn Yǎhǔ for the [contribution](https://github.com/FastEndpoints/FastEndpoints/pull/500).

😃 yes,that's right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants