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
Copy link
Contributor

@jcrugzz jcrugzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jcrugzz
Copy link
Contributor

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 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
mcheshkov pushed 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
This was referenced Mar 17, 2021
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.

None yet

3 participants