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

Allow ShinySDR to function behind a reverse proxy #48

Closed
wants to merge 2 commits into from

Conversation

thefinn93
Copy link

@thefinn93 thefinn93 commented May 3, 2016

I wanted ShinySDR behind a reverse proxy, and ended up finding some fun stuff with the way it determines the websocket URL. Not sure if you want to accept this, but I figured I'd submit it anyway.

This change basically causes it to set the websocket URI to ws<s>://<host>/websocket/<path> if document.location.port is undefined, as it is when the page is loaded on the default port (80 for http, 443 for https). I put the following in my Apache vhost to make the reverse proxying happen, if you'd like to add it to some documentation somewhere:

        RewriteEngine On
        RewriteCond            %{REQUEST_URI}   ^/websocket             [NC]
        RewriteRule             /websocket/(.*) ws://localhost:8101/$1  [P,L]

        ProxyPass               /               http://127.0.0.1:8100/
        ProxyPassReverse        /               http://127.0.0.1:8100/

Also, sorry, my editor appears to have removed all tailing whitespace on every line in the file, which makes the diff look weird. The relevant changes start on line 216.

@kpreid
Copy link
Owner

kpreid commented May 4, 2016

I entirely approve of making reverse proxying work better, but this particular change is making the wrong thing the code currently does worse. The right thing is as commented:

// TODO: Have server deliver websocket URL, remove port number requirement

That is, the info delivered to the client (in index.template.xhtml) should include the full ws://... URL to connect to, and the client should use that URL. The client should not, as it currently does, be constructing URLs according to hardcoded rules.

Since you've pointed out a good reason to fix this, I'll probably get to it sooner.

In the even bigger picture, really I ought to drop txWS in favor of something that lets us use a single port. (I've got an experimental branch that uses Autobahn instead, but it doesn't work completely.)

@kpreid
Copy link
Owner

kpreid commented Jul 23, 2016

As of commit 5dfb897, the WebSocket URLs are provided by the server instead of derived by the client.

The remaining element of fixing this “correctly” is to provide a configuration option to change the generated URLs.

@kpreid
Copy link
Owner

kpreid commented Aug 5, 2017

Closing this pull request (per above explanation) in favor of #92 to track the remaining work (changing the generated URLs).

@kpreid kpreid closed this Aug 5, 2017
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