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

provided option to use x-forwarded-for as remote address [updated] #226

Closed
wants to merge 1 commit into from

Conversation

@midnightcodr
Copy link

midnightcodr commented Oct 25, 2014

Providing the option to use X-Forwarded-For as remote client's ip address can be very useful when hapi serves behind nginx or a load balancing web server. It makes more sense to record the 'true' remote ip instead of the proxy server's ip in that kind of scenario.

@arb arb self-assigned this Oct 30, 2014
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Oct 30, 2014

Would there ever be a time where you wouldn't want this? If not, rather than adding another option, why not just always log this value and if that header isn't set, it'll be undefined and will come out in the wash when it's stringified.

@midnightcodr

This comment has been minimized.

Copy link
Author

midnightcodr commented Oct 31, 2014

If x-forwarde-for is not in the header remoteAddress will fall back to request.info.remoteAddress. Also I would love to be able get x-forwarde-for logged without adding another option. Could you clarify "always log this value"? Can it be done without modifying the code? Thanks.

Sent from my iPhone

On Oct 30, 2014, at 5:27 PM, Adam Bretz notifications@github.com wrote:

Would there ever be a time where you wouldn't want this? If not, rather than adding another option, why not just always log this value and if that header isn't set, it'll be undefined and will come out in the wash when it's stringified.


Reply to this email directly or view it on GitHub.

@arb arb added this to the 3.2.0 milestone Oct 31, 2014
@arb arb added the feature label Oct 31, 2014
@@ -228,7 +229,7 @@ internals.Monitor.prototype._requestHandler = function (request) {
path: request.path,
query: request.query,
source: {
remoteAddress: request.info.remoteAddress,
remoteAddress: this.settings.xforward && req.headers['x-forwarded-for'] || request.info.remoteAddress,
userAgent: req.headers['user-agent'],

This comment has been minimized.

Copy link
@arb

arb Oct 31, 2014

Contributor

Under remoteAddress, you can add forwardedFor and set it to req.headers['x-forwarded-for'] Then you don't need the extra option.

This comment has been minimized.

Copy link
@midnightcodr

midnightcodr Oct 31, 2014

Author

Awesome your solution is better. Will update PR tonight. Thanks.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 3, 2014

Can you rebase this branch? 7 commits for a single line change is a bit much 😄

@midnightcodr midnightcodr force-pushed the midnightcodr:master branch from 18fd379 to 0a95170 Nov 4, 2014
@midnightcodr

This comment has been minimized.

Copy link
Author

midnightcodr commented Nov 4, 2014

Agreed :) I just did. See if you can merge now. Thanks for being so patient with a complete PR newbie.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 4, 2014

Still have 7 commits. At this point it would probably be easier to just start a new local branch and make the changes there since it's only one line.

@midnightcodr midnightcodr force-pushed the midnightcodr:master branch from 0a95170 to 4b3a269 Nov 4, 2014
@midnightcodr

This comment has been minimized.

Copy link
Author

midnightcodr commented Nov 4, 2014

Learned how to squash. How about now? Thanks.

@arb arb removed this from the 4.0.0 milestone Nov 4, 2014
settings fix

added tests for X-Forwarded-For header

Revert "settings fix"

This reverts commit d5ab86d.

Revert "provided option to use x-forwarded-for as remote address"

This reverts commit bcb5c97.

logs X-Forwarded-For ip

Revert "added tests for X-Forwarded-For header"

This reverts commit 4134e17.
@midnightcodr midnightcodr force-pushed the midnightcodr:master branch from 4b3a269 to cb4f1c3 Nov 9, 2014
@arb arb modified the milestone: 4.1.0 Nov 10, 2014
@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 10, 2014

Can you please add or update a test showing that this works? Both in cases where this header is set and unset?

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Nov 10, 2014

UPDATE actually before we make this change, have you tried setting logRequestHeaders to true? That dumbs all the headers out as well and the "x-forwarded-for" header should be in there.

@midnightcodr

This comment has been minimized.

Copy link
Author

midnightcodr commented Nov 10, 2014

Yup you nailed it. Should've tried that option first. Will close the PR soon.

@arb arb closed this Nov 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.