Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize http.url attribute #1961

Merged
merged 15 commits into from
Apr 8, 2021
Merged

Sanitize http.url attribute #1961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e3494f6
Sanitize http.url in HTTP instrumentation
pellared Apr 6, 2021
3e88ae6
Revert
pellared Apr 6, 2021
91d083d
Refine GetUriTagValueFromRequestUri
pellared Apr 7, 2021
824183b
Fix build warning
pellared Apr 7, 2021
0df727e
Add new test case in HttpInListenerTests
pellared Apr 7, 2021
e02fc4d
Add missing whitespace
pellared Apr 7, 2021
736ecb1
Add missing line
pellared Apr 7, 2021
a2db594
Fix AspNetRequestsAreCollectedSuccessfully
pellared Apr 7, 2021
0259b3a
Sanitize http.url in ASP.NET instrumentation
pellared Apr 7, 2021
c7dec90
Make the tests passing
pellared Apr 7, 2021
721c529
Fix test case in AspNetRequestsAreCollectedSuccessfully
pellared Apr 7, 2021
683ec3e
Update Changelog
pellared Apr 7, 2021
5f517db
Add URL fragments to HTTP test cases
pellared Apr 8, 2021
275d431
Add complex URLs to ASP.NET test cases
pellared Apr 8, 2021
8c5360e
Merge branch 'main' into sanitize-http.url
cijothomas Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

* Sanitize `http.url` attribute. ([#1961](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1961))

## 1.0.0-rc3

Released 2021-Mar-19
Expand Down
17 changes: 16 additions & 1 deletion src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public override void OnStartActivity(Activity activity, object payload)
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.HttpMethod);
activity.SetTag(SpanAttributeConstants.HttpPathKey, path);
activity.SetTag(SemanticConventions.AttributeHttpUserAgent, request.UserAgent);
activity.SetTag(SemanticConventions.AttributeHttpUrl, request.Url.ToString());
activity.SetTag(SemanticConventions.AttributeHttpUrl, GetUriTagValueFromRequestUri(request.Url));

try
{
Expand Down Expand Up @@ -265,5 +265,20 @@ public override void OnStopActivity(Activity activity, object payload)
}
}
}

/// <summary>
/// Gets the OpenTelemetry standard uri tag value for a span based on its request <see cref="Uri"/>.
/// </summary>
/// <param name="uri"><see cref="Uri"/>.</param>
/// <returns>Span uri value.</returns>
private static string GetUriTagValueFromRequestUri(Uri uri)
{
if (string.IsNullOrEmpty(uri.UserInfo))
{
return uri.ToString();
}

return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);
}
}
}
2 changes: 2 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Unreleased

* Sanitize `http.url` attribute. ([#1961](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1961))

## 1.0.0-rc3

Released 2021-Mar-19
Expand Down
3 changes: 1 addition & 2 deletions src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,7 @@ public override void OnStartActivity(Activity activity, object payload)

activity.SetTag(SemanticConventions.AttributeHttpMethod, HttpTagHelper.GetNameForHttpMethod(request.Method));
activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);

activity.SetTag(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
if (this.options.SetHttpFlavor)
{
activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.Version));
Expand Down
15 changes: 15 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,21 @@ public static string GetHostTagValueFromRequestUri(Uri requestUri)
return hostTagValue;
}

/// <summary>
/// Gets the OpenTelemetry standard uri tag value for a span based on its request <see cref="Uri"/>.
/// </summary>
/// <param name="uri"><see cref="Uri"/>.</param>
/// <returns>Span uri value.</returns>
public static string GetUriTagValueFromRequestUri(Uri uri)
{
if (string.IsNullOrEmpty(uri.UserInfo))
{
return uri.OriginalString;
}

return string.Concat(uri.Scheme, Uri.SchemeDelimiter, uri.Authority, uri.PathAndQuery, uri.Fragment);
}

private static string ConvertMethodToOperationName(string method) => $"HTTP {method}";

private static string ConvertHttpMethodToOperationName(HttpMethod method) => $"HTTP {method}";
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ private static void AddRequestTagsAndInstrumentRequest(HttpWebRequest request, A
{
activity.SetTag(SemanticConventions.AttributeHttpMethod, request.Method);
activity.SetTag(SemanticConventions.AttributeHttpHost, HttpTagHelper.GetHostTagValueFromRequestUri(request.RequestUri));
activity.SetTag(SemanticConventions.AttributeHttpUrl, request.RequestUri.OriginalString);
activity.SetTag(SemanticConventions.AttributeHttpUrl, HttpTagHelper.GetUriTagValueFromRequestUri(request.RequestUri));
if (Options.SetHttpFlavor)
{
activity.SetTag(SemanticConventions.AttributeHttpFlavor, HttpTagHelper.GetFlavorTagValueFromProtocolVersion(request.ProtocolVersion));
Expand Down
34 changes: 19 additions & 15 deletions test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,25 @@ public void Dispose()
}

[Theory]
[InlineData("http://localhost/", 0, null, "TraceContext")]
[InlineData("http://localhost/", 0, null, "TraceContext", true)]
[InlineData("https://localhost/", 0, null, "TraceContext")]
[InlineData("http://localhost:443/", 0, null, "TraceContext")] // Test http over 443
[InlineData("https://localhost:80/", 0, null, "TraceContext")] // Test https over 80
[InlineData("http://localhost:80/Index", 1, "{controller}/{action}/{id}", "TraceContext")]
[InlineData("https://localhost:443/about_attr_route/10", 2, "about_attr_route/{customerId}", "TraceContext")]
[InlineData("http://localhost:1880/api/weatherforecast", 3, "api/{controller}/{id}", "TraceContext")]
[InlineData("https://localhost:1843/subroute/10", 4, "subroute/{customerId}", "TraceContext")]
[InlineData("http://localhost/api/value", 0, null, "TraceContext", false, "/api/value")] // Request will be filtered
[InlineData("http://localhost/api/value", 0, null, "TraceContext", false, "{ThrowException}")] // Filter user code will throw an exception
[InlineData("http://localhost/api/value/2", 0, null, "CustomContextMatchParent")]
[InlineData("http://localhost/api/value/2", 0, null, "CustomContextNonmatchParent")]
[InlineData("http://localhost/api/value/2", 0, null, "CustomContextNonmatchParent", false, null, true)]
[InlineData("http://localhost/", "http://localhost/", 0, null, "TraceContext")]
[InlineData("http://localhost/", "http://localhost/", 0, null, "TraceContext", true)]
[InlineData("https://localhost/", "https://localhost/", 0, null, "TraceContext")]
[InlineData("https://localhost/", "https://user:pass@localhost/", 0, null, "TraceContext")] // Test URL sanitization
Copy link
Contributor

@utpilla utpilla Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment.

[InlineData("http://localhost:443/", "http://localhost:443/", 0, null, "TraceContext")] // Test http over 443
[InlineData("https://localhost:80/", "https://localhost:80/", 0, null, "TraceContext")] // Test https over 80
[InlineData("https://localhost:80/Home/Index.htm?q1=v1&q2=v2#FragmentName", "https://localhost:80/Home/Index.htm?q1=v1&q2=v2#FragmentName", 0, null, "TraceContext")] // Test complex URL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

[InlineData("https://localhost:80/Home/Index.htm?q1=v1&q2=v2#FragmentName", "https://user:password@localhost:80/Home/Index.htm?q1=v1&q2=v2#FragmentName", 0, null, "TraceContext")] // Test complex URL sanitization
[InlineData("http://localhost:80/Index", "http://localhost:80/Index", 1, "{controller}/{action}/{id}", "TraceContext")]
[InlineData("https://localhost:443/about_attr_route/10", "https://localhost:443/about_attr_route/10", 2, "about_attr_route/{customerId}", "TraceContext")]
[InlineData("http://localhost:1880/api/weatherforecast", "http://localhost:1880/api/weatherforecast", 3, "api/{controller}/{id}", "TraceContext")]
[InlineData("https://localhost:1843/subroute/10", "https://localhost:1843/subroute/10", 4, "subroute/{customerId}", "TraceContext")]
[InlineData("http://localhost/api/value", "http://localhost/api/value", 0, null, "TraceContext", false, "/api/value")] // Request will be filtered
[InlineData("http://localhost/api/value", "http://localhost/api/value", 0, null, "TraceContext", false, "{ThrowException}")] // Filter user code will throw an exception
[InlineData("http://localhost/api/value/2", "http://localhost/api/value/2", 0, null, "CustomContextMatchParent")]
[InlineData("http://localhost/api/value/2", "http://localhost/api/value/2", 0, null, "CustomContextNonmatchParent")]
[InlineData("http://localhost/api/value/2", "http://localhost/api/value/2", 0, null, "CustomContextNonmatchParent", false, null, true)]
public void AspNetRequestsAreCollectedSuccessfully(
string expectedUrl,
string url,
int routeType,
string routeTemplate,
Expand Down Expand Up @@ -302,7 +306,7 @@ public void AspNetRequestsAreCollectedSuccessfully(
Assert.True(string.IsNullOrEmpty(span.GetStatus().Description));
}

var expectedUri = new Uri(url);
var expectedUri = new Uri(expectedUrl);
var actualUrl = span.GetTagValue(SemanticConventions.AttributeHttpUrl);

Assert.Equal(expectedUri.ToString(), actualUrl);
Expand Down
30 changes: 30 additions & 0 deletions test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,36 @@
"http.url": "http://{host}:{port}/path/to/resource/"
}
},
{
"name": "URL with fragment",
"method": "GET",
"url": "http://{host}:{port}/path/to/resource#fragment",
"responseCode": 200,
"spanName": "HTTP GET",
"spanStatus": "UNSET",
"responseExpected": true,
"spanAttributes": {
"http.method": "GET",
"http.host": "{host}:{port}",
"http.status_code": "200",
"http.url": "http://{host}:{port}/path/to/resource#fragment"
}
},
{
"name": "http.url must not contain username nor password",
"method": "GET",
"url": "http://username:password@{host}:{port}/path/to/resource#fragment",
"responseCode": 200,
"spanName": "HTTP GET",
"spanStatus": "UNSET",
"responseExpected": true,
"spanAttributes": {
"http.method": "GET",
"http.host": "{host}:{port}",
"http.status_code": "200",
"http.url": "http://{host}:{port}/path/to/resource#fragment"
}
},
{
"name": "Call that cannot resolve DNS will be reported as error span",
"method": "GET",
Expand Down