Add context-aware CORS policy to enhance security#1609
Add context-aware CORS policy to enhance security#1609g2vinay wants to merge 3 commits intomicrosoft:mainfrom
Conversation
g2vinay
requested review from
JonathanCrd,
conniey,
Copilot,
fanyang-mono,
hallipr,
jairmyree,
vukelich and
wbreza
There was a problem hiding this comment.
Pull request overview
This PR adds a context-aware CORS policy to enhance security for the HTTP transport mode. The policy behaves differently based on the authentication mode: in development mode (unauthenticated), it restricts requests to localhost origins only to prevent CSRF attacks, while in production mode (authenticated), it allows all origins since JWT Bearer authentication validates all requests.
Changes:
- Introduced a new
ConfigureCors()method that configures CORS policy based on theDangerouslyDisableHttpIncomingAuthflag - Replaced the permissive "AllowAll" CORS policy with a context-aware "McpCorsPolicy"
- Updated both
CreateHttpHostandCreateIncomingAuthDisabledHttpHostmethods to use the new shared CORS configuration
| { | ||
| if (Uri.TryCreate(origin, UriKind.Absolute, out var uri)) | ||
| { | ||
| // Allow localhost and 127.0.0.1 with any port | ||
| return uri.Host == "localhost" || | ||
| uri.Host == "127.0.0.1" || | ||
| uri.Host == "[::1]"; // IPv6 loopback | ||
| } | ||
| return false; | ||
| }) | ||
| .AllowAnyMethod() | ||
| .AllowAnyHeader() | ||
| .AllowCredentials(); // Required when using SetIsOriginAllowed |
There was a problem hiding this comment.
The lambda expression passed to SetIsOriginAllowed has inconsistent indentation. The opening brace on line 684 is indented with extra spaces compared to the standard indentation pattern used in the rest of the codebase. This should be aligned consistently with the method call on line 683.
| { | |
| if (Uri.TryCreate(origin, UriKind.Absolute, out var uri)) | |
| { | |
| // Allow localhost and 127.0.0.1 with any port | |
| return uri.Host == "localhost" || | |
| uri.Host == "127.0.0.1" || | |
| uri.Host == "[::1]"; // IPv6 loopback | |
| } | |
| return false; | |
| }) | |
| .AllowAnyMethod() | |
| .AllowAnyHeader() | |
| .AllowCredentials(); // Required when using SetIsOriginAllowed | |
| { | |
| if (Uri.TryCreate(origin, UriKind.Absolute, out var uri)) | |
| { | |
| // Allow localhost and 127.0.0.1 with any port | |
| return uri.Host == "localhost" || | |
| uri.Host == "127.0.0.1" || | |
| uri.Host == "[::1]"; // IPv6 loopback | |
| } | |
| return false; | |
| }) | |
| .AllowAnyMethod() | |
| .AllowAnyHeader() | |
| .AllowCredentials(); // Required when using SetIsOriginAllowed |
| .AddHttpClientInstrumentation(o => | ||
| o.FilterHttpRequestMessage = ShouldInstrumentHttpRequest) | ||
| .AddAzureMonitorTraceExporter(exporterOptions => | ||
| exporterOptions.ConnectionString = connectionString) |
There was a problem hiding this comment.
These formatting changes to the AddHttpClientInstrumentation and AddAzureMonitorTraceExporter method calls appear to be unrelated to the CORS policy changes described in the PR. These changes should either be removed or explained in the PR description if they are intentional refactoring.
| .AddHttpClientInstrumentation(o => | |
| o.FilterHttpRequestMessage = ShouldInstrumentHttpRequest) | |
| .AddAzureMonitorTraceExporter(exporterOptions => | |
| exporterOptions.ConnectionString = connectionString) | |
| // Configure HTTP client instrumentation to filter out specific requests (e.g., App Insights ingestion, Azure SDK duplicates). | |
| .AddHttpClientInstrumentation(o => | |
| { | |
| o.FilterHttpRequestMessage = ShouldInstrumentHttpRequest; | |
| }) | |
| // Configure Azure Monitor exporter with the connection string from environment configuration. | |
| .AddAzureMonitorTraceExporter(exporterOptions => | |
| { | |
| exporterOptions.ConnectionString = connectionString; | |
| }) |
| private static void ConfigureCors(IServiceCollection services, ServiceStartOptions serverOptions) | ||
| { | ||
| services.AddCors(options => | ||
| { | ||
| options.AddPolicy("McpCorsPolicy", policy => | ||
| { | ||
| // In unauthenticated mode (development), restrict to localhost by default | ||
| // Allows localhost with any port to support various development scenarios: | ||
| // - Port 1031: Default HTTP mode (launchSettings.json) | ||
| // - Port 5008: SSE mode default | ||
| // - Port 5173: MCP Inspector tool | ||
| // - Custom ports: User-specified via ASPNETCORE_URLS | ||
| if (serverOptions.DangerouslyDisableHttpIncomingAuth) | ||
| { | ||
| policy.SetIsOriginAllowed(origin => | ||
| { | ||
| if (Uri.TryCreate(origin, UriKind.Absolute, out var uri)) | ||
| { | ||
| // Allow localhost and 127.0.0.1 with any port | ||
| return uri.Host == "localhost" || | ||
| uri.Host == "127.0.0.1" || | ||
| uri.Host == "[::1]"; // IPv6 loopback | ||
| } | ||
| return false; | ||
| }) | ||
| .AllowAnyMethod() | ||
| .AllowAnyHeader() | ||
| .AllowCredentials(); // Required when using SetIsOriginAllowed | ||
| } | ||
| // In authenticated mode (production), allow all origins by default | ||
| // This is safe because: | ||
| // 1. Authentication (JWT Bearer) validates all requests regardless of origin | ||
| // 2. CORS is a browser security mechanism, not a server security feature | ||
| // 3. MCP clients (GitHub Copilot in VS Code/Codespaces) need to connect from various origins | ||
| // 4. The server still enforces authentication and authorization on every request | ||
| else | ||
| { | ||
| policy.AllowAnyOrigin() | ||
| .AllowAnyMethod() | ||
| .AllowAnyHeader(); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The new ConfigureCors method introduces important security logic that restricts localhost origins in development mode but lacks unit test coverage. Given that the ServiceStartCommandTests.cs file exists with tests for other configuration options, tests should be added to verify:
- Localhost origins (localhost, 127.0.0.1, [::1]) are allowed when DangerouslyDisableHttpIncomingAuth is true
- Non-localhost origins are rejected when DangerouslyDisableHttpIncomingAuth is true
- All origins are allowed when DangerouslyDisableHttpIncomingAuth is false
- AllowCredentials is set correctly in development mode
| }) | ||
| .AllowAnyMethod() | ||
| .AllowAnyHeader() | ||
| .AllowCredentials(); // Required when using SetIsOriginAllowed |
There was a problem hiding this comment.
The comment states that AllowCredentials is "Required when using SetIsOriginAllowed", but this is not accurate. While AllowCredentials is commonly used with SetIsOriginAllowed for authenticated scenarios, it is not technically required by the API. The comment should clarify that it's needed to allow browsers to send credentials (cookies, authorization headers) in cross-origin requests, which is important for the development mode scenario.
| .AllowCredentials(); // Required when using SetIsOriginAllowed | |
| .AllowCredentials(); // Allows browsers to send credentials (cookies, auth headers) with cross-origin requests when using this localhost-only policy in development |
1 participant
Add context-aware CORS policy for HTTP transport mode
Development mode (unauthenticated): Restricts to localhost origins only to prevent CSRF attacks
Production mode (authenticated): Allows all origins safely
Replaces permissive 'AllowAll' policy with context-aware 'McpCorsPolicy'
Implements ConfigureCors() method shared by both authenticated and unauthenticated HTTP hosts