Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RequestIp configuration for requestIP and clientIP forwarding #544

Closed
johnkors opened this issue Apr 7, 2020 · 2 comments · Fixed by #546
Closed

RequestIp configuration for requestIP and clientIP forwarding #544

johnkors opened this issue Apr 7, 2020 · 2 comments · Fixed by #546
Labels
Milestone

Comments

@johnkors
Copy link
Contributor

johnkors commented Apr 7, 2020

Informational:

This is not an urgent issue, just mentioning it since I stumbled across it.

NLog.Web/NLog.Web.AspNetCore version: Latest
Platform: ASP.NET Core

The X-Forwarded-For headername is hardcoded here, but it's configurable in ASP.NET Core.

You might want to add to docs that the CheckForwardedForHeader flag in the AspNetRequestIpLayoutRenderer only works with a header named X-Forwarded-For, and no other headers - or make it configurable in the renderer :)

Again, no urgency (I'm using the same naming as you do here for my forwarded headers).

@304NotModified
Copy link
Member

Thanks!

Should we make it configurable or read it from the ASP.NET Core config? (and if the latter, how?)

@johnkors
Copy link
Contributor Author

Reading it from the ASP.NET Core config:
I think you would have to try to resolve this options type using DI: ForwardedHeadersOptions, and if a registration/configuration of those options is present — use the value of that property. I'm a bit unsure if the app startup fails if you try to resolve an options type that hasn't been configured, so probably would have to handle that gracefully as well.

The drawback of linking it to the ASP.NET Core config: In 2.0/2.1./2.2 this options type is in the Http Overrides nuget, and in 3.0 it's available as part of the framework. I

Maybe it's a bit too much to have NLog.Web.AspNetCore depend on this nuget (for 2.X apps) just for the options wiring..? If it was me, I would probably just have it be configurable and avoid the dependency, but up to you to, really :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants