Review comments changed as requested

NLog · May 31, 2022 · 8603c6d · 8603c6d
1 parent b3b67ff
commit 8603c6d
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 76 deletions.
diff --git a/src/NLog.Web.AspNetCore/NLogRequestPostedBodyMiddleware.cs b/src/NLog.Web.AspNetCore/NLogRequestPostedBodyMiddleware.cs
@@ -19,14 +19,14 @@ public class NLogRequestPostedBodyMiddleware
     {
         private readonly RequestDelegate _next;
 
-        private readonly NLogRequestPostedBodyMiddlewareConfiguration _configuration;
+        private readonly NLogRequestPostedBodyMiddlewareOptions _configuration;
 
         /// <summary>
         /// Constructor that takes a configuration
         /// </summary>
         /// <param name="next"></param>
         /// <param name="configuration"></param>
-        public NLogRequestPostedBodyMiddleware(RequestDelegate next, NLogRequestPostedBodyMiddlewareConfiguration configuration)
+        public NLogRequestPostedBodyMiddleware(RequestDelegate next, NLogRequestPostedBodyMiddlewareOptions configuration)
         {
             _next = next;
             _configuration = configuration;
@@ -117,7 +117,7 @@ private async Task<string> GetString(Stream stream)
             using (var streamReader = new StreamReader(
                        stream,
                        Encoding.UTF8,
-                       _configuration.DetectEncodingFromByteOrderMark,
+                       true,
                        1024,
                        leaveOpen: true))
             {

diff --git a/...questPostedBodyMiddlewareConfiguration.cs → ...NLogRequestPostedBodyMiddlewareOptions.cs b/...questPostedBodyMiddlewareConfiguration.cs → ...NLogRequestPostedBodyMiddlewareOptions.cs
@@ -6,21 +6,27 @@ namespace NLog.Web
     /// <summary>
     /// Contains the configuration for the NLogRequestPostedBodyMiddleware
     /// </summary>
-    public class NLogRequestPostedBodyMiddlewareConfiguration
+    public class NLogRequestPostedBodyMiddlewareOptions
     {
         /// <summary>
         /// The default configuration
         /// </summary>
-        public static readonly NLogRequestPostedBodyMiddlewareConfiguration Default = new NLogRequestPostedBodyMiddlewareConfiguration();
+        internal static readonly NLogRequestPostedBodyMiddlewareOptions Default = new NLogRequestPostedBodyMiddlewareOptions();
 
         /// <summary>
-        /// Defaults to true
+        /// The default constructor
         /// </summary>
-        public bool DetectEncodingFromByteOrderMark { get; set; } = true;
+        public NLogRequestPostedBodyMiddlewareOptions()
+        {
+            ShouldCapture = DefaultCapture;
+        }
 
         /// <summary>
         /// The maximum request size that will be captured
-        /// Defaults to 30KB
+        /// Defaults to 30KB.  This checks against the ContentLength.
+        /// HttpRequest.EnableBuffer() writes the request to TEMP files on disk if the request ContentLength is > 30KB
+        /// but uses memory otherwise if &lt;= 30KB, so we should protect against "very large"
+        /// request post body payloads.
         /// </summary>
         public int MaximumRequestSize { get; set; } = 30 * 1024;
 
@@ -31,16 +37,15 @@ public class NLogRequestPostedBodyMiddlewareConfiguration
         /// only certain hosts, only below a certain request body size, and so forth
         /// </summary>
         /// <returns></returns>
-        public Predicate<HttpContext> ShouldCapture { get; set; } = DefaultCapture;
+        public Predicate<HttpContext> ShouldCapture { get; set; }
 
         /// <summary>
         /// The default predicate for ShouldCapture
         /// Returns true if content length &lt;= 30KB
         /// </summary>
-        public static bool DefaultCapture(HttpContext context)
+        private bool DefaultCapture(HttpContext context)
         {
-            return context?.Request?.ContentLength != null && context?.Request?.ContentLength <=
-                new NLogRequestPostedBodyMiddlewareConfiguration().MaximumRequestSize;
+            return context?.Request?.ContentLength != null && context?.Request?.ContentLength <= MaximumRequestSize;
         }
     }
 }
diff --git a/tests/NLog.Web.AspNetCore.Tests/NLogRequestPostedBodyMiddlewareConfigurationTests.cs b/tests/NLog.Web.AspNetCore.Tests/NLogRequestPostedBodyMiddlewareConfigurationTests.cs
@@ -10,46 +10,31 @@ public class NLogRequestPostedBodyMiddlewareConfigurationTests
         [Fact]
         public void SetMaximumRequestSizeTest()
         {
-            var config = new NLogRequestPostedBodyMiddlewareConfiguration();
+            var config = new NLogRequestPostedBodyMiddlewareOptions();
             var size = new Random().Next();
             config.MaximumRequestSize = size;
 
             Assert.Equal(size, config.MaximumRequestSize);
         }
 
-        [Fact]
-        public void SetByteOrderMarkTest()
-        {
-            var config = new NLogRequestPostedBodyMiddlewareConfiguration();
-            var bom = true;
-            config.DetectEncodingFromByteOrderMark = bom;
-
-            Assert.Equal(bom, config.DetectEncodingFromByteOrderMark);
-
-            bom = false;
-            config.DetectEncodingFromByteOrderMark = bom;
-
-            Assert.Equal(bom, config.DetectEncodingFromByteOrderMark);
-        }
-
         [Fact]
         public void GetDefault()
         {
-            var config = NLogRequestPostedBodyMiddlewareConfiguration.Default;
+            var config = NLogRequestPostedBodyMiddlewareOptions.Default;
 
             Assert.NotNull(config);
         }
 
         [Fact]
         public void DefaultCaptureTrue()
         {
-            var config = NLogRequestPostedBodyMiddlewareConfiguration.Default;
+            var config = NLogRequestPostedBodyMiddlewareOptions.Default;
 
             HttpContext httpContext = Substitute.For<HttpContext>();
 
             HttpRequest request = Substitute.For<HttpRequest>();
 
-            request.ContentLength.Returns(NLogRequestPostedBodyMiddlewareConfiguration.Default.MaximumRequestSize - 1);
+            request.ContentLength.Returns(NLogRequestPostedBodyMiddlewareOptions.Default.MaximumRequestSize - 1);
 
             httpContext.Request.Returns(request);
 
@@ -59,7 +44,7 @@ public void DefaultCaptureTrue()
         [Fact]
         public void DefaultCaptureFalseNullContentLength()
         {
-            var config = NLogRequestPostedBodyMiddlewareConfiguration.Default;
+            var config = NLogRequestPostedBodyMiddlewareOptions.Default;
 
             HttpContext httpContext = Substitute.For<HttpContext>();
 
@@ -75,13 +60,13 @@ public void DefaultCaptureFalseNullContentLength()
         [Fact]
         public void DefaultCaptureExcessiveContentLength()
         {
-            var config = NLogRequestPostedBodyMiddlewareConfiguration.Default;
+            var config = NLogRequestPostedBodyMiddlewareOptions.Default;
 
             HttpContext httpContext = Substitute.For<HttpContext>();
 
             HttpRequest request = Substitute.For<HttpRequest>();
 
-            request.ContentLength.Returns(NLogRequestPostedBodyMiddlewareConfiguration.Default.MaximumRequestSize + 1);
+            request.ContentLength.Returns(NLogRequestPostedBodyMiddlewareOptions.Default.MaximumRequestSize + 1);
 
             httpContext.Request.Returns(request);
 

diff --git a/tests/NLog.Web.AspNetCore.Tests/NLogRequestPostedBodyMiddlewareTests.cs b/tests/NLog.Web.AspNetCore.Tests/NLogRequestPostedBodyMiddlewareTests.cs
@@ -35,40 +35,7 @@ public void SuccessTest()
 
             long streamBeforePosition = defaultContext.Request.Body.Position;
 
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareConfiguration.Default);
-            middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
-
-            long streamAfterPosition = defaultContext.Request.Body.Position;
-
-            // Assert
-            Assert.NotNull(defaultContext.Items);
-            Assert.Single(defaultContext.Items);
-            Assert.NotNull(defaultContext.Items[AspNetRequestPostedBodyLayoutRenderer.NLogPostedRequestBodyKey]);
-            Assert.True(defaultContext.Items[AspNetRequestPostedBodyLayoutRenderer.NLogPostedRequestBodyKey] is string);
-            Assert.Equal("This is a test request body", defaultContext.Items[AspNetRequestPostedBodyLayoutRenderer.NLogPostedRequestBodyKey] as string);
-            Assert.Equal(streamBeforePosition, streamAfterPosition);
-        }
-
-        [Fact]
-        public void SuccessWithCustomConfigurationTest()
-        {
-            // Arrange
-            DefaultHttpContext defaultContext = new DefaultHttpContext();
-            defaultContext.Request.Body = new MemoryStream();
-            byte[] bodyBytes = Encoding.ASCII.GetBytes("This is a test request body");
-            defaultContext.Request.Body.Write(bodyBytes, 0, bodyBytes.Length);
-            defaultContext.Request.ContentLength = bodyBytes.Length;
-
-            // Act
-
-            long streamBeforePosition = defaultContext.Request.Body.Position;
-
-            var configuration = new NLogRequestPostedBodyMiddlewareConfiguration
-            {
-                DetectEncodingFromByteOrderMark = false
-            };
-
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, configuration);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             long streamAfterPosition = defaultContext.Request.Body.Position;
@@ -91,7 +58,7 @@ public void EmptyBodyTest()
             defaultContext.Request.ContentLength = 0;
 
             // Act
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareConfiguration.Default);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -107,7 +74,7 @@ public void NullContextTest()
             HttpContext defaultContext = null;
 
             // Act
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareConfiguration.Default);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -123,7 +90,7 @@ public void NullRequestTest()
             defaultContext.Request.ReturnsNull();
 
             // Act
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareConfiguration.Default);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next, NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -139,7 +106,7 @@ public void NullBodyTest()
             defaultContext.Request.Body = null;
 
             // Act
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareConfiguration.Default);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -157,7 +124,7 @@ public void ContentLengthTooLargeTest()
             defaultContext.Request.ContentLength = 30 * 1024 + 1;
 
             // Act
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareConfiguration.Default);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -175,7 +142,7 @@ public void MissingContentLengthTest()
             defaultContext.Request.ContentLength = null;
 
             // Act
-            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareConfiguration.Default);
+            var middlewareInstance = new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -196,7 +163,7 @@ public void CannotReadLengthTest()
 
             // Act
             var middlewareInstance =
-                new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareConfiguration.Default);
+                new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert
@@ -217,7 +184,7 @@ public void CannotSeekLengthTest()
 
             // Act
             var middlewareInstance =
-                new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareConfiguration.Default);
+                new NLogRequestPostedBodyMiddleware(Next,NLogRequestPostedBodyMiddlewareOptions.Default);
             middlewareInstance.Invoke(defaultContext).ConfigureAwait(false).GetAwaiter().GetResult();
 
             // Assert