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

Fix forwarded logic #2219

Merged
merged 2 commits into from Mar 21, 2017
Merged

Conversation

dmcgowan
Copy link
Collaborator

Removes all support for X-Forwarded-Port in the URL builder. This header is not informative for determining the host given by the client. Updated the logic to always prefer the standard Forwarded header over the non-standard X-Forwarded-* headers when provided. Only use the host parameter of the Forwarded header and never use the for parameter, which is incorrect for setting as the server host.

Note the original reason for providing support for X-Forwarded-Port was related to a bug in a load balancer (see reference in #2008). Amazon ELB always sets X-Forwarded-Port regardless of whether the port is specified in the Host header, and leaves the original Host header alone. The goal of the URL builder is to recreate the host exactly as given by the client, which this header is useless for.

Carry #2214
Partially reverts #2008
Fixes #2177

Please review this so we can get a patch release for 2.6
Ping @tt @stevvooe @aaronlehmann @miminar @nwt @marcusmartins

tt and others added 2 commits March 20, 2017 16:10
Signed-off-by: Troels Thomsen <troels@thomsen.io>
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Partially reverts change adding support for X-Forwarded-Port.
Changes the logic to prefer the standard Forwarded header over
X-Forwarded headers. Prefer forwarded "host" over "for" since
"for" represents the client and not the client's request.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@stevvooe
Copy link
Collaborator

LGTM

@marcusmartins
Copy link
Contributor

LGTM

1 similar comment
@aaronlehmann
Copy link
Contributor

LGTM

Copy link
Contributor

@miminar miminar left a comment

Choose a reason for hiding this comment

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

LGTM

The bug on our side has been fixed. Sorry for any inconvenience this might have caused.

@dmcgowan dmcgowan merged commit 0700fa5 into distribution:master Mar 21, 2017
@stevvooe stevvooe deleted the fix-forwarded-logic branch March 21, 2017 18:51
@marcusmartins marcusmartins added this to the Registry/2.7 milestone Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants