Fix port mismatch when used behind a reverse proxy #286

Merged
merged 1 commit into from Jan 19, 2014

Projects

None yet

3 participants

@BYK
BYK commented Jan 18, 2014

I tried using Subway on a Dokku instance and since it uses a reverse proxy to map port 80 to the port Subway uses, the generated script was using the internal port and causing subway to stop working. This patch solves the problem by simply using the same port where the document is loaded.

@thedjpetersen thedjpetersen and 1 other commented on an outdated diff Jan 18, 2014
views/layout.jade
@@ -10,7 +10,7 @@ html
script
var ENV = '#{env}',
- PORT = #{port};
+ PORT = document.location.port;
@thedjpetersen
thedjpetersen Jan 18, 2014 owner

I would rather this be:

PORT = #{port} || document.location.port

That way you can just set the client_port to null and it will fallback to use the port of the document

@BYK
BYK Jan 18, 2014

I disagree since we always bind the socket.io server to the port where we serve the files, thus client_ports is actually redundant after this change.

What do you think?

@thedjpetersen
thedjpetersen Jan 18, 2014 owner

If this is the case then the pull request should remove client_port as well from webserver.js

@BYK
BYK Jan 18, 2014

Agreed. Let me look out for more clean-up opportunities and the nupdate the PR.

@BYK BYK Fix port mismatch when used behind a reverse proxy
Websockets port is always the same as the static web server port so
instead of having some tricks, this patch simply removes that and uses
`document.location.protocol` for the port.
945f05f
@BYK
BYK commented Jan 18, 2014

Updated!

@thedjpetersen
Owner

@badosu could you take a quick look at this since you added the client_port attribute?

@badosu
Collaborator
badosu commented Jan 18, 2014

@thedjpetersen @BYK How will the users set a different port for the client?

For example, will I not be able anymore to run subway on heroku with this patch?

If not, can we use another approach to solve our issue?

@BYK
BYK commented Jan 19, 2014

For example, will I not be able anymore to run subway on heroku with this patch?

Definitely not. On the contrary, I'm making this change to prevent custom configuration for Heroku-like systems (I use Dokku).

The point is, client_port is always the port you get from document.location.port since we listen on the same port with the asset server.

If you think this will break your setup, feel free to get this fork and push it to Heroku but again, I did this to be able to run this on my Dokku server which runs smoothly, without any configuration change.

@badosu
Collaborator
badosu commented Jan 19, 2014

Is'nt it possible to support Dokku and support people who can't use websockets as well?

I am totally into supporting a new infrasctructure provider, but removing support for another or the fallback for websockets, as a consequence, does not seem a good idea.

Also, this would revert #226 too.

@BYK
BYK commented Jan 19, 2014

@badosu, this is not removing support for Heroku or anything else. Even if you don't use websockets, you still use the same port on the client: document.location.port.

And it does not revert #226 either. We still use the port provided by the environment. Dokku is not a new provider, it is just an open source Heroku clone which has exactly the same properties with it except for the websocket limitation: https://github.com/progrium/dokku

@badosu
Collaborator
badosu commented Jan 19, 2014

So it is ok for me. 👍

@BYK
BYK commented Jan 19, 2014

@badosu, if this regresses anything related to Heroku or other service, I give you my word that I'll fix it ;)

Thanks for the quick response! 👍

@thedjpetersen
Owner

Thanks for the input @badosu , guess it all LGTM!

@thedjpetersen thedjpetersen merged commit 7d191cb into thedjpetersen:master Jan 19, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment