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

Feature Request: IBffApiSkipAntiforgery with Skip method #1732

Open
Erwinvandervalk opened this issue Jan 24, 2025 · 2 comments
Open

Feature Request: IBffApiSkipAntiforgery with Skip method #1732

Erwinvandervalk opened this issue Jan 24, 2025 · 2 comments
Assignees
Labels
area/bff Related to all BFF
Milestone

Comments

@Erwinvandervalk
Copy link
Contributor

Erwinvandervalk commented Jan 24, 2025

This issue is moved from the support repo to the products repo

Original issue:
DuendeSoftware/Support#1561

Which version of Duende BFF are you using?
2.3

Which version of .NET are you using?
8.0

Hello, I don't know if this is actually the process of feature request but here goes.

I think it would be beneficial if the interface IBffApiSkipAntiforgery had a Skip method that would return true by default, and would also allow us to implement it and have more fine-grained control over skipping or not skipping inside the same route.

For example, in my case, I wanted to support WebSockets in a normal proxy route, but I have to skip anti forgery in case it's a websocket request or not.. And currently for that I need add Middleware before the BffMiddleware and append manually the BffApiSkipAntiforgery attribute to the endpoint metadata, which is extremely cumbersome. Eg:

public class PreBffMiddleware : IMiddleware
{
    public async Task InvokeAsync(HttpContext context, RequestDelegate next)
    {
        if (context.WebSockets.IsWebSocketRequest)
        {
            var endpoint = context.GetEndpoint();
            if (endpoint is RouteEndpoint routeEndpoint)
            {
                var metadata = new List<object>(routeEndpoint.Metadata);

                if (!metadata.Exists(m => m is BffApiSkipAntiforgeryAttribute))
                {
                    metadata.Add(new BffApiSkipAntiforgeryAttribute());
                    var newEndpoint = new RouteEndpoint(
                        routeEndpoint.RequestDelegate,
                        routeEndpoint.RoutePattern,
                        routeEndpoint.Order,
                        new EndpointMetadataCollection(metadata),
                        routeEndpoint.DisplayName);
                    context.SetEndpoint(newEndpoint);
                }
            }
        }

        await next.Invoke(context);
    }
}

With my suggestion, it would look like this.

public interface IBffApiSkipAntiforgery {
  public bool Skip(HttpContext context) => true;
}
// BffMiddleware snippet
var isBffEndpoint = endpoint.Metadata.GetMetadata<IBffApiEndpoint>() != null;
if (isBffEndpoint)
{
    var requireAntiForgeryCheck = endpoint.Metadata.OfType<IBffApiSkipAntiforgery>().FirstOrDefault();
    if (requireAntiForgeryCheck == null || !requireAntiForgeryCheck.Skip(context))
    {
        if (!context.CheckAntiForgeryHeader(_options))
        {
            _logger.AntiForgeryValidationFailed(context.Request.Path);

            context.Response.StatusCode = 401;
            return;
        }
    }
}

Using this we could create our own attributes, and use them instead, like this one

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
public class WebSocketSkipAntiForgery : Attribute, IBffApiSkipAntiforgery
{
    public bool Skip(HttpContext context){
      if (request == null) return false;

      if (request.WebSockets.IsWebSocketRequest) {
        return true;
      }

      return false;
    };
}

Thanks for the attention

@Erwinvandervalk Erwinvandervalk added the area/bff Related to all BFF label Jan 24, 2025
@Erwinvandervalk Erwinvandervalk self-assigned this Jan 24, 2025
@Erwinvandervalk Erwinvandervalk added this to the bff-future milestone Jan 24, 2025
@Erwinvandervalk
Copy link
Contributor Author

Better support for websockets with the BFF is something we have on the roadmap but is not currently planned for any specific milestones. There are a number of caveats to take into account. For one, you should ensure your endpoints are not vulnerable to csrf attacks by verifying the origin.
After that, you need to configure yarp in a specific way. Lastly, the websocket connection can live longer than the validity of the access token so you'd need to compensate for that as well.

Similar to: DuendeSoftware/Support#972

@greg-signi
Copy link

Just fyi I think this might have more applications beyond WebSockets, it was a general improvement to the way BffMiddleware verifies the Anti Forgery header or not.
Also I wouldn't mind making the Pull Request if you guys agree but are overall are too busy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bff Related to all BFF
Projects
None yet
Development

No branches or pull requests

2 participants