Protect against UriFormatException

tracer/missing-nullability-files.csv

@@ -348,8 +348,6 @@ src/Datadog.Trace/Telemetry/DTOs/IPayload.cs
 src/Datadog.Trace/Telemetry/Metrics/IntegrationIdExtensions.cs
 src/Datadog.Trace/Telemetry/Transports/TelemetryTransportStrategy.cs
 src/Datadog.Trace/Util/Http/HttpRequestExtensions.Core.cs
-src/Datadog.Trace/Util/Http/HttpRequestExtensions.Framework.cs
-src/Datadog.Trace/Util/Http/HttpRequestUtils.cs
 src/Datadog.Trace/Util/Http/QueryStringManager.cs
 src/Datadog.Trace/AppSec/Waf/Initialization/LibraryLocationHelper.cs
 src/Datadog.Trace/AppSec/Waf/NativeBindings/DdwafConfigStruct.cs

tracer/src/Datadog.Trace/Util/Http/HttpRequestExtensions.Framework.cs

@@ -8,6 +8,8 @@
 using System.Web;
 using Datadog.Trace.ClrProfiler.AutoInstrumentation.AspNet;
 
+#nullable enable
+
 namespace Datadog.Trace.Util.Http
 {
     internal static partial class HttpRequestExtensions
@@ -34,15 +36,17 @@ internal static string GetUrlForSpan(this HttpRequestBase request, QueryStringMa
         /// <param name="bypassHttpRequestUrl">Whether or not to call access HttpRequest.Url (which caches the URL in HttpRequest).</param>
         /// <returns>The retrieved Url.</returns>
         /// <seealso cref="Configuration.ConfigurationKeys.FeatureFlags.BypassHttpRequestUrlCachingEnabled"/>
-        internal static string GetUrlForSpan(this HttpRequest request, QueryStringManager queryStringManager, bool bypassHttpRequestUrl)
+        internal static string? GetUrlForSpan(this HttpRequest request, QueryStringManager queryStringManager, bool bypassHttpRequestUrl)
         {
-            if (bypassHttpRequestUrl)
+            var url = bypassHttpRequestUrl ? RequestDataHelper.BuildUrl(request) : RequestDataHelper.GetUrl(request);
+
+            if (url is not null)
             {
-                return HttpRequestUtils.GetUrl(RequestDataHelper.BuildUrl(request), queryStringManager);
+                return HttpRequestUtils.GetUrl(url, queryStringManager);
             }
             else
             {
-                return HttpRequestUtils.GetUrl(RequestDataHelper.GetUrl(request), queryStringManager);
+                return null;
             }
         }
     }

tracer/src/Datadog.Trace/Util/Http/HttpRequestUtils.cs

@@ -7,13 +7,15 @@
 using Datadog.Trace.Configuration;
 using Datadog.Trace.Util.Http.QueryStringObfuscation;
 
+#nullable enable
+
 namespace Datadog.Trace.Util.Http
 {
     internal class HttpRequestUtils
     {
         private const string NoHostSpecified = "UNKNOWN_HOST";
 
-        internal static string GetUrl(Uri uri, QueryStringManager queryStringManager = null)
+        internal static string GetUrl(Uri uri, QueryStringManager? queryStringManager = null)
             => GetUrl(
                 uri.Scheme,
                 uri.Host,
@@ -23,7 +25,7 @@ internal static string GetUrl(Uri uri, QueryStringManager queryStringManager = n
                 uri.Query,
                 queryStringManager);
 
-        internal static string GetUrl(string scheme, string host, int? port, string pathBase, string path, string queryString, QueryStringManager queryStringManager = null)
+        internal static string GetUrl(string scheme, string host, int? port, string pathBase, string path, string queryString, QueryStringManager? queryStringManager = null)
         {
             if (queryStringManager != null)
             {
@@ -34,6 +36,6 @@ internal static string GetUrl(string scheme, string host, int? port, string path
             return $"{scheme}://{GetNormalizedHost(host)}{(port.HasValue ? $":{port}" : string.Empty)}{pathBase}{path}";
         }
 
-        internal static string GetNormalizedHost(string host) => string.IsNullOrEmpty(host) ? NoHostSpecified : host;
+        internal static string GetNormalizedHost(string? host) => StringUtil.IsNullOrEmpty(host) ? NoHostSpecified : host;
     }
 }

tracer/src/Datadog.Trace/Util/RequestDataHelper.cs

@@ -161,11 +161,12 @@ internal static IHeaderDictionary GetHeaders(HttpRequest request)
     /// <returns>The <c>Uri</c>; otherwise <see langword="null"/>.</returns>
     internal static Uri? GetUrl(HttpRequest request)
     {
+        // UriFormatException can happen if, for example, the request contains the variable "SERVER_NAME" with an invalid value
         try
         {
             return request.Url;
         }
-        catch (HttpRequestValidationException)
+        catch (Exception ex) when (ex is HttpRequestValidationException || ex is UriFormatException)
         {
             Log.Debug("Error reading request.Url from the request.");
             return null;
@@ -205,7 +206,16 @@ internal static IHeaderDictionary GetHeaders(HttpRequest request)
                 // it might impact error cases. Consider a situation in which some method in workerRequest throws.
                 // If we evaluate Path early, then some other method might throw, thus producing a different
                 // error behavior for the same conditions. Passing in a Func preserves the old ordering.
-                return duckRequest.BuildUrl(() => path);
+                // UriFormatException can happen if, for example, the request contains the variable "SERVER_NAME" with an invalid value
+                try
+                {
+                    return duckRequest.BuildUrl(() => path);
+                }
+                catch (UriFormatException)
+                {
+                    Log.Debug("Error building request.Url.");
+                    return null;
+                }
             }
         }
 

tracer/test/Datadog.Trace.Tests/Util/RequestDataHelperTests.cs

@@ -6,10 +6,12 @@
 #if NETFRAMEWORK
 using System;
 using System.Collections.Generic;
+using System.Collections.Specialized;
 using System.IO;
 using System.Reflection;
 using System.Web;
 using System.Web.Hosting;
+using Castle.Core.Internal;
 using Datadog.Trace.Agent;
 using Datadog.Trace.AppSec;
 using Datadog.Trace.AppSec.Coordinator;
@@ -46,6 +48,27 @@ public void GivenADangerousQueryString_WhenGetQueryString_HelperAvoidsException(
         }
     }
 
+#if NETFRAMEWORK
+    [Fact]
+    public void GivenAIncorrectHost_WhenGetUrl_HelperAvoidsException()
+    {
+        var workerRequest = new SimpleWorkerInvalidHost("/test", "/test", "test.aspx", null, new StringWriter());
+        var context = new HttpContext(workerRequest);
+        var request = context.Request;
+
+        try
+        {
+            var url = request.Url;
+            Assert.Fail();
+        }
+        catch (UriFormatException)
+        {
+            RequestDataHelper.BuildUrl(request).Should().BeNull();
+            RequestDataHelper.GetUrl(request).Should().BeNull();
+        }
+    }
+#endif
+
     [Fact]
     public void GivenADangerousBody_WhenGetQueryString_HelperAvoidsException()
     {
@@ -346,5 +369,19 @@ public void SetHost(string host)
             _host = host;
         }
     }
+
+    public class SimpleWorkerInvalidHost : SimpleWorkerRequest
+    {
+        public SimpleWorkerInvalidHost(string appVirtualDir, string appPhysicalDir, string page, string query, TextWriter output)
+            : base(appVirtualDir, appPhysicalDir, page, query, output)
+        {
+        }
+
+        public override string GetServerName()
+        {
+            return "invalid:rrr";
+        }
+    }
 }
+
 #endif