Runtime: User-Agent header is ignored in requests

[lkischler]

I try to set User-Agent header in .NET Core 2.2 console app:

var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com");
request.Headers.Add("User-Agent", "PostmanRuntime/7.6.0");
var cl = new HttpClient();
await cl.SendAsync(request);

but when I sniff request by Fiddler:

The same code in net461 console app produces:

[rmkerr]

It looks like the issue here is specific to CONNECT requests -- with this repro we still send the User-Agent along with the actual request.

CONNECT, without header:

The subsequent GET, with the header:

I'm actually not sure if we're supposed to send User-Agent headers along with a CONNECT. My guess is that we probably are, but we should verify that before making a fix here.

[rmkerr]

It looks like CONNECT requests should include the User-Agent, and some proxies even require them. The fix should likely go here:
https://github.com/dotnet/corefx/blob/5543097d8b6039dedffdcd8b8e713de38e5a1ece/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs#L632-L637

Though we will need to determine if we want to include other headers too.

[davidsh]

It looks like CONNECT requests should include the User-Agent, and some proxies even require them.

Yes. We fixed a similar bug in .NET Framework 4.7.2:
https://github.com/Microsoft/dotnet/blob/master/releases/net472/dotnet472-changes.md

Fixed System.Net HTTP APIs (WebClient, HttpWebRequest, HttpClient) so that connections to the proxy to set up the CONNECT tunnel (for TLS/SSL requests) will now have the 'User-Agent' header passed to the request if that header was added by the developer to the original http request. [486995, System.dll, Bug]

[Marusyk]

Hi, @rmkerr
Did you start work on it? Or is it still up-for-grabs?

[rmkerr]

Hey @Marusyk! I spent a little bit of time trying to get a test written, but didn't get a fix together. If you'd like to give it a shot instead, I can assign the issue to you.

[Marusyk]

I got an error when building tests projects:

error CS0006: Metadata file 'C:\Users\admin\source\corefx\artifacts\bin\RemoteExecutorConsoleApp\netstandard-AnyOS-Debug\RemoteExecutorConsoleApp.exe' could not be found

Could you please suggest how can I test my changes in System.Net.Http?

[davidsh]

Can you be more specific about the problem you see? Can you build the repo without your changes? Can you run System.Net.Http.Functional.Tests without your changes?

Generally speaking, the process should look like this (Windows build example):

s:\GitHub\corefx>build

// build the whole repo first

s:\GitHub\corefx>cd src\System.Net.Http\tests\FunctionalTests
s:\GitHub\corefx\src\System.Net.Http\tests\FunctionalTests>msbuild /t:rebuildandtest

or to run both Innerloop and Outerloop tests

s:\GitHub\corefx\src\System.Net.Http\tests\FunctionalTests>msbuild /t:rebuildandtest /p:Outerloop=true

Do those commands above work for you?

[Marusyk]

I'm trying to build solution System.Net.Http.sln in VS2017 and got that errors for test projects.

1. build the whole repo first (build.cmd)
2. open System.Net.Http.sln in VS2017
3. Build
4. Errors

Can you build the repo without your changes?

of course no! My changes definitely won't add reference to RemoteExecutorConsoleApp.exe

When I unloaded projects with tests and build solution then everything is ok (Build succeeded)

Can I work with that solution in VS2017? And how can I connect a new System.Net.Http.dll to my console app to test it? I didn't find any info how to do that.

[davidsh]

I'm trying to build solution System.Net.Http.sln in VS2017 and got that errors for test projects.

While Visual Studio supports .NET Core generally speaking, building CoreFx test projects is not supported right now in Visual Studio. You should use the command line only as I demonstrated above.

[davidsh]

Here is code for a test you can use to support a PR you might submit for this fix. This test was the one used for a similar fix in .NET Framework that I mentioned above.

You'll need to change some of the code below to compile and work in CoreFx. And the "FakeProxyServer" you can replace with the one we use already in our tests in CoreFx:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/tests/FunctionalTests/LoopbackGetRequestHttpProxy.cs

You should add a test similar to this as part of your PR. You can add the test to an appropriate existing test class in src/System.Net.Http/tests/FunctionalTests/

Proxy test to validate 'User-Agent' header sent to proxy

```c#
using System;
using System.Collections.Generic;
using System.Net;
using System.Net.Http;
using System.Threading;
using System.Threading.Tasks;

public class ProxyTest
{
    private const string UserAgentHeaderName = "User-Agent";
    private const string UserAgentHeaderValue = "ProxyTest";

    [Fact]
    public async Task UserAgent_SecureRequestUsingHttpClient_UserAgentHeaderSentToProxy()
    {
        Task<FakeProxyServer.ProxyResult> proxyTask = FakeProxyServer.StartAsync(out int port);

        var handler = new HttpClientHandler();
        handler.Proxy = new WebProxy(new Uri($"http://localhost:{port}"));
        using (var client = new HttpClient(handler))
        {
            client.DefaultRequestHeaders.Add(UserAgentHeaderName, UserAgentHeaderValue);

            // Start request to server. It should fail because the fake
            // proxy server will always return 407. But this test only
            // cares about the headers sent to the proxy.
            Uri server = ResourceDirectory.Current.GetServer(ResourceServer.HttpsNoAuth).Uri;
            await ExceptionAssert.ThrowsAsync<HttpRequestException>(() => client.GetAsync(server));
            FakeProxyServer.ProxyResult proxyResult = await proxyTask;
            VerifyProxyRequestHeaders(proxyResult);
        }
    }

    [Fact]
    public async Task UserAgent_SecureRequestUsingWebClient_UserAgentHeaderSentToProxy()
    {
        Task<FakeProxyServer.ProxyResult> proxyTask = FakeProxyServer.StartAsync(out int port);
        using (var client = new WebClient())
        {
            client.Headers[UserAgentHeaderName] = UserAgentHeaderValue;
            client.Proxy = new WebProxy(new Uri($"http://localhost:{port}"));

            // Start request to server. It should fail because the fake
            // proxy server will always return 407. But this test only
            // cares about the headers sent to the proxy.
            Uri server = ResourceDirectory.Current.GetServer(ResourceServer.HttpsNoAuth).Uri;
            Assert.Throws<WebException>(() => client.DownloadString(server), null);

            FakeProxyServer.ProxyResult proxyResult = await proxyTask;
            VerifyProxyRequestHeaders(proxyResult);
        }
    }

    private void VerifyProxyRequestHeaders(FakeProxyServer.ProxyResult proxyResult)
    {
        Log.Info(proxyResult.RequestLine);
        foreach (KeyValuePair<string, string> pair in proxyResult.RequestHeaders)
        {
            Log.Info($"{pair.Key}: {pair.Value}");
        }

        Assert.True(proxyResult.RequestLine.Contains("CONNECT"));
        Assert.True(proxyResult.RequestHeaders.TryGetValue(UserAgentHeaderName, out string headerValue));
        Assert.Equal(UserAgentHeaderValue, headerValue);
    }
}

}
```

[Marusyk]

Ok, thanks a lot.
I meant that I made changes in System.Net.Http, build it, and want to use this new dll in Console app to test it. But my console still use System.Net.Http.dll from SDK but not my, even when I add System.Net.Http.dll as a reference.

[davidsh]

I meant that I made changes in System.Net.Http, build it, and want to use this new dll in Console app to test it. But my console still use System.Net.Http.dll from SDK but not my, even when I add System.Net.Http.dll as a reference.

That isn't a recommend way of testing changes to CoreFx libraries. You have to do various manual hacks to copy a compiled System.Net.Http.dll binary from the CoreFx build tree and overwrite some shared install of .NET Core libraries. You can try it but it is complicated and not recommended as a best-practice for a 'dev innerlop' pattern. And it won't be the way you do it when you submit a PR which should include both your produce code changes as well as test code changes.

[rmkerr]

In order to make sure we don't regress the code later, we usually add tests to our functional or unit tests. Generally if we're going to take a change, it will need to have tests in one of those locations.

If you want to do some manual testing with your own console app though, you can check out the dogfooding instructions here. As David mentioned the process is pretty complicated though.

[MarcoRossignoli]

and want to use this new dll in Console app to test it.

You could create a custom temporary test and launch from console(it would work like a "console app" but inside dev/test environment) i.e.

