Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

System.UriFormatException : Invalid URI: The hostname could not be parsed. #1481

Closed
Tratcher opened this issue Jul 5, 2018 · 7 comments
Closed
Labels
3 - Done bug cost: XS Will take about half a day to complete up-for-grabs We will consider contributions
Milestone

Comments

@Tratcher
Copy link
Member

Tratcher commented Jul 5, 2018

From @nulltoken on July 2, 2018 12:39

Hello,

It's possible I may have found a bug (or at the very least a surprising behavior 馃槈 ).

Running an integration test, using the TestServer, against an Mvc backed API (monitored by ApplicationInsights), throws a UriFormatException.

It looks like (cf. the screenshots below) that the host being parsed end up being "localhost,localhost".

I've managed to trim down the code as much as possible into a repro case available at https://github.com/nulltoken/UriFormatException

Beware, that looks pretty random. The repro case breaks most of time with one failing test, and from time to time two tests fail.

FWIW, removing this line from the repro case makes the problem disappear.

Versions:

  • .Net Core 2.1.1
  • ApplicationInsightsAspNetCore 2.4-beta1

hostingapplication

uriext

Copied from original issue: aspnet/Mvc#7997

@Tratcher
Copy link
Member Author

Tratcher commented Jul 5, 2018

From @nulltoken on July 2, 2018 19:9

FWIW this was working against .NET Core 2.0.5

@Tratcher
Copy link
Member Author

Tratcher commented Jul 5, 2018

From @pranavkm on July 2, 2018 20:25

cc @Tratcher

@Tratcher
Copy link
Member Author

Tratcher commented Jul 5, 2018

Confirmed, this is a bug in TestHost.

Minimized repro:

        [Fact]
        public async Task ManuallySetHost_Overwritten()
        {
            RequestDelegate appDelegate = ctx =>
                ctx.Response.WriteAsync(ctx.Request.Headers[HeaderNames.Host]);
            var builder = new WebHostBuilder().Configure(app => app.Run(appDelegate));
            var server = new TestServer(builder);
            var client = server.CreateClient();
            
            var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost:12345");
            request.Headers.Host = "otherhost:5678";
            var response = await client.SendAsync(request);
            var responseBody = await response.Content.ReadAsStringAsync();
                
            Assert.Equal("localhost:12345", responseBody);
        }

Expected: localhost:12345
Actual: localhost:12345,otherhost:5678

The Host is set from the Url here:

req.Host = HostString.FromUriComponent(request.RequestUri);

But then the request headers are copied over here:

foreach (var header in request.Headers)
{
req.Headers.Append(header.Key, header.Value.ToArray());
}

This adds an invalid second Host rather than overwriting.

Recommendation:
Host looks like the only request header we set, all the others are request line fields (e.g. path, query, etc..), it should get special cased so only one ends up in the request. The one from HttpRequestMessage.Headers should win over the one in HttpRequestMessage.RequestUri.Host.

@Tratcher Tratcher added cost: XS Will take about half a day to complete up-for-grabs We will consider contributions labels Jul 5, 2018
@nulltoken
Copy link
Contributor

@Tratcher Would you mind if I tried and gave a try at fixing it?

@Tratcher
Copy link
Member Author

Tratcher commented Jul 5, 2018

Go for it.

@nulltoken
Copy link
Contributor

Go for it.

@Tratcher Done. That was indeed a pretty easy fix. You wonderfully paved the way with your detailed explanation. Thanks a lot!

The only things that keeps on bugging me is that in my initial repro, not all the tests were failing. Only some of them.

Condering that ClientHandler.SendAsync() performs this normalization for each and every call, all the tests should fail, shouldn't they?

@Tratcher
Copy link
Member Author

Tratcher commented Jul 6, 2018

I don't have a good answer for that. The error wouldn't happen until the Uri was constructed, and only AI does that. Either that wasn't happening on some requests, or the error was being handled differently on some code paths.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
3 - Done bug cost: XS Will take about half a day to complete up-for-grabs We will consider contributions
Projects
None yet
Development

No branches or pull requests

2 participants