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

BUGFIX: Port is set correctly when Trusted Proxy only sends protocol override #493

Merged
merged 1 commit into from Sep 8, 2016

Conversation

albe
Copy link
Member

@albe albe commented Sep 7, 2016

Before the port of the request URI was not set correctly if the Trusted Proxy would
send an X-Forwarded-Proto: HTTPS header without also specifying a forwarded port.
This resulted in generated URLs like "https://acme.com:80".

The change fixes that by doing a case insensitive check for the protocol to be
'https' and set port 443 accordingly.

…override

Before the port of the request URI would not be correctly set if the Trusted Proxy
would send an `X-Forwarded-Proto: HTTPS` header without also specifying a forwarded port.
This resulted in generated URLs like "https://acme.com:80".

The change fixes that by doing a case insensitive check for the protocol to be
'https' and set port 443 accordingly.
@@ -440,6 +440,12 @@ public function forwardHeaderTestsDataProvider()
'requestUri' => 'https://acme.com',
'expectedUri' => 'https://acme.com:80',
),
array(
Copy link
Member

Choose a reason for hiding this comment

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

Small nitpick: Use short array syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Not if the other declarations use the long form. And since it's better not to mix functional and cleanup, let's leave it like that.

@kdambekalns kdambekalns merged commit fad761b into neos:3.3 Sep 8, 2016
@kdambekalns kdambekalns deleted the https-forwarded-port branch September 8, 2016 06:51
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

3 participants