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

x-forwarded-host overwrite for mutli level proxies #1267

Merged
merged 1 commit into from Aug 22, 2019

Conversation

@jaggernoth
Copy link
Contributor

@jaggernoth jaggernoth commented May 23, 2018

Let's consider a scenario
proxy (Internet-facing) -> proxy(load balancer) -> application where both proxies will be based on node-http-proxy, with current logic original host to be lost
With updated logic the original forwarded host will be passed down the proxy chain

With more than 1 proxy the original host was lost, now it will be passed down the proxy chain
@jcrugzz
jcrugzz approved these changes Jun 6, 2018
Copy link
Contributor

@jcrugzz jcrugzz left a comment

LGTM 👍

@jcrugzz
Copy link
Contributor

@jcrugzz jcrugzz commented Jun 6, 2018

@jaggernoth host is probably most important but should we also default the other values so we are consistent?

@jaggernoth
Copy link
Contributor Author

@jaggernoth jaggernoth commented Jun 11, 2018

@jcrugzz : From what I can see in the code for other x-forwarded header entries new levels are concatenated as CSV values and I was considering making x-forwarded-host a CSV value as well but it would be much riskier as it may cause compatibility issues (CSV requires additional parsing).
AFAIK there is no real consensus on how the mechanism should work, but bare in mind I've done very minimal research just to make my code work;).
One thing is certain: just overwriting the host seems to defeat the purpose of this header.

Copy link
Contributor

@indexzero indexzero left a comment

Agree with @jcrugzz comment, but this PR is still 👍

As to the CSV value question @jaggernoth the research I briefly looked at did not suggest that as a format:

@indexzero indexzero merged commit 36bfe56 into http-party:master Aug 22, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mcheshkov added a commit to gemini-testing/node-http-proxy that referenced this pull request Sep 16, 2019
With more than 1 proxy the original host was lost, now it will be passed down the proxy chain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.