AspNetRequestPostedBody - Check ContentLength before using Body + Ad…

…ded MaxContentLength (default 30Kib) + update build script (#401) * AspNetExtensions - Make use of AddHttpContextAccessor * AspNetRequestPostedBody - Check ContentLength before using Body * AppVeyor - Include PullRequest in nuget-package-build * AspNetRequestPostedBody - Added MaxContentLength protection * OpenCover - Attempt to mergeoutput with NLog.Web.AspNetCore.Tests * NLog.Web.AspNetCore.Tests - Removed unnecessary references * docs * refactor (#1) * refactor + docs * more refactor
NLog · May 3, 2019 · f9cf329 · f9cf329
1 parent fc00355
commit f9cf329
Show file tree

Hide file tree

Showing 9 changed files with 237 additions and 55 deletions.
diff --git a/NLog.Web.AspNetCore.Tests/LayoutRenderers/AspNetRequestPostedBodyTests.cs b/NLog.Web.AspNetCore.Tests/LayoutRenderers/AspNetRequestPostedBodyTests.cs
@@ -2,10 +2,11 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Text;
-using System.Web;
 #if ASP_NET_CORE
 using Microsoft.AspNetCore.Http;
 using HttpContextBase = Microsoft.AspNetCore.Http.HttpContext;
+#else
+using System.Web;
 #endif
 using NLog.Web.LayoutRenderers;
 using NLog.Web.Tests.LayoutRenderers;
@@ -55,22 +56,64 @@ public void CorrectStreamRendersFullStreamAndRestorePosition(long position)
             Assert.Equal(position, stream.Position);
         }
 
+        [Theory]
+        [InlineData("", null, "")]
+        [InlineData("", 0, "")]
+        [InlineData("ABCDEFGHIJK", null, "ABCDEFGHIJK")]
+        [InlineData("ABCDEFGHIJK", 0, "ABCDEFGHIJK")]
+        [InlineData("ABCDEFGHIJK", 50, "ABCDEFGHIJK")]
+        [InlineData("ABCDEFGHIJK", 10, "")]
+        public void MaxContentLengthProtectsAgainstLargeBodyStreamTest(string body, int? maxContentLength, string expectedResult)
+        {
+            MaxContentLengthProtectsAgainstLargeBodyStream(body, maxContentLength, expectedResult, false);
+#if ASP_NET_CORE
+            MaxContentLengthProtectsAgainstLargeBodyStream(body, maxContentLength, expectedResult, true);
+#endif
+        }
+
+        private static void MaxContentLengthProtectsAgainstLargeBodyStream(string body, int? maxContentLength, string expectedResult, bool canReadOnce)
+        {
+            // Arrange
+            var (renderer, httpContext) = CreateWithHttpContext();
+            if (maxContentLength.HasValue)
+                renderer.MaxContentLength = maxContentLength.Value;
+
+            var stream = CreateStream(body, canReadOnce);
+            SetBodyStream(httpContext, stream);
+
+            var logEventInfo = new LogEventInfo();
+
+            // Act
+            string result = renderer.Render(logEventInfo);
+
+            // Assert
+            Assert.Equal(expectedResult, result);
+        }
+
         private static void SetBodyStream(HttpContextBase httpContext, Stream stream)
         {
 #if ASP_NET_CORE
             httpContext.Request.Body.Returns(stream);
 #else
             httpContext.Request.InputStream.Returns(stream);
 #endif
+            httpContext.Request.ContentLength.Returns((int)(stream?.Length ?? 0));
         }
 
-        private static MemoryStream CreateStream(string content)
+        private static MemoryStream CreateStream(string content, bool readOnce = false)
         {
-            var stream = new MemoryStream();
+            var stream = readOnce ? new ReadOnceStream() : new MemoryStream();
             var streamWriter = new StreamWriter(stream);
             streamWriter.Write(content);
             streamWriter.Flush();
+            if (readOnce)
+                stream.Position = 0;
             return stream;
         }
+
+        class ReadOnceStream : MemoryStream
+        {
+            public override bool CanSeek => false;
+        }
     }
 }
diff --git a/NLog.Web.AspNetCore.Tests/LayoutRenderers/LayoutRenderersTestBase.cs b/NLog.Web.AspNetCore.Tests/LayoutRenderers/LayoutRenderersTestBase.cs
@@ -1,12 +1,13 @@
-using System.Web;
-using NLog.Web.LayoutRenderers;
+using NLog.Web.LayoutRenderers;
 using NLog.Web.Tests;
 using NSubstitute;
 using Xunit;
 #if ASP_NET_CORE
 using Microsoft.AspNetCore.Http.Features;
 using Microsoft.AspNetCore.Routing;
 using HttpContextBase = Microsoft.AspNetCore.Http.HttpContext;
