Fix: ArgumentNullException in FormRequestPayloadExtractor when handling invalid form data on ASP.NET

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixes
+
+- Fixed ArgumentNullException on ASP.NET in FormRequestPayloadExtractor when handling poorly structured form data ([#3734](https://github.com/getsentry/sentry-dotnet/pull/3734))
+
 ### Dependencies
 
 - Bump Cocoa SDK from v8.36.0 to v8.39.0 ([#3727](https://github.com/getsentry/sentry-dotnet/pull/3727))

samples/Sentry.Samples.AspNetCore.Basic/Program.cs

@@ -1,3 +1,6 @@
+using Microsoft.AspNetCore.Mvc;
+using Sentry.Extensibility;
+
 var builder = WebApplication.CreateBuilder(args);
 
 builder.WebHost.UseSentry(options =>
@@ -7,42 +10,56 @@
 
     // Enable Sentry performance monitoring
     options.TracesSampleRate = 1.0;
+    options.MaxRequestBodySize = RequestSize.Always;
 
 #if DEBUG
     // Log debug information about the Sentry SDK
     options.Debug = true;
 #endif
 });
 
+// Register HttpClient as a service
+builder.Services.AddHttpClient("local", client =>
+{
+    client.BaseAddress = new Uri("http://localhost:59740");
+});
+
 var app = builder.Build();
 
 // An example ASP.NET Core middleware that throws an
 // exception when serving a request to path: /throw
-app.MapGet("/throw/{message?}", context =>
+app.MapGet("/throw/{message?}", async ([FromServices] IHttpClientFactory clientFactory) =>
 {
-    var exceptionMessage = context.GetRouteValue("message") as string;
-
-    var log = context.RequestServices.GetRequiredService<ILoggerFactory>()
-        .CreateLogger<Program>();
-
-    log.LogInformation("Handling some request...");
+    try
+    {
+        var client = clientFactory.CreateClient("local");
+        var content = new StringContent("x=1&y=2&", System.Text.Encoding.UTF8, "application/x-www-form-urlencoded");
+        var response = await client.PostAsync("/submit", content);
+        var responseString = await response.Content.ReadAsStringAsync();
 
-    var hub = context.RequestServices.GetRequiredService<IHub>();
-    hub.ConfigureScope(s =>
+        return Results.Content(responseString, "application/json");
+    }
+    catch (Exception e)
     {
-        // More data can be added to the scope like this:
-        s.SetTag("Sample", "ASP.NET Core"); // indexed by Sentry
-        s.SetExtra("Extra!", "Some extra information");
-    });
-
-    log.LogInformation("Logging info...");
-    log.LogWarning("Logging some warning!");
-
-    // The following exception will be captured by the SDK and the event
-    // will include the Log messages and any custom scope modifications
-    // as exemplified above.
-    throw new Exception(
-        exceptionMessage ?? "An exception thrown from the ASP.NET Core pipeline");
+        throw new Exception("An error occurred while processing the request", e);
+    }
+});
+
+// POST endpoint to handle form submission
+app.MapPost("/submit", async (HttpContext context) =>
+{
+    await Task.Delay(50); // Simulate a delay
+    throw new Exception("Test exception - needs a body");
+    // var form = await context.Request.ReadFormAsync();
+    // var x = form["x"];
+    // var y = form["y"];
+    //
+    // var log = context.RequestServices.GetRequiredService<ILoggerFactory>()
+    //     .CreateLogger<Program>();
+    //
+    // log.LogInformation("Received form data: x={x}, y={y}", x, y);
+    //
+    // return Results.Ok(new { x, y });
 });
 
 app.Run();

src/Sentry.AspNet/Internal/SystemWebHttpRequest.cs

@@ -1,3 +1,4 @@
+using System.Collections.Specialized;
 using Sentry.Extensibility;
 
 namespace Sentry.AspNet.Internal;
@@ -12,8 +13,19 @@ internal class SystemWebHttpRequest : IHttpRequest
 
     public Stream? Body => _request?.InputStream;
 
-    public IEnumerable<KeyValuePair<string, IEnumerable<string>>>? Form
-        => _request.Form.AllKeys.Select(kv => new KeyValuePair<string, IEnumerable<string>>(kv, _request.Form.GetValues(kv)));
+    public IEnumerable<KeyValuePair<string, IEnumerable<string>>>? Form => GetFormData(_request.Form);
 
     public SystemWebHttpRequest(HttpRequest request) => _request = request;
+
+    internal static IEnumerable<KeyValuePair<string, IEnumerable<string>>> GetFormData(NameValueCollection formdata)
+    {
+        return StripNulls(formdata.AllKeys).Select(key => new KeyValuePair<string, IEnumerable<string>>(
+            key, StripNulls(formdata.GetValues(key)
+            )));
+
+        // Poorly constructed form submissions can result in null keys/values on .NET Framework.
+        // See: https://github.com/getsentry/sentry-dotnet/issues/3701
+        IEnumerable<string> StripNulls(IEnumerable<string>? values) => values?.Where(x => x is not null) ?? [];
+    }
+
 }

src/Sentry/Extensibility/FormRequestPayloadExtractor.cs

@@ -10,9 +10,9 @@ public class FormRequestPayloadExtractor : BaseRequestPayloadExtractor
     /// <summary>
     /// Supports <see cref="IHttpRequest"/> with content type application/x-www-form-urlencoded.
     /// </summary>
-    protected override bool IsSupported(IHttpRequest request)
-        => SupportedContentType
-            .Equals(request.ContentType, StringComparison.InvariantCulture);
+    protected override bool IsSupported(IHttpRequest request) =>
+        SupportedContentType.Equals(request.ContentType, StringComparison.InvariantCulture)
+        || (request.ContentType?.StartsWith($"{SupportedContentType};", StringComparison.InvariantCulture) == true);
 
     /// <summary>
     /// Extracts the request form data as a dictionary.

test/Sentry.AspNet.Tests/Internal/SystemWebHttpRequestTests.cs

@@ -0,0 +1,48 @@
+using System.Collections.Specialized;
+using Sentry.AspNet.Internal;
+
+namespace Sentry.AspNet.Tests.Internal;
+
+public class SystemWebHttpRequestTests
+{
+    [Fact]
+    public void GetFormData_GoodData_ReturnsCorrectValues()
+    {
+        // Arrange
+        var formCollection = new NameValueCollection
+        {
+            { "key1", "value1" },
+            { "key2", "value2" }
+        };
+
+        // Act
+        var form = SystemWebHttpRequest.GetFormData(formCollection).ToDict();
+
+        // Assert
+        form.Should().NotBeNull();
+        form.Should().Contain(kvp => kvp.Key == "key1" && kvp.Value.Contains("value1"));
+        form.Should().Contain(kvp => kvp.Key == "key2" && kvp.Value.Contains("value2"));
+    }
+
+    [Fact]
+    public void GetFormData_BadData_ReturnsCorrectValues()
+    {
+        // Arrange
+        var formCollection = new NameValueCollection
+        {
+            { "key1", "value1" },
+            { "key2", "value2" },
+            { null, "badkey" },
+            { "badvalue", null },
+            { null, null }
+        };
+
+        // Act
+        var form = SystemWebHttpRequest.GetFormData(formCollection).ToDict();
+
+        // Assert
+        form.Should().NotBeNull();
+        form.Should().Contain(kvp => kvp.Key == "key1" && kvp.Value.Contains("value1"));
+        form.Should().Contain(kvp => kvp.Key == "key2" && kvp.Value.Contains("value2"));
+    }
+}

test/Sentry.AspNetCore.Tests/BaseRequestPayloadExtractorTests.cs

@@ -32,6 +32,7 @@ public void ExtractPayload_OriginalStreamPosition_Reset()
     {
         const int originalPosition = 100;
         _ = TestFixture.Stream.Position.Returns(originalPosition);
+        TestFixture.HttpRequestCore.ContentType.Returns(SupportedContentType);
 
         var sut = TestFixture.GetSut();
 
@@ -40,6 +41,8 @@ public void ExtractPayload_OriginalStreamPosition_Reset()
         TestFixture.Stream.Received().Position = originalPosition;
     }
 
+    protected abstract string SupportedContentType { get; }
+
     [Fact]
     public void ExtractPayload_OriginalStream_NotClosed()
     {

test/Sentry.AspNetCore.Tests/DefaultRequestPayloadExtractorTests.cs

@@ -2,6 +2,8 @@ namespace Sentry.AspNetCore.Tests;
 
 public class DefaultRequestPayloadExtractorTests : BaseRequestPayloadExtractorTests<DefaultRequestPayloadExtractor>
 {
+    protected override string SupportedContentType => string.Empty;
+
     [Fact]
     public void ExtractPayload_StringData_ReadCorrectly()
     {

test/Sentry.AspNetCore.Tests/FormRequestPayloadExtractorTests.cs

@@ -5,15 +5,20 @@ namespace Sentry.AspNetCore.Tests;
 
 public class FormRequestPayloadExtractorTests : BaseRequestPayloadExtractorTests<FormRequestPayloadExtractor>
 {
+    protected override string SupportedContentType => "application/x-www-form-urlencoded";
+
     public FormRequestPayloadExtractorTests()
     {
         TestFixture = new Fixture();
-        _ = TestFixture.HttpRequest.ContentType.Returns("application/x-www-form-urlencoded");
     }
 
-    [Fact]
-    public void ExtractPayload_SupportedContentType_ReadForm()
+    [Theory]
+    [InlineData("application/x-www-form-urlencoded")]
+    [InlineData("application/x-www-form-urlencoded; charset=utf-8")]
+    public void ExtractPayload_SupportedContentType_ReadForm(string contentType)
     {
+        TestFixture.HttpRequest.ContentType.Returns(contentType);
+
         var expected = new Dictionary<string, StringValues> { { "key", new StringValues("val") } };
         var f = new FormCollection(expected);
         _ = TestFixture.HttpRequestCore.Form.Returns(f);
@@ -32,7 +37,7 @@ public void ExtractPayload_SupportedContentType_ReadForm()
     [Fact]
     public void ExtractPayload_UnsupportedContentType_DoesNotReadStream()
     {
-        _ = TestFixture.HttpRequest.ContentType.Returns("application/json");
+        TestFixture.HttpRequest.ContentType.Returns("application/json");
 
         var sut = TestFixture.GetSut();