Fix WinHttpProvider caching a null WinHTTP session permanently

[Andy McCalib (amccalib)]

Problem

WinHttpProvider::GetHSession() could return a null HINTERNET as a successful Result, and then cache it permanently.

Result<T>'s single-argument constructor is documented as "Construct successful result" (Source/Common/Result.h:13-14), so return hSession; with a null handle produces hr == S_OK. Both call sites check only RETURN_IF_FAILED(getHSessionResult.hr) and never null-check the payload, so the null was passed straight to WinHttpConnection::Initialize.

The cache makes it permanent:

• m_hSessions is keyed only by securityProtocolFlags.
• It is cleared only in Suspend(), which never runs on desktop platforms.
• Once a null is stored, every later call short-circuits on the m_hSessions.find() early return.

So a single transient failure poisons the provider for the lifetime of the process, and does so silently — the short-circuit path emits no trace output whatsoever.

Why this is reachable in practice

WinHttpSetOption(WINHTTP_OPTION_SECURE_PROTOCOLS) fails with ERROR_ACCESS_DENIED whenever the session was opened with WINHTTP_FLAG_SECURE_DEFAULTS and the requested mask includes TLS 1.0/1.1 — and the Win32 default mask (winhttp_provider.cpp:306-309) does exactly that. SECURE_DEFAULTS requires TLS 1.2+ and rejects attempts to re-enable older protocols.

I observed this on stock Windows 11 with no fault injection at all. That means the close-and-reopen fallback is the normal path for every HTTPS session, not a rare edge case — so only the second WinHttpOpen has to fail to poison the cache.

Changes

All three are in GetHSession() and are the same defect family — a failed or null session being treated as success:

1. Return on fallback-open failure. When the fallback WinHttpOpen(WINHTTP_FLAG_ASYNC) returns null, return the error instead of warning and falling through to cache the null. This is the actual fix for the bug above.
2. Only read GetLastError() after a failure. It was read unconditionally after WinHttpOpen; its value is undefined after success. A stale ERROR_INVALID_PARAMETER could open a second session over a successful first one and leak the first handle.
3. Map a zero GetLastError() to E_FAIL on the initial-open failure path. HRESULT_FROM_WIN32(0) is S_OK, which selects the Result(HRESULT) "failed result (no payload)" overload while reporting success — Payload() then asserts and ExtractPayload() is undefined.

Verification

A standalone Win32 harness injects WinHttpOpen / WinHttpSetOption failures by patching the harness's own import address table, so libHttpClient itself compiles and runs completely unmodified — no test hooks or #ifdefs in the library.

Phase	Before	After
1 — faults injected	fails (expected)	fails (expected)
2 — all faults removed, healthy network, x3	fails 3/3	succeeds 3/3

Before the fix, phase 2 produced zero trace output: the cached null short-circuits before any logging can happen. That silent-failure signature is what makes this so hard to diagnose from customer logs.

Built and compiles clean in both libHttpClient.143.Win32.C and libHttpClient.143.GDK.C.

Deliberately out of scope

Two adjacent issues found while investigating, left for separate changes to keep this reviewable:

• TLS downgrade. The reopen drops WINHTTP_FLAG_SECURE_DEFAULTS and re-permits TLS 1.0/1.1, and also loses the "no TLS fallback" behavior. The better fix is to skip SECURE_PROTOCOLS entirely when the session already opened with SECURE_DEFAULTS.
• Cache key is incomplete. m_hSessions is keyed only by securityProtocolFlags, but session setup also depends on isHttps — and on Win32 GetSecurityInformation() returns the same constant mask for both schemes. An HTTP request that populates the entry first will hand a later HTTPS request a session that never had secure protocols configured, making behavior order-dependent.