${AspNetRequestIp} added CheckForwardedForHeader to check for X-Forwarded-For header

[Giorgi]

No description provided.

[Giorgi]

Fixes #324

[codecov]

Codecov Report

Merging #325 into master will increase coverage by 1%.
The diff coverage is 69%.

@@          Coverage Diff          @@
##           master   #325   +/-   ##
=====================================
+ Coverage      62%    63%   +1%     
=====================================
  Files          32     32           
  Lines         454    466   +12     
  Branches       97    101    +4     
=====================================
+ Hits          282    294   +12     
+ Misses        135    132    -3     
- Partials       37     40    +3

Impacted Files	Coverage Δ
...e/LayoutRenderers/AspNetRequestIpLayoutRenderer.cs	75% <69%> (+75%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ebb807...215e803. Read the comment docs.

[snakefoot]

Instead of calling httpContext.TryGetRequest() twice, then do it once and reuse it for Headers[ForwardedForHeader] and ServerVariables["REMOTE_ADDR"] (Maybe also handle null value from TryGetRequest)

[Giorgi]

Done

[snakefoot]

Maybe perform if (Headers.ContainsKey(ForwardedForHeader)) before doing the GetCommaSeparatedValues-call. To reduce allocation when nothing to parse.

[Giorgi]

If the header is missing GetCommaSeparatedValues returns StringValues.Empty which is a static field so I think it shouldn't cause any extra allocation

[snakefoot]

Not when reading the code:

https://github.com/aspnet/HttpAbstractions/blob/master/src/Microsoft.AspNetCore.Http.Abstractions/Extensions/HeaderDictionaryExtensions.cs#L42

https://github.com/aspnet/HttpAbstractions/blob/master/src/Microsoft.AspNetCore.Http.Abstractions/Internal/ParsingHelpers.cs#L22

But yes StringValues.Empty and ToArray does not allocate. But the ParsingHelpers is very eager as it allocates a yield-state-machine no matter what.

[Giorgi]

OK, I will add that check

[Giorgi]

Done

[snakefoot]

Maybe possible to create a unit-test similar to AspNetRequestReferrerRendererTests where Headers are mocked to include "X-Forwarded-For" with dummy-address ? (And verify it is returned)

[Giorgi]

Tests added

[snakefoot]

Thank you for the unit-tests. One last request from me. Could the logic be refactored so it becomes:

string ip = CheckForwardedForHeader ? TryLookupForwardHeader(httpContext) : string.Empty;
if (string.IsNullOrEmpty(ip))
{
#if !ASP_NET_CORE
      ip = httpContext.TryGetRequest()?.ServerVariables["REMOTE_ADDR"];
#else
      ip = httpContext.Connection?.RemoteIpAddress;
#endif
}
builder.Append(ip);

Then it would reduce the amount of code in one method. Will also skip double lookup of ip-address. And maybe make the "Better Code Hub" more happy.

[Giorgi]

What should be the type of context parameter in TryLookupForwardHeader method?

[snakefoot]

Maybe something like this

#if !ASP_NET_CORE
string TryLookupForwardHeader(System.Web.HttpContextBase httpContext)
{
    // Blah Blah
}
#else
string TryLookupForwardHeader(Microsoft.AspNetCore.Http.HttpContext httpContext)
{
    // Blah Blah
}
#endif

[Giorgi]

Do you think it's worth the hassle? I will have to call httpContext.TryGetRequest() twice again.

[snakefoot]

Alternative like this:

{
   var httpRequest = httpContext.TryGetRequest();
   if (httpRequest==null)
      return;

   string ip = CheckForwardedForHeader ? TryLookupForwardHeader(httpRequest) : string.Empty;
   if (string.IsNullOrEmpty(ip))
   {
#if !ASP_NET_CORE
      ip = httpRequest.ServerVariables["REMOTE_ADDR"];
#else
      ip = httpContext.Connection?.RemoteIpAddress;
#endif
   }
   builder.Append(ip);
}

#if !ASP_NET_CORE
string TryLookupForwardHeader(System.Web.HttpRequestBase httpRequest)
{
    // New logic
}
#else
string TryLookupForwardHeader(Microsoft.AspNetCore.Http.HttpRequest httpRequest)
{
    // New logic
}
#endif

[Giorgi]

OK, I don't fully agree with that but I can do it.

[snakefoot]

Thank you. All lights have become green. Now just need the final approval stamp from @304NotModified

[304NotModified]

Thanks for the contribution! Also thanks @snakefoot for the reviews! 👍 🎉