Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/2.1] Fix WinHttpHandler when using authenticating proxies #30212

Merged
merged 1 commit into from
Jun 27, 2018
Merged

[release/2.1] Fix WinHttpHandler when using authenticating proxies #30212

merged 1 commit into from
Jun 27, 2018

Conversation

davidsh
Copy link
Contributor

@davidsh davidsh commented Jun 8, 2018

Port of #30196 to release/2.1

This was a regression from .NET Core 2.0 due to PR #28105.

Proxy authentication using system default proxy settings that involve a PAC file
(either autodiscovery or explicit PAC file) cause a NullReferenceException in WinHttpHandler in the
CheckResponseForAuthentication() method.

This problem is only discovered when using an authenticating proxy server (any auth scheme) that is
discovered using PAC files. This is considered the "system default" proxy. When this occurs, the
handler's Proxy property is null and dereferencing it caused the exception.

Due to the problems described in #6997, the uri of the proxy can't be determined yet since it is only
known to WinHTTP. Fixing #6997 is complicated and impacts performance. However, in most cases, as
long as the credentials are a NetworkCredential object, knowing the uri of the proxy is needed.

I tested this manually using the steps I described in #30191. I did not add any tests to this PR since
they can't be run in CI. However, I am working on a task that will eventually add Enterprise-Scenario
testing like this (PAC files, authenticating proxies, etc.) to our systems.

Fixes #30191
Contributes to #6997

This was a regression from .NET Core 2.0 due to PR #28105.

Proxy authentication using system default proxy settings that involve a PAC file
(either autodiscovery or explicit PAC file) cause a NullReferenceException in WinHttpHandler in the
CheckResponseForAuthentication() method.

This problem is only discovered when using an authenticating proxy server (any auth scheme) that is
discovered using PAC files. This is considered the "system default" proxy. When this occurs, the
handler's Proxy property is null and dereferencing it caused the exception.

Due to the problems described in #6997, the uri of the proxy can't be determined yet since it is only
known to WinHTTP. Fixing #6997 is complicated and impacts performance. However, in most cases, as
long as the credentials are a NetworkCredential object, knowing the uri of the proxy is needed.

I tested this manually using the steps I described in #30191. I did not add any tests to this PR since
they can't be run in CI. However, I am working on a task that will eventually add Enterprise-Scenario
testing like this (PAC files, authenticating proxies, etc.) to our systems.

Fixes #30191
Contributes to #6997
@davidsh davidsh added os-windows area-System.Net.Http Servicing-consider Issue for next servicing release review labels Jun 8, 2018
@davidsh davidsh added this to the 2.1.x milestone Jun 8, 2018
@davidsh davidsh self-assigned this Jun 8, 2018
@davidsh
Copy link
Contributor Author

davidsh commented Jun 8, 2018

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

@davidsh davidsh changed the title Fix WinHttpHandler when using authenticating proxies [release/2.1] Fix WinHttpHandler when using authenticating proxies Jun 8, 2018
@davidsh
Copy link
Contributor Author

davidsh commented Jun 8, 2018

CI Failures are unrelated.

@stephentoub
Copy link
Member

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test NETFX x86 Release Build please

@stephentoub stephentoub merged commit be49d38 into dotnet:release/2.1 Jun 27, 2018
@davidsh davidsh deleted the port_30196 branch June 28, 2018 15:58
@danmoseley danmoseley removed the Servicing-consider Issue for next servicing release review label Aug 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants