Repository navigation
Enforce HTTPS for WS-Trust endpoints - #6165
Conversation
Reject insecure MEX and WS-Trust endpoints, prevent automatic redirects for credential-bearing WS-Trust requests, and add regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens WS-Trust and federation metadata handling by enforcing HTTPS-only endpoints and adding redirect-related protections, along with unit test coverage to prevent regressions in endpoint selection and request behavior.
Changes:
- Enforces HTTPS for federation metadata URLs and for WS-Trust endpoints selected/used by WS-Trust flows, with explicit non-HTTPS redirect rejection.
- Introduces redirect-control plumbing (new
allowAutoRedirectparameter onIHttpManager.SendRequestAsync,HttpResponse.RequestUri, and an internalIHttpClientFactoryWithRedirectControl) to support disabling auto-redirect for sensitive requests. - Adds/extends unit tests for MEX parsing, endpoint selection, and redirect/insecure-endpoint rejection scenarios.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Microsoft.Identity.Test.Unit/RequestsTests/IntegratedWindowsAuthUsernamePasswordTests.cs | Adds regression test ensuring injected HTTP WS-Trust endpoints from metadata are rejected before any WS-Trust request is sent. |
| tests/Microsoft.Identity.Test.Unit/Helpers/ParallelRequestMockHandler.cs | Updates test IHttpManager implementation signature to include redirect control. |
| tests/Microsoft.Identity.Test.Unit/CoreTests/WsTrustTests/WsTrustTests.cs | Adds tests for disallowing auto-redirect and rejecting non-HTTPS redirects / HTTP WS-Trust endpoints. |
| tests/Microsoft.Identity.Test.Unit/CoreTests/WsTrustTests/MexParserTests.cs | Adds tests for HTTPS-only endpoint selection and rejecting HTTP MEX endpoints / non-HTTPS redirects; expands input validation coverage. |
| tests/Microsoft.Identity.Test.Unit/CoreTests/HttpTests/HttpClientFactoryTests.cs | Verifies redirect-disabled clients use a separate cached instance from the default pool. |
| src/client/Microsoft.Identity.Lab.Api/Http/MockHttpManager.cs | Propagates allowAutoRedirect through mocks and applies it to the handler used by test HttpClient instances. |
| src/client/Microsoft.Identity.Client/WsTrust/WsTrustWebRequestManager.cs | Adds federation metadata URL validation, HTTPS enforcement, redirect checks, disables auto-redirect for WS-Trust calls, and introduces helpers for HTTPS/redirect detection. |
| src/client/Microsoft.Identity.Client/WsTrust/MexDocument.cs | Filters out non-HTTPS WS-Trust endpoint addresses during MEX parsing. |
| src/client/Microsoft.Identity.Client/PlatformsCommon/Shared/SimpleHttpClientFactory.cs | Adds redirect-controlled pooling for non-mTLS clients and supports redirect control via internal interface. |
| src/client/Microsoft.Identity.Client/MsalErrorMessage.cs | Updates/introduces WS-Trust-specific error messages for HTTPS requirements and redirect rejection. |
| src/client/Microsoft.Identity.Client/Http/IHttpManager.cs | Extends SendRequestAsync contract to include allowAutoRedirect. |
| src/client/Microsoft.Identity.Client/Http/IHttpClientFactoryWithRedirectControl.cs | Adds internal interface for factories that can supply redirect-enabled/disabled clients. |
| src/client/Microsoft.Identity.Client/Http/HttpResponse.cs | Adds RequestUri to help detect final request URI after redirects. |
| src/client/Microsoft.Identity.Client/Http/HttpManager.cs | Threads redirect control through request execution and records RequestUri in HttpResponse. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Prevent federation metadata fetches from following redirects before HTTPS validation and assert redirect suppression in regression coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Bogdan Gavril (@bgavrilMS) / Travis Walker (@trwalke) / Neha Bhargava (@neha-bhargava) can I get a review on this? I've got an active ICM for this bug and need it merged. |
Follow bounded HTTPS redirect chains for federation metadata and WS-Trust requests while rejecting insecure redirect targets before transmission. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/client/Microsoft.Identity.Client/WsTrust/WsTrustWebRequestManager.cs:186
- When MaxRedirects is reached for a WS-Trust request, the loop breaks and the method throws a FederatedServiceReturnedError based on the last 3xx response, which can be misleading. Throwing a dedicated exception when the redirect limit is exceeded would make failures easier to understand.
if (!TryGetSecureRedirectUri(resp, requestUri, out Uri redirectUri) ||
redirectCount >= MaxRedirects)
{
break;
}
Changes proposed in this request
Testing
Performance impact
No expected measurable impact. The additional HTTP client configuration is created only when needed and is pooled for reuse.
Documentation