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

Preserve original header value in a new header #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sporkmonger
Copy link
Contributor

Since the XFF header potentially has security implications, it seems worth preserving the original value, particularly in cases where the transparent directive in the proxy module is likely to be interacting w/ this. I'm not sure if this should be 'always-on' or optional though. Happy to drop it behind a flag if preferred.

@captncraig
Copy link
Owner

I'm confused why this is needed here. This plugin does not change XFF at all. Just reads it.

@sporkmonger
Copy link
Contributor Author

That's true, but proxy transparent does change it. And since there's poten

@sporkmonger sporkmonger reopened this Jun 27, 2017
@sporkmonger
Copy link
Contributor Author

Bumped the close button.

It's true that realip doesn't change the header but proxy transparent does and because realip changes the remote IP, in conjunction, they're really hard to debug. In fairness, the more I think about it, the more I think this should be behind a preserve or preserve_header etc flag, but yeah, having the original header value makes debugging XFF issues in longer load balancer/proxy scenarios massively easier.

I could see an argument that proxy should be responsible for preserving the header, but in that case realip should probably be responsible for providing an option to expose the original remote IP value.

@sporkmonger
Copy link
Contributor Author

sporkmonger commented Jul 5, 2017

Refactored, adds a preserve flag, documents it, and preserves the RemoteAddr value (the specific information that's destroyed by the realip plugin rather than something that's modified only in conjunction w/ another plugin). Adds tests for the above.

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

Successfully merging this pull request may close these issues.

2 participants