From 73f626f304be266a4cbaadb639c5ffb26b5d9330 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 19 Jan 2023 16:28:24 -0500 Subject: [PATCH 1/4] Add Index property to RequertIpLayoutRenderer for mal-formed X-Forwarded-For headers where the desired client IP is NOT at the start of the comma separated list --- .../AspNetRequestIpLayoutRenderer.cs | 27 +++++++-- .../AspNetRequestIpLayoutRendererTests.cs | 58 +++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs index df7a843d..018f9148 100644 --- a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs @@ -1,7 +1,5 @@ -using System; -using System.ComponentModel; +using System.ComponentModel; using System.Text; -using NLog.Config; using NLog.LayoutRenderers; using NLog.Layouts; using NLog.Web.Internal; @@ -17,7 +15,10 @@ namespace NLog.Web.LayoutRenderers /// ASP.NET Request IP address of the remote client /// /// - /// ${aspnet-request-ip} + /// ${aspnet-request-ip} to return the Remote IP + /// ${aspnet-request-ip:CheckForwardedForHeader=true} to return first element in the X-Forwarded-For header + /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=1} to return second element in the X-Forwarded-For header + /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=1:ForwardedForHeader=myHeader} to return second element in the myHeader header /// /// Documentation on NLog Wiki [LayoutRenderer("aspnet-request-ip")] @@ -35,6 +36,12 @@ public class AspNetRequestIpLayoutRenderer : AspNetLayoutRendererBase /// public bool CheckForwardedForHeader { get; set; } + /// + /// Gets or sets the array index of the X-Forwarded-For header to use, if the desired client IP is not at + /// the zeroth index. Defaults to zero. If the index is too large the last array element is returned instead. + /// + public int Index { get; set; } = 0; + /// protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) { @@ -71,7 +78,11 @@ string TryLookupForwardHeader(HttpRequestBase httpRequest, LogEventInfo logEvent var addresses = forwardedHeader.Split(','); if (addresses.Length > 0) { - return addresses[0]; + if (Index >= addresses.Length) + { + Index = addresses.Length - 1; + } + return addresses[Index]?.Trim(); } } @@ -86,7 +97,11 @@ private string TryLookupForwardHeader(HttpRequest httpRequest, LogEventInfo logE var forwardedHeaders = httpRequest.Headers.GetCommaSeparatedValues(headerName); if (forwardedHeaders.Length > 0) { - return forwardedHeaders[0]; + if (Index >= forwardedHeaders.Length) + { + Index = forwardedHeaders.Length - 1; + } + return forwardedHeaders[Index]?.Trim(); } } diff --git a/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs b/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs index ea342000..a3f3c880 100644 --- a/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs +++ b/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs @@ -116,5 +116,63 @@ public void ForwardedForHeaderPresentWithCustomRenderForwardedValue() // Assert Assert.Equal("127.0.0.1", result); } + + [Fact] + public void ForwardedForHeaderContainsMultipleEntriesRendersIndexValue() + { + // Arrange + var (renderer, httpContext) = CreateWithHttpContext(); + +#if !ASP_NET_CORE + httpContext.Request.ServerVariables.Returns(new NameValueCollection {{"REMOTE_ADDR", "192.0.0.0"}}); + httpContext.Request.Headers.Returns( + new NameValueCollection {{ForwardedForHeader, "192.168.1.1, 127.0.0.1"}}); +#else + var headers = new HeaderDict(); + headers.Add(ForwardedForHeader, new StringValues("192.168.1.1, 127.0.0.1")); + httpContext.Request.Headers.Returns(callinfo => headers); +#endif + renderer.CheckForwardedForHeader = true; + renderer.Index = 1; + + // Act + string result = renderer.Render(new LogEventInfo()); + + // Assert + Assert.Equal("127.0.0.1", result); + } + + [Fact] + public void ForwardedForHeaderContainsMultipleEntriesExcessiveIndexRendersLastIndexValue() + { + // Arrange + var (renderer, httpContext) = CreateWithHttpContext(); + +#if !ASP_NET_CORE + httpContext.Request.ServerVariables.Returns(new NameValueCollection {{"REMOTE_ADDR", "192.0.0.0"}}); + httpContext.Request.Headers.Returns( + new NameValueCollection {{ForwardedForHeader, "192.168.1.1, 127.0.0.1"}}); +#else + var headers = new HeaderDict(); + headers.Add(ForwardedForHeader, new StringValues("192.168.1.1, 127.0.0.1")); + httpContext.Request.Headers.Returns(callinfo => headers); +#endif + renderer.CheckForwardedForHeader = true; + renderer.Index = 2; + + // Act + string result = renderer.Render(new LogEventInfo()); + + // Assert + Assert.Equal("127.0.0.1", result); + + renderer.Index = 3; + + // Act + result = renderer.Render(new LogEventInfo()); + + // Assert + Assert.Equal("127.0.0.1", result); + } } } \ No newline at end of file From feca8828a530ca8b9e42f8a6aab56c35142ed545 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 19 Jan 2023 20:40:50 -0500 Subject: [PATCH 2/4] Allow negative indexes for X-Forwarded-For header if network places client IP at end, or at 2nd to end, etc of the header --- .../AspNetRequestIpLayoutRenderer.cs | 34 +++++++---- .../AspNetRequestIpLayoutRendererTests.cs | 60 ++++++++++++++++++- 2 files changed, 83 insertions(+), 11 deletions(-) diff --git a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs index 018f9148..82ab70d0 100644 --- a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs @@ -18,6 +18,7 @@ namespace NLog.Web.LayoutRenderers /// ${aspnet-request-ip} to return the Remote IP /// ${aspnet-request-ip:CheckForwardedForHeader=true} to return first element in the X-Forwarded-For header /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=1} to return second element in the X-Forwarded-For header + /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=-1} to return last element in the X-Forwarded-For header /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=1:ForwardedForHeader=myHeader} to return second element in the myHeader header /// /// Documentation on NLog Wiki @@ -67,6 +68,25 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) builder.Append(ip); } + private int CalculatePosition(string[] headerContents) + { + var position = Index; + + if (position < 0) + { + position = headerContents.Length + position; + } + if (position < 0) + { + position = 0; + } + if (position >= headerContents.Length) + { + position = headerContents.Length - 1; + } + return position; + } + #if !ASP_NET_CORE string TryLookupForwardHeader(HttpRequestBase httpRequest, LogEventInfo logEvent) { @@ -78,11 +98,8 @@ string TryLookupForwardHeader(HttpRequestBase httpRequest, LogEventInfo logEvent var addresses = forwardedHeader.Split(','); if (addresses.Length > 0) { - if (Index >= addresses.Length) - { - Index = addresses.Length - 1; - } - return addresses[Index]?.Trim(); + var position = CalculatePosition(addresses); + return addresses[position]?.Trim(); } } @@ -97,11 +114,8 @@ private string TryLookupForwardHeader(HttpRequest httpRequest, LogEventInfo logE var forwardedHeaders = httpRequest.Headers.GetCommaSeparatedValues(headerName); if (forwardedHeaders.Length > 0) { - if (Index >= forwardedHeaders.Length) - { - Index = forwardedHeaders.Length - 1; - } - return forwardedHeaders[Index]?.Trim(); + var position = CalculatePosition(forwardedHeaders); + return forwardedHeaders[position]?.Trim(); } } diff --git a/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs b/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs index a3f3c880..0174ab61 100644 --- a/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs +++ b/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs @@ -143,7 +143,32 @@ public void ForwardedForHeaderContainsMultipleEntriesRendersIndexValue() } [Fact] - public void ForwardedForHeaderContainsMultipleEntriesExcessiveIndexRendersLastIndexValue() + public void ForwardedForHeaderContainsMultipleEntriesRendersLastValue() + { + // Arrange + var (renderer, httpContext) = CreateWithHttpContext(); + +#if !ASP_NET_CORE + httpContext.Request.ServerVariables.Returns(new NameValueCollection {{"REMOTE_ADDR", "192.0.0.0"}}); + httpContext.Request.Headers.Returns( + new NameValueCollection {{ForwardedForHeader, "192.168.1.1, 127.0.0.1"}}); +#else + var headers = new HeaderDict(); + headers.Add(ForwardedForHeader, new StringValues("192.168.1.1, 127.0.0.1")); + httpContext.Request.Headers.Returns(callinfo => headers); +#endif + renderer.CheckForwardedForHeader = true; + renderer.Index = -1; + + // Act + string result = renderer.Render(new LogEventInfo()); + + // Assert + Assert.Equal("127.0.0.1", result); + } + + [Fact] + public void ForwardedForHeaderContainsMultipleEntriesExcessiveIndexRendersLastValue() { // Arrange var (renderer, httpContext) = CreateWithHttpContext(); @@ -174,5 +199,38 @@ public void ForwardedForHeaderContainsMultipleEntriesExcessiveIndexRendersLastIn // Assert Assert.Equal("127.0.0.1", result); } + + [Fact] + public void ForwardedForHeaderContainsMultipleEntriesExcessiveNegativeIndexRendersFirstValue() + { + // Arrange + var (renderer, httpContext) = CreateWithHttpContext(); + +#if !ASP_NET_CORE + httpContext.Request.ServerVariables.Returns(new NameValueCollection {{"REMOTE_ADDR", "192.0.0.0"}}); + httpContext.Request.Headers.Returns( + new NameValueCollection {{ForwardedForHeader, "127.0.0.1, 192.168.1.1"}}); +#else + var headers = new HeaderDict(); + headers.Add(ForwardedForHeader, new StringValues("127.0.0.1, 192.168.1.1")); + httpContext.Request.Headers.Returns(callinfo => headers); +#endif + renderer.CheckForwardedForHeader = true; + renderer.Index = -3; + + // Act + string result = renderer.Render(new LogEventInfo()); + + // Assert + Assert.Equal("127.0.0.1", result); + + renderer.Index = -4; + + // Act + result = renderer.Render(new LogEventInfo()); + + // Assert + Assert.Equal("127.0.0.1", result); + } } } \ No newline at end of file From 57f91648a125b866dc751a132744f5e9553def23 Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Thu, 19 Jan 2023 20:44:41 -0500 Subject: [PATCH 3/4] Additional comments for the Index property --- src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs index 82ab70d0..eec623d4 100644 --- a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs @@ -40,6 +40,9 @@ public class AspNetRequestIpLayoutRenderer : AspNetLayoutRendererBase /// /// Gets or sets the array index of the X-Forwarded-For header to use, if the desired client IP is not at /// the zeroth index. Defaults to zero. If the index is too large the last array element is returned instead. + /// If a negative index is used, this is used as the position from the end of the array. + /// Minus one will indicate the last element in the array. If the negative index is too large the first index + /// of the array is returned instead. /// public int Index { get; set; } = 0; From 6ee95e792076003e8ad117f6b2218a91974eb87f Mon Sep 17 00:00:00 2001 From: Burak Akgerman Date: Mon, 23 Jan 2023 12:53:15 -0500 Subject: [PATCH 4/4] Implement review comments --- .../AspNetRequestIpLayoutRenderer.cs | 19 ++++++++++++++----- .../AspNetRequestIpLayoutRendererTests.cs | 16 ++++++---------- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs index eec623d4..9731fc6f 100644 --- a/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs +++ b/src/Shared/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs @@ -17,9 +17,9 @@ namespace NLog.Web.LayoutRenderers /// /// ${aspnet-request-ip} to return the Remote IP /// ${aspnet-request-ip:CheckForwardedForHeader=true} to return first element in the X-Forwarded-For header - /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=1} to return second element in the X-Forwarded-For header - /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=-1} to return last element in the X-Forwarded-For header - /// ${aspnet-request-ip:CheckForwardedForHeader=true:Index=1:ForwardedForHeader=myHeader} to return second element in the myHeader header + /// ${aspnet-request-ip:CheckForwardedForHeaderOffset=1} to return second element in the X-Forwarded-For header + /// ${aspnet-request-ip:CheckForwardedForHeaderOffset=-1} to return last element in the X-Forwarded-For header + /// ${aspnet-request-ip:CheckForwardedForHeaderOffset=1:ForwardedForHeader=myHeader} to return second element in the myHeader header /// /// Documentation on NLog Wiki [LayoutRenderer("aspnet-request-ip")] @@ -44,7 +44,16 @@ public class AspNetRequestIpLayoutRenderer : AspNetLayoutRendererBase /// Minus one will indicate the last element in the array. If the negative index is too large the first index /// of the array is returned instead. /// - public int Index { get; set; } = 0; + public int CheckForwardedForHeaderOffset + { + get => _checkForwardedForHeaderOffset; + set + { + _checkForwardedForHeaderOffset = value; + CheckForwardedForHeader = true; + } + } + private int _checkForwardedForHeaderOffset; /// protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) @@ -73,7 +82,7 @@ protected override void DoAppend(StringBuilder builder, LogEventInfo logEvent) private int CalculatePosition(string[] headerContents) { - var position = Index; + var position = CheckForwardedForHeaderOffset; if (position < 0) { diff --git a/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs b/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs index 0174ab61..9d33dc8c 100644 --- a/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs +++ b/tests/Shared/LayoutRenderers/AspNetRequestIpLayoutRendererTests.cs @@ -132,8 +132,7 @@ public void ForwardedForHeaderContainsMultipleEntriesRendersIndexValue() headers.Add(ForwardedForHeader, new StringValues("192.168.1.1, 127.0.0.1")); httpContext.Request.Headers.Returns(callinfo => headers); #endif - renderer.CheckForwardedForHeader = true; - renderer.Index = 1; + renderer.CheckForwardedForHeaderOffset = 1; // Act string result = renderer.Render(new LogEventInfo()); @@ -157,8 +156,7 @@ public void ForwardedForHeaderContainsMultipleEntriesRendersLastValue() headers.Add(ForwardedForHeader, new StringValues("192.168.1.1, 127.0.0.1")); httpContext.Request.Headers.Returns(callinfo => headers); #endif - renderer.CheckForwardedForHeader = true; - renderer.Index = -1; + renderer.CheckForwardedForHeaderOffset = -1; // Act string result = renderer.Render(new LogEventInfo()); @@ -182,8 +180,7 @@ public void ForwardedForHeaderContainsMultipleEntriesExcessiveIndexRendersLastVa headers.Add(ForwardedForHeader, new StringValues("192.168.1.1, 127.0.0.1")); httpContext.Request.Headers.Returns(callinfo => headers); #endif - renderer.CheckForwardedForHeader = true; - renderer.Index = 2; + renderer.CheckForwardedForHeaderOffset = 2; // Act string result = renderer.Render(new LogEventInfo()); @@ -191,7 +188,7 @@ public void ForwardedForHeaderContainsMultipleEntriesExcessiveIndexRendersLastVa // Assert Assert.Equal("127.0.0.1", result); - renderer.Index = 3; + renderer.CheckForwardedForHeaderOffset = 3; // Act result = renderer.Render(new LogEventInfo()); @@ -215,8 +212,7 @@ public void ForwardedForHeaderContainsMultipleEntriesExcessiveNegativeIndexRende headers.Add(ForwardedForHeader, new StringValues("127.0.0.1, 192.168.1.1")); httpContext.Request.Headers.Returns(callinfo => headers); #endif - renderer.CheckForwardedForHeader = true; - renderer.Index = -3; + renderer.CheckForwardedForHeaderOffset = -3; // Act string result = renderer.Render(new LogEventInfo()); @@ -224,7 +220,7 @@ public void ForwardedForHeaderContainsMultipleEntriesExcessiveNegativeIndexRende // Assert Assert.Equal("127.0.0.1", result); - renderer.Index = -4; + renderer.CheckForwardedForHeaderOffset = -4; // Act result = renderer.Render(new LogEventInfo());