msbuild /v:m /t:RebuildAndTest /p:XunitOptions="-method System.IO.Tests.PathTests.Try_GetTempPath_Default"  System.Runtime.Extensions.Tests.csproj  

@Marusyk let me know if you need further help on dev workflow we can talk on gitter/twitter

[Marusyk]

Hello @MarcoRossignoli,
I've tried to fix that but it seems was wrong place. I've followed this

The fix should likely go here:
corefx/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

and added that headers manually (just for test) but it doesn't fix the problem. I can't debug it (don't know how). Any suggestion how can I help?

[MarcoRossignoli]

and added that headers manually (just for test) but it doesn't fix the problem. I can't debug it (don't know how). Any suggestion how can I help?

To debug:
1) download VS 2019 preview https://visualstudio.microsoft.com/vs/preview/
2) Open solution https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/System.Net.Http.sln here you should find functional test projects for http classes.
3) Search a correct test(that cover your issue) and add breakpoint
4) Go to project properties and on debug tab add filter to your tests i.e.

For instance my filter allow me to debug System.Net.Http.Tests.GetTokenLength_SetOfValidTokens_AllConsideredValid test(I use wildcard on filter to avoid full namespace name)
And after you can launch Debug(ger)(F5) and you can step(F11) inside http classes implementation

Let me know!

[MarcoRossignoli]

@Marusyk are you able to debug?

[Marusyk]

Hello, thanks for that information. I'll try it at the weekend because a lot of work. I do not want to install VS 2019 preview but if it help then ok. I will let you know the status. Thanks

[MarcoRossignoli]

Don't worry!Let me know if you need further help!

[Marusyk]

@MarcoRossignoli Thanks for your instruction how to debug. It was really helpful. So, I'm trying to fix the issue, it's draft for now and may not yet deserve a separate PR. Please see the changes and let me know what do you think. Am I on the proper way? Should it be only User-Agent or we want to include other headers too?

Thank you in advance

[MarcoRossignoli]

@Marusyk you should also try to write some test to validate the code change, something like https://github.com/dotnet/corefx/blob/ac9c746c44a5c0decf551fe33fb2acbdc895d021/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L493
This test is an "Outerloop" test https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/writing-tests.md#outerloop
to run outerloop test cmd

D:\git\corefx\src\System.Net.Http (master -> origin)
λ msbuild /t:rebuildandtest /v:m /p:XunitOptions="-method *ProxyTunnelRequest_PortSpecified_NotStrippedOffInUri"  tests\FunctionalTests\System.Net.Http.Functional.Tests.csproj /p:outerloop=true

To debug on VS comment out attribute [OuterLoop("Uses external server")] temporarly(@ViktorHofer is there a way to pass some switch to avoid outerloop filtering? -trait category=OuterLoop doesn't work I think filter is inside $(RunArguments) )
You could try to add a sample header and assert on server side

...
 client.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue("Mozilla", "5.0"));
...
 Assert.Contains($"CONNECT {requestTarget} HTTP/1.1", headers);
// Assert UserAgent 

@rmkerr @davidsh seem that even WinHttp doesn't send agent by default, should do it?

[Marusyk]

Yeah, of course I will add tests. But I ask you suggest if my direction is ok. If so, I will prepare a PR with changes and some tests. Now I tested it, and it seems works.
Just tell me what do you think about that changes.

[davidsh]

@rmkerr @davidsh seem that even WinHttp doesn't send agent by default, should do it?

It doesn't matter that WinHTTP doesn't send 'User-Agent' to proxy with CONNECT verb. We fixed .NET Framework for this bug already as I mentioned above. And so, we would like to have .NET Core fixed the same way.

[davidsh]

Should it be only User-Agent or we want to include other headers too?

Yes, only 'User-Agent' should be the additional header to be fixed in this PR. In general, we don't send all the headers to a proxy with the CONNECT verb. But 'User-Agent' is becoming an important header to send to proxies since they have filtering logic and lately expect to see that header.

[davidsh]

Taking a quick look at your changes, this line should be changed:

c# tunnelRequest.Headers.Add(HttpKnownHeaderNames.UserAgent, _headers.UserAgent.ToString());

