Sanitize http.url attribute

[pellared]

Fixes #1884.

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Design discussion issue http.url MUST NOT contain credentials opentelemetry-specification#1502
• Changes in public API reviewed

Constraints

• For requests without username/password data the http.url value would remain the same. For ASP.NET instrumentation it is uri.ToString() for HTTP instrumentation it is uri.OriginalString. We can consider unifying it but rather in separate PR.
• The public API is untouched. In theory, we could expose the method which sanitizes an URL in a separate PR.

[codecov]

Codecov Report

Merging #1961 (8c5360e) into main (d073906) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1961      +/-   ##
==========================================
+ Coverage   83.54%   83.57%   +0.03%     
==========================================
  Files         192      192              
  Lines        6156     6162       +6     
==========================================
+ Hits         5143     5150       +7     
+ Misses       1013     1012       -1     

Impacted Files	Coverage Δ
...umentation.AspNet/Implementation/HttpInListener.cs	88.88% <100.00%> (+0.31%)	⬆️
...tp/Implementation/HttpHandlerDiagnosticListener.cs	70.96% <100.00%> (ø)
...strumentation.Http/Implementation/HttpTagHelper.cs	93.54% <100.00%> (+0.69%)	⬆️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	100.00% <0.00%> (+2.85%)	⬆️

[CodeBlanch]

LGTM

[utpilla]

Could we add test cases to cover all the parts that would make up the new Uri such as scheme, authority, path and query, fragment etc.?

How about adding two unit tests like these which would ensure that Uri.PathAndQuery (/Home/Index.html) and Uri.Fragment (#search) are concatenated?

Url: https://user:password@localhost:443/Home/Index.html#search
ExpectedUrl: https://localhost:443/Home/Index.html#search

And one for http
Url: http://user:password@localhost:80/Home/Index.html#search
ExpectedUrl: http://localhost:80/Home/Index.html#search

[pellared]

I did my best to address it here: 5f517db and here: 275d431

[utpilla]

Added a comment.

[pellared]

I noticed that there is a potentially unstable test:

  Failed OpenTelemetry.Instrumentation.Grpc.Tests.GrpcTests.GrpcAspNetCoreInstrumentationAddsCorrectAttributes(enableGrpcAspNetCoreSupport: True) [22 ms]
  Error Message:
   System.NullReferenceException : Object reference not set to an instance of an object.
  Stack Trace:
     at OpenTelemetry.Instrumentation.Grpc.Tests.GrpcTests.GrpcAspNetCoreInstrumentationAddsCorrectAttributes(Nullable`1 enableGrpcAspNetCoreSupport) in D:\a\opentelemetry-dotnet\opentelemetry-dotnet\test\OpenTelemetry.Instrumentation.Grpc.Tests\GrpcTests.server.cs:line 83

[utpilla]

We could update this test case to use http instead of https to test that we return the right Uri.Scheme in the URL.

[utpilla]

@pellared Could you please also add the logic to sanitize http.url to ASP.NET Core Instrumentation as well?

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 154 in d073906

activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request));

[pellared]

@pellared Could you please also add the logic to sanitize http.url to ASP.NET Core Instrumentation as well?

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 154 in d073906

activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request));

@utpilla I do not see this issue there. See:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Lines 293 to 326 in d073906

	private static string GetUri(HttpRequest request)
	{
	var builder = new StringBuilder();
	builder.Append(request.Scheme).Append("://");
	if (request.Host.HasValue)
	{
	builder.Append(request.Host.Value);
	}
	else
	{
	// HTTP 1.0 request with NO host header would result in empty Host.
	// Use placeholder to avoid incorrect URL like "http:///"
	builder.Append(UnknownHostName);
	}
	if (request.PathBase.HasValue)
	{
	builder.Append(request.PathBase.Value);
	}
	if (request.Path.HasValue)
	{
	builder.Append(request.Path.Value);
	}
	if (request.QueryString.HasValue)
	{
	builder.Append(request.QueryString);
	}
	return builder.ToString();
	}

[utpilla]

@pellared Could you please also add the logic to sanitize http.url to ASP.NET Core Instrumentation as well?

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Line 154 in d073906

activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUri(request));

@utpilla I do not see this issue there. See:

opentelemetry-dotnet/src/OpenTelemetry.Instrumentation.AspNetCore/Implementation/HttpInListener.cs

Lines 293 to 326 in d073906

	private static string GetUri(HttpRequest request)
	{
	var builder = new StringBuilder();
	builder.Append(request.Scheme).Append("://");
	if (request.Host.HasValue)
	{
	builder.Append(request.Host.Value);
	}
	else
	{
	// HTTP 1.0 request with NO host header would result in empty Host.
	// Use placeholder to avoid incorrect URL like "http:///"
	builder.Append(UnknownHostName);
	}
	if (request.PathBase.HasValue)
	{
	builder.Append(request.PathBase.Value);
	}
	if (request.Path.HasValue)
	{
	builder.Append(request.Path.Value);
	}
	if (request.QueryString.HasValue)
	{
	builder.Append(request.QueryString);
	}
	return builder.ToString();
	}

Thanks @pellared for pointing this out. This issue does not exist for ASP.NET Core Instrumentation.