+#else
+using System.Web;
 #endif
 
 namespace NLog.Web.Tests.LayoutRenderers

diff --git a/NLog.Web.AspNetCore.Tests/NLog.Web.AspNetCore.Tests.csproj b/NLog.Web.AspNetCore.Tests/NLog.Web.AspNetCore.Tests.csproj
@@ -20,9 +20,6 @@
   <ItemGroup>
     <ProjectReference Include="..\NLog.Web.AspNetCore\NLog.Web.AspNetCore.csproj" />
     <PackageReference Include="NSubstitute" Version="4.0.0" />
-
-    <PackageReference Include="System.Runtime.InteropServices" Version="4.3.0" />
-
     <PackageReference Include="System.ValueTuple" Version="4.5.0" />
     <PackageReference Include="xunit.runner.visualstudio" Version="2.4.1" />
     <PackageReference Include="xunit" Version="2.4.1" />
@@ -41,9 +38,4 @@
   <ItemGroup>
     <Folder Include="Properties\" />
   </ItemGroup>
-  <ItemGroup>
-    <Reference Include="System.Web">
-      <HintPath>C:\Windows\Microsoft.NET\Framework\v2.0.50727\System.Web.dll</HintPath>
-    </Reference>
-  </ItemGroup>
 </Project>
diff --git a/NLog.Web.AspNetCore/AspNetExtensions.cs b/NLog.Web.AspNetCore/AspNetExtensions.cs
@@ -178,7 +178,7 @@ private static void ConfigureServicesNLog(NLogAspNetCoreOptions options, IServic
             //note: this one is called before  services.AddSingleton<ILoggerFactory>
             if ((options ?? NLogAspNetCoreOptions.Default).RegisterHttpContextAccessor)
             {
-                services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
+                services.AddHttpContextAccessor();
             }
         }
 #endif

diff --git a/NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestPostedBody.cs b/NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestPostedBody.cs
@@ -8,6 +8,9 @@
 
 #if !ASP_NET_CORE
 using System.Web;
+using HttpRequest = System.Web.HttpRequestBase;
+#else
+using HttpRequest = Microsoft.AspNetCore.Http.HttpRequest;
 #endif
 
 namespace NLog.Web.LayoutRenderers
@@ -25,6 +28,16 @@ namespace NLog.Web.LayoutRenderers
     [ThreadSafe]
     public class AspNetRequestPostedBody : AspNetLayoutRendererBase
     {
+        private const int Size64KiloBytes = 64 * 1024;
+        private const int Size30Kilobytes = 30 * 1024;
+
+        /// <summary>
+        /// Max size in bytes of the body. Skip logging of the body when larger.
+        /// Default 30720 Bytes = 30 KiB 
+        /// (0 = No limit, -1 = No Buffer Limit)
+        /// </summary>
+        public int MaxContentLength { get; set; } = Size30Kilobytes;
+
         /// <summary>
         /// Renders the ASP.NET posted body
         /// </summary>
@@ -38,38 +51,137 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent)
                 return;
             }
 
+            long? contentLength = httpRequest.ContentLength;
+            if (!TryGetBody(httpRequest, contentLength, out var body))
+            {
+                return; // No Body to read
+            }
+
+            var content = BodyToString(body);
+            builder.Append(content);
+        }
+
+        private static string BodyToString(Stream body)
+        {
+            var oldPosition = body.Position;
+            body.Position = 0;
+            try
+            {
+                // Note: don't dispose the StreamReader, it will close the stream and that's unwanted. You could pass that that
+                // to the StreamReader in some platforms, but then the dispose will be a NOOP, so for platform compat just don't dispose
+                var bodyReader = new StreamReader(body);
+                var content = bodyReader.ReadToEnd();
+                return content;
+            }
+            finally
+            {
+                //restore
+                body.Position = oldPosition;
+            }
+        }
+
+        private bool TryGetBody(HttpRequest httpRequest, long? contentLength, out Stream body)
+        {
+            body = null;
+            if (contentLength <= 0)
+            {
+                return false;
+            }
+
+            if (MaxContentLength > 0 && contentLength > MaxContentLength)
+            {
+                InternalLogger.Debug("AspNetRequestPostedBody: body stream is too big. ContentLength={0}", contentLength);
+                return false;
+            }
+
+            body = GetBodyStream(httpRequest);
+
+            if (body == null)
+            {
+                InternalLogger.Debug("AspNetRequestPostedBody: body stream was null");
+                return false;
+            }
+
+            if (!body.CanRead)
+            {
+                InternalLogger.Debug("AspNetRequestPostedBody: body stream has been closed");
+                return false;
+            }
+
+            if (!body.CanSeek)
+            {
+                var oldPosition = body.Position;
+                if (oldPosition > 0 && oldPosition >= contentLength)
+                {
+                    InternalLogger.Debug("AspNetRequestPostedBody: body stream cannot seek and already read. StreamPosition={0}", oldPosition);
+                    return false;
+                }
+
+                if (!TryEnableBuffering(httpRequest, contentLength, out body))
+                    return false;
+            }
+            else
+            {
+                if (MaxContentLength > 0 && !contentLength.HasValue && body.Length > MaxContentLength)
+                {
+                    InternalLogger.Debug("AspNetRequestPostedBody: body stream too big. Body.Length={0}", body.Length);
+                    body = null;
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        private static Stream GetBodyStream(HttpRequest httpRequest)
+        {
 #if !ASP_NET_CORE
             var body = httpRequest.InputStream;
 #else
             var body = httpRequest.Body;
 #endif
+            return body;
+        }
 
-            if (body == null)
+        ///<returns>Can seek now?</returns>
+        private bool TryEnableBuffering(HttpRequest httpRequest, long? contentLength, out Stream bodyStream)
+        {
+            bodyStream = null;
+
+            if (MaxContentLength >= 0 && !contentLength.HasValue)
             {
-                InternalLogger.Debug("AspNetRequestPostedBody: body stream was null");
-                return;
+                InternalLogger.Debug("AspNetRequestPostedBody: body stream cannot seek with unknown ContentLength");
+                return false;
             }
 
-            long oldPosition = -1;
-
-            // reset if possible
-            if (body.CanSeek)
+            int bufferThreshold = MaxContentLength <= 0 ? Size64KiloBytes : MaxContentLength;
+            if (MaxContentLength == 0 && contentLength > bufferThreshold)
             {
-                oldPosition = body.Position;
-                body.Position = 0;
+                InternalLogger.Debug("AspNetRequestPostedBody: body stream cannot seek and stream is too big. ContentLength={0}", contentLength);
+                return false;
             }
 
-            //note: dispose of StreamReader isn't doing things besides closing the stream (which can be turn off, and then it's a NOOP)
-            var bodyReader = new StreamReader(body);
-            var content = bodyReader.ReadToEnd();
-
-            //restore
-            if (body.CanSeek)
+            bodyStream = EnableRewind(httpRequest, bufferThreshold);
+            if (bodyStream?.CanSeek != true)
             {
-                body.Position = oldPosition;
+                InternalLogger.Debug("AspNetRequestPostedBody: body stream cannot seek");
+                return false;
             }
 
-            builder.Append(content);
+            return true;
+        }
+
+        private static Stream EnableRewind(HttpRequest httpRequest, int bufferThreshold)
+        {
+#if ASP_NET_CORE2
+            Microsoft.AspNetCore.Http.HttpRequestRewindExtensions.EnableBuffering(httpRequest, bufferThreshold);
+            return httpRequest.Body;
+#elif ASP_NET_CORE1
+            Microsoft.AspNetCore.Http.Internal.BufferingHelper.EnableRewind(httpRequest, bufferThreshold);
+            return httpRequest.Body;
+#else
+            return null;
+#endif
         }
     }
-}
+}
diff --git a/NLog.Web.AspNetCore/NLog.Web.AspNetCore.csproj b/NLog.Web.AspNetCore/NLog.Web.AspNetCore.csproj
@@ -5,14 +5,7 @@
 
     <TargetFrameworks>netstandard1.5;net451;net461;netstandard2.0</TargetFrameworks>
 
-    <VersionPrefix>4.8.1</VersionPrefix>
-    <VersionSuffix></VersionSuffix>
-    <Version>$(VersionPrefix)$(VersionSuffix)</Version>
-    <InformationalVersion>$(Version)</InformationalVersion>
-    <FileVersion>$(VersionPrefix).0</FileVersion>
-    <FileVersion Condition="'$(APPVEYOR_BUILD_NUMBER)' != ''">$(VersionPrefix).$(APPVEYOR_BUILD_NUMBER)</FileVersion>
-
-    <Product>NLog.Web.AspNetCore v$(Version)</Product>
+    <Product>NLog.Web.AspNetCore v$(VersionPrefix)</Product>
     <Description>
 Use NLog with the ASP.NET Core platform. Adds helpers and layout renderers for websites and webapplications.
 