You need to test if the UserAgent header is actually present (i.e. non-NULL, non-empty string) in the _headers before you try to add it to the tunnelRequest.Headers. And you should use the 'TryAddWithoutValidation' method and not the 'Add' method. It's possible the user added a malformed header to the original request. But we don't want to validate it here when adding to tunnelRequest.headers.

[MarcoRossignoli]

@davidsh ok I misunderstood...I thought that .NET full uses WinHTTP and you fixed that(passing in some way header to native apis) and so fix .net core in case of WinHTTP usage.

[davidsh]

@davidsh ok I misunderstood...I thought that .NET full uses WinHTTP and you fixed that(passing in some way header to native apis) and so fix .net core in case of WinHTTP usage.

No. .NET Framework doesn't use WinHTTP. It uses a separate managed stack.

[MarcoRossignoli]

No. .NET Framework doesn't use WinHTTP. It uses a separate managed stack.

Ok clear thank's for the info.

[ViktorHofer]

To debug on VS comment out attribute [OuterLoop("Uses external server")] temporarly(@ViktorHofer is there a way to pass some switch to avoid outerloop filtering? -trait category=OuterLoop doesn't work I think filter is inside $(RunArguments) )

Not easily... Would need some changes in the infra system for better VS support.

[MarcoRossignoli]

Not easily... Would need some changes in the infra system for better VS support.

Don't worry not fundamental

[Marusyk]

I'm sorry I can't write unit test for that. I've tried something like this:

public async Task Test()
{
    string requestTarget = $"{Configuration.Http.SecureHost}:443";
    string addressUri = $"https://{requestTarget}/";
    bool connectionAccepted = false;

    await LoopbackServer.CreateClientAndServerAsync(async proxyUri =>
    {
        using (HttpClientHandler handler = CreateHttpClientHandler())
        using (var client = new HttpClient(handler))
        {
            handler.Proxy = new WebProxy(proxyUri);
            handler.ServerCertificateCustomValidationCallback = TestHelper.AllowAllCertificates;
            client.DefaultRequestHeaders.UserAgent.Add(new ProductInfoHeaderValue("Mozilla", "5.0"));
            try
            { await client.GetAsync(addressUri); }
            catch { }
        }
    }, server => server.AcceptConnectionAsync(async connection =>
    {
        connectionAccepted = true;
        List<string> headers = await connection.ReadRequestHeaderAndSendResponseAsync();
        Assert.Contains($"CONNECT {requestTarget} HTTP/1.1", headers);
        Assert.Contains("User-Agent: Mozilla/5.0", headers);
    }));

    Assert.True(connectionAccepted);
}

but it runs twice and in one case everything is ok and in the other one it fall. I did not find any solution so I won't send PR for that.

[MarcoRossignoli]

@Marusyk it's ok...test run twice because if you're writing test inside class HttpClientHandlerTest this class is abstract and it's inherited by some tests classes i.e.:
1) PlatformHandler_HttpClientHandler and this use WinHTTP platform handler(windows native) that doesn't send Agent and you cannot do anything, take a look to protected override bool UseSocketsHttpHandler => false; to disable SocketHttpHandler and use WinHTTP.
2) SocketHttpHandler and this use your update(this handler is fully managed)
I think you should add if like
https://github.com/dotnet/corefx/blob/94a2d0e52360f24d08c26191e20dfa8f015f9f68/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.MaxConnectionsPerServer.cs#L87
to skip(not fail) test for non SocketHttpHandler and your test should work.

@davidsh is correct?

[davidsh]

@davidsh is correct?

Yes. It isn't useful to run this new test in WinHttpHandler nor CurlHander nor on .NET Framework or UWP either.

The simplest way to ensure that is just skip the test on everything but SocketsHttpHandler.

c# if (!UseSocketsHttpHandler) { return; // Skip test since the fix is only in SocketsHttpHandler. }

[Marusyk]

Thanks all for help. Test passed. Please review all changes in PR and let me know if I need to fix something.

[MarcoRossignoli]

@davidsh is this up-for-grabs? I think that we could flow request headers with new method parameter here and add to tunnelRequest.Headers per SendProxyConnectAsync.