From 64b3f6d6dd2cdf32d471fefd2191b09bae1a3144 Mon Sep 17 00:00:00 2001 From: Brennan Date: Tue, 31 Aug 2021 09:35:12 -0700 Subject: [PATCH 1/4] Properly reject non-json FromBody parameter binding --- .../src/RequestDelegateFactory.cs | 22 ++++++- .../test/RequestDelegateFactoryTests.cs | 60 ++++++++++++++++++- 2 files changed, 80 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index ab30b6aecfc8..6d9f5c893ae9 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -558,6 +558,12 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var feature = httpContext.Features.Get(); if (feature?.CanHaveBody == true) { + if (!httpContext.Request.HasJsonContentType()) + { + Log.UnexpectedContentType(httpContext, httpContext.Request.ContentType); + httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; + return; + } try { bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType); @@ -590,6 +596,12 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, var feature = httpContext.Features.Get(); if (feature?.CanHaveBody == true) { + if (!httpContext.Request.HasJsonContentType()) + { + Log.UnexpectedContentType(httpContext, httpContext.Request.ContentType); + httpContext.Response.StatusCode = StatusCodes.Status415UnsupportedMediaType; + return; + } try { bodyValue = await httpContext.Request.ReadFromJsonAsync(bodyType); @@ -603,7 +615,7 @@ private static Expression AddResponseWritingToMethodCall(Expression methodCall, { Log.RequestBodyInvalidDataException(httpContext, ex, factoryContext.ThrowOnBadRequest); - httpContext.Response.StatusCode = 400; + httpContext.Response.StatusCode = StatusCodes.Status400BadRequest; return; } } @@ -1204,6 +1216,14 @@ public static void RequiredParameterNotProvided(HttpContext httpContext, string [LoggerMessage(4, LogLevel.Debug, RequiredParameterNotProvidedLogMessage, EventName = "RequiredParameterNotProvided")] private static partial void RequiredParameterNotProvided(ILogger logger, string parameterType, string parameterName, string source); + public static void UnexpectedContentType(HttpContext httpContext, string? contentType) + => UnexpectedContentType(GetLogger(httpContext), contentType ?? "(none)"); + + [LoggerMessage(6, LogLevel.Debug, + @"Expected json content-type but got {ContentType}.", + EventName = "UnexpectedContentType")] + private static partial void UnexpectedContentType(ILogger logger, string contentType); + private static ILogger GetLogger(HttpContext httpContext) { var loggerFactory = httpContext.RequestServices.GetRequiredService(); diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 9f9fd442589b..0845f5e088fb 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -2360,7 +2360,65 @@ public async Task CanExecuteRequestDelegateWithResultsExtension() Assert.False(httpContext.RequestAborted.IsCancellationRequested); var decodedResponseBody = Encoding.UTF8.GetString(responseBodyStream.ToArray()); Assert.Equal(@"""Hello Tester. This is from an extension method.""", decodedResponseBody); + } + + [Fact] + public async Task RequestDelegateRejectsNonJsonContent() + { + var httpContext = new DefaultHttpContext(); + httpContext.Request.Headers["Content-Type"] = "application/xml"; + httpContext.Request.Headers["Content-Length"] = "1"; + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(LoggerFactory); + httpContext.RequestServices = serviceCollection.BuildServiceProvider(); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, Todo todo) => + { + }); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(415, httpContext.Response.StatusCode); + var logMessage = Assert.Single(TestSink.Writes); + Assert.Equal(new EventId(6, "UnexpectedContentType"), logMessage.EventId); + Assert.Equal(LogLevel.Debug, logMessage.LogLevel); + } + + [Fact] + public async Task RequestDelegateWithBindAndImplicitBodyRejectsNonJsonContent() + { + Todo originalTodo = new() + { + Name = "Write more tests!" + }; + + var httpContext = new DefaultHttpContext(); + + var requestBodyBytes = JsonSerializer.SerializeToUtf8Bytes(originalTodo); + var stream = new MemoryStream(requestBodyBytes); + httpContext.Request.Body = stream; + httpContext.Request.Headers["Content-Type"] = "application/xml"; + httpContext.Request.Headers["Content-Length"] = stream.Length.ToString(CultureInfo.InvariantCulture); + httpContext.Features.Set(new RequestBodyDetectionFeature(true)); + var serviceCollection = new ServiceCollection(); + serviceCollection.AddSingleton(LoggerFactory); + httpContext.RequestServices = serviceCollection.BuildServiceProvider(); + + var factoryResult = RequestDelegateFactory.Create((HttpContext context, CustomTodo customTodo, Todo todo) => + { + }); + var requestDelegate = factoryResult.RequestDelegate; + + await requestDelegate(httpContext); + + Assert.Equal(415, httpContext.Response.StatusCode); + var logMessage = Assert.Single(TestSink.Writes); + Assert.Equal(new EventId(6, "UnexpectedContentType"), logMessage.EventId); + Assert.Equal(LogLevel.Debug, logMessage.LogLevel); } private DefaultHttpContext CreateHttpContext() @@ -2393,7 +2451,7 @@ private class CustomTodo : Todo Assert.Equal(typeof(CustomTodo), parameter.ParameterType); Assert.Equal("customTodo", parameter.Name); - var body = await context.Request.ReadFromJsonAsync(); + var body = await JsonSerializer.DeserializeAsync(context.Request.Body); context.Request.Body.Position = 0; return body; } From a330d0bc6670921d2a236c088597627686b63ad1 Mon Sep 17 00:00:00 2001 From: Brennan Date: Tue, 31 Aug 2021 11:50:41 -0700 Subject: [PATCH 2/4] Update src/Http/Http.Extensions/src/RequestDelegateFactory.cs Co-authored-by: Damian Edwards --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 6d9f5c893ae9..66b89f3d8c3c 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1220,7 +1220,7 @@ public static void UnexpectedContentType(HttpContext httpContext, string? conten => UnexpectedContentType(GetLogger(httpContext), contentType ?? "(none)"); [LoggerMessage(6, LogLevel.Debug, - @"Expected json content-type but got {ContentType}.", + @"Expected a supported JSON media type but got \"{ContentType}\".", EventName = "UnexpectedContentType")] private static partial void UnexpectedContentType(ILogger logger, string contentType); From 6112761dd9ce393ff1179151c8bf4f403c6bd2b7 Mon Sep 17 00:00:00 2001 From: Brennan Date: Tue, 31 Aug 2021 12:31:31 -0700 Subject: [PATCH 3/4] Update src/Http/Http.Extensions/src/RequestDelegateFactory.cs --- src/Http/Http.Extensions/src/RequestDelegateFactory.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs index 66b89f3d8c3c..80afaac46aad 100644 --- a/src/Http/Http.Extensions/src/RequestDelegateFactory.cs +++ b/src/Http/Http.Extensions/src/RequestDelegateFactory.cs @@ -1220,7 +1220,7 @@ public static void UnexpectedContentType(HttpContext httpContext, string? conten => UnexpectedContentType(GetLogger(httpContext), contentType ?? "(none)"); [LoggerMessage(6, LogLevel.Debug, - @"Expected a supported JSON media type but got \"{ContentType}\".", + "Expected a supported JSON media type but got \"{ContentType}\".", EventName = "UnexpectedContentType")] private static partial void UnexpectedContentType(ILogger logger, string contentType); From 5f18b0ee1880a1e692591afbbc339ee1b934156f Mon Sep 17 00:00:00 2001 From: Brennan Date: Thu, 2 Sep 2021 13:21:10 -0700 Subject: [PATCH 4/4] test --- .../test/RequestDelegateFactoryTests.cs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs index 0845f5e088fb..b35764bf4490 100644 --- a/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs +++ b/src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs @@ -2408,7 +2408,7 @@ public async Task RequestDelegateWithBindAndImplicitBodyRejectsNonJsonContent() serviceCollection.AddSingleton(LoggerFactory); httpContext.RequestServices = serviceCollection.BuildServiceProvider(); - var factoryResult = RequestDelegateFactory.Create((HttpContext context, CustomTodo customTodo, Todo todo) => + var factoryResult = RequestDelegateFactory.Create((HttpContext context, JsonTodo customTodo, Todo todo) => { }); var requestDelegate = factoryResult.RequestDelegate; @@ -2451,7 +2451,18 @@ private class CustomTodo : Todo Assert.Equal(typeof(CustomTodo), parameter.ParameterType); Assert.Equal("customTodo", parameter.Name); - var body = await JsonSerializer.DeserializeAsync(context.Request.Body); + var body = await context.Request.ReadFromJsonAsync(); + context.Request.Body.Position = 0; + return body; + } + } + + private class JsonTodo : Todo + { + public static async ValueTask BindAsync(HttpContext context, ParameterInfo parameter) + { + // manually call deserialize so we don't check content type + var body = await JsonSerializer.DeserializeAsync(context.Request.Body); context.Request.Body.Position = 0; return body; }