Invalid usage of X-Forwarded-For

[NatMarchand]

Hello there!
When I deploy a sample api app to Azure Web App (Windows) using the latest package (5.6.0), a servers property appears in the swagger json :

{
  "openapi": "3.0.1",
  "info": {
    "title": "TestWebApp",
    "version": "v1"
  },
  "servers": [
    {
      "url": "https://www.xxx.yyy.zzz"
    }
  ],
...

However, the url in the server is not at all a server : it's my client IP.
I've pinpointed the issue to the following PR : #1801
If you look closely at the "spec" of X-Forwarded-For (see here) the syntax is the following : X-Forwarded-For: <client>, <proxy1>, <proxy2>
The PR introduced this line where it use the first value of X-Forwaded-For to create a base url. It then use X-Forwarded-Host, if available, to override the host.
However, on Azure WebApp, X-Forwarded-For is set but not X-Forwarded-Host (as the Host header is not rewritten). We end with a server url which in fact is the client. I'm not sure if this line is useful or we can try to use the second value :

if (request.Headers.TryGetValue("X-Forwarded-For", out StringValues forwardedFor) && forwardedFor.Length > 1)
                hostBuilder = new UriBuilder($"http://{forwardedFor[1]}");

In all cases, the first value of the array should never be use as it is the client ip.
I can make the PR if you tell me which solution you like.

[NatMarchand]

Thinking a little bit more, I don't think those changes were needed (at least not for Host, For, Proto and Port). If there is a need to support X-Forwarded-* headers, then aspnetcore offers the UseForwardedHeaders middleware that sets the correct values in the HttpRequest object. You just have to read them there and not read the headers.

[domaindrivendev]

@NatMarchand thx for pointing out this mistake with X-Forwarded-For. I'll update SB to ignore this header and push out a hotfix release in the next hour or so

[NatMarchand]

I have few ideas to share tomorrow (it’s evening for me) in order to properly handle reverse proxies. In my opinion, the less is done in Swashbuckle the better it is as if it’s not done in aspnet pipeline before swashbuckle, same problems could arise further (ex: using urlhelper or using Created method in a controller). Last point I’m not sure it’s Swashbuckle concern to do all this plumbing 😄

[domaindrivendev]

@NatMarchand I wasn't aware of the UseForwardedHeaders middleware and it certainly changes my thinking about how Swashbuckle should be dealling with the servers metadata and reverse proxy environments. Specifically (and I think this is what you're suggesting), I could do the following:

• Remove the recently added logic to honor X-Forwarded headers
• Instead, populate the servers metadata by default, using HttpRequest.Scheme, HttpRequest.Host
• Prompt users to enable the UseForwardedHeaders middleware if they're using SB in a reverse proxy environment

This certainly seems a lot cleaner and takes the responsibility away from SB, which I like :)

The one caveat is that I don't see any mention of the X-Forwarded-Prefix header in the UseForwardedFor docs. Automatic support for this is something people have been requesting.

[NatMarchand]

I recently had to deal with such a prefix, unfortunately there’s no clear standard and the header name is specific to who is using it. I ended up creating a small middleware for this. Application Insights had same problem with this prefix, I contributed to take it in account. IMHO best place to deal with this is in the ForwardedHeader middleware.

[NatMarchand]

Ok so, here are my 2cts.

When having to deal with a reverse proxy using a prefix in the path, one should be careful.
First step is to use UseForwardedHeaders() to take in account headers such as X-Forwarded-For, X-Forwarded-Proto, and X-Forwarded-Host (see docs). This middleware will try to read the headers to update the HttpRequest in the aspnetcore accordingly (by modifying the host, the client ip/port and the scheme). Be careful using those headers as there are some serious security concerns (you should restrict which proxy is able to call you).
When this prerequisite is ok, remains the prefix. This can be dealt with a middleware, but the implementation depends on the configuration of the proxy.
Considering a header X-Forwarded-Prefix /api and a controller which listen to the route http://server/myroute
First configuration, the proxy is transparent (doesn't change the url): http://proxy/api/myroute -> http://server/api/myroute

            app.Use((context, next) =>
            {
                if (context.Request.Headers.TryGetValue("X-Forwarded-Prefix", out var pathBase)
                    && context.Request.Path.StartsWithSegments(pathBase.ToString(), out var matched, out var remaining))
                {
                    context.Request.PathBase = matched;
                    context.Request.Path = remaining;
                }
                return next();
            });

Second configuration, the proxy is additive (adds the prefix): http://proxy/myroute -> http://server/api/myroute

            app.Use((context, next) =>
            {
                if (context.Request.Headers.TryGetValue("X-Forwarded-Prefix", out var pathBase)
                    && context.Request.Path.StartsWithSegments(pathBase.ToString(), out var matched, out var remaining))
                {
                    context.Request.Path = remaining;
                }
                return next();
            });

Third configuration, the proxy is substractive (strips the prefix): http://proxy/api/myroute -> http://server/myroute

            app.Use((context, next) =>
            {
                if (context.Request.Headers.TryGetValue("X-Forwarded-Prefix", out var pathBase))
                {
                    context.Request.PathBase = new PathString(pathBase);
                }
                return next();
            });

If you want to try these middleware you can use an api with this dummy controller:

[ApiController]
[Route("[controller]")]
public class WhereAmIController : ControllerBase
{
    [HttpGet(Name = nameof(Get))]
    public string Get()
    {
        return Url.Link(nameof(Get), null);
    }
}

IMHO, one argument against having it in Swashbuckle is that if you implement this logic inside, some features of aspnet and its ecosystem won't work correctly. For example in the controller, I use the UrlHelper to get the public facing url of the route, it uses the different values of HttpRequest (Scheme, Host, PathBase and Path). Not using one of the middleware above will lead to incorrect urls generated (this helper is used also when using method such as CreatedAt***(), AcceptedAt***() and RedirectTo***() and maybe at other places). Telemetry in Application Insights (and probably other APMs) will also display an incorrect value.

Also keep in mind that X-Forwarded-* is a de-facto standard, a RFC (RFC7239) should replace all of this with one header to rule them all™

[domaindrivendev]

Sounds good. Based on this, my plan is as follows:

• Remove the recently added logic to honor X-Forwarded headers
• Instead, populate the servers metadata by default, using HttpRequest.Scheme, HttpRequest.Host
• Document UseForwardedHeaders along with Prefix workaround you've described above for reverse proxy users

This means the server metadata will be populated by default, whereas currently it's left empty in most cases, and as such I'm going to release the above changes as a major version increment.

[ziga34]

Hi,

Works as expected now. I'll open new ticket with other concerns.

[adamhathcock]

I'm glad to hear that this addition to 5.6.0 will be removed as it's tripping me up and I didn't previously have an issue.

[niemyjski]

@domaindrivendev is this going into the 6.0 release or is this fixed in a >= 5.6.1 release which has no notes? I'm on 5.6.3 and still running into these issues where my server url is http://host:443