Support forwarded headers in servers URL resolution #60725
Conversation
ghost
added
the
area-mvc
captainsafia
added
feature-openapi
area-minimal
There was a problem hiding this comment.
PR Overview
The PR introduces support for forwarded headers in the URL resolution logic for OpenAPI servers.
- Added new test cases to verify the behavior when forwarded headers are provided.
- Updated OpenApiDocumentService to extract the forwarded scheme and host from request headers.
Reviewed Changes
| File | Description |
|---|---|
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Servers.cs | Added tests to validate forwarded header support. |
| src/OpenApi/src/Services/OpenApiDocumentService.cs | Modified server URL construction to handle forwarded headers. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/OpenApi/src/Services/OpenApiDocumentService.cs:209
- [nitpick] Consider adding test cases for scenarios where the 'X-Forwarded-Proto' header contains multiple values to verify that the current extraction logic meets all expected use cases.
var scheme = httpRequest.Headers.GetCommaSeparatedValues("X-Forwarded-Proto") is [var forwardedScheme, ..] ? forwardedScheme : httpRequest.Scheme;
...NetCore.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Servers.cs
Show resolved
Hide resolved
| @@ -205,14 +205,26 @@ internal List<OpenApiServer> GetOpenApiServers(HttpRequest? httpRequest = null) | |||
| { | |||
| if (httpRequest is not null) | |||
| { | |||
| var serverUrl = UriHelper.BuildAbsolute(httpRequest.Scheme, httpRequest.Host, httpRequest.PathBase); | |||
| // Handle forwarded headers directly if present | |||
| var scheme = httpRequest.Headers.GetCommaSeparatedValues("X-Forwarded-Proto") is [var forwardedScheme, ..] | |||
There was a problem hiding this comment.
Why is this happening in the document service? We have a whole middleware for forwarded headers
There was a problem hiding this comment.
We can't always rely on the forwarded headers middleware being enabled for this feature to work --especially since the training people are used to is that OpenAPI operates independent of any other options.
There's also some local dev scenarios (Codespaces, for example) that make it hard to discover that you need to enable forwarded headers middleware.
There was a problem hiding this comment.
That applies to all of asp.net core, not this feature. There are also security concerns with blindly reading xff headers (the middleware goes through great lengths to make that configurable but secure by default). You can (as a customer) enable his flag ASPNETCORE_FORWARDEDHEADERS_ENABLED, to turn on the middleware from the outside.
The custom should be using one of those techniques. This middleware is no more special than the rest.
There was a problem hiding this comment.
Lucky for you I don't feel argumentative today. 😆 I'll close this PR out and make the same change in the servicing PR.
Follow-up to #60673 for supporting forwarded headers.