@@ -37,7 +30,6 @@ Supported platforms:
     <PackageLicenseUrl>https://github.com/NLog/NLog.Web/blob/master/LICENSE</PackageLicenseUrl>
     <RepositoryType>git</RepositoryType>
     <RepositoryUrl>git://github.com/NLog/NLog.Web</RepositoryUrl>
-    <GeneratePackageOnBuild>True</GeneratePackageOnBuild>
 
     <SignAssembly>true</SignAssembly>
     <AssemblyVersion>4.0.0.0</AssemblyVersion>

diff --git a/appveyor.yml b/appveyor.yml
@@ -7,8 +7,8 @@ assembly_info:
   patch: true
   file: '**\AssemblyInfo.cs'
   assembly_version: '4.0.0'
-  assembly_file_version: '4.8.1.{build}' #NLog.Web
-  assembly_informational_version: '4.8.1' #NLog.Web
+  assembly_file_version: '{version}' #NLog.Web
+  assembly_informational_version: '{version}' #NLog.Web
 nuget:
   project_feed: true
 matrix:
@@ -24,10 +24,27 @@ environment:
 skip_tags: true
 
 build_script:
-- cmd: >-
-    call build_aspnet.bat -nuget_version=4.8.1 # NLog.Web package
-
-    call build_aspnetcore.bat # update NLog.Web.AspNetCore.csproj for version number
+- ps: |
+    $versionPrefix = "4.8.1"
+    $versionSuffix = ""
+    $versionBuild = $versionPrefix + "." + ${env:APPVEYOR_BUILD_NUMBER}
+    $versionNuget = $versionPrefix
+    if ($env:APPVEYOR_PULL_REQUEST_NUMBER)
+    {
+        $versionPrefix = $versionBuild
+        $versionSuffix = "PR" + $env:APPVEYOR_PULL_REQUEST_NUMBER
+        $versionNuget = $versionPrefix + "-" + $versionSuffix
+    }
+    $build_aspnet = "build_aspnet.bat", "-nuget_version=$versionNuget"
+    & cmd /c $build_aspnet
+    if ($LastExitCode -ne 0) {
+        throw "Exec: $ErrorMessage"
+    }
+    $build_aspnetcore = "build_aspnetcore.bat", "-version_prefix=$versionPrefix", "-version_suffix=$versionSuffix", "-version_build=$versionBuild"
+    & cmd /c $build_aspnetcore
+    if ($LastExitCode -ne 0) {
+        throw "Exec: $ErrorMessage"
+    }
 
 deploy:
 - provider: NuGet
@@ -38,7 +55,8 @@ deploy:
     branch: master
 test_script:
   - nuget.exe install OpenCover -ExcludeVersion -DependencyVersion Ignore
-  - OpenCover\tools\OpenCover.Console.exe -register:user -target:"%xunit20%\xunit.console.x86.exe" -targetargs:"\"c:\projects\nlogweb\NLog.Web.Tests\bin\Release\NLog.Web.Tests.dll\" -appveyor -noshadow"  -returntargetcode -filter:"+[NLog.Web]*" -excludebyattribute:*.ExcludeFromCodeCoverage* -hideskipped:All -output:coverage.xml
+  - OpenCover\tools\OpenCover.Console.exe -register:user -target:"%xunit20%\xunit.console.x86.exe" -targetargs:"\"c:\projects\nlogweb\NLog.Web.Tests\bin\Release\NLog.Web.Tests.dll\" -appveyor -noshadow"  -returntargetcode -filter:"+[NLog.Web]*" -excludebyattribute:*.ExcludeFromCodeCoverage* -hideskipped:All -oldstyle -output:coverage.xml
+  - OpenCover\tools\OpenCover.Console.exe -register:user -mergeoutput -target:"%xunit20%\xunit.console.x86.exe" -targetargs:"\"c:\projects\nlogweb\NLog.Web.AspNetCore.Tests\bin\Release\net461\NLog.Web.AspNetCore.Tests.dll\" -appveyor -noshadow" -filter:"+[NLog.Web]*" -excludebyattribute:*.ExcludeFromCodeCoverage* -hideskipped:All -oldstyle -output:coverage.xml
   - pip install codecov
   - codecov -f "coverage.xml"
   - ps: .\test_aspnetcore.ps1