HttpRequestExtensions.GetUri throws Invalid URI when multiple Host headers are present

[saikatguha]

If you are reporting bug/issue, please provide detailed Repro instructions.

Repro Steps

1. Send HTTP request with multiple Host headers.
2. Observe 500 Internal Error

Reason is that request.Host.ToString() returns comma-separated list of all the Host header values. If only one Host header is present, all is fine. However, if multiple Host headers are present, it results in a malformed URI being created in line 38 of the form https://host1,host2/path

Actual Behavior

System.UriFormatException: Invalid URI: The hostname could not be parsed.
   at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind)
   at System.Uri..ctor(String uriString)
   at Microsoft.ApplicationInsights.AspNetCore.Extensions.HttpRequestExtensions.GetUri(HttpRequest request)
   at Microsoft.ApplicationInsights.AspNetCore.DiagnosticListeners.HostingDiagnosticListener.EndRequest(HttpContext httpContext, Int64 timestamp)
 ...

Expected Behavior

Request completes as normal.

Version Info

SDK Version : 2.6.1
.NET Version : core
How Application was onboarded with SDK(VisualStudio/StatusMonitor/Azure Extension) :
OS :
Hosting Info (IIS/Azure WebApps/ etc) : Azure WebApps

[cijothomas]

@Dmitry-Matveev You are already working on get URI right? Can you take a look at this ?

[Dmitry-Matveev]

@saikatguha, yep, I concur, the previous and the existing implementations are both subject to this issue. We now take Value if it exists, but Value can still be malformed for URI purposes, I guess.

AI is not supposed to fail the app under any circumstance, though - did it fail your request and service?
@cijothomas , do we still have try-catch all rule across SDKs?

[saikatguha]

@Dmitry-Matveev yes, all requests with multiple host headers fail before returning response to the client. If I remove AI logger, requests succeed.

[cijothomas]

This is a bug - SDK should have try..catch..ed everything so as to never let user request fails.

@Dmitry-Matveev please do the fix for this particular issue, while you are in that area, i will check entire SDK to make sure we have try..catched everything to conform to SDK guidance of never throwing.

@saikatguha Thanks for bringing this to our attention. We'll address this before next stable release.

[Dmitry-Matveev]

@saikatguha , what would you say the expected behavior looks like apart from not failing? @cijothomas is ensuring we catch all exceptions in #865.

Which value would you expect we report in Request URL if there are several hosts? The first host reported in URL or the last?

[saikatguha]

Short version:
Just like for missing host header AI uses fixed constant UNKNOWN-HOST,
I'd suggest using a constant MULTIPLE-HOST.

Long version:
Using MULTIPLE-HOST makes it clear to the app developer that there was a problem with the request rather than silently hiding a problem in the request.

Silently picking just one host (first or last) runs the risk that different components of the HTTP stack might pick inconsistently. For example, some authorization check component might pick the last value, while the logging component might pick the first value. This could result in nasty bugs impossible to diagnose.

Note that the HTTP RFC itself suggests ignoring or errorring on multiple host fields.
However, that is for the application server to decide, not the logging component.
Like you said, the logging component shouldn't fail.

As per RFC 7230 - Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing

A server MUST respond with a 400 (Bad Request) status code to any
HTTP/1.1 request message that lacks a Host header field and to any
request message that contains more than one Host header field or a
Host header field with an invalid field-value.

The default behavior for generic singleton header fields that occur multiple times is to ignore.

As per RFC 7231 - Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content

If [some header field] does not use the list syntax, document how to 
treat messages where the field occurs multiple times (a sensible default 
would be to ignore the field, but this might not always be the right choice).

[lmolkova]

should be fixed with #944.