Test: failing regression guard for custom IMsalHttpClientFactory bypass (#6124) - #6160
Closed
Gladwin Johnson (gladjohn) wants to merge 1 commit into
Closed
Test: failing regression guard for custom IMsalHttpClientFactory bypass (#6124)#6160Gladwin Johnson (gladjohn) wants to merge 1 commit into
Gladwin Johnson (gladjohn) wants to merge 1 commit into
Conversation
…being bypassed (#6124) Adds an end-to-end transport-selection regression test that drives a real HttpManager with a plain custom IMsalHttpClientFactory (the shape Azure Identity's HttpClientTransport surfaces as) and asserts the caller's factory is actually used. This reproduces the transport bypass behind issue #6124 (recurrence of #5286): when a request carries a server-certificate validation callback (the Service Fabric managed identity path) and the factory is not an IMsalSFHttpClientFactory, HttpManager fabricates its own HttpClient and discards the caller's configured transport. The test intentionally FAILS on current code; the fix follows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e65d13d3-9f55-4d28-8224-6b08ff1bd85b
Copilot started reviewing on behalf of
Gladwin Johnson (gladjohn)
August 11, 2026 20:23
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new end-to-end regression test to reproduce and guard against the Managed Identity transport-selection bug where MSAL bypasses a caller-supplied IMsalHttpClientFactory when a server-certificate validation callback is present (issues #6124 / #5286). This test targets the HttpManager.GetHttpClient behavior to ensure custom transports (e.g., Azure Identity HttpClientTransport) are not silently discarded.
Changes:
- Introduces
HttpClientTransportRegressionTests.CustomHttpClientFactory_MustNotBeBypassed_WhenServerCertificateValidationCallbackPresentAsync. - Implements a minimal
IMsalHttpClientFactoryplus a trackingHttpMessageHandlerto assert factory invocation and request flow. - Drives a real
HttpManager(notMockHttpManager) to reproduce the bypass path.
Suppressed comments (1)
tests/Microsoft.Identity.Test.Unit/ManagedIdentityTests/HttpClientTransportRegressionTests.cs:130
- RequestCount is used as an assertion signal; incrementing a non-atomic property can be racy if requests ever occur concurrently (e.g., future retries/parallelization). Use Interlocked/Volatile for a thread-safe counter.
public int RequestCount { get; private set; }
protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
RequestCount++;
return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+56
to
+73
| Exception thrown = null; | ||
|
|
||
| // Act | ||
| try | ||
| { | ||
| await httpManager.SendRequestAsync( | ||
| // Unroutable endpoint: only ever contacted if MSAL bypasses the tracking factory and | ||
| // creates its own HttpClient. Connection is refused immediately (no external network). | ||
| endpoint: new Uri("https://127.0.0.1:1/token"), | ||
| headers: new Dictionary<string, string>(), | ||
| body: null, | ||
| method: HttpMethod.Get, | ||
| logger: Substitute.For<ILoggerAdapter>(), | ||
| doNotThrow: true, | ||
| bindingCertificate: null, | ||
| validateServerCert: validateServerCert, | ||
| cancellationToken: CancellationToken.None, | ||
| retryPolicy: new TestDefaultRetryPolicy(RequestType.STS)) |
Comment on lines
+104
to
+117
| private readonly HttpClient _httpClient; | ||
|
|
||
| public int GetHttpClientCallCount { get; private set; } | ||
|
|
||
| public PlainTrackingHttpClientFactory(HttpMessageHandler handler) | ||
| { | ||
| _httpClient = new HttpClient(handler); | ||
| } | ||
|
|
||
| public HttpClient GetHttpClient() | ||
| { | ||
| GetHttpClientCallCount++; | ||
| return _httpClient; | ||
| } |
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
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.
Summary
Adds an end-to-end transport-selection regression test for the custom-
IMsalHttpClientFactorybypass reported in #6124 (a recurrence of #5286: "HttpClientTransport setting is not applied after MSAL upgraded").What the test does
HttpClientTransportRegressionTests.CustomHttpClientFactory_MustNotBeBypassed_WhenServerCertificateValidationCallbackPresentAsync:HttpManager(notMockHttpManager) with a plain customIMsalHttpClientFactory— implementing only the normal interface, exactly the shape Azure Identity'sHttpClientTransportsurfaces as (notIMsalSFHttpClientFactory/IMsalMtlsHttpClientFactory).GetHttpClientinvocation count + request count), per the repo guidance that transport/pooling regression tests must assert factory-invocation counts.Why the existing coverage missed it
#5292regression test only asserts an implementation detail (SF returns a callback; other sources returnnull); it never executes a request to verify the caller's factory is used.MockHttpManager, whose mock factory implements every factory interface — so they can't catch a path that discards a caller supplying only the normal factory interface.Root cause the test pins
HttpManager.GetHttpClientfabricates a freshHttpClientwhen a validation callback is present and the factory is notIMsalSFHttpClientFactory:This discards the caller's configured transport (proxy, custom
RootCAs, dialer, tracing).Next step
Product fix so a caller-supplied
IMsalHttpClientFactoryis never discarded (attach the validation callback to a handler derived from the caller's factory / route SF throughIMsalSFHttpClientFactoryonly), which flips this test green.Related: #6124, #5286, #5292.