WebHeaderCollection needs some internal APIs made public

[davidsh]

While investigating failures in System.Net.Requests tests ( #20873), I discovered that FtpWebResponse.Headers and FileWebResponse.Headers is returning a WebHeaderCollection object that does not have the private field WebHeaderCollectionType properly initialized within the collection.

This causes code like this:

FileWebResponse response = ...
Assert.Equal(data.Length.ToString(), response.Headers[HttpResponseHeader.ContentLength]);

to pass on .NET Core (which is wrong). Instead, it should throw an exception

System.InvalidOperationException : This collection holds request headers and cannot contain the specified response header.

similar to what happens on .NET Framework. The exception is because FtpWebResponse/FileWebResponse don't have Http type headers.

So, the net result is that we don't have proper validation for WebHeaderCollection type.

The problem is that on .NET Core, the WebHeaderCollection is being created with the default contructor which doesn't pass it any specific WebHeaderCollectionType enum value. On .NET Framework, it uses an internal constructor overload. Also, the WebHeaderCollectionType enum is also marked as internal in .NET Framework and is not a public API.

This was ok for .NET Framework since everything was in System.dll. But on .NET Core, WebHeaderCollection lives in a different library from System.Net.Requests where *WebRequest, *WebResponse classes live.

    internal enum WebHeaderCollectionType : ushort {
        Unknown,
        WebRequest,
        WebResponse,
        HttpWebRequest,
        HttpWebResponse,
        HttpListenerRequest,
        HttpListenerResponse,
        FtpWebRequest,
        FtpWebResponse,
        FileWebRequest,
        FileWebResponse,
    }
    internal WebHeaderCollection(WebHeaderCollectionType type) : base(DBNull.Value);

This is resulting in all WebHeaderCollection objects being set with the WebHeaderCollectionType.Unknown enum value.

[davidsh]

cc: @stephentoub @karelz @CIPop

[CIPop]

From my experience with the TLS Alerts work, adding new public APIs required by the implementation complicates packaging and may create issues when packaging netstandard 2.0.
/cc @ericstj @weshaggard

[karelz]

Legacy APIs

Options:

1. Merge contracts - all are legacy APIs (it is messy as they are separate contracts since 1.0) + implement on top of internal API
2. Make the type & constructor public on Core only (not in .NET Standard) + implement on top of public it
3. Live with sub-optimal validation

Triage recommendation: option [2]. Doesn't fit in, moving to Future.

[caesar-chen]

This issue is blocking UWP work for S.N.HttpListener.
The same tests failed in dotnet/corefx#20425 (netcore scenario) also failed in UAP mode, for the same reason: WebHeaderCollectionType is set to Unknown by default, which skip the validation part: here

I will disable those tests for now.

[karelz]

@Caesar1995 Please add "disabled-test" label to the main issue which tracks the disabled test (is it this one?), thanks!

[DavidGoll]

Nice to have. Not needed for UWP. Won't break apps. Implementation is only needed to do extra error checking when adding to the collection.

[karelz]

Triage: Similar problem affects also non-UWP (we may have another tracking issue for that).

[davidsh]

This issue was specific to the UAP implementation of the networking stack which had a different implementation for things like HTTP and WebSockets.

As of PR dotnet/corefx#41759, we no longer build the separate implementation for the UAP platform. So, this issue can't occur anymore with .NET 5 because we now will be using the same .NET Core implementation as on Windows, Linux, Mac.