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

Bug Fix: Pick port defaults depending if the host was in HTTP_HOST or SERVER #2487

Merged
merged 3 commits into from Sep 16, 2018

Conversation

Projects
None yet
6 participants
@lornajane
Contributor

lornajane commented Aug 13, 2018

This change relates to #2486 where a local server on port 8080 receiving a request from ngrok that is on port 80 ends up with a URI containing the ngrok host but the local server port number. This change fixes it for me BUT it causes a test to fail (UriTest::testCreateEnvironmentWithIPv6HostNoPort) because the test expects the response to be port 8080, but I'm not sure I agree that's the right behaviour. I can fix the test if we can agree that NULL is the expected response to getPort() called on a request without a port number?

@coveralls

This comment has been minimized.

coveralls commented Aug 13, 2018

Coverage Status

Coverage increased (+0.001%) to 97.814% when pulling 77bd8c9 on lornajane:uri-port-defaults into 0618c56 on slimphp:3.x.

@silentworks

This comment has been minimized.

Member

silentworks commented Aug 14, 2018

In a case that someone should have an application (intranet apps) running on a different port, I don't think this change would work. e.g. http://myintranet.net:3200

@akrabat

This comment has been minimized.

Member

akrabat commented Aug 20, 2018

In a case that someone should have an application (intranet apps) running on a different port, I don't think this change would work. e.g. http://myintranet.net:3200

Actually, I think that this scenario would continue to work as the HTTP_HOST value includes the port number when it's non-standard.

i.e. when I run php -S 0.0.0.0:8888 -t public/, then $_SERVER['HTTP_HOST'] is set to localhost:8888 as that's the URL that I navigate to to get to the website. We have this code which then kicks in to extract the port number.

@akrabat

This comment has been minimized.

Member

akrabat commented Aug 20, 2018

I think it's reasonable to default to port 80 if HTTP_HOST is set. We need to consider SSL though as that needs the port to be set to 443, so may be we need to check SERVER_PORT for 443 explicitly before setting the default to 80?

@akrabat akrabat self-assigned this Aug 20, 2018

@lornajane

This comment has been minimized.

Contributor

lornajane commented Aug 23, 2018

Hmm, this is a really good point. Because the default should be 443 or 80, depending on the protocol, but I'm not sure we always know the protocol of the incoming request, only what the local server is using. If I use https:// for my ngrok tunnel, I still don't get that (default 443) port number in the HTTP_HOST

@tuupola

This comment has been minimized.

Contributor

tuupola commented Aug 23, 2018

For tunneled requests these could be derived from X-Forwarded-Port and X-Forwarded-Proto. Not sure if it is the best idea to trust them by default, nor if ngrok sets these. I do know that atleast Cloudflare sets them them when using SSL proxying.

@akrabat

This comment has been minimized.

Member

akrabat commented Sep 10, 2018

I've looked it up. According to the spec, if the Host header exists and doesn't have a port, then it is the default for the protocol.

i.e. if the port is missing from Host header, we should check getScheme() and set 443 or 80 as appropriate.

@lornajane

This comment has been minimized.

Contributor

lornajane commented Sep 10, 2018

At the risk of thickening the plot, we don't have scheme either. Reading the git history the X-Forwarded* fields were deliberately removed since they can't always be trusted and that's the only place I can see the scheme. Luckily @akrabat has a middleware that reads those headers and applies the correct scheme to the request object which helped part of my problem but not the port number part.

@akrabat

This comment has been minimized.

Member

akrabat commented Sep 10, 2018

Yes, you’d still need rka-scheme-and-host-middleware (now renamed to proxy-detection-middleware) to pick up the scheme. That middleware should probably handle the port better too.

Further thinking about the Uri part, it might be enough to set $port to null when the host header exists.

akrabat added some commits Sep 11, 2018

Don't set the default port when the Host header exists
If there is a Host header, then we can expect the port to be part of it.
If the port is not there, then we should use the default for the scheme
as per RFC 2616, section 14.23.
Update tests for handling a default port in the Host header
Also remove test where SERVER_PORT is used when Host header exists as
this was an incorrect assumption that is wrong.
@lornajane

This comment has been minimized.

Contributor

lornajane commented Sep 16, 2018

This works perfectly for me (using the middleware as well) both locally with a custom port number and with the ngrok tunnel in front of it.

@akrabat akrabat added Slim 3 and removed pending response labels Sep 16, 2018

@akrabat akrabat added this to the 3.11 milestone Sep 16, 2018

@akrabat

This comment has been minimized.

Member

akrabat commented Sep 16, 2018

Thanks.

@akrabat akrabat merged commit 77bd8c9 into slimphp:3.x Sep 16, 2018

3 checks passed

Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 97.814%
Details

akrabat added a commit that referenced this pull request Sep 16, 2018

@akrabat akrabat changed the title from Pick port defaults depending if the host was in HTTP_HOST or SERVER to Bug Fix: Pick port defaults depending if the host was in HTTP_HOST or SERVER Sep 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment