Fix OAuth Protected Resource Metadata flows with AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS #1263
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…Y_ENABLE_FORWARDED_HEADERS. This configuration will read the X-Forwarded-Proto request header sent from Azure Container Apps to construct the URLs needed for OAuth PRM within the original claims challenge and the PRM endpoint's JSON body.
vukelich
requested review from
fanyang-mono,
g2vinay,
jongio,
msalaman,
sandeep-sen,
weshaggard and
xiangyan99
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes OAuth Protected Resource Metadata flows when using forwarded headers in Azure Container Apps environments. The fix addresses an issue where the OAuth metadata URLs were incorrectly using the internal HTTP scheme instead of the external HTTPS scheme that clients use.
Key Changes
- Added environment variable
AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERSto opt-in to usingX-Forwarded-Protoheader - Introduced
GetSchemeForOAuthProtectedResourceMetadatalocal function to determine the correct scheme for OAuth URLs - Applied the fix to both the WWW-Authenticate challenge response and the OAuth protected resource metadata endpoint
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| using System.CommandLine.Parsing; | ||
| using System.Diagnostics; | ||
| using System.Net; | ||
| using Azure.Core; |
There was a problem hiding this comment.
The using Azure.Core; directive appears to be unused. The only reference to Azure.Core in the file is a string literal comparison ("Azure.Core.Http") at line 872, which doesn't require this import.
Suggested change
| using Azure.Core; |
Comment on lines
+560
to
+602
| string GetSchemeForOAuthProtectedResourceMetadata(HttpRequest request) | ||
| { | ||
| string scheme = request.Scheme; | ||
|
|
||
| // Default to "false" for enabling forwarded headers. The env var must be present, | ||
| // and it must be parsed to "true". | ||
| bool enableForwardedHeaders = | ||
| bool.TryParse( | ||
| Environment.GetEnvironmentVariable("AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS"), | ||
| out bool parsedEnvVar) | ||
| && parsedEnvVar; | ||
|
|
||
| // Azure Container Apps setups usually use HTTP between the ACA platform's | ||
| // reverse proxy and the application container. Our OAuth claims challenge | ||
| // needs to match what the client will use as a scheme. So only in this | ||
| // case do we use the X-Forwarded-Proto header if present. We're also going | ||
| // to limit specifically to "http" and "https" values and use their | ||
| // lowercase forms rather than the casing in the header. | ||
| // | ||
| // Other reverse proxies or load balancers may also use X-Forwarded-Proto or | ||
| // may use something different. We only special case ACA here because it's | ||
| // part of the samples as of 2.0-beta.5. More thorough logic and any | ||
| // configuration options can be added later if needed, and that could use | ||
| // ASP.NET Core's Forwarded Headers Middleware. See: | ||
| // https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer | ||
| if (enableForwardedHeaders | ||
| && request.Headers.TryGetValue("X-Forwarded-Proto", out StringValues forwardedProto)) | ||
| { | ||
| if (forwardedProto.FirstOrDefault() is string forwardedProtoValue) | ||
| { | ||
| if (string.Equals(forwardedProtoValue, "https", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| scheme = "https"; | ||
| } | ||
| else if (string.Equals(forwardedProtoValue, "http", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| scheme = "http"; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return scheme; | ||
| } |
There was a problem hiding this comment.
The new GetSchemeForOAuthProtectedResourceMetadata local function lacks test coverage. Given that this file has comprehensive unit tests in ServiceStartCommandTests.cs, consider adding tests to cover:
- Default behavior when
AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERSis not set - Behavior when the environment variable is set to
trueandX-Forwarded-Protoheader is present with various values (https,http,HTTPS,HTTP, invalid values) - Behavior when the environment variable is set to
falsebut the header is present - Behavior when the environment variable is set but the header is missing
Comment on lines
+585
to
+599
| if (enableForwardedHeaders | ||
| && request.Headers.TryGetValue("X-Forwarded-Proto", out StringValues forwardedProto)) | ||
| { | ||
| if (forwardedProto.FirstOrDefault() is string forwardedProtoValue) | ||
| { | ||
| if (string.Equals(forwardedProtoValue, "https", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| scheme = "https"; | ||
| } | ||
| else if (string.Equals(forwardedProtoValue, "http", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| scheme = "http"; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the remote MCP server being called by VS Code.
Fix OAuth Protected Resource Metadata flows with AZURE_MCP_DANGEROUSLY_ENABLE_FORWARDED_HEADERS. This configuration will read the X-Forwarded-Proto request header sent from Azure Container Apps to construct the URLs needed for OAuth PRM within the original claims challenge and the PRM endpoint's JSON body.
GitHub issue number?
#1214
Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationeng/scripts/Process-PackageReadMe.ps1. See Package README/servers/Azure.Mcp.Server/docs/azmcp-commands.mdand/or/docs/fabric-commands.md.\eng\scripts\Update-AzCommandsMetadata.ps1to update tool metadata in azmcp-commands.md (required for CI)ToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.json/servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline