Sanitize http.url attribute (#1961)

open-telemetry · Apr 8, 2021 · 44cd88e · 44cd88e
1 parent d073906
commit 44cd88e
Show file tree

Hide file tree

Showing 8 changed files with 86 additions and 19 deletions.
diff --git a/src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.AspNet/CHANGELOG.md
@@ -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

diff --git a/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs b/src/OpenTelemetry.Instrumentation.AspNet/Implementation/HttpInListener.cs
@@ -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
                 {
@@ -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);
+        }
     }
 }
diff --git a/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md b/src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
@@ -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

diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerDiagnosticListener.cs
@@ -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));

diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpTagHelper.cs
@@ -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}";

diff --git a/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs b/src/OpenTelemetry.Instrumentation.Http/Implementation/HttpWebRequestActivitySource.netfx.cs
@@ -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));

diff --git a/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs b/test/OpenTelemetry.Instrumentation.AspNet.Tests/HttpInListenerTests.cs
@@ -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
+        [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
+        [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,
@@ -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);

diff --git a/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json b/test/OpenTelemetry.Instrumentation.Http.Tests/http-out-test-cases.json
@@ -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",