From f9cf329c2daa59eca3ec355ff2229177506b99ac Mon Sep 17 00:00:00 2001 From: Rolf Kristensen Date: Sat, 4 May 2019 01:40:14 +0200 Subject: [PATCH] AspNetRequestPostedBody - Check ContentLength before using Body + Added 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 --- .../AspNetRequestPostedBodyTests.cs | 49 +++++- .../LayoutRenderersTestBase.cs | 5 +- .../NLog.Web.AspNetCore.Tests.csproj | 8 - NLog.Web.AspNetCore/AspNetExtensions.cs | 2 +- .../AspNetRequestPostedBody.cs | 148 +++++++++++++++--- .../NLog.Web.AspNetCore.csproj | 10 +- appveyor.yml | 32 +++- build_aspnet.bat | 7 +- build_aspnetcore.bat | 31 +++- 9 files changed, 237 insertions(+), 55 deletions(-) diff --git a/NLog.Web.AspNetCore.Tests/LayoutRenderers/AspNetRequestPostedBodyTests.cs b/NLog.Web.AspNetCore.Tests/LayoutRenderers/AspNetRequestPostedBodyTests.cs index be9ee85d..66b9dc4c 100644 --- 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,6 +56,40 @@ 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 @@ -62,15 +97,23 @@ private static void SetBodyStream(HttpContextBase httpContext, Stream 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 index f6b0bb15..e9393c24 100644 --- a/NLog.Web.AspNetCore.Tests/LayoutRenderers/LayoutRenderersTestBase.cs +++ b/NLog.Web.AspNetCore.Tests/LayoutRenderers/LayoutRenderersTestBase.cs @@ -1,5 +1,4 @@ -using System.Web; -using NLog.Web.LayoutRenderers; +using NLog.Web.LayoutRenderers; using NLog.Web.Tests; using NSubstitute; using Xunit; @@ -7,6 +6,8 @@ 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 index c3a3034e..23a232e3 100644 --- a/NLog.Web.AspNetCore.Tests/NLog.Web.AspNetCore.Tests.csproj +++ b/NLog.Web.AspNetCore.Tests/NLog.Web.AspNetCore.Tests.csproj @@ -20,9 +20,6 @@ - - - @@ -41,9 +38,4 @@ - - - C:\Windows\Microsoft.NET\Framework\v2.0.50727\System.Web.dll - - diff --git a/NLog.Web.AspNetCore/AspNetExtensions.cs b/NLog.Web.AspNetCore/AspNetExtensions.cs index ae0f2324..60a8a5eb 100644 --- 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 if ((options ?? NLogAspNetCoreOptions.Default).RegisterHttpContextAccessor) { - services.TryAddSingleton(); + services.AddHttpContextAccessor(); } } #endif diff --git a/NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestPostedBody.cs b/NLog.Web.AspNetCore/LayoutRenderers/AspNetRequestPostedBody.cs index b84fd70e..ed21c438 100644 --- 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; + + /// + /// 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) + /// + public int MaxContentLength { get; set; } = Size30Kilobytes; + /// /// Renders the ASP.NET posted body /// @@ -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) + ///Can seek now? + 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 } } -} \ No newline at end of file +} diff --git a/NLog.Web.AspNetCore/NLog.Web.AspNetCore.csproj b/NLog.Web.AspNetCore/NLog.Web.AspNetCore.csproj index c9074417..a0f3021a 100644 --- a/NLog.Web.AspNetCore/NLog.Web.AspNetCore.csproj +++ b/NLog.Web.AspNetCore/NLog.Web.AspNetCore.csproj @@ -5,14 +5,7 @@ netstandard1.5;net451;net461;netstandard2.0 - 4.8.1 - - $(VersionPrefix)$(VersionSuffix) - $(Version) - $(VersionPrefix).0 - $(VersionPrefix).$(APPVEYOR_BUILD_NUMBER) - - NLog.Web.AspNetCore v$(Version) + NLog.Web.AspNetCore v$(VersionPrefix) Use NLog with the ASP.NET Core platform. Adds helpers and layout renderers for websites and webapplications. @@ -37,7 +30,6 @@ Supported platforms: https://github.com/NLog/NLog.Web/blob/master/LICENSE git git://github.com/NLog/NLog.Web - True true 4.0.0.0 diff --git a/appveyor.yml b/appveyor.yml index 8592a7c9..0e9f3d23 100644 --- 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 diff --git a/build_aspnet.bat b/build_aspnet.bat index 358edd96..5356b23c 100644 --- a/build_aspnet.bat +++ b/build_aspnet.bat @@ -1,14 +1,17 @@ -echo off +@echo off rem fallback if not passed -set nuget_version=2.2.0 +set nuget_version=1.0.0 call :read_params %* nuget restore NLog.Web.sln -verbosity quiet msbuild NLog.Web.sln /verbosity:minimal /t:rebuild /p:configuration=release +IF ERRORLEVEL 1 EXIT /B 1 nuget pack NLog.Web\NLog.Web.csproj -properties Configuration=Release;Platform=AnyCPU -version %nuget_version% +IF ERRORLEVEL 1 EXIT /B 1 nuget pack NLog.Web\NLog.Web.csproj -properties Configuration=Release;Platform=AnyCPU -version %nuget_version% -symbols +IF ERRORLEVEL 1 EXIT /B 1 rem read pass parameters by name :read_params diff --git a/build_aspnetcore.bat b/build_aspnetcore.bat index 474fdcb5..4abb8389 100644 --- a/build_aspnetcore.bat +++ b/build_aspnetcore.bat @@ -1,7 +1,28 @@ -echo off +@echo off -rem update NLog.Web.AspNetCore.csproj for version number +rem fallback if not passed +set version_prefix=1.0.0 +set version_suffix= +set version_build=%version_prefix% -cd NLog.Web.AspNetCore -msbuild /t:restore /t:build /p:configuration=release /verbosity:minimal /p:IncludeSymbols=true /p:SymbolPackageFormat=snupkg -cd .. +call :read_params %* + +msbuild .\NLog.Web.AspNetCore /t:restore,pack /p:configuration=release /verbosity:minimal /p:IncludeSymbols=true /p:SymbolPackageFormat=snupkg /p:VersionPrefix=%version_prefix% /p:FileVersion=%version_build% /p:VersionSuffix=%version_suffix% +IF ERRORLEVEL 1 EXIT /B 1 + +rem read pass parameters by name +:read_params +if not %1/==/ ( + if not "%__var%"=="" ( + if not "%__var:~0,1%"=="-" ( + endlocal + goto read_params + ) + endlocal & set %__var:~1%=%~1 + ) else ( + setlocal & set __var=%~1 + ) + shift + goto read_params +) +exit /B \ No newline at end of file