[HTTP] MultiProxy

[ManickaP]

Clear proxy auth header when failing over to another proxy from multi-proxy list.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates SocketsHttpHandler multi-proxy failover logic to avoid leaking a Proxy-Authorization header from one proxy attempt into subsequent proxy attempts, and adds a regression test to validate the behavior on Windows.

Changes:

• Restore the original HttpRequestMessage.Headers.ProxyAuthorization value when retrying against the next proxy in a multi-proxy list.
• Extend LoopbackProxyServer test infrastructure with an option to drop the connection after receiving a Proxy-Authorization header.
• Add a Windows-only functional test covering PAC multi-proxy failover and ensuring no proxy auth is sent to the succeeding proxy.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPoolManager.cs	Restores the original ProxyAuthorization header between multi-proxy retries to prevent credential/header carryover.
src/libraries/Common/tests/System/Net/Http/LoopbackProxyServer.cs	Adds a test-only option to forcefully end the connection after proxy auth is observed.
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Proxy.cs	Adds a regression test for PAC failover ensuring proxy auth from the failing proxy isn’t sent to the succeeding proxy.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "237884efe1813afb99a142ce5d8b36441d3b5d80",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "af85196d561d26c2f8a0589d435cffed1c90752c",
  "last_reviewed_commit": "237884efe1813afb99a142ce5d8b36441d3b5d80",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "af85196d561d26c2f8a0589d435cffed1c90752c",
  "last_recorded_worker_run_id": "29747442447",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "237884efe1813afb99a142ce5d8b36441d3b5d80",
      "review_id": 4735596596
    }
  ]
}

[github-actions]

Holistic Review

Motivation: When SocketsHttpHandler fails over across a multi-proxy list (e.g. a PAC/WinInetProxyHelper-derived list), proxy credentials pulled from the credential cache for a failed proxy could linger on the request's ProxyAuthorization header and get sent to the next proxy. That leaks credentials scoped to one proxy to a different, potentially untrusted proxy. This PR clears that stale header on failover.

Approach: In HttpConnectionPoolManager.SendAsyncMultiProxy, the original request.Headers.ProxyAuthorization value is captured once before the retry loop. On each retryable HttpRequestException, the header is reset to that original value before advancing to the next proxy via multiProxy.ReadNext. This clears cache-derived credentials from the failed attempt while preserving any credentials the user explicitly set on the request. A regression test (MultiProxy_PAC_Failover_CredentialsNotSend_Succeeds, Windows/SocketsHttpHandler only) reflection-constructs an HttpWindowsProxy over a two-proxy config where the first proxy demands Basic auth then kills the connection after receiving credentials, and asserts the succeeding proxy receives no Authorization token. LoopbackProxyServer gains a KillConnectionAfterAuth option to simulate the failing proxy.

Summary: The fix is minimal, correct, and directly addresses a credential-leak-on-failover bug. Capturing the original header before the loop and restoring it on each retry correctly distinguishes user-set credentials (preserved) from cache-derived ones (cleared). The KillConnectionAfterAuth test hook is well-contained and only alters behavior when explicitly opted in. The test validates the intended behavior end-to-end. I found no correctness, security, or style concerns in the changed code. LGTM.

Detailed Findings

None. The production change is a well-scoped, low-risk correctness fix, and the accompanying test and loopback-server hook are consistent with existing patterns in these files.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 45.5 AIC · ⌖ 9.53 AIC · ⊞ 10K

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.Proxy.cs:624

• The new test can pass without exercising the bug it intends to cover. Currently it only asserts that the succeeding proxy never sees a Proxy-Authorization token, but it doesn’t assert that the failing proxy attempt actually sent any Proxy-Authorization header (i.e., that proxy auth was attempted and the header was populated). If the handler fails over before setting Proxy-Authorization, this would be a false-positive regression test.

Consider adding an assertion that the failing proxy observed at least one request with a Proxy-Authorization header (e.g., Basic with a token), and tighten the succeeding-proxy validation to ensure the header is fully absent (scheme and token).

            Assert.Equal(2, succeedingProxyServer.Requests.Count);
            foreach (var request in succeedingProxyServer.Requests)
            {
                Assert.Null(request.AuthorizationHeaderValueToken);
            }

[ManickaP]

/ba-g unrelated

[MihaZupan]

Still looks good :)